Re: [PATCH 2/3] virtio-net: Only enable userland vq if using tap backend

[Eugenio Perez Martin]

On Thu, Nov 18, 2021 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Nov 18, 2021 at 3:29 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Qemu falls back on userland handlers even if vhost-user and vhost-vdpa
> > cases. These assumes a tap device can handle the packets.
> >
> > If a vdpa device fail to start, it can trigger a sigsegv because of
> > that. Do not resort on them unless actually possible.
>
> It would be better to show the calltrace here then we can see the root cause.
>

Sure, I'll paste here and I'll resend to the next version:
#1  0x000055955f696e92 in nc_sendv_compat (flags=<optimized out>,
iovcnt=2, iov=0x7ffe73abe300, nc=0x7fcf22d6d010) at ../net/net.c:756
#2  qemu_deliver_packet_iov (sender=<optimized out>,
opaque=0x7fcf22d6d010, iovcnt=2, iov=0x7ffe73abe300, flags=<optimized
out>) at ../net/net.c:784
#3  qemu_deliver_packet_iov (sender=<optimized out>, flags=<optimized
out>, iov=0x7ffe73abe300, iovcnt=2, opaque=0x7fcf22d6d010) at
../net/net.c:763
#4  0x000055955f69a078 in qemu_net_queue_deliver_iov (iovcnt=2,
iov=0x7ffe73abe300, flags=0, sender=0x5595631f5ac0,
    queue=0x559561c7baa0) at ../net/queue.c:179
#5  qemu_net_queue_send_iov (queue=0x559561c7baa0,
sender=0x5595631f5ac0, flags=flags@entry=0,
iov=iov@entry=0x7ffe73abe300,
    iovcnt=iovcnt@entry=2, sent_cb=sent_cb@entry=0x55955f82ae60
<virtio_net_tx_complete>) at ../net/queue.c:246
#6  0x000055955f697d43 in qemu_sendv_packet_async
(sent_cb=0x55955f82ae60 <virtio_net_tx_complete>, iovcnt=2,
iov=0x7ffe73abe300,
    sender=<optimized out>) at ../net/net.c:825
#7  qemu_sendv_packet_async (sender=<optimized out>,
iov=iov@entry=0x7ffe73abe300, iovcnt=iovcnt@entry=1662966768,
    sent_cb=sent_cb@entry=0x55955f82ae60 <virtio_net_tx_complete>) at
../net/net.c:794
#8  0x000055955f82aba9 in virtio_net_flush_tx (q=0x0,
q@entry=0x5595631edbf0) at ../hw/net/virtio-net.c:2577
#9  0x000055955f82ade8 in virtio_net_tx_bh (opaque=0x5595631edbf0) at
../hw/net/virtio-net.c:2694
#10 0x000055955f9e847d in aio_bh_call (bh=0x559561c7e590) at ../util/async.c:169
#11 aio_bh_poll (ctx=ctx@entry=0x559561c81650) at ../util/async.c:169
#12 0x000055955f9d6912 in aio_dispatch (ctx=0x559561c81650) at
../util/aio-posix.c:381
#13 0x000055955f9e8322 in aio_ctx_dispatch (source=<optimized out>,
callback=<optimized out>, user_data=<optimized out>)
    at ../util/async.c:311
#14 0x00007fcf20a5495d in g_main_context_dispatch () from
/lib64/libglib-2.0.so.0
#15 0x000055955f9f2fc0 in glib_pollfds_poll () at ../util/main-loop.c:232
#16 os_host_main_loop_wait (timeout=<optimized out>) at ../util/main-loop.c:255
#17 main_loop_wait (nonblocking=nonblocking@entry=0) at ../util/main-loop.c:531
#18 0x000055955f7eee49 in qemu_main_loop () at ../softmmu/runstate.c:726
#19 0x000055955f6235c2 in main (argc=<optimized out>, argv=<optimized
out>, envp=<optimized out>) at ../softmmu/main.c:50

In nc_sendv_compat, nc->info is net_vhost_vdpa_info, so
nc->info->receive is NULL.

> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  include/hw/virtio/virtio.h |  2 ++
> >  hw/net/virtio-net.c        |  4 ++++
> >  hw/virtio/virtio.c         | 21 +++++++++++++--------
> >  3 files changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index 8bab9cfb75..1712ba0b4c 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -105,6 +105,8 @@ struct VirtIODevice
> >      VMChangeStateEntry *vmstate;
> >      char *bus_name;
> >      uint8_t device_endian;
> > +    /* backend does not support userspace handler */
> > +    bool disable_ioeventfd_handler;
> >      bool use_guest_notifier_mask;
> >      AddressSpace *dma_as;
> >      QLIST_HEAD(, VirtQueue) *vector_queues;
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 004acf858f..8c5c4e5a9d 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3501,6 +3501,10 @@ static void virtio_net_device_realize(DeviceState 
> > *dev, Error **errp)
> >      nc = qemu_get_queue(n->nic);
> >      nc->rxfilter_notify_enabled = 1;
> >
> > +    if (!nc->peer || nc->peer->info->type != NET_CLIENT_DRIVER_TAP) {
> > +        /* Only tap can use userspace networking */
> > +        vdev->disable_ioeventfd_handler = true;
> > +    }
> >      if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> >          struct virtio_net_config netcfg = {};
> >          memcpy(&netcfg.mac, &n->nic_conf.macaddr, ETH_ALEN);
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index ea7c079fb0..1e04db6650 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3734,17 +3734,22 @@ static int 
> > virtio_device_start_ioeventfd_impl(VirtIODevice *vdev)
> >              err = r;
> >              goto assign_error;
> >          }
> > -        event_notifier_set_handler(&vq->host_notifier,
> > -                                   virtio_queue_host_notifier_read);
> > +
> > +        if (!vdev->disable_ioeventfd_handler) {
> > +            event_notifier_set_handler(&vq->host_notifier,
> > +                                       virtio_queue_host_notifier_read);
>
> This is just about not responding to ioeventfd. Does this happen only
> when ioeventfd is enabled? If yes, we probably need a consistent way
> to deal with that. Will having a dummy receiver be more simpler?
>

If you mean NetClientInfo receiver, that would make qemu to actually
read from the virtqueue, I'm not sure if that is the right behavior
even for net devices. I see way simpler for qemu not to monitor
virtqueue kicks at all, isn't it?

net_vhost_user_info has a receiver to treat the special case of
reverse ARP. But I think vhost-user can't fall back to qemu userspace
networking at all.

But the crash is still reproducible with ioeventfd=off, so I need to
improve the patch either way.

Thanks!

> Thanks
>
> > +        }
> >      }
> >
> > -    for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
> > -        /* Kick right away to begin processing requests already in vring */
> > -        VirtQueue *vq = &vdev->vq[n];
> > -        if (!vq->vring.num) {
> > -            continue;
> > +    if (!vdev->disable_ioeventfd_handler) {
> > +        for (n = 0; n < VIRTIO_QUEUE_MAX; n++) {
> > +            /* Kick right away to begin processing requests already in 
> > vring */
> > +            VirtQueue *vq = &vdev->vq[n];
> > +            if (!vq->vring.num) {
> > +                continue;
> > +            }
> > +            event_notifier_set(&vq->host_notifier);
> >          }
> > -        event_notifier_set(&vq->host_notifier);
> >      }
> >      memory_region_transaction_commit();
> >      return 0;
> > --
> > 2.27.0
> >
>