Re: [PATCH v7 03/14] vdpa: Sync calls set/get config/status with cf_mutex

[Si-Wei Liu <si-wei.liu@xxxxxxxxxx>]

On 1/5/2022 3:46 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.
>
> Signed-off-by: Eli Cohen <elic@xxxxxxxxxx>
> ---
>   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);
Not sure why we need to take cf_mutex here. Appears to me only setters 
(set_status and reset) need to take the lock in this function.

>   
>   	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);
Ditto.

>   
>   	/*
>   	 * 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);
The reset() call in the if branch above needs to take the cf_mutex, the 
same way as that for set_status(). The reset() is effectively 
set_status(vdpa, 0).

Thanks,
-Siwei

>   
>   	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

_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization