Re: [PATCH linux-next v3 1/6] vdpa: Introduce and use vdpa device get, set config helpers

[Jason Wang <jasowang@xxxxxxxxxx>]

在 2021/6/17 上午3:11, Parav Pandit 写道:
> Subsequent patches enable get and set configuration either
> via management device or via vdpa device' config ops.
>
> This requires synchronization between multiple callers to get and set
> config callbacks. Features setting also influence the layout of the
> configuration fields endianness.
>
> To avoid exposing synchronization primitives to callers, introduce
> helper for setting the configuration and use it.
>
> Signed-off-by: Parav Pandit <parav@xxxxxxxxxx>
> Reviewed-by: Eli Cohen <elic@xxxxxxxxxx>


Acked-by: Jason Wang <jasowang@xxxxxxxxxx>


> ---
> changelog:
> v1->v2
>   - new patch to have synchronized access to features and config space
> ---
>   drivers/vdpa/vdpa.c  | 36 ++++++++++++++++++++++++++++++++++++
>   drivers/vhost/vdpa.c |  3 +--
>   include/linux/vdpa.h | 18 ++++--------------
>   3 files changed, 41 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index bb3f1d1f0422..bc44cdc34114 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -284,6 +284,42 @@ void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev)
>   }
>   EXPORT_SYMBOL_GPL(vdpa_mgmtdev_unregister);
>   
> +/**
> + * vdpa_get_config - Get one or more device configuration fields.
> + * @vdev: vdpa device to operate on
> + * @offset: starting byte offset of the field
> + * @buf: buffer pointer to read to
> + * @len: length of the configuration fields in bytes
> + */
> +void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> +		     void *buf, unsigned int len)
> +{
> +	const struct vdpa_config_ops *ops = vdev->config;
> +
> +	/*
> +	 * Config accesses aren't supposed to trigger before features are set.
> +	 * If it does happen we assume a legacy guest.
> +	 */
> +	if (!vdev->features_valid)
> +		vdpa_set_features(vdev, 0);
> +	ops->get_config(vdev, offset, buf, len);
> +}
> +EXPORT_SYMBOL_GPL(vdpa_get_config);
> +
> +/**
> + * vdpa_set_config - Set one or more device configuration fields.
> + * @vdev: vdpa device to operate on
> + * @offset: starting byte offset of the field
> + * @buf: buffer pointer to read from
> + * @length: length of the configuration fields in bytes
> + */
> +void vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
> +		     void *buf, unsigned int length)
> +{
> +	vdev->config->set_config(vdev, offset, buf, length);
> +}
> +EXPORT_SYMBOL_GPL(vdpa_set_config);
> +
>   static bool mgmtdev_handle_match(const struct vdpa_mgmt_dev *mdev,
>   				 const char *busname, const char *devname)
>   {
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index fb41db3da611..908b4fb251b3 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -231,7 +231,6 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
>   				  struct vhost_vdpa_config __user *c)
>   {
>   	struct vdpa_device *vdpa = v->vdpa;
> -	const struct vdpa_config_ops *ops = vdpa->config;
>   	struct vhost_vdpa_config config;
>   	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
>   	u8 *buf;
> @@ -245,7 +244,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
>   	if (IS_ERR(buf))
>   		return PTR_ERR(buf);
>   
> -	ops->set_config(vdpa, config.off, buf, config.len);
> +	vdpa_set_config(vdpa, config.off, buf, config.len);
>   
>   	kvfree(buf);
>   	return 0;
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index f311d227aa1b..993d99519452 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -332,20 +332,10 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
>           return ops->set_features(vdev, features);
>   }
>   
> -
> -static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset,
> -				   void *buf, unsigned int len)
> -{
> -        const struct vdpa_config_ops *ops = vdev->config;
> -
> -	/*
> -	 * Config accesses aren't supposed to trigger before features are set.
> -	 * If it does happen we assume a legacy guest.
> -	 */
> -	if (!vdev->features_valid)
> -		vdpa_set_features(vdev, 0);
> -	ops->get_config(vdev, offset, buf, len);
> -}
> +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,
> +		     void *buf, unsigned int length);
>   
>   /**
>    * struct vdpa_mgmtdev_ops - vdpa device ops

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