thr3ads.net - Virtualization - [PATCH linux-next v2 03/14] vdpa: Introduce and use vdpa device get,set config, features helpers [Apr 2021]

If this information is useful, please help other people find it:
Share via:

Parav Pandit

2021-Apr-06 17:04 UTC

[PATCH linux-next v2 03/14] vdpa: Introduce and use vdpa device get, set config, features helpers

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 at nvidia.com>
Reviewed-by: Eli Cohen <elic at nvidia.com>
---
changelog:
v1->v2
 - new patch to have synchronized access to features and config space
---
 drivers/vdpa/vdpa.c  | 57 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vdpa.c |  6 ++---
 include/linux/vdpa.h | 28 +++++-----------------
 3 files changed, 65 insertions(+), 26 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index bb3f1d1f0422..116e076c72fd 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -261,6 +261,63 @@ int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev)
 }
 EXPORT_SYMBOL_GPL(vdpa_mgmtdev_register);
 
+/**
+ * vdpa_get_features - Read vDPA device features
+ * @vdev: vdpa device whose features to read
+ *
+ * Return: Returns device features.
+ */
+u64 vdpa_get_features(struct vdpa_device *vdev)
+{
+	return vdev->config->get_features(vdev);
+}
+EXPORT_SYMBOL_GPL(vdpa_get_features);
+
+/**
+ * vdpa_set_features - Set vDPA device features
+ * @vdev: vdpa device whose features to set
+ * @features: features of the vdpa device to enable/disable
+ *
+ * Return: Returns 0 on success or error code.
+ */
+int vdpa_set_features(struct vdpa_device *vdev, u64 features)
+{
+	const struct vdpa_config_ops *ops = vdev->config;
+
+	vdev->features_valid = true;
+	return ops->set_features(vdev, features);
+}
+EXPORT_SYMBOL_GPL(vdpa_set_features);
+
+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.
+ * @dev: 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 *dev, unsigned int offset,
+		     void *buf, unsigned int length)
+{
+	dev->config->set_config(dev, offset, buf, length);
+}
+EXPORT_SYMBOL_GPL(vdpa_set_config);
+
 static int vdpa_match_remove(struct device *dev, void *data)
 {
 	struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index e0a27e336293..383f5be8ffa6 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -236,7 +236,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;
@@ -250,7 +249,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;
@@ -259,10 +258,9 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)
 {
 	struct vdpa_device *vdpa = v->vdpa;
-	const struct vdpa_config_ops *ops = vdpa->config;
 	u64 features;
 
-	features = ops->get_features(vdpa);
+	features = vdpa_get_features(vdpa);
 
 	if (copy_to_user(featurep, &features, sizeof(features)))
 		return -EFAULT;
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 37b65ca940cf..62e68ccc4c96 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -320,28 +320,12 @@ static inline void vdpa_reset(struct vdpa_device *vdev)
         ops->set_status(vdev, 0);
 }
 
-static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
-{
-        const struct vdpa_config_ops *ops = vdev->config;
-
-	vdev->features_valid = true;
-        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);
-}
+u64 vdpa_get_features(struct vdpa_device *vdev);
+int vdpa_set_features(struct vdpa_device *vdev, u64 features);
+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
-- 
2.26.2

Jason Wang

2021-Apr-07 03:18 UTC

head link

[PATCH linux-next v2 03/14] vdpa: Introduce and use vdpa device get,set config, features helpers

? 2021/4/7 ??1:04, 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 at nvidia.com>
> Reviewed-by: Eli Cohen <elic at nvidia.com>
> ---
> changelog:
> v1->v2
>   - new patch to have synchronized access to features and config space
> ---
>   drivers/vdpa/vdpa.c  | 57 ++++++++++++++++++++++++++++++++++++++++++++
>   drivers/vhost/vdpa.c |  6 ++---
>   include/linux/vdpa.h | 28 +++++-----------------
>   3 files changed, 65 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index bb3f1d1f0422..116e076c72fd 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -261,6 +261,63 @@ int vdpa_mgmtdev_register(struct vdpa_mgmt_dev *mdev)
>   }
>   EXPORT_SYMBOL_GPL(vdpa_mgmtdev_register);
>   
> +/**
> + * vdpa_get_features - Read vDPA device features
> + * @vdev: vdpa device whose features to read
> + *
> + * Return: Returns device features.
> + */
> +u64 vdpa_get_features(struct vdpa_device *vdev)
> +{
> +	return vdev->config->get_features(vdev);
> +}
> +EXPORT_SYMBOL_GPL(vdpa_get_features);
> +
> +/**
> + * vdpa_set_features - Set vDPA device features
> + * @vdev: vdpa device whose features to set
> + * @features: features of the vdpa device to enable/disable
> + *
> + * Return: Returns 0 on success or error code.
> + */
> +int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> +{
> +	const struct vdpa_config_ops *ops = vdev->config;
> +
> +	vdev->features_valid = true;
> +	return ops->set_features(vdev, features);
> +}
> +EXPORT_SYMBOL_GPL(vdpa_set_features);
> +

Let's add a doc for this function?

> +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.
> + * @dev: 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 *dev, unsigned int offset,
> +		     void *buf, unsigned int length)
> +{
> +	dev->config->set_config(dev, offset, buf, length);
> +}
> +EXPORT_SYMBOL_GPL(vdpa_set_config);
> +
>   static int vdpa_match_remove(struct device *dev, void *data)
>   {
>   	struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index e0a27e336293..383f5be8ffa6 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c

We probably need to convert drivers/virtio/vdpa.c as well.

> @@ -236,7 +236,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;
> @@ -250,7 +249,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;
> @@ -259,10 +258,9 @@ static long vhost_vdpa_set_config(struct vhost_vdpa
*v,
>   static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user
*featurep)
>   {
>   	struct vdpa_device *vdpa = v->vdpa;
> -	const struct vdpa_config_ops *ops = vdpa->config;
>   	u64 features;
>   
> -	features = ops->get_features(vdpa);
> +	features = vdpa_get_features(vdpa);
>   
>   	if (copy_to_user(featurep, &features, sizeof(features)))
>   		return -EFAULT;
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 37b65ca940cf..62e68ccc4c96 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -320,28 +320,12 @@ static inline void vdpa_reset(struct vdpa_device
*vdev)
>           ops->set_status(vdev, 0);
>   }
>   
> -static inline int vdpa_set_features(struct vdpa_device *vdev, u64
features)
> -{
> -        const struct vdpa_config_ops *ops = vdev->config;
> -
> -	vdev->features_valid = true;
> -        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);
> -}
> +u64 vdpa_get_features(struct vdpa_device *vdev);
> +int vdpa_set_features(struct vdpa_device *vdev, u64 features);
> +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);

I think it's better to use a separate patch for this moving.

Thanks

>   
>   /**
>    * struct vdpa_mgmtdev_ops - vdpa device ops

Virtualization - Apr 2021 - [PATCH linux-next v2 03/14] vdpa: Introduce and use vdpa device get,set config, features helpers

[PATCH linux-next v2 03/14] vdpa: Introduce and use vdpa device get, set config, features helpers

[PATCH linux-next v2 03/14] vdpa: Introduce and use vdpa device get,set config, features helpers