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;
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));
+ 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));
+ 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.1On 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.
>
> 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.
> + 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
>
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?
-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
>>
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()
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
> >>
>
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?
virtio_pci_vector_unmask() has validation through virtio_queue_get_num()
that ensures the vq index is valid. While it doesn't seem possible for
VIRTIO_NET_F_CTRL_VQ to be cleared without device reset first, 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?
-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
>>>>
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.
>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
> >>>>
>
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
>>>>>>
>
© 2016 - 2025 Red Hat, Inc.