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

[Jason Wang]

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?

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
>