Re: [PATCH 1/7] virtio-net: align ctrl_vq index for non-mq guest for vhost_vdpa

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 4/1/2022 7:10 PM, Jason Wang wrote:
> On Sat, Apr 2, 2022 at 6:32 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 3/31/2022 1:39 AM, Jason Wang wrote:
>>> On Wed, Mar 30, 2022 at 11:48 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>>
>>>> On 3/30/2022 2:00 AM, Jason Wang wrote:
>>>>> On Wed, Mar 30, 2022 at 2:33 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>>>> With MQ enabled vdpa device and non-MQ supporting guest e.g.
>>>>>> booting vdpa with mq=on over OVMF of single vqp, below assert
>>>>>> failure is seen:
>>>>>>
>>>>>> ../hw/virtio/vhost-vdpa.c:560: vhost_vdpa_get_vq_index: Assertion `idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs' failed.
>>>>>>
>>>>>> 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
>>>>>> 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 /home/opc/qemu-upstream/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
>>>>>>
>>>>>> The cause for the assert failure is due to that the vhost_dev index
>>>>>> for the ctrl vq was not aligned with actual one in use by the guest.
>>>>>> Upon multiqueue feature negotiation in virtio_net_set_multiqueue(),
>>>>>> if guest doesn't support multiqueue, the guest vq layout would shrink
>>>>>> to a single queue pair, consisting of 3 vqs in total (rx, tx and ctrl).
>>>>>> This results in ctrl_vq taking a different vhost_dev group index than
>>>>>> the default. We can map vq to the correct vhost_dev group by checking
>>>>>> if MQ is supported by guest and successfully negotiated. Since the
>>>>>> MQ feature is only present along with CTRL_VQ, we make sure the index
>>>>>> 2 is only meant for the control vq while MQ is not supported by guest.
>>>>>>
>>>>>> Be noted if QEMU or guest doesn't support control vq, there's no bother
>>>>>> exposing vhost_dev and guest notifier for the control vq. Since
>>>>>> vhost_net_start/stop implies DRIVER_OK is set in device status, feature
>>>>>> negotiation should be completed when reaching virtio_net_vhost_status().
>>>>>>
>>>>>> Fixes: 22288fe ("virtio-net: vhost control virtqueue support")
>>>>>> Suggested-by: Jason Wang <jasowang@redhat.com>
>>>>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>>>> ---
>>>>>>     hw/net/virtio-net.c | 19 ++++++++++++++++---
>>>>>>     1 file changed, 16 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index 1067e72..484b215 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -245,7 +245,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>>>>         VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>>>>         NetClientState *nc = qemu_get_queue(n->nic);
>>>>>>         int queue_pairs = n->multiqueue ? n->max_queue_pairs : 1;
>>>>>> -    int cvq = n->max_ncs - n->max_queue_pairs;
>>>>>> +    int cvq = virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) ?
>>>>>> +              n->max_ncs - n->max_queue_pairs : 0;
>>>>> Let's use a separate patch for this.
>>>> Yes, I can do that. Then the new patch will become a requisite for this
>>>> patch.
>>>>
>>>>>>         if (!get_vhost_net(nc->peer)) {
>>>>>>             return;
>>>>>> @@ -3170,8 +3171,14 @@ static NetClientInfo net_virtio_info = {
>>>>>>     static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
>>>>>>     {
>>>>>>         VirtIONet *n = VIRTIO_NET(vdev);
>>>>>> -    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(idx));
>>>>>> +    NetClientState *nc;
>>>>>>         assert(n->vhost_started);
>>>>>> +    if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_MQ) && idx == 2) {
>>>>>> +        assert(virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ));
>>>>> This assert seems guest trigger-able. If yes, I would remove this or
>>>>> replace it with log_guest_error.
>>>> This assert actually is relevant to the cvq change in
>>>> virtio_net_vhost_status(). Since the same check on VIRTIO_NET_F_CTRL_VQ
>>>> has been done earlier, it is assured that CTRL_VQ is negotiated when
>>>> getting here.
>>>> Noted the vhost_started is asserted in the same function, which in turn
>>>> implies DRIVER_OK is set meaning feature negotiation is complete. I
>>>> can't easily think of a scenario which guest may inadvertently or
>>>> purposely trigger the assert?
>>> So the code can be triggered like e.g unmasking:
>>>
>>> virtio_pci_vq_vector_unmask()
>>>           k->guest_notifier_pending()
>> Hmmm, are you concerned more about idx being invalid, or
>> VIRTIO_NET_F_CTRL_VQ getting cleared?
> Something like this, we can't let a buggy driver crash into Qemu.
>
>> virtio_pci_vector_unmask() has validation through virtio_queue_get_num()
>> that ensures the vq index is valid.
> Actually not, it just check whether the vq size is set:
>
> int virtio_queue_get_num(VirtIODevice *vdev, int n)
> {
>      return vdev->vq[n].vring.num;
> }
>
>> While it doesn't seem possible for
>> VIRTIO_NET_F_CTRL_VQ to be cleared without device reset first,
> Probably, since we had a check in virtio_set_features():
>
>      /*
>       * The driver must not attempt to set features after feature negotiation
>       * has finished.
>       */
>      if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) {
>          return -EINVAL;
>      }
>
> But another interesting part is that the guest_feautres come from the
> migration stream as well:
>
> static const VMStateDescription vmstate_virtio_64bit_features = {
>      .name = "virtio/64bit_features",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = &virtio_64bit_features_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64(guest_features, VirtIODevice),
>          VMSTATE_END_OF_LIST()
>      }
> };
>
> We should also be ready to let the buggy migration flow to crash us.
Fair enough. Given the possibility of introduction through migration 
stream I think now it's needed to converting assert to error. Thanks for 
pointing it out.

Thanks,
-Siwei

>
>> during
>> which the pending event left over on guest notifier eventfd should have
>> been completed within virtio_pci_set_guest_notifiers(false) before
>> vhost_net_stop() returns. If I am not missing something here, I guess
>> we're probably fine?
> I'm not sure I got here, but the mask/unmask is not necessarily
> related to vhost stop. E.g it can happen if guest want to change IRQ
> affinity.
>
> Thanks
>
>> -Siwei
>>
>>> Thanks
>>>
>>>
>>>> -Siwei
>>>>
>>>>>> +        nc = qemu_get_subqueue(n->nic, n->max_queue_pairs);
>>>>>> +    } else {
>>>>>> +        nc = qemu_get_subqueue(n->nic, vq2q(idx));
>>>>>> +    }
>>>>>>         return vhost_net_virtqueue_pending(get_vhost_net(nc->peer), idx);
>>>>>>     }
>>>>>>
>>>>>> @@ -3179,8 +3186,14 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>>>                                                bool mask)
>>>>>>     {
>>>>>>         VirtIONet *n = VIRTIO_NET(vdev);
>>>>>> -    NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(idx));
>>>>>> +    NetClientState *nc;
>>>>>>         assert(n->vhost_started);
>>>>>> +    if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_MQ) && idx == 2) {
>>>>>> +        assert(virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ));
>>>>> And this.
>>>>>
>>>>> Thanks
>>>>>
>>>>>
>>>>>> +        nc = qemu_get_subqueue(n->nic, n->max_queue_pairs);
>>>>>> +    } else {
>>>>>> +        nc = qemu_get_subqueue(n->nic, vq2q(idx));
>>>>>> +    }
>>>>>>         vhost_net_virtqueue_mask(get_vhost_net(nc->peer),
>>>>>>                                  vdev, idx, mask);
>>>>>>     }
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>