[2/2] vringh: fetch used_idx from vring at vringh_init_iotlb

  Starting from an used_idx different than 0 is needed in use cases like
virtual machine migration.  Not doing so and letting the caller set an
avail idx different than 0 causes destination device to try to use old
buffers that source driver already recover and are not available
anymore.

While callers like vdpa_sim set avail_idx directly it does not set
used_idx.  Instead of let the caller do the assignment, fetch it from
the guest at initialization like vhost-kernel do.

To perform the same at vring_kernel_init and vring_user_init is left for
the future.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
  

On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Starting from an used_idx different than 0 is needed in use cases like
> virtual machine migration.  Not doing so and letting the caller set an
> avail idx different than 0 causes destination device to try to use old
> buffers that source driver already recover and are not available
> anymore.
>
> While callers like vdpa_sim set avail_idx directly it does not set
> used_idx.  Instead of let the caller do the assignment, fetch it from
> the guest at initialization like vhost-kernel do.
>
> To perform the same at vring_kernel_init and vring_user_init is left for
> the future.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 33eb941fcf15..0eed825197f2 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
>         return 0;
>  }
>
> +/**
> + * vringh_update_used_idx - fetch used idx from driver's used split vring
> + * @vrh: The vring.
> + *
> + * Returns -errno or 0.
> + */
> +static inline int vringh_update_used_idx(struct vringh *vrh)
> +{
> +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> +}
> +
>  /**
>   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
>   * @vrh: the vringh to initialize.
> @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
>                       struct vring_avail *avail,
>                       struct vring_used *used)
>  {

While at this, I wonder if it's better to have a dedicated parameter
for last_avail_idx?

> -       return vringh_init_kern(vrh, features, num, weak_barriers,
> -                               desc, avail, used);
> +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> +                                avail, used);
> +
> +       if (r != 0)
> +               return r;
> +
> +       /* Consider the ring not initialized */
> +       if ((void *)desc == used)
> +               return 0;

I don't understand when we can get this (actually it should be a bug
of the caller).

Thanks

> +
> +       return vringh_update_used_idx(vrh);
> +
>  }
>  EXPORT_SYMBOL(vringh_init_iotlb);
>
> --
> 2.31.1
>
  

On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > Starting from an used_idx different than 0 is needed in use cases like
> > virtual machine migration.  Not doing so and letting the caller set an
> > avail idx different than 0 causes destination device to try to use old
> > buffers that source driver already recover and are not available
> > anymore.
> >
> > While callers like vdpa_sim set avail_idx directly it does not set
> > used_idx.  Instead of let the caller do the assignment, fetch it from
> > the guest at initialization like vhost-kernel do.
> >
> > To perform the same at vring_kernel_init and vring_user_init is left for
> > the future.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 33eb941fcf15..0eed825197f2 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> >         return 0;
> >  }
> >
> > +/**
> > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > + * @vrh: The vring.
> > + *
> > + * Returns -errno or 0.
> > + */
> > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > +{
> > +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > +}
> > +
> >  /**
> >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> >   * @vrh: the vringh to initialize.
> > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> >                       struct vring_avail *avail,
> >                       struct vring_used *used)
> >  {
>
> While at this, I wonder if it's better to have a dedicated parameter
> for last_avail_idx?
>

I also had that thought. To directly assign last_avail_idx is not a
specially elegant API IMO.

Maybe expose a way to fetch used_idx from device vring and pass
used_idx as parameter too?

> > -       return vringh_init_kern(vrh, features, num, weak_barriers,
> > -                               desc, avail, used);
> > +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > +                                avail, used);
> > +
> > +       if (r != 0)
> > +               return r;
> > +
> > +       /* Consider the ring not initialized */
> > +       if ((void *)desc == used)
> > +               return 0;
>
> I don't understand when we can get this (actually it should be a bug
> of the caller).
>

You can see it in vdpasim_vq_reset.

Note that to consider desc == 0 to be an uninitialized ring is a bug
IMO. QEMU considers it that way also, but the standard does not forbid
any ring to be at address 0. Especially if we use vIOMMU.

So I think the best way to know if we can use the vringh is either
this way, or provide an explicit "initialized" boolean attribute.
Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to
add new attributes.

Thanks!

> Thanks
>
> > +
> > +       return vringh_update_used_idx(vrh);
> > +
> >  }
> >  EXPORT_SYMBOL(vringh_init_iotlb);
> >
> > --
> > 2.31.1
> >
>
  

On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > Starting from an used_idx different than 0 is needed in use cases like
> > > virtual machine migration.  Not doing so and letting the caller set an
> > > avail idx different than 0 causes destination device to try to use old
> > > buffers that source driver already recover and are not available
> > > anymore.
> > >
> > > While callers like vdpa_sim set avail_idx directly it does not set
> > > used_idx.  Instead of let the caller do the assignment, fetch it from
> > > the guest at initialization like vhost-kernel do.
> > >
> > > To perform the same at vring_kernel_init and vring_user_init is left for
> > > the future.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> > >  1 file changed, 23 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > index 33eb941fcf15..0eed825197f2 100644
> > > --- a/drivers/vhost/vringh.c
> > > +++ b/drivers/vhost/vringh.c
> > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> > >         return 0;
> > >  }
> > >
> > > +/**
> > > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > > + * @vrh: The vring.
> > > + *
> > > + * Returns -errno or 0.
> > > + */
> > > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > > +{
> > > +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > > +}
> > > +
> > >  /**
> > >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> > >   * @vrh: the vringh to initialize.
> > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> > >                       struct vring_avail *avail,
> > >                       struct vring_used *used)
> > >  {
> >
> > While at this, I wonder if it's better to have a dedicated parameter
> > for last_avail_idx?
> >
>
> I also had that thought. To directly assign last_avail_idx is not a
> specially elegant API IMO.
>
> Maybe expose a way to fetch used_idx from device vring and pass
> used_idx as parameter too?

If I was not wrong, we can start from last_avail_idx, for used_idx it
is only needed for inflight descriptors which might require other
APIs?

(All the current vDPA user of vringh is doing in order processing)

>
> > > -       return vringh_init_kern(vrh, features, num, weak_barriers,
> > > -                               desc, avail, used);
> > > +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > > +                                avail, used);
> > > +
> > > +       if (r != 0)
> > > +               return r;
> > > +
> > > +       /* Consider the ring not initialized */
> > > +       if ((void *)desc == used)
> > > +               return 0;
> >
> > I don't understand when we can get this (actually it should be a bug
> > of the caller).
> >
>
> You can see it in vdpasim_vq_reset.
>
> Note that to consider desc == 0 to be an uninitialized ring is a bug
> IMO. QEMU considers it that way also, but the standard does not forbid
> any ring to be at address 0. Especially if we use vIOMMU.
>
> So I think the best way to know if we can use the vringh is either
> this way, or provide an explicit "initialized" boolean attribute.
> Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to
> add new attributes.

I wonder if we can avoid this in the simulator level instead of the
vringh (anyhow it only exposes a vringh_init_xxx() helper now).

Thanks

>
> Thanks!
>
> > Thanks
> >
> > > +
> > > +       return vringh_update_used_idx(vrh);
> > > +
> > >  }
> > >  EXPORT_SYMBOL(vringh_init_iotlb);
> > >
> > > --
> > > 2.31.1
> > >
> >
>
  

On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >
> > > > Starting from an used_idx different than 0 is needed in use cases like
> > > > virtual machine migration.  Not doing so and letting the caller set an
> > > > avail idx different than 0 causes destination device to try to use old
> > > > buffers that source driver already recover and are not available
> > > > anymore.
> > > >
> > > > While callers like vdpa_sim set avail_idx directly it does not set
> > > > used_idx.  Instead of let the caller do the assignment, fetch it from
> > > > the guest at initialization like vhost-kernel do.
> > > >
> > > > To perform the same at vring_kernel_init and vring_user_init is left for
> > > > the future.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> > > >  1 file changed, 23 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > > index 33eb941fcf15..0eed825197f2 100644
> > > > --- a/drivers/vhost/vringh.c
> > > > +++ b/drivers/vhost/vringh.c
> > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> > > >         return 0;
> > > >  }
> > > >
> > > > +/**
> > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > > > + * @vrh: The vring.
> > > > + *
> > > > + * Returns -errno or 0.
> > > > + */
> > > > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > > > +{
> > > > +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > > > +}
> > > > +
> > > >  /**
> > > >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> > > >   * @vrh: the vringh to initialize.
> > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> > > >                       struct vring_avail *avail,
> > > >                       struct vring_used *used)
> > > >  {
> > >
> > > While at this, I wonder if it's better to have a dedicated parameter
> > > for last_avail_idx?
> > >
> >
> > I also had that thought. To directly assign last_avail_idx is not a
> > specially elegant API IMO.
> >
> > Maybe expose a way to fetch used_idx from device vring and pass
> > used_idx as parameter too?
>
> If I was not wrong, we can start from last_avail_idx, for used_idx it
> is only needed for inflight descriptors which might require other
> APIs?
>
> (All the current vDPA user of vringh is doing in order processing)
>

That was actually my first attempt and it works equally well for the
moment, but it diverges from vhost-kernel behavior for little benefit.

To assign both values at set_vring_base mean that if vDPA introduces
an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future,
the initialization process would vary a lot:
* Without that feature, the used_idx starts with 0, and the avail one
is 0 or whatever value the user set with vring_set_base.
* With that feature, the device will read guest's used_idx as
vhost-kernel? We would enable a new ioctl to set it, or expand
set_base to include used_idx, effectively diverting from vhost-kernel?

To me the wisest option is to move this with vhost-kernel. Maybe we
need to add a feature bit to know that the hypervisor can trust the
device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?),
but we should keep it orthogonal to inflight descriptor migration in
my opinion.

Having said that, I'm totally ok to do it otherwise (or to expand the
patch message if needed).

> >
> > > > -       return vringh_init_kern(vrh, features, num, weak_barriers,
> > > > -                               desc, avail, used);
> > > > +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > > > +                                avail, used);
> > > > +
> > > > +       if (r != 0)
> > > > +               return r;
> > > > +
> > > > +       /* Consider the ring not initialized */
> > > > +       if ((void *)desc == used)
> > > > +               return 0;
> > >
> > > I don't understand when we can get this (actually it should be a bug
> > > of the caller).
> > >
> >
> > You can see it in vdpasim_vq_reset.
> >
> > Note that to consider desc == 0 to be an uninitialized ring is a bug
> > IMO. QEMU considers it that way also, but the standard does not forbid
> > any ring to be at address 0. Especially if we use vIOMMU.
> >
> > So I think the best way to know if we can use the vringh is either
> > this way, or provide an explicit "initialized" boolean attribute.
> > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to
> > add new attributes.
>
> I wonder if we can avoid this in the simulator level instead of the
> vringh (anyhow it only exposes a vringh_init_xxx() helper now).
>

In my opinion that is a mistake if other drivers will use it to
implement the emulated control virtqueue. And it requires more
changes. But it is doable for sure.

Thanks!
  

On Tue, Jan 31, 2023 at 12:39 AM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > >
> > > > > Starting from an used_idx different than 0 is needed in use cases like
> > > > > virtual machine migration.  Not doing so and letting the caller set an
> > > > > avail idx different than 0 causes destination device to try to use old
> > > > > buffers that source driver already recover and are not available
> > > > > anymore.
> > > > >
> > > > > While callers like vdpa_sim set avail_idx directly it does not set
> > > > > used_idx.  Instead of let the caller do the assignment, fetch it from
> > > > > the guest at initialization like vhost-kernel do.
> > > > >
> > > > > To perform the same at vring_kernel_init and vring_user_init is left for
> > > > > the future.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> > > > >  1 file changed, 23 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > > > index 33eb941fcf15..0eed825197f2 100644
> > > > > --- a/drivers/vhost/vringh.c
> > > > > +++ b/drivers/vhost/vringh.c
> > > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > > > > + * @vrh: The vring.
> > > > > + *
> > > > > + * Returns -errno or 0.
> > > > > + */
> > > > > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > > > > +{
> > > > > +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> > > > >   * @vrh: the vringh to initialize.
> > > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> > > > >                       struct vring_avail *avail,
> > > > >                       struct vring_used *used)
> > > > >  {
> > > >
> > > > While at this, I wonder if it's better to have a dedicated parameter
> > > > for last_avail_idx?
> > > >
> > >
> > > I also had that thought. To directly assign last_avail_idx is not a
> > > specially elegant API IMO.
> > >
> > > Maybe expose a way to fetch used_idx from device vring and pass
> > > used_idx as parameter too?
> >
> > If I was not wrong, we can start from last_avail_idx, for used_idx it
> > is only needed for inflight descriptors which might require other
> > APIs?
> >
> > (All the current vDPA user of vringh is doing in order processing)
> >
>
> That was actually my first attempt and it works equally well for the
> moment, but it diverges from vhost-kernel behavior for little benefit.
>
> To assign both values at set_vring_base mean that if vDPA introduces
> an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future,
> the initialization process would vary a lot:
> * Without that feature, the used_idx starts with 0, and the avail one
> is 0 or whatever value the user set with vring_set_base.
> * With that feature, the device will read guest's used_idx as
> vhost-kernel? We would enable a new ioctl to set it, or expand
> set_base to include used_idx, effectively diverting from vhost-kernel?

Adding Longpeng who is looking at this.

We can leave this to the caller to decide.

Btw, a question, at which case the device used index does not equal to
the used index in the vring when the device is suspended? I think the
correct way is to flush the pending used indexes before suspending.
Otherwise we need an API to get pending used indices?

>
> To me the wisest option is to move this with vhost-kernel. Maybe we
> need to add a feature bit to know that the hypervisor can trust the
> device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?),
> but we should keep it orthogonal to inflight descriptor migration in
> my opinion.

I think we need to understand if there are any other possible use
cases for setting used idx other than inflight stuff.

>
> Having said that, I'm totally ok to do it otherwise (or to expand the
> patch message if needed).

I tend to do that in another series (not mix with the fixes).

>
> > >
> > > > > -       return vringh_init_kern(vrh, features, num, weak_barriers,
> > > > > -                               desc, avail, used);
> > > > > +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > > > > +                                avail, used);
> > > > > +
> > > > > +       if (r != 0)
> > > > > +               return r;
> > > > > +
> > > > > +       /* Consider the ring not initialized */
> > > > > +       if ((void *)desc == used)
> > > > > +               return 0;
> > > >
> > > > I don't understand when we can get this (actually it should be a bug
> > > > of the caller).
> > > >
> > >
> > > You can see it in vdpasim_vq_reset.
> > >
> > > Note that to consider desc == 0 to be an uninitialized ring is a bug
> > > IMO. QEMU considers it that way also, but the standard does not forbid
> > > any ring to be at address 0. Especially if we use vIOMMU.
> > >
> > > So I think the best way to know if we can use the vringh is either
> > > this way, or provide an explicit "initialized" boolean attribute.
> > > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to
> > > add new attributes.
> >
> > I wonder if we can avoid this in the simulator level instead of the
> > vringh (anyhow it only exposes a vringh_init_xxx() helper now).
> >
>
> In my opinion that is a mistake if other drivers will use it to
> implement the emulated control virtqueue. And it requires more
> changes. But it is doable for sure.

The problem is, there's no reset API in vringh, that's why you need to
do if ((void *)desc == used) which depends on behaviour of the vringh
user.

So I think we should either:

1) move that check in vdpa_sim (since it's not guaranteed that all the
vringh users will make desc equal to used during reset)

or

2) introduce a vringh_reset_xxx()

1) seems a good step for -stable.

Thanks

>
> Thanks!
>
  

On Tue, Jan 31, 2023 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Jan 31, 2023 at 12:39 AM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > >
> > > > > > Starting from an used_idx different than 0 is needed in use cases like
> > > > > > virtual machine migration.  Not doing so and letting the caller set an
> > > > > > avail idx different than 0 causes destination device to try to use old
> > > > > > buffers that source driver already recover and are not available
> > > > > > anymore.
> > > > > >
> > > > > > While callers like vdpa_sim set avail_idx directly it does not set
> > > > > > used_idx.  Instead of let the caller do the assignment, fetch it from
> > > > > > the guest at initialization like vhost-kernel do.
> > > > > >
> > > > > > To perform the same at vring_kernel_init and vring_user_init is left for
> > > > > > the future.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> > > > > >  1 file changed, 23 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > > > > index 33eb941fcf15..0eed825197f2 100644
> > > > > > --- a/drivers/vhost/vringh.c
> > > > > > +++ b/drivers/vhost/vringh.c
> > > > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +/**
> > > > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > > > > > + * @vrh: The vring.
> > > > > > + *
> > > > > > + * Returns -errno or 0.
> > > > > > + */
> > > > > > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > > > > > +{
> > > > > > +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> > > > > >   * @vrh: the vringh to initialize.
> > > > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> > > > > >                       struct vring_avail *avail,
> > > > > >                       struct vring_used *used)
> > > > > >  {
> > > > >
> > > > > While at this, I wonder if it's better to have a dedicated parameter
> > > > > for last_avail_idx?
> > > > >
> > > >
> > > > I also had that thought. To directly assign last_avail_idx is not a
> > > > specially elegant API IMO.
> > > >
> > > > Maybe expose a way to fetch used_idx from device vring and pass
> > > > used_idx as parameter too?
> > >
> > > If I was not wrong, we can start from last_avail_idx, for used_idx it
> > > is only needed for inflight descriptors which might require other
> > > APIs?
> > >
> > > (All the current vDPA user of vringh is doing in order processing)
> > >
> >
> > That was actually my first attempt and it works equally well for the
> > moment, but it diverges from vhost-kernel behavior for little benefit.
> >
> > To assign both values at set_vring_base mean that if vDPA introduces
> > an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future,
> > the initialization process would vary a lot:
> > * Without that feature, the used_idx starts with 0, and the avail one
> > is 0 or whatever value the user set with vring_set_base.
> > * With that feature, the device will read guest's used_idx as
> > vhost-kernel? We would enable a new ioctl to set it, or expand
> > set_base to include used_idx, effectively diverting from vhost-kernel?
>
> Adding Longpeng who is looking at this.
>

Sorry, I'll CC longpeng2@huawei.com in future series like this one.

> We can leave this to the caller to decide.
>
> Btw, a question, at which case the device used index does not equal to
> the used index in the vring when the device is suspended? I think the
> correct way is to flush the pending used indexes before suspending.
> Otherwise we need an API to get pending used indices?
>
> >
> > To me the wisest option is to move this with vhost-kernel. Maybe we
> > need to add a feature bit to know that the hypervisor can trust the
> > device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?),
> > but we should keep it orthogonal to inflight descriptor migration in
> > my opinion.
>
> I think we need to understand if there are any other possible use
> cases for setting used idx other than inflight stuff.
>

Answering this and the previous comment, I cannot think in any case
outside of inflight migration. I'm just trying to avoid different
behavior between vhost-kernel and vhost-vdpa, and to make features as
orthogonal as possible.

> >
> > Having said that, I'm totally ok to do it otherwise (or to expand the
> > patch message if needed).
>
> I tend to do that in another series (not mix with the fixes).
>
> >
> > > >
> > > > > > -       return vringh_init_kern(vrh, features, num, weak_barriers,
> > > > > > -                               desc, avail, used);
> > > > > > +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > > > > > +                                avail, used);
> > > > > > +
> > > > > > +       if (r != 0)
> > > > > > +               return r;
> > > > > > +
> > > > > > +       /* Consider the ring not initialized */
> > > > > > +       if ((void *)desc == used)
> > > > > > +               return 0;
> > > > >
> > > > > I don't understand when we can get this (actually it should be a bug
> > > > > of the caller).
> > > > >
> > > >
> > > > You can see it in vdpasim_vq_reset.
> > > >
> > > > Note that to consider desc == 0 to be an uninitialized ring is a bug
> > > > IMO. QEMU considers it that way also, but the standard does not forbid
> > > > any ring to be at address 0. Especially if we use vIOMMU.
> > > >
> > > > So I think the best way to know if we can use the vringh is either
> > > > this way, or provide an explicit "initialized" boolean attribute.
> > > > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to
> > > > add new attributes.
> > >
> > > I wonder if we can avoid this in the simulator level instead of the
> > > vringh (anyhow it only exposes a vringh_init_xxx() helper now).
> > >
> >
> > In my opinion that is a mistake if other drivers will use it to
> > implement the emulated control virtqueue. And it requires more
> > changes. But it is doable for sure.
>
> The problem is, there's no reset API in vringh, that's why you need to
> do if ((void *)desc == used) which depends on behaviour of the vringh
> user.
>

That's a very good point indeed.

> So I think we should either:
>
> 1) move that check in vdpa_sim (since it's not guaranteed that all the
> vringh users will make desc equal to used during reset)
>
> or
>
> 2) introduce a vringh_reset_xxx()
>
> 1) seems a good step for -stable.
>

We can go to 1 for sure. So let's set last_used_idx at
vdpasim_set_vq_state, same value as last_avail_idx, and stash it at
vdpasim_queue_ready.

Do I need to resend the previous patch in this series?

Do we need to offer a new feature flag indicating we will set used_idx
with avail_idx?

Thanks!
  

On Wed, Jan 18, 2023 at 05:43:59PM +0100, Eugenio Pérez wrote:
> Starting from an used_idx different than 0 is needed in use cases like
> virtual machine migration.  Not doing so and letting the caller set an
> avail idx different than 0 causes destination device to try to use old
> buffers that source driver already recover and are not available
> anymore.
> 
> While callers like vdpa_sim set avail_idx directly it does not set
> used_idx.  Instead of let the caller do the assignment, fetch it from
> the guest at initialization like vhost-kernel do.
> 
> To perform the same at vring_kernel_init and vring_user_init is left for
> the future.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>


So I applied 1/2 and dropped 2/2 for now, right?

> ---
>  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 33eb941fcf15..0eed825197f2 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
>  	return 0;
>  }
>  
> +/**
> + * vringh_update_used_idx - fetch used idx from driver's used split vring
> + * @vrh: The vring.
> + *
> + * Returns -errno or 0.
> + */
> +static inline int vringh_update_used_idx(struct vringh *vrh)
> +{
> +	return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> +}
> +
>  /**
>   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
>   * @vrh: the vringh to initialize.
> @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
>  		      struct vring_avail *avail,
>  		      struct vring_used *used)
>  {
> -	return vringh_init_kern(vrh, features, num, weak_barriers,
> -				desc, avail, used);
> +	int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> +				 avail, used);
> +
> +	if (r != 0)
> +		return r;
> +
> +	/* Consider the ring not initialized */
> +	if ((void *)desc == used)
> +		return 0;
> +
> +	return vringh_update_used_idx(vrh);
> +
>  }
>  EXPORT_SYMBOL(vringh_init_iotlb);
>  
> -- 
> 2.31.1
  

On Wed, Feb 1, 2023 at 5:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jan 18, 2023 at 05:43:59PM +0100, Eugenio Pérez wrote:
> > Starting from an used_idx different than 0 is needed in use cases like
> > virtual machine migration.  Not doing so and letting the caller set an
> > avail idx different than 0 causes destination device to try to use old
> > buffers that source driver already recover and are not available
> > anymore.
> >
> > While callers like vdpa_sim set avail_idx directly it does not set
> > used_idx.  Instead of let the caller do the assignment, fetch it from
> > the guest at initialization like vhost-kernel do.
> >
> > To perform the same at vring_kernel_init and vring_user_init is left for
> > the future.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
>
> So I applied 1/2 and dropped 2/2 for now, right?
>

Yes, please. 2/2 needs tweaking, I'll address them ASAP.

Thanks!

> > ---
> >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 33eb941fcf15..0eed825197f2 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> >       return 0;
> >  }
> >
> > +/**
> > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > + * @vrh: The vring.
> > + *
> > + * Returns -errno or 0.
> > + */
> > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > +{
> > +     return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > +}
> > +
> >  /**
> >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> >   * @vrh: the vringh to initialize.
> > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> >                     struct vring_avail *avail,
> >                     struct vring_used *used)
> >  {
> > -     return vringh_init_kern(vrh, features, num, weak_barriers,
> > -                             desc, avail, used);
> > +     int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > +                              avail, used);
> > +
> > +     if (r != 0)
> > +             return r;
> > +
> > +     /* Consider the ring not initialized */
> > +     if ((void *)desc == used)
> > +             return 0;
> > +
> > +     return vringh_update_used_idx(vrh);
> > +
> >  }
> >  EXPORT_SYMBOL(vringh_init_iotlb);
> >
> > --
> > 2.31.1
>
  

On Tue, Jan 31, 2023 at 08:58:37AM +0100, Eugenio Perez Martin wrote:
> On Tue, Jan 31, 2023 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Jan 31, 2023 at 12:39 AM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin
> > > > <eperezma@redhat.com> wrote:
> > > > >
> > > > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > > > > >
> > > > > > > Starting from an used_idx different than 0 is needed in use cases like
> > > > > > > virtual machine migration.  Not doing so and letting the caller set an
> > > > > > > avail idx different than 0 causes destination device to try to use old
> > > > > > > buffers that source driver already recover and are not available
> > > > > > > anymore.
> > > > > > >
> > > > > > > While callers like vdpa_sim set avail_idx directly it does not set
> > > > > > > used_idx.  Instead of let the caller do the assignment, fetch it from
> > > > > > > the guest at initialization like vhost-kernel do.
> > > > > > >
> > > > > > > To perform the same at vring_kernel_init and vring_user_init is left for
> > > > > > > the future.
> > > > > > >
> > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > > ---
> > > > > > >  drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
> > > > > > >  1 file changed, 23 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > > > > > index 33eb941fcf15..0eed825197f2 100644
> > > > > > > --- a/drivers/vhost/vringh.c
> > > > > > > +++ b/drivers/vhost/vringh.c
> > > > > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh,
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +/**
> > > > > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring
> > > > > > > + * @vrh: The vring.
> > > > > > > + *
> > > > > > > + * Returns -errno or 0.
> > > > > > > + */
> > > > > > > +static inline int vringh_update_used_idx(struct vringh *vrh)
> > > > > > > +{
> > > > > > > +       return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx);
> > > > > > > +}
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * vringh_init_iotlb - initialize a vringh for a ring with IOTLB.
> > > > > > >   * @vrh: the vringh to initialize.
> > > > > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features,
> > > > > > >                       struct vring_avail *avail,
> > > > > > >                       struct vring_used *used)
> > > > > > >  {
> > > > > >
> > > > > > While at this, I wonder if it's better to have a dedicated parameter
> > > > > > for last_avail_idx?
> > > > > >
> > > > >
> > > > > I also had that thought. To directly assign last_avail_idx is not a
> > > > > specially elegant API IMO.
> > > > >
> > > > > Maybe expose a way to fetch used_idx from device vring and pass
> > > > > used_idx as parameter too?
> > > >
> > > > If I was not wrong, we can start from last_avail_idx, for used_idx it
> > > > is only needed for inflight descriptors which might require other
> > > > APIs?
> > > >
> > > > (All the current vDPA user of vringh is doing in order processing)
> > > >
> > >
> > > That was actually my first attempt and it works equally well for the
> > > moment, but it diverges from vhost-kernel behavior for little benefit.
> > >
> > > To assign both values at set_vring_base mean that if vDPA introduces
> > > an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future,
> > > the initialization process would vary a lot:
> > > * Without that feature, the used_idx starts with 0, and the avail one
> > > is 0 or whatever value the user set with vring_set_base.
> > > * With that feature, the device will read guest's used_idx as
> > > vhost-kernel? We would enable a new ioctl to set it, or expand
> > > set_base to include used_idx, effectively diverting from vhost-kernel?
> >
> > Adding Longpeng who is looking at this.
> >
> 
> Sorry, I'll CC longpeng2@huawei.com in future series like this one.
> 
> > We can leave this to the caller to decide.
> >
> > Btw, a question, at which case the device used index does not equal to
> > the used index in the vring when the device is suspended? I think the
> > correct way is to flush the pending used indexes before suspending.
> > Otherwise we need an API to get pending used indices?
> >
> > >
> > > To me the wisest option is to move this with vhost-kernel. Maybe we
> > > need to add a feature bit to know that the hypervisor can trust the
> > > device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?),
> > > but we should keep it orthogonal to inflight descriptor migration in
> > > my opinion.
> >
> > I think we need to understand if there are any other possible use
> > cases for setting used idx other than inflight stuff.
> >
> 
> Answering this and the previous comment, I cannot think in any case
> outside of inflight migration. I'm just trying to avoid different
> behavior between vhost-kernel and vhost-vdpa, and to make features as
> orthogonal as possible.
> 
> > >
> > > Having said that, I'm totally ok to do it otherwise (or to expand the
> > > patch message if needed).
> >
> > I tend to do that in another series (not mix with the fixes).
> >
> > >
> > > > >
> > > > > > > -       return vringh_init_kern(vrh, features, num, weak_barriers,
> > > > > > > -                               desc, avail, used);
> > > > > > > +       int r = vringh_init_kern(vrh, features, num, weak_barriers, desc,
> > > > > > > +                                avail, used);
> > > > > > > +
> > > > > > > +       if (r != 0)
> > > > > > > +               return r;
> > > > > > > +
> > > > > > > +       /* Consider the ring not initialized */
> > > > > > > +       if ((void *)desc == used)
> > > > > > > +               return 0;
> > > > > >
> > > > > > I don't understand when we can get this (actually it should be a bug
> > > > > > of the caller).
> > > > > >
> > > > >
> > > > > You can see it in vdpasim_vq_reset.
> > > > >
> > > > > Note that to consider desc == 0 to be an uninitialized ring is a bug
> > > > > IMO. QEMU considers it that way also, but the standard does not forbid
> > > > > any ring to be at address 0. Especially if we use vIOMMU.
> > > > >
> > > > > So I think the best way to know if we can use the vringh is either
> > > > > this way, or provide an explicit "initialized" boolean attribute.
> > > > > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to
> > > > > add new attributes.
> > > >
> > > > I wonder if we can avoid this in the simulator level instead of the
> > > > vringh (anyhow it only exposes a vringh_init_xxx() helper now).
> > > >
> > >
> > > In my opinion that is a mistake if other drivers will use it to
> > > implement the emulated control virtqueue. And it requires more
> > > changes. But it is doable for sure.
> >
> > The problem is, there's no reset API in vringh, that's why you need to
> > do if ((void *)desc == used) which depends on behaviour of the vringh
> > user.
> >
> 
> That's a very good point indeed.
> 
> > So I think we should either:
> >
> > 1) move that check in vdpa_sim (since it's not guaranteed that all the
> > vringh users will make desc equal to used during reset)
> >
> > or
> >
> > 2) introduce a vringh_reset_xxx()
> >
> > 1) seems a good step for -stable.
> >
> 
> We can go to 1 for sure. So let's set last_used_idx at
> vdpasim_set_vq_state, same value as last_avail_idx, and stash it at
> vdpasim_queue_ready.
> 
> Do I need to resend the previous patch in this series?
> 
> Do we need to offer a new feature flag indicating we will set used_idx
> with avail_idx?
> 
> Thanks!

Jason did you forget to answer or did I miss it?