Messages in this thread | | | Date | Tue, 24 Oct 2023 09:25:30 -0700 | Subject | Re: [PATCH v4 5/7] vhost-vdpa: clean iotlb map during reset for older userspace | From | 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@oracle.com> >> 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@oracle.com> >>> --- >>> 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 >>> >
| |