[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d71dc008-9920-463d-948a-0375c5754810@oracle.com>
Date: Tue, 24 Oct 2023 09:25:30 -0700
From: Si-Wei Liu <si-wei.liu@...cle.com>
To: Jason Wang <jasowang@...hat.com>
Cc: mst@...hat.com, eperezma@...hat.com, sgarzare@...hat.com,
dtatulea@...dia.com, virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/7] vhost-vdpa: clean iotlb map during reset for older
userspace
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@...cle.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@...cle.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
>>>
>
Powered by blists - more mailing lists