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

[Eugenio Perez Martin]

On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin
> <eperezma@xxxxxxxxxx> wrote:
> >
> > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >  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!