Virtualization - Dec 2021 - [PATCH v6 03/13] vdpa: Sync calls set/get config/status with cf_mutex

On 12/22/2021 9:46 PM, Eli Cohen wrote:
> On Wed, Dec 22, 2021 at 02:58:18PM -0800, Si-Wei Liu wrote:
>>
>> On 12/22/2021 6:20 AM, Eli Cohen wrote:
>>> Add wrappers to get/set status and protect these operations with
>>> cf_mutex to serialize these operations with respect to get/set
config
>>> operations.
>> Need to protect vdpa_reset() which is essentially vdpa_set_status(0)
> And what about vdpa_set_features()?
Oh, that was missed... Indeed, it also needs to take the lock.
> Maybe it should be done as part of another patch to address this?
Isn't this patch the one adding the helpers for the locked API? I think 
it's fine if you'd like specific ones for reset or set_features.
>
> Shouldn't all these serializations requirements be handled by the
caller?
If the caller remains in the vdpa core I think it should be fine. Or you 
want the vdpa bus drivers to take the serialization requirement? Perhaps 
not so good.

Thanks,
-Siwei
>
>> -Siwei
>>> Signed-off-by: Eli Cohen <elic at nvidia.com>
>>> ---
>>>    drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++
>>>    drivers/vhost/vdpa.c         |  7 +++----
>>>    drivers/virtio/virtio_vdpa.c |  3 +--
>>>    include/linux/vdpa.h         |  3 +++
>>>    4 files changed, 26 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 42d71d60d5dc..5134c83c4a22 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
>>>    static DEFINE_MUTEX(vdpa_dev_mutex);
>>>    static DEFINE_IDA(vdpa_index_ida);
>>> +u8 vdpa_get_status(struct vdpa_device *vdev)
>>> +{
>>> +	u8 status;
>>> +
>>> +	mutex_lock(&vdev->cf_mutex);
>>> +	status = vdev->config->get_status(vdev);
>>> +	mutex_unlock(&vdev->cf_mutex);
>>> +	return status;
>>> +}
>>> +EXPORT_SYMBOL(vdpa_get_status);
>>> +
>>> +void vdpa_set_status(struct vdpa_device *vdev, u8 status)
>>> +{
>>> +	mutex_lock(&vdev->cf_mutex);
>>> +	vdev->config->set_status(vdev, status);
>>> +	mutex_unlock(&vdev->cf_mutex);
>>> +}
>>> +EXPORT_SYMBOL(vdpa_set_status);
>>> +
>>>    static struct genl_family vdpa_nl_family;
>>>    static int vdpa_dev_probe(struct device *d)
>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>> index ebaa373e9b82..d9d499465e2e 100644
>>> --- a/drivers/vhost/vdpa.c
>>> +++ b/drivers/vhost/vdpa.c
>>> @@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct
vhost_vdpa *v, u8 __user *argp)
>>>    static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8
__user *statusp)
>>>    {
>>>    	struct vdpa_device *vdpa = v->vdpa;
>>> -	const struct vdpa_config_ops *ops = vdpa->config;
>>>    	u8 status;
>>> -	status = ops->get_status(vdpa);
>>> +	status = vdpa_get_status(vdpa);
>>>    	if (copy_to_user(statusp, &status, sizeof(status)))
>>>    		return -EFAULT;
>>> @@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct
vhost_vdpa *v, u8 __user *statusp)
>>>    	if (copy_from_user(&status, statusp, sizeof(status)))
>>>    		return -EFAULT;
>>> -	status_old = ops->get_status(vdpa);
>>> +	status_old = vdpa_get_status(vdpa);
>>>    	/*
>>>    	 * Userspace shouldn't remove status bits unless reset the
>>> @@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct
vhost_vdpa *v, u8 __user *statusp)
>>>    		if (ret)
>>>    			return ret;
>>>    	} else
>>> -		ops->set_status(vdpa, status);
>>> +		vdpa_set_status(vdpa, status);
>>>    	if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
!(status_old & VIRTIO_CONFIG_S_DRIVER_OK))
>>>    		for (i = 0; i < nvqs; i++)
>>> diff --git a/drivers/virtio/virtio_vdpa.c
b/drivers/virtio/virtio_vdpa.c
>>> index a84b04ba3195..76504559bc25 100644
>>> --- a/drivers/virtio/virtio_vdpa.c
>>> +++ b/drivers/virtio/virtio_vdpa.c
>>> @@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct
virtio_device *vdev)
>>>    static void virtio_vdpa_set_status(struct virtio_device *vdev,
u8 status)
>>>    {
>>>    	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
>>> -	const struct vdpa_config_ops *ops = vdpa->config;
>>> -	return ops->set_status(vdpa, status);
>>> +	return vdpa_set_status(vdpa, status);
>>>    }
>>>    static void virtio_vdpa_reset(struct virtio_device *vdev)
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index 9cc4291a79b3..ae047fae2603 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev,
unsigned int offset,
>>>    		     void *buf, unsigned int len);
>>>    void vdpa_set_config(struct vdpa_device *dev, unsigned int
offset,
>>>    		     const void *buf, unsigned int length);
>>> +u8 vdpa_get_status(struct vdpa_device *vdev);
>>> +void vdpa_set_status(struct vdpa_device *vdev, u8 status);
>>> +
>>>    /**
>>>     * struct vdpa_mgmtdev_ops - vdpa device ops
>>>     * @dev_add: Add a vdpa device using alloc and register