Re: [RFC v2 06/13] vhost: delay set_vring_ready after DRIVER_OK

[Eugenio Perez Martin]

On Fri, Jan 13, 2023 at 5:36 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Fri, Jan 13, 2023 at 1:25 AM Eugenio Pérez <eperezma@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:
>
> parent_set_status():
> if (DRIVER_OK)
>     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
> > vdpa.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@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
> vhost_vdpa_dev_start()?
>

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.

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?

Thanks!

> Thanks
>
> >
> >          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;
> > --
> > 2.31.1
> >
>