Re: [RFC PATCH 04/12] vdpa: delay set_vring_ready after DRIVER_OK

[Eugenio Perez Martin]

On Mon, Jul 18, 2022 at 8:34 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Sat, Jul 16, 2022 at 7:34 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > To restore the device in the destination of a live migration we send the
> > commands through control virtqueue. For a device to read CVQ it must
> > have received DRIVER_OK status bit.
> >
> > However this open a window where the device could start receiving
> > packets in rx queue 0 before it receive the RSS configuration.
>
> A note here is that, after chatting with Michael, the device should
> not start processing the buffer until a kick. So I wonder if it's more
> than enough to not kick the data vq paris until we've recovered the
> state via cvq?
>

Oh, I see I misunderstood the mail now :). You both have a very good point.

Take into account the set of the state will also happen when
transitioning from passthrough mode to SVQ mode. In that switch we're
not pausing the VM (AFAIK), so guests are free to kick dvqs.

But it is totally right that it is not strictly needed for this
particular series, as:
* We're in full control of the device with SVQ.
* Guest still has not seen the DRIVER_OK bit set.

So I'll delay that part for the one able to do the transition. It's
totally safe that the guest kicks before DRIVER_OK at this moment (as
I explained in the other mail).

Thanks!

>
> > To avoid
> > that, we will not send vring_enable until all configuration is used by
> > the device.
> >
> > As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
> >  1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 906c365036..1d8829c619 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -730,13 +730,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev 
> > *dev, int idx)
> >      return idx;
> >  }
> >
> > +/**
> > + * Set ready all vring of the device
>
> It should be better to mention, for device we mean virtio device
> instead of vhost device (which we may have multiple ones if mq is
> enabled).
>
> Thanks
>
> > + *
> > + * @dev: Vhost device
> > + */
> >  static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> >  {
> >      int i;
> >      trace_vhost_vdpa_set_vring_ready(dev);
> > -    for (i = 0; i < dev->nvqs; ++i) {
> > +    for (i = 0; i < dev->vq_index_end; ++i) {
> >          struct vhost_vring_state state = {
> > -            .index = dev->vq_index + i,
> > +            .index = i,
> >              .num = 1,
> >          };
> >          vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> > @@ -1111,7 +1116,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
> > *dev, bool started)
> >          if (unlikely(!ok)) {
> >              return -1;
> >          }
> > -        vhost_vdpa_set_vring_ready(dev);
> >      } else {
> >          ok = vhost_vdpa_svqs_stop(dev);
> >          if (unlikely(!ok)) {
> > @@ -1125,16 +1129,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev 
> > *dev, bool started)
> >      }
> >
> >      if (started) {
> > +        int r;
> > +
> >          memory_listener_register(&v->listener, &address_space_memory);
> > -        return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > +        r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > +        if (unlikely(r)) {
> > +            return r;
> > +        }
> > +        vhost_vdpa_set_vring_ready(dev);
> >      } else {
> >          vhost_vdpa_reset_device(dev);
> >          vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> >                                     VIRTIO_CONFIG_S_DRIVER);
> >          memory_listener_unregister(&v->listener);
> > -
> > -        return 0;
> >      }
> > +
> > +    return 0;
> >  }
> >
> >  static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
> > --
> > 2.31.1
> >
>