hw/net/vhost_net.c | 13 +++++++++---- hw/virtio/vhost-vdpa.c | 25 ++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 5 deletions(-)
With MQ enabled vdpa device and non-MQ supporting guest e.g.
booting vdpa with mq=on over OVMF of single vqp, it's easy
to hit assert failure as the following:
../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 single queue pair of 3 vqs in total (rx, tx and ctrl). This results
in ctrl_vq taking a different vhost_dev group index than the default
n->max_queue_pairs, the latter of which is only valid for multiqueue
guest. While on those additional vqs not exposed to the guest,
vhost_net_set_vq_index() never populated vq_index properly, hence
getting the assert failure.
A possible fix is to pick the correct vhost_dev group for the control
vq according to this table [*]:
vdpa tool / QEMU arg / guest config / ctrl_vq group index
----------------------------------------------------------------
max_vqp 8 / mq=on / mq=off (UEFI) => data_queue_pairs
max_vqp 8 / mq=on / mq=on (Linux) => n->max_queue_pairs(>1)
max_vqp 8 / mq=off / mq=on (Linux) => n->max_queue_pairs(=1)
[*] Please see FIXME in the code for open question and discussion
Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
hw/net/vhost_net.c | 13 +++++++++----
hw/virtio/vhost-vdpa.c | 25 ++++++++++++++++++++++++-
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 30379d2..9a4479b 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -322,6 +322,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
VirtioBusState *vbus = VIRTIO_BUS(qbus);
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
+ bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
int total_notifiers = data_queue_pairs * 2 + cvq;
VirtIONet *n = VIRTIO_NET(dev);
int nvhosts = data_queue_pairs + cvq;
@@ -343,7 +344,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
if (i < data_queue_pairs) {
peer = qemu_get_peer(ncs, i);
} else { /* Control Virtqueue */
- peer = qemu_get_peer(ncs, n->max_queue_pairs);
+ peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
}
net = get_vhost_net(peer);
@@ -368,7 +369,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
if (i < data_queue_pairs) {
peer = qemu_get_peer(ncs, i);
} else {
- peer = qemu_get_peer(ncs, n->max_queue_pairs);
+ peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
}
r = vhost_net_start_one(get_vhost_net(peer), dev);
@@ -390,7 +391,10 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
err_start:
while (--i >= 0) {
- peer = qemu_get_peer(ncs , i);
+ if (mq)
+ peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : data_queue_pairs);
+ else
+ peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : n->max_queue_pairs);
vhost_net_stop_one(get_vhost_net(peer), dev);
}
e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
@@ -409,6 +413,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
VirtioBusState *vbus = VIRTIO_BUS(qbus);
VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
VirtIONet *n = VIRTIO_NET(dev);
+ bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
NetClientState *peer;
int total_notifiers = data_queue_pairs * 2 + cvq;
int nvhosts = data_queue_pairs + cvq;
@@ -418,7 +423,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
if (i < data_queue_pairs) {
peer = qemu_get_peer(ncs, i);
} else {
- peer = qemu_get_peer(ncs, n->max_queue_pairs);
+ peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
}
vhost_net_stop_one(get_vhost_net(peer), dev);
}
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 27ea706..623476e 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -1097,7 +1097,30 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
}
- if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
+ /* FIXME the vhost_dev group for the control vq may have bogus nvqs=2
+ * value rather than nvqs=1. This can happen in case the guest doesn't
+ * support multiqueue, as a result of virtio_net_change_num_queue_pairs()
+ * destroying and rebuilding all the vqs, the guest index for control vq
+ * will no longer align with the host's. Currently net_init_vhost_vdpa()
+ * only initializes all vhost_dev's and net_clients once during
+ * net_client_init1() time, way earlier before multiqueue feature
+ * negotiation can kick in.
+ *
+ * Discussion - some possible fixes so far I can think of:
+ *
+ * option 1: fix vhost_net->dev.nvqs and nc->is_datapath in place for
+ * vdpa's ctrl vq, or rebuild all vdpa's vhost_dev groups and the
+ * net_client array, in the virtio_net_set_multiqueue() path;
+ *
+ * option 2: fix vhost_dev->nvqs in place at vhost_vdpa_set_features()
+ * before coming down to vhost_vdpa_dev_start() (Q: nc->is_datapath
+ * seems only used in virtio_net_device_realize, is it relevant?);
+ *
+ * option 3: use host queue index all along in vhost-vdpa ioctls instead
+ * of using guest vq index, so that vhost_net_start/stop() can remain
+ * as-is today
+ */
+ if (dev->vq_index + dev->nvqs < dev->vq_index_end) {
return 0;
}
--
1.8.3.1On Sat, Mar 19, 2022 at 12:14 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, it's easy
> to hit assert failure as the following:
>
> ../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 single queue pair of 3 vqs in total (rx, tx and ctrl). This results
> in ctrl_vq taking a different vhost_dev group index than the default
> n->max_queue_pairs, the latter of which is only valid for multiqueue
> guest. While on those additional vqs not exposed to the guest,
> vhost_net_set_vq_index() never populated vq_index properly, hence
> getting the assert failure.
>
> A possible fix is to pick the correct vhost_dev group for the control
> vq according to this table [*]:
>
> vdpa tool / QEMU arg / guest config / ctrl_vq group index
> ----------------------------------------------------------------
> max_vqp 8 / mq=on / mq=off (UEFI) => data_queue_pairs
> max_vqp 8 / mq=on / mq=on (Linux) => n->max_queue_pairs(>1)
> max_vqp 8 / mq=off / mq=on (Linux) => n->max_queue_pairs(=1)
>
> [*] Please see FIXME in the code for open question and discussion
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
> hw/net/vhost_net.c | 13 +++++++++----
> hw/virtio/vhost-vdpa.c | 25 ++++++++++++++++++++++++-
> 2 files changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 30379d2..9a4479b 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -322,6 +322,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
> VirtioBusState *vbus = VIRTIO_BUS(qbus);
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
> int total_notifiers = data_queue_pairs * 2 + cvq;
> VirtIONet *n = VIRTIO_NET(dev);
> int nvhosts = data_queue_pairs + cvq;
> @@ -343,7 +344,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> if (i < data_queue_pairs) {
> peer = qemu_get_peer(ncs, i);
> } else { /* Control Virtqueue */
> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
> }
>
> net = get_vhost_net(peer);
> @@ -368,7 +369,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> if (i < data_queue_pairs) {
> peer = qemu_get_peer(ncs, i);
> } else {
> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
> }
> r = vhost_net_start_one(get_vhost_net(peer), dev);
>
> @@ -390,7 +391,10 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>
> err_start:
> while (--i >= 0) {
> - peer = qemu_get_peer(ncs , i);
> + if (mq)
> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : data_queue_pairs);
> + else
> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : n->max_queue_pairs);
> vhost_net_stop_one(get_vhost_net(peer), dev);
> }
> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
> @@ -409,6 +413,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> VirtioBusState *vbus = VIRTIO_BUS(qbus);
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> VirtIONet *n = VIRTIO_NET(dev);
> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
> NetClientState *peer;
> int total_notifiers = data_queue_pairs * 2 + cvq;
> int nvhosts = data_queue_pairs + cvq;
> @@ -418,7 +423,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> if (i < data_queue_pairs) {
> peer = qemu_get_peer(ncs, i);
> } else {
> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
> }
> vhost_net_stop_one(get_vhost_net(peer), dev);
> }
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 27ea706..623476e 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -1097,7 +1097,30 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
> }
>
> - if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
> + /* FIXME the vhost_dev group for the control vq may have bogus nvqs=2
> + * value rather than nvqs=1. This can happen in case the guest doesn't
> + * support multiqueue, as a result of virtio_net_change_num_queue_pairs()
> + * destroying and rebuilding all the vqs, the guest index for control vq
> + * will no longer align with the host's. Currently net_init_vhost_vdpa()
> + * only initializes all vhost_dev's and net_clients once during
> + * net_client_init1() time, way earlier before multiqueue feature
> + * negotiation can kick in.
See below, it looks like the code doesn't find the correct vhost_dev.
> + *
> + * Discussion - some possible fixes so far I can think of:
> + *
> + * option 1: fix vhost_net->dev.nvqs and nc->is_datapath in place for
> + * vdpa's ctrl vq, or rebuild all vdpa's vhost_dev groups and the
> + * net_client array, in the virtio_net_set_multiqueue() path;
> + *
> + * option 2: fix vhost_dev->nvqs in place at vhost_vdpa_set_features()
> + * before coming down to vhost_vdpa_dev_start() (Q: nc->is_datapath
> + * seems only used in virtio_net_device_realize, is it relevant?);
Relevant but not directly related, for the vhost_dev where
nc->is_datapath is false, it will assume it is backed by a single
queue not a queue pair.
> + *
> + * option 3: use host queue index all along in vhost-vdpa ioctls instead
> + * of using guest vq index, so that vhost_net_start/stop() can remain
> + * as-is today
> + */
Note that the vq_index of each vhost_dev is assigned during
vhost_net_start() according to whether or not the MQ or CVQ is
negotiated in vhost_net_start()
for (i = 0; i < nvhosts; i++) {
if (i < data_queue_pairs) {
peer = qemu_get_peer(ncs, i);
} else { /* Control Virtqueue */
peer = qemu_get_peer(ncs, n->max_queue_pairs);
}
net = get_vhost_net(peer);
vhost_net_set_vq_index(net, i * 2, index_end);
It means some of the peers won't be used when MQ is not negotiated. So
it looks to me the evil came from virtio_net_get_notifier_mask().
Where it doesn't mask the correct vhost dev when the guest doesn't
support MQ but the host does. So we had option 4:
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 2087516253..5e9ac019cd 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3179,7 +3179,13 @@ 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;
+
+ if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_MQ) && idx == 2) {
+ nc = qemu_get_subqueue(n->nic, n->max_queue_pairs);
+ } else {
+ nc = qemu_get_subqueue(n->nic, vq2q(idx));
+ }
assert(n->vhost_started);
vhost_net_virtqueue_mask(get_vhost_net(nc->peer),
vdev, idx, mask);
Thanks
> + if (dev->vq_index + dev->nvqs < dev->vq_index_end) {
> return 0;
> }
>
> --
> 1.8.3.1
>
On 3/21/2022 8:47 PM, Jason Wang wrote:
> On Sat, Mar 19, 2022 at 12:14 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, it's easy
>> to hit assert failure as the following:
>>
>> ../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 single queue pair of 3 vqs in total (rx, tx and ctrl). This results
>> in ctrl_vq taking a different vhost_dev group index than the default
>> n->max_queue_pairs, the latter of which is only valid for multiqueue
>> guest. While on those additional vqs not exposed to the guest,
>> vhost_net_set_vq_index() never populated vq_index properly, hence
>> getting the assert failure.
>>
>> A possible fix is to pick the correct vhost_dev group for the control
>> vq according to this table [*]:
>>
>> vdpa tool / QEMU arg / guest config / ctrl_vq group index
>> ----------------------------------------------------------------
>> max_vqp 8 / mq=on / mq=off (UEFI) => data_queue_pairs
>> max_vqp 8 / mq=on / mq=on (Linux) => n->max_queue_pairs(>1)
>> max_vqp 8 / mq=off / mq=on (Linux) => n->max_queue_pairs(=1)
>>
>> [*] Please see FIXME in the code for open question and discussion
>>
>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>> ---
>> hw/net/vhost_net.c | 13 +++++++++----
>> hw/virtio/vhost-vdpa.c | 25 ++++++++++++++++++++++++-
>> 2 files changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 30379d2..9a4479b 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -322,6 +322,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
>> VirtioBusState *vbus = VIRTIO_BUS(qbus);
>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
>> int total_notifiers = data_queue_pairs * 2 + cvq;
>> VirtIONet *n = VIRTIO_NET(dev);
>> int nvhosts = data_queue_pairs + cvq;
>> @@ -343,7 +344,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>> if (i < data_queue_pairs) {
>> peer = qemu_get_peer(ncs, i);
>> } else { /* Control Virtqueue */
>> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
>> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
>> }
>>
>> net = get_vhost_net(peer);
>> @@ -368,7 +369,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>> if (i < data_queue_pairs) {
>> peer = qemu_get_peer(ncs, i);
>> } else {
>> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
>> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
>> }
>> r = vhost_net_start_one(get_vhost_net(peer), dev);
>>
>> @@ -390,7 +391,10 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>
>> err_start:
>> while (--i >= 0) {
>> - peer = qemu_get_peer(ncs , i);
>> + if (mq)
>> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : data_queue_pairs);
>> + else
>> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : n->max_queue_pairs);
>> vhost_net_stop_one(get_vhost_net(peer), dev);
>> }
>> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>> @@ -409,6 +413,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>> VirtioBusState *vbus = VIRTIO_BUS(qbus);
>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>> VirtIONet *n = VIRTIO_NET(dev);
>> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
>> NetClientState *peer;
>> int total_notifiers = data_queue_pairs * 2 + cvq;
>> int nvhosts = data_queue_pairs + cvq;
>> @@ -418,7 +423,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>> if (i < data_queue_pairs) {
>> peer = qemu_get_peer(ncs, i);
>> } else {
>> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
>> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
>> }
>> vhost_net_stop_one(get_vhost_net(peer), dev);
>> }
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 27ea706..623476e 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -1097,7 +1097,30 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>> vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>> }
>>
>> - if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
>> + /* FIXME the vhost_dev group for the control vq may have bogus nvqs=2
>> + * value rather than nvqs=1. This can happen in case the guest doesn't
>> + * support multiqueue, as a result of virtio_net_change_num_queue_pairs()
>> + * destroying and rebuilding all the vqs, the guest index for control vq
>> + * will no longer align with the host's. Currently net_init_vhost_vdpa()
>> + * only initializes all vhost_dev's and net_clients once during
>> + * net_client_init1() time, way earlier before multiqueue feature
>> + * negotiation can kick in.
> See below, it looks like the code doesn't find the correct vhost_dev.
>
>> + *
>> + * Discussion - some possible fixes so far I can think of:
>> + *
>> + * option 1: fix vhost_net->dev.nvqs and nc->is_datapath in place for
>> + * vdpa's ctrl vq, or rebuild all vdpa's vhost_dev groups and the
>> + * net_client array, in the virtio_net_set_multiqueue() path;
>> + *
>> + * option 2: fix vhost_dev->nvqs in place at vhost_vdpa_set_features()
>> + * before coming down to vhost_vdpa_dev_start() (Q: nc->is_datapath
>> + * seems only used in virtio_net_device_realize, is it relevant?);
> Relevant but not directly related, for the vhost_dev where
> nc->is_datapath is false, it will assume it is backed by a single
> queue not a queue pair.
>
>> + *
>> + * option 3: use host queue index all along in vhost-vdpa ioctls instead
>> + * of using guest vq index, so that vhost_net_start/stop() can remain
>> + * as-is today
>> + */
> Note that the vq_index of each vhost_dev is assigned during
> vhost_net_start() according to whether or not the MQ or CVQ is
> negotiated in vhost_net_start()
>
> for (i = 0; i < nvhosts; i++) {
>
> if (i < data_queue_pairs) {
> peer = qemu_get_peer(ncs, i);
> } else { /* Control Virtqueue */
> peer = qemu_get_peer(ncs, n->max_queue_pairs);
> }
>
> net = get_vhost_net(peer);
> vhost_net_set_vq_index(net, i * 2, index_end);
>
> It means some of the peers won't be used when MQ is not negotiated. So
> it looks to me the evil came from virtio_net_get_notifier_mask().
Yes, there it is. Where the control virtqueue first ever needs a
guest_notifier for vhost_dev.
> Where it doesn't mask the correct vhost dev when the guest doesn't
> support MQ but the host does. So we had option 4:
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 2087516253..5e9ac019cd 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -3179,7 +3179,13 @@ 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;
> +
> + if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_MQ) && idx == 2) {
Hmmm, I thought it would be more natural to align the layout of
vhost_dev's with that of virtqueue's, not the other way around. Not sure
how this vhost_dev selection scheme may work with additional queues
discovered through transport specific mechanism, such as the admin
virtqueue, but I can live with it for now:
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -244,7 +244,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;
@@ -3161,8 +3162,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);
}
@@ -3170,8 +3177,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);
}
Thanks,
-Siwei
> + nc = qemu_get_subqueue(n->nic, n->max_queue_pairs);
> + } else {
> + nc = qemu_get_subqueue(n->nic, vq2q(idx));
> + }
> assert(n->vhost_started);
> vhost_net_virtqueue_mask(get_vhost_net(nc->peer),
> vdev, idx, mask);
>
> Thanks
>
>> + if (dev->vq_index + dev->nvqs < dev->vq_index_end) {
>> return 0;
>> }
>>
>> --
>> 1.8.3.1
>>
On Fri, Mar 25, 2022 at 3:02 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 3/21/2022 8:47 PM, Jason Wang wrote:
> > On Sat, Mar 19, 2022 at 12:14 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, it's easy
> >> to hit assert failure as the following:
> >>
> >> ../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 single queue pair of 3 vqs in total (rx, tx and ctrl). This results
> >> in ctrl_vq taking a different vhost_dev group index than the default
> >> n->max_queue_pairs, the latter of which is only valid for multiqueue
> >> guest. While on those additional vqs not exposed to the guest,
> >> vhost_net_set_vq_index() never populated vq_index properly, hence
> >> getting the assert failure.
> >>
> >> A possible fix is to pick the correct vhost_dev group for the control
> >> vq according to this table [*]:
> >>
> >> vdpa tool / QEMU arg / guest config / ctrl_vq group index
> >> ----------------------------------------------------------------
> >> max_vqp 8 / mq=on / mq=off (UEFI) => data_queue_pairs
> >> max_vqp 8 / mq=on / mq=on (Linux) => n->max_queue_pairs(>1)
> >> max_vqp 8 / mq=off / mq=on (Linux) => n->max_queue_pairs(=1)
> >>
> >> [*] Please see FIXME in the code for open question and discussion
> >>
> >> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >> ---
> >> hw/net/vhost_net.c | 13 +++++++++----
> >> hw/virtio/vhost-vdpa.c | 25 ++++++++++++++++++++++++-
> >> 2 files changed, 33 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >> index 30379d2..9a4479b 100644
> >> --- a/hw/net/vhost_net.c
> >> +++ b/hw/net/vhost_net.c
> >> @@ -322,6 +322,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
> >> VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> >> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
> >> int total_notifiers = data_queue_pairs * 2 + cvq;
> >> VirtIONet *n = VIRTIO_NET(dev);
> >> int nvhosts = data_queue_pairs + cvq;
> >> @@ -343,7 +344,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >> if (i < data_queue_pairs) {
> >> peer = qemu_get_peer(ncs, i);
> >> } else { /* Control Virtqueue */
> >> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
> >> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
> >> }
> >>
> >> net = get_vhost_net(peer);
> >> @@ -368,7 +369,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >> if (i < data_queue_pairs) {
> >> peer = qemu_get_peer(ncs, i);
> >> } else {
> >> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
> >> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
> >> }
> >> r = vhost_net_start_one(get_vhost_net(peer), dev);
> >>
> >> @@ -390,7 +391,10 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
> >>
> >> err_start:
> >> while (--i >= 0) {
> >> - peer = qemu_get_peer(ncs , i);
> >> + if (mq)
> >> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : data_queue_pairs);
> >> + else
> >> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : n->max_queue_pairs);
> >> vhost_net_stop_one(get_vhost_net(peer), dev);
> >> }
> >> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
> >> @@ -409,6 +413,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> >> VirtioBusState *vbus = VIRTIO_BUS(qbus);
> >> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> >> VirtIONet *n = VIRTIO_NET(dev);
> >> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
> >> NetClientState *peer;
> >> int total_notifiers = data_queue_pairs * 2 + cvq;
> >> int nvhosts = data_queue_pairs + cvq;
> >> @@ -418,7 +423,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
> >> if (i < data_queue_pairs) {
> >> peer = qemu_get_peer(ncs, i);
> >> } else {
> >> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
> >> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
> >> }
> >> vhost_net_stop_one(get_vhost_net(peer), dev);
> >> }
> >> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >> index 27ea706..623476e 100644
> >> --- a/hw/virtio/vhost-vdpa.c
> >> +++ b/hw/virtio/vhost-vdpa.c
> >> @@ -1097,7 +1097,30 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> >> vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
> >> }
> >>
> >> - if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
> >> + /* FIXME the vhost_dev group for the control vq may have bogus nvqs=2
> >> + * value rather than nvqs=1. This can happen in case the guest doesn't
> >> + * support multiqueue, as a result of virtio_net_change_num_queue_pairs()
> >> + * destroying and rebuilding all the vqs, the guest index for control vq
> >> + * will no longer align with the host's. Currently net_init_vhost_vdpa()
> >> + * only initializes all vhost_dev's and net_clients once during
> >> + * net_client_init1() time, way earlier before multiqueue feature
> >> + * negotiation can kick in.
> > See below, it looks like the code doesn't find the correct vhost_dev.
> >
> >> + *
> >> + * Discussion - some possible fixes so far I can think of:
> >> + *
> >> + * option 1: fix vhost_net->dev.nvqs and nc->is_datapath in place for
> >> + * vdpa's ctrl vq, or rebuild all vdpa's vhost_dev groups and the
> >> + * net_client array, in the virtio_net_set_multiqueue() path;
> >> + *
> >> + * option 2: fix vhost_dev->nvqs in place at vhost_vdpa_set_features()
> >> + * before coming down to vhost_vdpa_dev_start() (Q: nc->is_datapath
> >> + * seems only used in virtio_net_device_realize, is it relevant?);
> > Relevant but not directly related, for the vhost_dev where
> > nc->is_datapath is false, it will assume it is backed by a single
> > queue not a queue pair.
> >
> >> + *
> >> + * option 3: use host queue index all along in vhost-vdpa ioctls instead
> >> + * of using guest vq index, so that vhost_net_start/stop() can remain
> >> + * as-is today
> >> + */
> > Note that the vq_index of each vhost_dev is assigned during
> > vhost_net_start() according to whether or not the MQ or CVQ is
> > negotiated in vhost_net_start()
> >
> > for (i = 0; i < nvhosts; i++) {
> >
> > if (i < data_queue_pairs) {
> > peer = qemu_get_peer(ncs, i);
> > } else { /* Control Virtqueue */
> > peer = qemu_get_peer(ncs, n->max_queue_pairs);
> > }
> >
> > net = get_vhost_net(peer);
> > vhost_net_set_vq_index(net, i * 2, index_end);
> >
> > It means some of the peers won't be used when MQ is not negotiated. So
> > it looks to me the evil came from virtio_net_get_notifier_mask().
> Yes, there it is. Where the control virtqueue first ever needs a
> guest_notifier for vhost_dev.
> > Where it doesn't mask the correct vhost dev when the guest doesn't
> > support MQ but the host does. So we had option 4:
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 2087516253..5e9ac019cd 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3179,7 +3179,13 @@ 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;
> > +
> > + if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_MQ) && idx == 2) {
> Hmmm, I thought it would be more natural to align the layout of
> vhost_dev's with that of virtqueue's, not the other way around.
The problem is that we need to make sure it works for vhost_net as
well where it doesn't support cvq.
> Not sure
> how this vhost_dev selection scheme may work with additional queues
> discovered through transport specific mechanism, such as the admin
> virtqueue, but I can live with it for now:
>
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -244,7 +244,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;
Any reason for this line?
Btw, would you mind to post a formal patch for this?
Thanks
>
> if (!get_vhost_net(nc->peer)) {
> return;
> @@ -3161,8 +3162,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);
> }
>
> @@ -3170,8 +3177,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);
> }
>
>
> Thanks,
> -Siwei
>
> > + nc = qemu_get_subqueue(n->nic, n->max_queue_pairs);
> > + } else {
> > + nc = qemu_get_subqueue(n->nic, vq2q(idx));
> > + }
> > assert(n->vhost_started);
> > vhost_net_virtqueue_mask(get_vhost_net(nc->peer),
> > vdev, idx, mask);
> >
> > Thanks
> >
> >> + if (dev->vq_index + dev->nvqs < dev->vq_index_end) {
> >> return 0;
> >> }
> >>
> >> --
> >> 1.8.3.1
> >>
>
On 3/25/2022 12:59 AM, Jason Wang wrote:
> On Fri, Mar 25, 2022 at 3:02 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 3/21/2022 8:47 PM, Jason Wang wrote:
>>> On Sat, Mar 19, 2022 at 12:14 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, it's easy
>>>> to hit assert failure as the following:
>>>>
>>>> ../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 single queue pair of 3 vqs in total (rx, tx and ctrl). This results
>>>> in ctrl_vq taking a different vhost_dev group index than the default
>>>> n->max_queue_pairs, the latter of which is only valid for multiqueue
>>>> guest. While on those additional vqs not exposed to the guest,
>>>> vhost_net_set_vq_index() never populated vq_index properly, hence
>>>> getting the assert failure.
>>>>
>>>> A possible fix is to pick the correct vhost_dev group for the control
>>>> vq according to this table [*]:
>>>>
>>>> vdpa tool / QEMU arg / guest config / ctrl_vq group index
>>>> ----------------------------------------------------------------
>>>> max_vqp 8 / mq=on / mq=off (UEFI) => data_queue_pairs
>>>> max_vqp 8 / mq=on / mq=on (Linux) => n->max_queue_pairs(>1)
>>>> max_vqp 8 / mq=off / mq=on (Linux) => n->max_queue_pairs(=1)
>>>>
>>>> [*] Please see FIXME in the code for open question and discussion
>>>>
>>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>>> ---
>>>> hw/net/vhost_net.c | 13 +++++++++----
>>>> hw/virtio/vhost-vdpa.c | 25 ++++++++++++++++++++++++-
>>>> 2 files changed, 33 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>> index 30379d2..9a4479b 100644
>>>> --- a/hw/net/vhost_net.c
>>>> +++ b/hw/net/vhost_net.c
>>>> @@ -322,6 +322,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(dev)));
>>>> VirtioBusState *vbus = VIRTIO_BUS(qbus);
>>>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>>>> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>> int nvhosts = data_queue_pairs + cvq;
>>>> @@ -343,7 +344,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>> if (i < data_queue_pairs) {
>>>> peer = qemu_get_peer(ncs, i);
>>>> } else { /* Control Virtqueue */
>>>> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
>>>> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
>>>> }
>>>>
>>>> net = get_vhost_net(peer);
>>>> @@ -368,7 +369,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>> if (i < data_queue_pairs) {
>>>> peer = qemu_get_peer(ncs, i);
>>>> } else {
>>>> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
>>>> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
>>>> }
>>>> r = vhost_net_start_one(get_vhost_net(peer), dev);
>>>>
>>>> @@ -390,7 +391,10 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>>>>
>>>> err_start:
>>>> while (--i >= 0) {
>>>> - peer = qemu_get_peer(ncs , i);
>>>> + if (mq)
>>>> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : data_queue_pairs);
>>>> + else
>>>> + peer = qemu_get_peer(ncs, i < data_queue_pairs ? i : n->max_queue_pairs);
>>>> vhost_net_stop_one(get_vhost_net(peer), dev);
>>>> }
>>>> e = k->set_guest_notifiers(qbus->parent, total_notifiers, false);
>>>> @@ -409,6 +413,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>>>> VirtioBusState *vbus = VIRTIO_BUS(qbus);
>>>> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
>>>> VirtIONet *n = VIRTIO_NET(dev);
>>>> + bool mq = virtio_host_has_feature(dev, VIRTIO_NET_F_MQ);
>>>> NetClientState *peer;
>>>> int total_notifiers = data_queue_pairs * 2 + cvq;
>>>> int nvhosts = data_queue_pairs + cvq;
>>>> @@ -418,7 +423,7 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>>>> if (i < data_queue_pairs) {
>>>> peer = qemu_get_peer(ncs, i);
>>>> } else {
>>>> - peer = qemu_get_peer(ncs, n->max_queue_pairs);
>>>> + peer = qemu_get_peer(ncs, mq ? data_queue_pairs : n->max_queue_pairs);
>>>> }
>>>> vhost_net_stop_one(get_vhost_net(peer), dev);
>>>> }
>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>>> index 27ea706..623476e 100644
>>>> --- a/hw/virtio/vhost-vdpa.c
>>>> +++ b/hw/virtio/vhost-vdpa.c
>>>> @@ -1097,7 +1097,30 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
>>>> vhost_vdpa_host_notifiers_uninit(dev, dev->nvqs);
>>>> }
>>>>
>>>> - if (dev->vq_index + dev->nvqs != dev->vq_index_end) {
>>>> + /* FIXME the vhost_dev group for the control vq may have bogus nvqs=2
>>>> + * value rather than nvqs=1. This can happen in case the guest doesn't
>>>> + * support multiqueue, as a result of virtio_net_change_num_queue_pairs()
>>>> + * destroying and rebuilding all the vqs, the guest index for control vq
>>>> + * will no longer align with the host's. Currently net_init_vhost_vdpa()
>>>> + * only initializes all vhost_dev's and net_clients once during
>>>> + * net_client_init1() time, way earlier before multiqueue feature
>>>> + * negotiation can kick in.
>>> See below, it looks like the code doesn't find the correct vhost_dev.
>>>
>>>> + *
>>>> + * Discussion - some possible fixes so far I can think of:
>>>> + *
>>>> + * option 1: fix vhost_net->dev.nvqs and nc->is_datapath in place for
>>>> + * vdpa's ctrl vq, or rebuild all vdpa's vhost_dev groups and the
>>>> + * net_client array, in the virtio_net_set_multiqueue() path;
>>>> + *
>>>> + * option 2: fix vhost_dev->nvqs in place at vhost_vdpa_set_features()
>>>> + * before coming down to vhost_vdpa_dev_start() (Q: nc->is_datapath
>>>> + * seems only used in virtio_net_device_realize, is it relevant?);
>>> Relevant but not directly related, for the vhost_dev where
>>> nc->is_datapath is false, it will assume it is backed by a single
>>> queue not a queue pair.
>>>
>>>> + *
>>>> + * option 3: use host queue index all along in vhost-vdpa ioctls instead
>>>> + * of using guest vq index, so that vhost_net_start/stop() can remain
>>>> + * as-is today
>>>> + */
>>> Note that the vq_index of each vhost_dev is assigned during
>>> vhost_net_start() according to whether or not the MQ or CVQ is
>>> negotiated in vhost_net_start()
>>>
>>> for (i = 0; i < nvhosts; i++) {
>>>
>>> if (i < data_queue_pairs) {
>>> peer = qemu_get_peer(ncs, i);
>>> } else { /* Control Virtqueue */
>>> peer = qemu_get_peer(ncs, n->max_queue_pairs);
>>> }
>>>
>>> net = get_vhost_net(peer);
>>> vhost_net_set_vq_index(net, i * 2, index_end);
>>>
>>> It means some of the peers won't be used when MQ is not negotiated. So
>>> it looks to me the evil came from virtio_net_get_notifier_mask().
>> Yes, there it is. Where the control virtqueue first ever needs a
>> guest_notifier for vhost_dev.
>>> Where it doesn't mask the correct vhost dev when the guest doesn't
>>> support MQ but the host does. So we had option 4:
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 2087516253..5e9ac019cd 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -3179,7 +3179,13 @@ 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;
>>> +
>>> + if (!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_MQ) && idx == 2) {
>> Hmmm, I thought it would be more natural to align the layout of
>> vhost_dev's with that of virtqueue's, not the other way around.
> The problem is that we need to make sure it works for vhost_net as
> well where it doesn't support cvq.
>
>> Not sure
>> how this vhost_dev selection scheme may work with additional queues
>> discovered through transport specific mechanism, such as the admin
>> virtqueue, but I can live with it for now:
>>
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -244,7 +244,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;
> Any reason for this line?
This corresponds to the following asserts:
assert(virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ));
If the QEMU or guest doesn't support control vq, there's no need to
bother exposing vhost_dev and guest notifier for the control vq. Noted
the vhost_net_start/stop implies DRIVER_OK is set in device status,
meaning feature negotiation is complete already (same as n->multiqueue).
>
> Btw, would you mind to post a formal patch for this?
I would love to, there was a set of mq bug fixes sitting in my queue
pending on paper work, but I had been dragged to other stuff earlier
this week. I will try to post it early next week.
-Siwei
>
> Thanks
>
>> if (!get_vhost_net(nc->peer)) {
>> return;
>> @@ -3161,8 +3162,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);
>> }
>>
>> @@ -3170,8 +3177,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);
>> }
>>
>>
>> Thanks,
>> -Siwei
>>
>>> + nc = qemu_get_subqueue(n->nic, n->max_queue_pairs);
>>> + } else {
>>> + nc = qemu_get_subqueue(n->nic, vq2q(idx));
>>> + }
>>> assert(n->vhost_started);
>>> vhost_net_virtqueue_mask(get_vhost_net(nc->peer),
>>> vdev, idx, mask);
>>>
>>> Thanks
>>>
>>>> + if (dev->vq_index + dev->nvqs < dev->vq_index_end) {
>>>> return 0;
>>>> }
>>>>
>>>> --
>>>> 1.8.3.1
>>>>
© 2016 - 2024 Red Hat, Inc.