[PATCH 0/2] Fix expected set_vq_state behavior on vdpa_sim

[PATCH 0/2] Fix expected set_vq_state behavior on vdpa_sim

[Eugenio Pérez]

The use of set_vq_state is to indicate vdpa device the state of a virtqueue.
In the case of split, it means the avail_idx.  This is mandatory for use
cases like live migration.

However, vdpa_sim reset the vq state at vdpasim_queue_ready since it calls
vringh_init_iotlb.

Also, to 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.

This series fixes both problems allowing to migrate to a vdpa_sim_net device.

Eugenio Pérez (2):
  vdpa_sim: not reset state in vdpasim_queue_ready
  vringh: fetch used_idx from vring at vringh_init_iotlb

 drivers/vdpa/vdpa_sim/vdpa_sim.c |  2 ++
 drivers/vhost/vringh.c           | 25 +++++++++++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH 1/2] vdpa_sim: not reset state in vdpasim_queue_ready

[Eugenio Pérez]

vdpasim_queue_ready calls vringh_init_iotlb, which resets split indexes.
But it can be called after setting a ring base with
vdpasim_set_vq_state.

Fix it by stashing them. They're still resetted in vdpasim_vq_reset.

This was discovered and tested live migrating the vdpa_sim_net device.

Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index cb88891b44a8..8839232a3fcb 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -66,6 +66,7 @@ static void vdpasim_vq_notify(struct vringh *vring)
 static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
 {
 	struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
+	uint16_t last_avail_idx = vq->vring.last_avail_idx;
 
 	vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
 			  (struct vring_desc *)(uintptr_t)vq->desc_addr,
@@ -74,6 +75,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
 			  (struct vring_used *)
 			  (uintptr_t)vq->device_addr);
 
+	vq->vring.last_avail_idx = last_avail_idx;
 	vq->vring.notify = vdpasim_vq_notify;
 }
 
-- 
2.31.1

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

[Eugenio Pérez]

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)
 {
-	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

Re: [PATCH 1/2] vdpa_sim: not reset state in vdpasim_queue_ready

[Jason Wang]

On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> vdpasim_queue_ready calls vringh_init_iotlb, which resets split indexes.
> But it can be called after setting a ring base with
> vdpasim_set_vq_state.
>
> Fix it by stashing them. They're still resetted in vdpasim_vq_reset.
>
> This was discovered and tested live migrating the vdpa_sim_net device.
>
> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> index cb88891b44a8..8839232a3fcb 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> @@ -66,6 +66,7 @@ static void vdpasim_vq_notify(struct vringh *vring)
>  static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
>  {
>         struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> +       uint16_t last_avail_idx = vq->vring.last_avail_idx;
>
>         vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
>                           (struct vring_desc *)(uintptr_t)vq->desc_addr,
> @@ -74,6 +75,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
>                           (struct vring_used *)
>                           (uintptr_t)vq->device_addr);
>
> +       vq->vring.last_avail_idx = last_avail_idx;

Does this need to be serialized with the datapath?

E.g in set_vq_state() we do:

spin_lock(&vdpasim->lock);
vrh->last_avail_idx = state->split.avail_index;
spin_unlock(&vdpasim->lock);

Thanks

>         vq->vring.notify = vdpasim_vq_notify;
>  }
>
> --
> 2.31.1
>

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

[Jason Wang]

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
>

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

[Eugenio Perez Martin]

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
> >
>

Re: [PATCH 1/2] vdpa_sim: not reset state in vdpasim_queue_ready

[Eugenio Perez Martin]

On Thu, Jan 19, 2023 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > vdpasim_queue_ready calls vringh_init_iotlb, which resets split indexes.
> > But it can be called after setting a ring base with
> > vdpasim_set_vq_state.
> >
> > Fix it by stashing them. They're still resetted in vdpasim_vq_reset.
> >
> > This was discovered and tested live migrating the vdpa_sim_net device.
> >
> > Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > index cb88891b44a8..8839232a3fcb 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> > @@ -66,6 +66,7 @@ static void vdpasim_vq_notify(struct vringh *vring)
> >  static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
> >  {
> >         struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> > +       uint16_t last_avail_idx = vq->vring.last_avail_idx;
> >
> >         vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
> >                           (struct vring_desc *)(uintptr_t)vq->desc_addr,
> > @@ -74,6 +75,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
> >                           (struct vring_used *)
> >                           (uintptr_t)vq->device_addr);
> >
> > +       vq->vring.last_avail_idx = last_avail_idx;
>
> Does this need to be serialized with the datapath?
>
> E.g in set_vq_state() we do:
>
> spin_lock(&vdpasim->lock);
> vrh->last_avail_idx = state->split.avail_index;
> spin_unlock(&vdpasim->lock);
>

vdpasim_queue_ready is called from vdpasim_set_vq_ready, which holds
these locks.

Maybe it's too much indirection and to embed vdpasim_queue_ready in
vdpasim_set_vq_ready would be clearer for the future?

Thanks!

Re: [PATCH 0/2] Fix expected set_vq_state behavior on vdpa_sim

[Michael S. Tsirkin]

On Wed, Jan 18, 2023 at 05:43:57PM +0100, Eugenio Pérez wrote:
> The use of set_vq_state is to indicate vdpa device the state of a virtqueue.
> In the case of split, it means the avail_idx.  This is mandatory for use
> cases like live migration.
> 
> However, vdpa_sim reset the vq state at vdpasim_queue_ready since it calls
> vringh_init_iotlb.
> 
> Also, to 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.
> 
> This series fixes both problems allowing to migrate to a vdpa_sim_net device.

Jason problems you pointed out are all consmetic do you ack
the patchset? Or expect another revision?

> Eugenio Pérez (2):
>   vdpa_sim: not reset state in vdpasim_queue_ready
>   vringh: fetch used_idx from vring at vringh_init_iotlb
> 
>  drivers/vdpa/vdpa_sim/vdpa_sim.c |  2 ++
>  drivers/vhost/vringh.c           | 25 +++++++++++++++++++++++--
>  2 files changed, 25 insertions(+), 2 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH 1/2] vdpa_sim: not reset state in vdpasim_queue_ready

[Jason Wang]

在 2023/1/19 17:14, Eugenio Perez Martin 写道:
> On Thu, Jan 19, 2023 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>>> vdpasim_queue_ready calls vringh_init_iotlb, which resets split indexes.
>>> But it can be called after setting a ring base with
>>> vdpasim_set_vq_state.
>>>
>>> Fix it by stashing them. They're still resetted in vdpasim_vq_reset.
>>>
>>> This was discovered and tested live migrating the vdpa_sim_net device.
>>>
>>> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>>   drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> index cb88891b44a8..8839232a3fcb 100644
>>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
>>> @@ -66,6 +66,7 @@ static void vdpasim_vq_notify(struct vringh *vring)
>>>   static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
>>>   {
>>>          struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
>>> +       uint16_t last_avail_idx = vq->vring.last_avail_idx;
>>>
>>>          vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
>>>                            (struct vring_desc *)(uintptr_t)vq->desc_addr,
>>> @@ -74,6 +75,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
>>>                            (struct vring_used *)
>>>                            (uintptr_t)vq->device_addr);
>>>
>>> +       vq->vring.last_avail_idx = last_avail_idx;
>> Does this need to be serialized with the datapath?
>>
>> E.g in set_vq_state() we do:
>>
>> spin_lock(&vdpasim->lock);
>> vrh->last_avail_idx = state->split.avail_index;
>> spin_unlock(&vdpasim->lock);
>>
> vdpasim_queue_ready is called from vdpasim_set_vq_ready, which holds
> these locks.
>
> Maybe it's too much indirection and to embed vdpasim_queue_ready in
> vdpasim_set_vq_ready would be clearer for the future?


Nope, I miss the caller.

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks


>
> Thanks!
>

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

[Jason Wang]

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
> > >
> >
>

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@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!

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

[Jason Wang]

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!
>

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

[Eugenio Perez Martin]

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!

Re: [PATCH 1/2] vdpa_sim: not reset state in vdpasim_queue_ready

[Lei Yang]

The patch was tested by QE in a test environment and regression tested
using vdpa_sim device with virtio_vdpa and vhost_vdpa;There are no new
issues caused by this patch.

Tested-by: Lei Yang <leiyang@redhat.com>

Jason Wang <jasowang@redhat.com> 于2023年1月29日周日 13:56写道：
>
>
> 在 2023/1/19 17:14, Eugenio Perez Martin 写道:
> > On Thu, Jan 19, 2023 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote:
> >> On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >>> vdpasim_queue_ready calls vringh_init_iotlb, which resets split indexes.
> >>> But it can be called after setting a ring base with
> >>> vdpasim_set_vq_state.
> >>>
> >>> Fix it by stashing them. They're still resetted in vdpasim_vq_reset.
> >>>
> >>> This was discovered and tested live migrating the vdpa_sim_net device.
> >>>
> >>> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> ---
> >>>   drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >>> index cb88891b44a8..8839232a3fcb 100644
> >>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
> >>> @@ -66,6 +66,7 @@ static void vdpasim_vq_notify(struct vringh *vring)
> >>>   static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
> >>>   {
> >>>          struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx];
> >>> +       uint16_t last_avail_idx = vq->vring.last_avail_idx;
> >>>
> >>>          vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false,
> >>>                            (struct vring_desc *)(uintptr_t)vq->desc_addr,
> >>> @@ -74,6 +75,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
> >>>                            (struct vring_used *)
> >>>                            (uintptr_t)vq->device_addr);
> >>>
> >>> +       vq->vring.last_avail_idx = last_avail_idx;
> >> Does this need to be serialized with the datapath?
> >>
> >> E.g in set_vq_state() we do:
> >>
> >> spin_lock(&vdpasim->lock);
> >> vrh->last_avail_idx = state->split.avail_index;
> >> spin_unlock(&vdpasim->lock);
> >>
> > vdpasim_queue_ready is called from vdpasim_set_vq_ready, which holds
> > these locks.
> >
> > Maybe it's too much indirection and to embed vdpasim_queue_ready in
> > vdpasim_set_vq_ready would be clearer for the future?
>
>
> Nope, I miss the caller.
>
> Acked-by: Jason Wang <jasowang@redhat.com>
>
> Thanks
>
>
> >
> > Thanks!
> >
>

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

[Michael S. Tsirkin]

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

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

[Eugenio Perez Martin]

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
>

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

[Michael S. Tsirkin]

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?