Re: [PATCH 4/7] virtio: don't read pending event on host notifier if disabled

[Jason Wang]

On Sat, Apr 2, 2022 at 4:37 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 3/31/2022 1:36 AM, Jason Wang wrote:
> > On Thu, Mar 31, 2022 at 12:41 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>
> >>
> >> On 3/30/2022 2:14 AM, Jason Wang wrote:
> >>> On Wed, Mar 30, 2022 at 2:33 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>>> Previous commit prevents vhost-user and vhost-vdpa from using
> >>>> userland vq handler via disable_ioeventfd_handler. The same
> >>>> needs to be done for host notifier cleanup too, as the
> >>>> virtio_queue_host_notifier_read handler still tends to read
> >>>> pending event left behind on ioeventfd and attempts to handle
> >>>> outstanding kicks from QEMU userland vq.
> >>>>
> >>>> If vq handler is not disabled on cleanup, it may lead to sigsegv
> >>>> with recursive virtio_net_set_status call on the control vq:
> >>>>
> >>>> 0  0x00007f8ce3ff3387 in raise () at /lib64/libc.so.6
> >>>> 1  0x00007f8ce3ff4a78 in abort () at /lib64/libc.so.6
> >>>> 2  0x00007f8ce3fec1a6 in __assert_fail_base () at /lib64/libc.so.6
> >>>> 3  0x00007f8ce3fec252 in  () at /lib64/libc.so.6
> >>>> 4  0x0000558f52d79421 in vhost_vdpa_get_vq_index (dev=<optimized out>, 
> >>>> idx=<optimized out>) at ../hw/virtio/vhost-vdpa.c:563
> >>>> 5  0x0000558f52d79421 in vhost_vdpa_get_vq_index (dev=<optimized out>, 
> >>>> idx=<optimized out>) at ../hw/virtio/vhost-vdpa.c:558
> >>>> 6  0x0000558f52d7329a in vhost_virtqueue_mask (hdev=0x558f55c01800, 
> >>>> vdev=0x558f568f91f0, n=2, mask=<optimized out>) at 
> >>>> ../hw/virtio/vhost.c:1557
> >>> I feel it's probably a bug elsewhere e.g when we fail to start
> >>> vhost-vDPA, it's the charge of the Qemu to poll host notifier and we
> >>> will fallback to the userspace vq handler.
> >> Apologies, an incorrect stack trace was pasted which actually came from
> >> patch #1. I will post a v2 with the corresponding one as below:
> >>
> >> 0  0x000055f800df1780 in qdev_get_parent_bus (dev=0x0) at
> >> ../hw/core/qdev.c:376
> >> 1  0x000055f800c68ad8 in virtio_bus_device_iommu_enabled
> >> (vdev=vdev@entry=0x0) at ../hw/virtio/virtio-bus.c:331
> >> 2  0x000055f800d70d7f in vhost_memory_unmap (dev=<optimized out>) at
> >> ../hw/virtio/vhost.c:318
> >> 3  0x000055f800d70d7f in vhost_memory_unmap (dev=<optimized out>,
> >> buffer=0x7fc19bec5240, len=2052, is_write=1, access_len=2052) at
> >> ../hw/virtio/vhost.c:336
> >> 4  0x000055f800d71867 in vhost_virtqueue_stop
> >> (dev=dev@entry=0x55f8037ccc30, vdev=vdev@entry=0x55f8044ec590,
> >> vq=0x55f8037cceb0, idx=0) at ../hw/virtio/vhost.c:1241
> >> 5  0x000055f800d7406c in vhost_dev_stop (hdev=hdev@entry=0x55f8037ccc30,
> >> vdev=vdev@entry=0x55f8044ec590) at ../hw/virtio/vhost.c:1839
> >> 6  0x000055f800bf00a7 in vhost_net_stop_one (net=0x55f8037ccc30,
> >> dev=0x55f8044ec590) at ../hw/net/vhost_net.c:315
> >> 7  0x000055f800bf0678 in vhost_net_stop (dev=dev@entry=0x55f8044ec590,
> >> ncs=0x55f80452bae0, data_queue_pairs=data_queue_pairs@entry=7,
> >> cvq=cvq@entry=1)
> >>      at ../hw/net/vhost_net.c:423
> >> 8  0x000055f800d4e628 in virtio_net_set_status (status=<optimized out>,
> >> n=0x55f8044ec590) at ../hw/net/virtio-net.c:296
> >> 9  0x000055f800d4e628 in virtio_net_set_status
> >> (vdev=vdev@entry=0x55f8044ec590, status=15 '\017') at
> >> ../hw/net/virtio-net.c:370
> > I don't understand why virtio_net_handle_ctrl() call 
> > virtio_net_set_stauts()...
> The pending request left over on the ctrl vq was a VIRTIO_NET_CTRL_MQ
> command, i.e. in virtio_net_handle_mq():

Completely forget that the code was actually written by me :\

>
> 1413     n->curr_queue_pairs = queue_pairs;
> 1414     /* stop the backend before changing the number of queue_pairs
> to avoid handling a
> 1415      * disabled queue */
> 1416     virtio_net_set_status(vdev, vdev->status);
> 1417     virtio_net_set_queue_pairs(n);
>
> Noted before the vdpa multiqueue support, there was never a vhost_dev
> for ctrl_vq exposed, i.e. there's no host notifier set up for the
> ctrl_vq on vhost_kernel as it is emulated in QEMU software.
>
> >
> >> 10 0x000055f800d534d8 in virtio_net_handle_ctrl (iov_cnt=<optimized
> >> out>, iov=<optimized out>, cmd=0 '\000', n=0x55f8044ec590) at
> >> ../hw/net/virtio-net.c:1408
> >> 11 0x000055f800d534d8 in virtio_net_handle_ctrl (vdev=0x55f8044ec590,
> >> vq=0x7fc1a7e888d0) at ../hw/net/virtio-net.c:1452
> >> 12 0x000055f800d69f37 in virtio_queue_host_notifier_read
> >> (vq=0x7fc1a7e888d0) at ../hw/virtio/virtio.c:2331
> >> 13 0x000055f800d69f37 in virtio_queue_host_notifier_read
> >> (n=n@entry=0x7fc1a7e8894c) at ../hw/virtio/virtio.c:3575
> >> 14 0x000055f800c688e6 in virtio_bus_cleanup_host_notifier
> >> (bus=<optimized out>, n=n@entry=14) at ../hw/virtio/virtio-bus.c:312
> >> 15 0x000055f800d73106 in vhost_dev_disable_notifiers
> >> (hdev=hdev@entry=0x55f8035b51b0, vdev=vdev@entry=0x55f8044ec590)
> >>      at ../../../include/hw/virtio/virtio-bus.h:35
> >> 16 0x000055f800bf00b2 in vhost_net_stop_one (net=0x55f8035b51b0,
> >> dev=0x55f8044ec590) at ../hw/net/vhost_net.c:316
> >> 17 0x000055f800bf0678 in vhost_net_stop (dev=dev@entry=0x55f8044ec590,
> >> ncs=0x55f80452bae0, data_queue_pairs=data_queue_pairs@entry=7,
> >> cvq=cvq@entry=1)
> >>      at ../hw/net/vhost_net.c:423
> >> 18 0x000055f800d4e628 in virtio_net_set_status (status=<optimized out>,
> >> n=0x55f8044ec590) at ../hw/net/virtio-net.c:296
> >> 19 0x000055f800d4e628 in virtio_net_set_status (vdev=0x55f8044ec590,
> >> status=15 '\017') at ../hw/net/virtio-net.c:370
> >> 20 0x000055f800d6c4b2 in virtio_set_status (vdev=0x55f8044ec590,
> >> val=<optimized out>) at ../hw/virtio/virtio.c:1945
> >> 21 0x000055f800d11d9d in vm_state_notify (running=running@entry=false,
> >> state=state@entry=RUN_STATE_SHUTDOWN) at ../softmmu/runstate.c:333
> >> 22 0x000055f800d04e7a in do_vm_stop
> >> (state=state@entry=RUN_STATE_SHUTDOWN, send_stop=send_stop@entry=false)
> >> at ../softmmu/cpus.c:262
> >> 23 0x000055f800d04e99 in vm_shutdown () at ../softmmu/cpus.c:280
> >> 24 0x000055f800d126af in qemu_cleanup () at ../softmmu/runstate.c:812
> >> 25 0x000055f800ad5b13 in main (argc=<optimized out>, argv=<optimized
> >> out>, envp=<optimized out>) at ../softmmu/main.c:51
> >>
> >>   From the trace pending read only occurs in stop path. The recursive
> >> virtio_net_set_status from virtio_net_handle_ctrl doesn't make sense to me.
> > Yes, we need to figure this out to know the root cause.
> I think it has something to do with the virtqueue unready issue that the
> vhost_reset_device() refactoring series attempt to fix. If that is fixed
> we should not see this sigsegv with mlx5_vdpa. However I guess we both
> agreed that the vq_unready support would need new uAPI (some flag) to
> define, hence this fix applies to the situation where uAPI doesn't exist
> on the kernel or the vq_unready is not supported by vdpa vendor driver.
>

Yes.

> >
> > The code should work for the case when vhost-vdp fails to start.
> Unlike the other datapath queues for net vdpa, the events left behind in
> the control queue can't be processed by the userspace, as unlike
> vhost-kernel we don't have a fallback path in the userspace.

So that's the question, we should have a safe fallback.

> To ignore
> the pending event and let vhost vdpa process it on resume/start is
> perhaps the best thing to do. This is even true for datapath queues for
> other vdpa devices than of network.
>
> >
> >> Not sure I got the reason why we need to handle pending host
> >> notification in userland vq, can you elaborate?
> > Because vhost-vDPA fails to start, we will "fallback" to a dummy userspace.
> Is the dummy userspace working or operational? What's the use case of
> this "fallback" dummy if what guest user can get is a busted NIC?

The problem is we can't do better in this case now. Such fallack (e.g
for vhost-user) has been used for years. Or do you have any better
ideas?

It doesn't differ too much of the two approaches:

1) dummy fallback which can do even cvq

and

2) disable host notifiers

Especially consider 2) requires more changes.

> I
> think this is very different from the vhost-kernel case in that once
> vhost fails, we can fallback to userspace to emulate the network through
> the tap fd in a good way. However, there's no equivalent yet for
> vhost-vdpa...
>

As said previously, technically we can have vhost-vDPA network backend
as a fallback. (So did for vhost-user).

Thanks

> Thanks,
> -Siwei
>
> >
> > Thanks
> >
> >> Thanks,
> >> -Siwei
> >>
> >>> Thanks
> >>>
> >>>> 7  0x0000558f52c6b89a in virtio_pci_set_guest_notifier 
> >>>> (d=d@entry=0x558f568f0f60, n=n@entry=2, assign=assign@entry=true, 
> >>>> with_irqfd=with_irqfd@entry=false)
> >>>>      at ../hw/virtio/virtio-pci.c:974
> >>>> 8  0x0000558f52c6c0d8 in virtio_pci_set_guest_notifiers 
> >>>> (d=0x558f568f0f60, nvqs=3, assign=true) at ../hw/virtio/virtio-pci.c:1019
> >>>> 9  0x0000558f52bf091d in vhost_net_start (dev=dev@entry=0x558f568f91f0, 
> >>>> ncs=0x558f56937cd0, data_queue_pairs=data_queue_pairs@entry=1, 
> >>>> cvq=cvq@entry=1)
> >>>>      at ../hw/net/vhost_net.c:361
> >>>> 10 0x0000558f52d4e5e7 in virtio_net_set_status (status=<optimized out>, 
> >>>> n=0x558f568f91f0) at ../hw/net/virtio-net.c:289
> >>>> 11 0x0000558f52d4e5e7 in virtio_net_set_status (vdev=0x558f568f91f0, 
> >>>> status=15 '\017') at ../hw/net/virtio-net.c:370
> >>>> 12 0x0000558f52d6c4b2 in virtio_set_status 
> >>>> (vdev=vdev@entry=0x558f568f91f0, val=val@entry=15 '\017') at 
> >>>> ../hw/virtio/virtio.c:1945
> >>>> 13 0x0000558f52c69eff in virtio_pci_common_write (opaque=0x558f568f0f60, 
> >>>> addr=<optimized out>, val=<optimized out>, size=<optimized out>) at 
> >>>> ../hw/virtio/virtio-pci.c:1292
> >>>> 14 0x0000558f52d15d6e in memory_region_write_accessor 
> >>>> (mr=0x558f568f19d0, addr=20, value=<optimized out>, size=1, 
> >>>> shift=<optimized out>, mask=<optimized out>, attrs=...)
> >>>>      at ../softmmu/memory.c:492
> >>>> 15 0x0000558f52d127de in access_with_adjusted_size (addr=addr@entry=20, 
> >>>> value=value@entry=0x7f8cdbffe748, size=size@entry=1, 
> >>>> access_size_min=<optimized out>, access_size_max=<optimized out>, 
> >>>> access_fn=0x558f52d15cf0 <memory_region_write_accessor>, 
> >>>> mr=0x558f568f19d0, attrs=...) at ../softmmu/memory.c:554
> >>>> 16 0x0000558f52d157ef in memory_region_dispatch_write 
> >>>> (mr=mr@entry=0x558f568f19d0, addr=20, data=<optimized out>, 
> >>>> op=<optimized out>, attrs=attrs@entry=...)
> >>>>      at ../softmmu/memory.c:1504
> >>>> 17 0x0000558f52d078e7 in flatview_write_continue 
> >>>> (fv=fv@entry=0x7f8accbc3b90, addr=addr@entry=103079215124, attrs=..., 
> >>>> ptr=ptr@entry=0x7f8ce6300028, len=len@entry=1, addr1=<optimized out>, 
> >>>> l=<optimized out>, mr=0x558f568f19d0) at 
> >>>> ../../../include/qemu/host-utils.h:165
> >>>> 18 0x0000558f52d07b06 in flatview_write (fv=0x7f8accbc3b90, 
> >>>> addr=103079215124, attrs=..., buf=0x7f8ce6300028, len=1) at 
> >>>> ../softmmu/physmem.c:2822
> >>>> 19 0x0000558f52d0b36b in address_space_write (as=<optimized out>, 
> >>>> addr=<optimized out>, attrs=..., buf=buf@entry=0x7f8ce6300028, 
> >>>> len=<optimized out>)
> >>>>      at ../softmmu/physmem.c:2914
> >>>> 20 0x0000558f52d0b3da in address_space_rw (as=<optimized out>, 
> >>>> addr=<optimized out>, attrs=...,
> >>>>      attrs@entry=..., buf=buf@entry=0x7f8ce6300028, len=<optimized out>, 
> >>>> is_write=<optimized out>) at ../softmmu/physmem.c:2924
> >>>> 21 0x0000558f52dced09 in kvm_cpu_exec (cpu=cpu@entry=0x558f55c2da60) at 
> >>>> ../accel/kvm/kvm-all.c:2903
> >>>> 22 0x0000558f52dcfabd in kvm_vcpu_thread_fn 
> >>>> (arg=arg@entry=0x558f55c2da60) at ../accel/kvm/kvm-accel-ops.c:49
> >>>> 23 0x0000558f52f9f04a in qemu_thread_start (args=<optimized out>) at 
> >>>> ../util/qemu-thread-posix.c:556
> >>>> 24 0x00007f8ce4392ea5 in start_thread () at /lib64/libpthread.so.0
> >>>> 25 0x00007f8ce40bb9fd in clone () at /lib64/libc.so.6
> >>>>
> >>>> Fixes: 4023784 ("vhost-vdpa: multiqueue support")
> >>>> Cc: Jason Wang <jasowang@redhat.com>
> >>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >>>> ---
> >>>>    hw/virtio/virtio-bus.c | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> >>>> index 0f69d1c..3159b58 100644
> >>>> --- a/hw/virtio/virtio-bus.c
> >>>> +++ b/hw/virtio/virtio-bus.c
> >>>> @@ -311,7 +311,8 @@ void virtio_bus_cleanup_host_notifier(VirtioBusState 
> >>>> *bus, int n)
> >>>>        /* Test and clear notifier after disabling event,
> >>>>         * in case poll callback didn't have time to run.
> >>>>         */
> >>>> -    virtio_queue_host_notifier_read(notifier);
> >>>> +    if (!vdev->disable_ioeventfd_handler)
> >>>> +        virtio_queue_host_notifier_read(notifier);
> >>>>        event_notifier_cleanup(notifier);
> >>>>    }
> >>>>
> >>>> --
> >>>> 1.8.3.1
> >>>>
>