Re: [PATCH v4 5/7] vhost-vdpa: clean iotlb map during reset for older userspace

[Si-Wei Liu]

On 10/24/2023 9:21 AM, Si-Wei Liu wrote:
> On 10/23/2023 10:45 PM, Jason Wang wrote:
> > On Sat, Oct 21, 2023 at 5:28 PM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> 
> > wrote:
> > > Using .compat_reset op from the previous patch, the buggy .reset
> > > behaviour can be kept as-is on older userspace apps, which don't ack 
> > > the
> > > IOTLB_PERSIST backend feature. As this compatibility quirk is 
> > > limited to
> > > those drivers that used to be buggy in the past, it won't affect change
> > > the behaviour or affect ABI on the setups with API compliant driver.
> > >
> > > The separation of .compat_reset from the regular .reset allows
> > > vhost-vdpa able to know which driver had broken behaviour before, so it
> > > can apply the corresponding compatibility quirk to the individual 
> > > driver
> > > whenever needed.  Compared to overloading the existing .reset with
> > > flags, .compat_reset won't cause any extra burden to the implementation
> > > of every compliant driver.
> > >
> > > Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> > > ---
> > >   drivers/vhost/vdpa.c         | 17 +++++++++++++----
> > >   drivers/virtio/virtio_vdpa.c |  2 +-
> > >   include/linux/vdpa.h         |  7 +++++--
> > >   3 files changed, 19 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > > index acc7c74ba7d6..9ce40003793b 100644
> > > --- a/drivers/vhost/vdpa.c
> > > +++ b/drivers/vhost/vdpa.c
> > > @@ -227,13 +227,22 @@ static void vhost_vdpa_unsetup_vq_irq(struct 
> > > vhost_vdpa *v, u16 qid)
> > > irq_bypass_unregister_producer(&vq->call_ctx.producer);
> > >   }
> > >
> > > -static int vhost_vdpa_reset(struct vhost_vdpa *v)
> > > +static int _compat_vdpa_reset(struct vhost_vdpa *v)
> > >   {
> > >          struct vdpa_device *vdpa = v->vdpa;
> > > +       u32 flags = 0;
> > >
> > > -       v->in_batch = 0;
> > > +       flags |= !vhost_backend_has_feature(v->vdev.vqs[0],
> > > + VHOST_BACKEND_F_IOTLB_PERSIST) ?
> > > +                VDPA_RESET_F_CLEAN_MAP : 0;
> > > +
> > > +       return vdpa_reset(vdpa, flags);
> > > +}
> > >
> > > -       return vdpa_reset(vdpa);
> > > +static int vhost_vdpa_reset(struct vhost_vdpa *v)
> > > +{
> > > +       v->in_batch = 0;
> > > +       return _compat_vdpa_reset(v);
> > >   }
> > >
> > >   static long vhost_vdpa_bind_mm(struct vhost_vdpa *v)
> > > @@ -312,7 +321,7 @@ static long vhost_vdpa_set_status(struct 
> > > vhost_vdpa *v, u8 __user *statusp)
> > >                          vhost_vdpa_unsetup_vq_irq(v, i);
> > >
> > >          if (status == 0) {
> > > -               ret = vdpa_reset(vdpa);
> > > +               ret = _compat_vdpa_reset(v);
> > >                  if (ret)
> > >                          return ret;
> > >          } else
> > > diff --git a/drivers/virtio/virtio_vdpa.c 
> > > b/drivers/virtio/virtio_vdpa.c
> > > index 06ce6d8c2e00..8d63e5923d24 100644
> > > --- a/drivers/virtio/virtio_vdpa.c
> > > +++ b/drivers/virtio/virtio_vdpa.c
> > > @@ -100,7 +100,7 @@ static void virtio_vdpa_reset(struct 
> > > virtio_device *vdev)
> > >   {
> > >          struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> > >
> > > -       vdpa_reset(vdpa);
> > > +       vdpa_reset(vdpa, 0);
> > >   }
> > >
> > >   static bool virtio_vdpa_notify(struct virtqueue *vq)
> > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > > index 6b8cbf75712d..db15ac07f8a6 100644
> > > --- a/include/linux/vdpa.h
> > > +++ b/include/linux/vdpa.h
> > > @@ -519,14 +519,17 @@ static inline struct device 
> > > *vdpa_get_dma_dev(struct vdpa_device *vdev)
> > >          return vdev->dma_dev;
> > >   }
> > >
> > > -static inline int vdpa_reset(struct vdpa_device *vdev)
> > > +static inline int vdpa_reset(struct vdpa_device *vdev, u32 flags)
> > >   {
> > >          const struct vdpa_config_ops *ops = vdev->config;
> > >          int ret;
> > >
> > >          down_write(&vdev->cf_lock);
> > >          vdev->features_valid = false;
> > > -       ret = ops->reset(vdev);
> > > +       if (ops->compat_reset && flags)
> > > +               ret = ops->compat_reset(vdev, flags);
> > > +       else
> > > +               ret = ops->reset(vdev);
> > Instead of inventing a new API that carries the flags. Tweak the
> > existing one seems to be simpler and better?
> Well, as indicated in the commit message, this allows vhost-vdpa be 
> able to know which driver had broken behavior before, so it
> can apply the corresponding compatibility quirk to the individual 
> driver when it's really necessary. If sending all flags 
> unconditionally down to every driver, it's hard for driver writers to 
> distinguish which are compatibility quirks that they can safely ignore 
> and which are feature flags that are encouraged to implement. In that 
> sense, gating features from being polluted by compatibility quirks 
> with an implicit op 
s/implicit/explicit/
> would be better.
>
> Regards,
> -Siwei
> > As compat_reset(vdev, 0) == reset(vdev)
> >
> > Then you don't need the switch in the parent as well
> >
> > +static int vdpasim_reset(struct vdpa_device *vdpa)
> > +{
> > +       return vdpasim_compat_reset(vdpa, 0);
> > +}
> >
> > Thanks
> >
> >
> > > up_write(&vdev->cf_lock);
> > >          return ret;
> > >   }
> > > --
> > > 2.39.3