Hi Eugenio,

On 1/13/23 11:03, Eugenio Perez Martin wrote:
On Fri, Jan 13, 2023 at 10:51 AM Stefano Garzarella <sgarz...@redhat.com> wrote:

On Fri, Jan 13, 2023 at 09:19:00AM +0100, Eugenio Perez Martin wrote:
On Fri, Jan 13, 2023 at 5:36 AM Jason Wang <jasow...@redhat.com> wrote:

On Fri, Jan 13, 2023 at 1:25 AM Eugenio Pérez <epere...@redhat.com> wrote:

To restore the device at the destination of a live migration we send the
commands through control virtqueue. For a device to read CVQ it must
have received the DRIVER_OK status bit.

This probably requires the support from the parent driver and requires
some changes or fixes in the parent driver.

Some drivers did:

     if (queue_enable)
         write queue_enable to the device

Examples are IFCVF or even vp_vdpa at least. MLX5 seems to be fine.

I don't get your point here. No device should start reading CVQ (or
any other VQ) without having received DRIVER_OK.

Some parent drivers do not support sending the queue enable command
after DRIVER_OK, usually because they clean part of the state like the
set by set_vring_base. Even vdpa_net_sim needs fixes here.

But my understanding is that it should be supported so I consider it a
bug. Especially after queue_reset patches. Is that what you mean?

However this opens a window where the device could start receiving
packets in rx queue 0 before it receives the RSS configuration. To avoid
that, we will not send vring_enable until all configuration is used by
the device.

As a first step, run vhost_set_vring_ready for all vhost_net backend after
all of them are started (with DRIVER_OK). This code should not affect

Signed-off-by: Eugenio Pérez <epere...@redhat.com>
  hw/net/vhost_net.c | 17 ++++++++++++-----
  1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index c4eecc6f36..3900599465 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -399,6 +399,18 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
          } else {
              peer = qemu_get_peer(ncs, n->max_queue_pairs);
+        r = vhost_net_start_one(get_vhost_net(peer), dev);
+        if (r < 0) {
+            goto err_start;
+        }
+    }
+    for (int j = 0; j < nvhosts; j++) {
+        if (j < data_queue_pairs) {
+            peer = qemu_get_peer(ncs, j);
+        } else {
+            peer = qemu_get_peer(ncs, n->max_queue_pairs);
+        }

I fail to understand why we need to change the vhost_net layer? This
is vhost-vDPA specific, so I wonder if we can limit the changes to e.g

The vhost-net layer explicitly calls vhost_set_vring_enable before
vhost_dev_start, and this is exactly the behavior we want to avoid.
Even if we make changes to vhost_dev, this change is still needed.

I'm working on something similar since I'd like to re-work the following
commit we merged just before 7.2 release:
      4daa5054c5 vhost: enable vrings in vhost_dev_start() for vhost-user

vhost-net wasn't the only one who enabled vrings independently, but it
was easy enough for others devices to avoid it and enable them in

Do you think can we avoid in some way this special behaviour of
vhost-net and enable the vrings in vhost_dev_start?

Actually looking forward to it :). If that gets merged before this
series, I think we could drop this patch.

If I'm not wrong the enable/disable dance is used just by vhost-user
at the moment.

Maxime, could you give us some hints about the tests to use to check
that changes do not introduce regressions in vhost-user?

You can use DPDK's testpmd [0] tool with Vhost PMD, e.g.:

#single queue pair
# dpdk-testpmd -l <CORE IDs> --no-pci --vdev=net_vhost0,iface=/tmp/vhost-user1 -- -i

# dpdk-testpmd -l <CORE IDs> --no-pci --vdev=net_vhost0,iface=/tmp/vhost-user1,queues=4 -- -i --rxq=4 --txq=4

[0]: https://doc.dpdk.org/guides/testpmd_app_ug/index.html




And we want to explicitly enable CVQ first, which "only" vhost_net
knows which is. To perform that in vhost_vdpa_dev_start would require
quirks, involving one or more of:
* Ignore vq enable calls if the device is not the CVQ one. How to
signal what is the CVQ? Can we trust it will be the last one for all
kind of devices?
* Enable queues that do not belong to the last vhost_dev from the enable call.
* Enable the rest of the queues from the last enable in reverse order.
* Intercalate the "net load" callback between enabling the last
vhost_vdpa device and enabling the rest of devices.
* Add an "enable priority" order?



          if (peer->vring_enable) {
              /* restore vring enable state */
@@ -408,11 +420,6 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
                  goto err_start;
-        r = vhost_net_start_one(get_vhost_net(peer), dev);
-        if (r < 0) {
-            goto err_start;
-        }

      return 0;

Reply via email to