[RFC PATCH 00/10] vdpa: get/set_config() rework

Following the discussion with Michael and Jason [1], I reworked a bit
get/set_config() in vdpa.

I changed vdpa_get_config() to check the boundaries and added vdpa_set_config().
When 'offset' or 'len' parameters exceed boundaries, we limit the reading to
the available configuration space in the device, and we return the amount of
bytes read/written.

In this way the user space can pass buffers bigger than config space.
I also returned the amount of bytes read and written to user space.

Patches also available here:
https://github.com/stefano-garzarella/linux/tree/vdpa-get-set-config-refactoring

Thanks for your comments,
Stefano

[1] https://lkml.org/lkml/2021/2/10/350

Stefano Garzarella (10):
  vdpa: add get_config_size callback in vdpa_config_ops
  vdpa: check vdpa_get_config() parameters and return bytes read
  vdpa: add vdpa_set_config() helper
  vdpa: remove param checks in the get/set_config callbacks
  vdpa: remove WARN_ON() in the get/set_config callbacks
  virtio_vdpa: use vdpa_set_config()
  vhost/vdpa: use vdpa_set_config()
  vhost/vdpa: allow user space to pass buffers bigger than config space
  vhost/vdpa: use get_config_size callback in
    vhost_vdpa_config_validate()
  vhost/vdpa: return configuration bytes read and written to user space

 include/linux/vdpa.h              | 22 ++++-------
 drivers/vdpa/ifcvf/ifcvf_base.c   |  3 +-
 drivers/vdpa/ifcvf/ifcvf_main.c   |  8 +++-
 drivers/vdpa/mlx5/net/mlx5_vnet.c |  9 ++++-
 drivers/vdpa/vdpa.c               | 51 ++++++++++++++++++++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 15 +++++---
 drivers/vhost/vdpa.c              | 64 ++++++++++++++++---------------
 drivers/virtio/virtio_vdpa.c      |  3 +-
 8 files changed, 116 insertions(+), 59 deletions(-)

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

This new callback is used to get the size of the configuration space
of vDPA devices.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/linux/vdpa.h              | 4 ++++
 drivers/vdpa/ifcvf/ifcvf_main.c   | 6 ++++++
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 9 +++++++++
 4 files changed, 25 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 4ab5494503a8..fddf42b17573 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -150,6 +150,9 @@ struct vdpa_iova_range {
  * @set_status:			Set the device status
  *				@vdev: vdpa device
  *				@status: virtio device status
+ * @get_config_size:		Get the size of the configuration space
+ *				@vdev: vdpa device
+ *				Returns size_t: configuration size
  * @get_config:			Read from device specific configuration space
  *				@vdev: vdpa device
  *				@offset: offset from the beginning of

@@ -231,6 +234,7 @@ struct vdpa_config_ops {
 	u32 (*get_vendor_id)(struct vdpa_device *vdev);
 	u8 (*get_status)(struct vdpa_device *vdev);
 	void (*set_status)(struct vdpa_device *vdev, u8 status);
+	size_t (*get_config_size)(struct vdpa_device *vdev);
 	void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
 			   void *buf, unsigned int len);
 	void (*set_config)(struct vdpa_device *vdev, unsigned int offset,

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 7c8bbfcf6c3e..2443271e17d2 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c

@@ -332,6 +332,11 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
 	return IFCVF_QUEUE_ALIGNMENT;
 }
 
+static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
+{
+	return sizeof(struct virtio_net_config);
+}
+
 static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
 				  unsigned int offset,
 				  void *buf, unsigned int len)

@@ -392,6 +397,7 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
 	.get_device_id	= ifcvf_vdpa_get_device_id,
 	.get_vendor_id	= ifcvf_vdpa_get_vendor_id,
 	.get_vq_align	= ifcvf_vdpa_get_vq_align,
+	.get_config_size	= ifcvf_vdpa_get_config_size,
 	.get_config	= ifcvf_vdpa_get_config,
 	.set_config	= ifcvf_vdpa_set_config,
 	.set_config_cb  = ifcvf_vdpa_set_config_cb,

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 10e9b09932eb..78043ee567b6 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1814,6 +1814,11 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 	ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
 }
 
+static size_t mlx5_vdpa_get_config_size(struct vdpa_device *vdev)
+{
+	return sizeof(struct virtio_net_config);
+}
+
 static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, void *buf,
 				 unsigned int len)
 {

@@ -1900,6 +1905,7 @@ static const struct vdpa_config_ops mlx5_vdpa_ops = {
 	.get_vendor_id = mlx5_vdpa_get_vendor_id,
 	.get_status = mlx5_vdpa_get_status,
 	.set_status = mlx5_vdpa_set_status,
+	.get_config_size = mlx5_vdpa_get_config_size,
 	.get_config = mlx5_vdpa_get_config,
 	.set_config = mlx5_vdpa_set_config,
 	.get_generation = mlx5_vdpa_get_generation,

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index d5942842432d..779ae6c144d7 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c

@@ -439,6 +439,13 @@ static void vdpasim_set_status(struct vdpa_device *vdpa, u8 status)
 	spin_unlock(&vdpasim->lock);
 }
 
+static size_t vdpasim_get_config_size(struct vdpa_device *vdpa)
+{
+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+	return vdpasim->dev_attr.config_size;
+}
+
 static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset,
 			     void *buf, unsigned int len)
 {

@@ -566,6 +573,7 @@ static const struct vdpa_config_ops vdpasim_config_ops = {
 	.get_vendor_id          = vdpasim_get_vendor_id,
 	.get_status             = vdpasim_get_status,
 	.set_status             = vdpasim_set_status,
+	.get_config_size        = vdpasim_get_config_size,
 	.get_config             = vdpasim_get_config,
 	.set_config             = vdpasim_set_config,
 	.get_generation         = vdpasim_get_generation,

@@ -593,6 +601,7 @@ static const struct vdpa_config_ops vdpasim_batch_config_ops = {
 	.get_vendor_id          = vdpasim_get_vendor_id,
 	.get_status             = vdpasim_get_status,
 	.set_status             = vdpasim_set_status,
+	.get_config_size        = vdpasim_get_config_size,
 	.get_config             = vdpasim_get_config,
 	.set_config             = vdpasim_set_config,
 	.get_generation         = vdpasim_get_generation,

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Now we have the 'get_config_size()' callback available, so we can
check that 'offset' and 'len' parameters are valid.

When these exceed boundaries, we limit the reading to the available
configuration space in the device, and we return the amount of bytes
read.

We also move vdpa_get_config() implementation in drivers/vdpa/vdpa.c,
since the function are growing.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/linux/vdpa.h | 16 ++--------------
 drivers/vdpa/vdpa.c  | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index fddf42b17573..8a679c98f8b1 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -332,20 +332,8 @@ 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);
-}
+int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
+		    void *buf, unsigned int len);
 
 /**
  * vdpa_mgmtdev_ops - vdpa device ops

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 3d997b389345..9ed6c779c63c 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c

@@ -51,6 +51,41 @@ static struct bus_type vdpa_bus = {
 	.remove = vdpa_dev_remove,
 };
 
+static int vdpa_config_size_wrap(struct vdpa_device *vdev, unsigned int offset,
+				 unsigned int len)
+{
+	const struct vdpa_config_ops *ops = vdev->config;
+	unsigned int config_size = ops->get_config_size(vdev);
+
+	if (offset > config_size || len > config_size)
+		return -1;
+
+	return min(len, config_size - offset);
+}
+
+int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
+		    void *buf, unsigned int len)
+{
+	const struct vdpa_config_ops *ops = vdev->config;
+	int bytes_get;
+
+	bytes_get = vdpa_config_size_wrap(vdev, offset, len);
+	if (bytes_get <= 0)
+		return bytes_get;
+
+	/*
+	 * 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, bytes_get);
+
+	return bytes_get;
+}
+EXPORT_SYMBOL_GPL(vdpa_get_config);
+
 static void vdpa_release_dev(struct device *d)
 {
 	struct vdpa_device *vdev = dev_to_vdpa(d);

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Let's add a function similar to vpda_get_config() to check the
'offset' and 'len' parameters, call the set_config() device callback,
and return the amount of bytes written.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/linux/vdpa.h |  2 ++
 drivers/vdpa/vdpa.c  | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 8a679c98f8b1..562fcd14f4b5 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -334,6 +334,8 @@ static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
 
 int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
 		    void *buf, unsigned int len);
+int vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
+		    const void *buf, unsigned int len);
 
 /**
  * vdpa_mgmtdev_ops - vdpa device ops

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 9ed6c779c63c..825afc690a7e 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c

@@ -86,6 +86,22 @@ int vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
 }
 EXPORT_SYMBOL_GPL(vdpa_get_config);
 
+int vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
+		    const void *buf, unsigned int len)
+{
+	const struct vdpa_config_ops *ops = vdev->config;
+	int bytes_set;
+
+	bytes_set = vdpa_config_size_wrap(vdev, offset, len);
+	if (bytes_set <= 0)
+		return bytes_set;
+
+	ops->set_config(vdev, offset, buf, bytes_set);
+
+	return bytes_set;
+}
+EXPORT_SYMBOL_GPL(vdpa_set_config);
+
 static void vdpa_release_dev(struct device *d)
 {
 	struct vdpa_device *vdev = dev_to_vdpa(d);

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

vdpa_get_config() and vdpa_set_config() now check parameters before
calling callbacks, so we can remove these redundant checks.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 +--
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 6 ------
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 78043ee567b6..ab63dc9b8432 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1825,8 +1825,7 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
 	struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
 	struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
 
-	if (offset + len <= sizeof(struct virtio_net_config))
-		memcpy(buf, (u8 *)&ndev->config + offset, len);
+	memcpy(buf, (u8 *)&ndev->config + offset, len);
 }
 
 static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 779ae6c144d7..392180c6f2cf 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c

@@ -451,9 +451,6 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset,
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
 
-	if (offset + len > vdpasim->dev_attr.config_size)
-		return;
-
 	if (vdpasim->dev_attr.get_config)
 		vdpasim->dev_attr.get_config(vdpasim, vdpasim->config);
 

@@ -465,9 +462,6 @@ static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset,
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
 
-	if (offset + len > vdpasim->dev_attr.config_size)
-		return;
-
 	memcpy(vdpasim->config + offset, buf, len);
 
 	if (vdpasim->dev_attr.set_config)

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

vdpa_get_config() and vdpa_set_config() now check parameters before
calling callbacks, so we can remove these warnings.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
Maybe we can skip this patch and leave the WARN_ONs in place.
What do you recommend?
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 3 +--
 drivers/vdpa/ifcvf/ifcvf_main.c | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index f2a128e56de5..5941ecf934d0 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c

@@ -222,7 +222,6 @@ void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
 	u8 old_gen, new_gen, *p;
 	int i;
 
-	WARN_ON(offset + length > sizeof(struct virtio_net_config));
 	do {
 		old_gen = ifc_ioread8(&hw->common_cfg->config_generation);
 		p = dst;

@@ -240,7 +239,7 @@ void ifcvf_write_net_config(struct ifcvf_hw *hw, u64 offset,
 	int i;
 
 	p = src;
-	WARN_ON(offset + length > sizeof(struct virtio_net_config));
+
 	for (i = 0; i < length; i++)
 		ifc_iowrite8(*p++, hw->net_cfg + offset + i);
 }

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 2443271e17d2..e55f88c57461 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c

@@ -343,7 +343,6 @@ static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	WARN_ON(offset + len > sizeof(struct virtio_net_config));
 	ifcvf_read_net_config(vf, offset, buf, len);
 }
 

@@ -353,7 +352,6 @@ static void ifcvf_vdpa_set_config(struct vdpa_device *vdpa_dev,
 {
 	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
 
-	WARN_ON(offset + len > sizeof(struct virtio_net_config));
 	ifcvf_write_net_config(vf, offset, buf, len);
 }
 

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Instead of calling the 'set_config' callback directly, we call the
new vdpa_set_config() helper which also checks the parameters.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/virtio/virtio_vdpa.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index e28acf482e0c..2f1c4a2dd241 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c

@@ -65,9 +65,8 @@ static void virtio_vdpa_set(struct virtio_device *vdev, unsigned offset,
 			    const void *buf, unsigned len)
 {
 	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-	const struct vdpa_config_ops *ops = vdpa->config;
 
-	ops->set_config(vdpa, offset, buf, len);
+	vdpa_set_config(vdpa, offset, buf, len);
 }
 
 static u32 virtio_vdpa_generation(struct virtio_device *vdev)

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Instead of calling the 'set_config' callback directly, we call the
new vdpa_set_config() helper which also checks the parameters.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vdpa.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index ef688c8c0e0e..cdd8f24168b2 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;

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

vdpa_get_config() and vdpa_set_config() now are able to read/write
only the bytes available in the device configuration space, also if
the buffer provided is bigger than that.

Let's use this feature to allow the user space application to pass any
buffer. We limit the size of the internal bounce buffer allocated with
the device config size.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vdpa.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index cdd8f24168b2..544f8582a42b 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -185,10 +185,10 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
 	return 0;
 }
 
-static int vhost_vdpa_config_validate(struct vhost_vdpa *v,
-				      struct vhost_vdpa_config *c)
+static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v,
+					  struct vhost_vdpa_config *c)
 {
-	long size = 0;
+	u32 size = 0;
 
 	switch (v->virtio_id) {
 	case VIRTIO_ID_NET:

@@ -199,10 +199,7 @@ static int vhost_vdpa_config_validate(struct vhost_vdpa *v,
 	if (c->len == 0)
 		return -EINVAL;
 
-	if (c->len > size - c->off)
-		return -E2BIG;
-
-	return 0;
+	return min(c->len, size);
 }
 
 static long vhost_vdpa_get_config(struct vhost_vdpa *v,

@@ -211,19 +208,23 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	struct vdpa_device *vdpa = v->vdpa;
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
+	ssize_t config_size;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))
 		return -EFAULT;
-	if (vhost_vdpa_config_validate(v, &config))
-		return -EINVAL;
-	buf = kvzalloc(config.len, GFP_KERNEL);
+
+	config_size = vhost_vdpa_config_validate(v, &config);
+	if (config_size <= 0)
+		return config_size;
+
+	buf = kvzalloc(config_size, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	vdpa_get_config(vdpa, config.off, buf, config.len);
+	vdpa_get_config(vdpa, config.off, buf, config_size);
 
-	if (copy_to_user(c->buf, buf, config.len)) {
+	if (copy_to_user(c->buf, buf, config_size)) {
 		kvfree(buf);
 		return -EFAULT;
 	}

@@ -238,18 +239,21 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	struct vdpa_device *vdpa = v->vdpa;
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
+	ssize_t config_size;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))
 		return -EFAULT;
-	if (vhost_vdpa_config_validate(v, &config))
-		return -EINVAL;
 
-	buf = vmemdup_user(c->buf, config.len);
+	config_size = vhost_vdpa_config_validate(v, &config);
+	if (config_size <= 0)
+		return config_size;
+
+	buf = vmemdup_user(c->buf, config_size);
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
 
-	vdpa_set_config(vdpa, config.off, buf, config.len);
+	vdpa_set_config(vdpa, config.off, buf, config_size);
 
 	kvfree(buf);
 	return 0;

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Let's use the new 'get_config_size()' callback available instead of
using the 'virtio_id' to get the size of the device config space.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vdpa.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 544f8582a42b..21eea2be5afa 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -188,13 +188,8 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
 static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v,
 					  struct vhost_vdpa_config *c)
 {
-	u32 size = 0;
-
-	switch (v->virtio_id) {
-	case VIRTIO_ID_NET:
-		size = sizeof(struct virtio_net_config);
-		break;
-	}
+	struct vdpa_device *vdpa = v->vdpa;
+	u32 size = vdpa->config->get_config_size(vdpa);
 
 	if (c->len == 0)
 		return -EINVAL;

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

vdpa_get_config() and vdpa_set_config() now return the amount
of bytes read and written, so let's return them to the user space.

We also modify vhost_vdpa_config_validate() to return 0 (bytes read
or written) instead of an error, when the buffer length is 0.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 21eea2be5afa..b754c53171a7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -191,9 +191,6 @@ static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v,
 	struct vdpa_device *vdpa = v->vdpa;
 	u32 size = vdpa->config->get_config_size(vdpa);
 
-	if (c->len == 0)
-		return -EINVAL;
-
 	return min(c->len, size);
 }
 

@@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	ssize_t config_size;
+	long ret;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))

@@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	if (!buf)
 		return -ENOMEM;
 
-	vdpa_get_config(vdpa, config.off, buf, config_size);
-
-	if (copy_to_user(c->buf, buf, config_size)) {
-		kvfree(buf);
-		return -EFAULT;
+	ret = vdpa_get_config(vdpa, config.off, buf, config_size);
+	if (ret < 0) {
+		ret = -EFAULT;
+		goto out;
 	}
 
+	if (copy_to_user(c->buf, buf, config_size))
+		ret = -EFAULT;
+
+out:
 	kvfree(buf);
-	return 0;
+	return ret;
 }
 
 static long vhost_vdpa_set_config(struct vhost_vdpa *v,

@@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	ssize_t config_size;
+	long ret;
 	u8 *buf;
 
 	if (copy_from_user(&config, c, size))

@@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
 
-	vdpa_set_config(vdpa, config.off, buf, config_size);
+	ret = vdpa_set_config(vdpa, config.off, buf, config_size);
+	if (ret < 0)
+		ret = -EFAULT;
 
 	kvfree(buf);
-	return 0;
+	return ret;
 }
 
 static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

nice ping :-)

On Tue, Feb 16, 2021 at 10:44:44AM +0100, Stefano Garzarella wrote:

Following the discussion with Michael and Jason [1], I reworked a bit
get/set_config() in vdpa.

I changed vdpa_get_config() to check the boundaries and added vdpa_set_config().
When 'offset' or 'len' parameters exceed boundaries, we limit the reading to
the available configuration space in the device, and we return the amount of
bytes read/written.

In this way the user space can pass buffers bigger than config space.
I also returned the amount of bytes read and written to user space.

Patches also available here:
https://github.com/stefano-garzarella/linux/tree/vdpa-get-set-config-refactoring

Thanks for your comments,
Stefano

[1] https://lkml.org/lkml/2021/2/10/350

Stefano Garzarella (10):
 vdpa: add get_config_size callback in vdpa_config_ops
 vdpa: check vdpa_get_config() parameters and return bytes read
 vdpa: add vdpa_set_config() helper
 vdpa: remove param checks in the get/set_config callbacks
 vdpa: remove WARN_ON() in the get/set_config callbacks
 virtio_vdpa: use vdpa_set_config()
 vhost/vdpa: use vdpa_set_config()
 vhost/vdpa: allow user space to pass buffers bigger than config space
 vhost/vdpa: use get_config_size callback in
   vhost_vdpa_config_validate()
 vhost/vdpa: return configuration bytes read and written to user space

include/linux/vdpa.h              | 22 ++++-------
drivers/vdpa/ifcvf/ifcvf_base.c   |  3 +-
drivers/vdpa/ifcvf/ifcvf_main.c   |  8 +++-
drivers/vdpa/mlx5/net/mlx5_vnet.c |  9 ++++-
drivers/vdpa/vdpa.c               | 51 ++++++++++++++++++++++++
drivers/vdpa/vdpa_sim/vdpa_sim.c  | 15 +++++---
drivers/vhost/vdpa.c              | 64 ++++++++++++++++---------------
drivers/virtio/virtio_vdpa.c      |  3 +-
8 files changed, 116 insertions(+), 59 deletions(-)

-- 
2.29.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

Rethink about this, how much we could gain by introducing a dedicated 
ops here? E.g would it be simpler if we simply introduce a 
max_config_size to vdpa device?

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

So I wonder whether it's worth to return the number of bytes since we 
can't propogate the result to driver or driver doesn't care about that.

Thanks

  
  static long vhost_vdpa_get_features(struct vhost_vdpa *v, u64 __user *featurep)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Tue, Mar 02, 2021 at 12:05:35PM +0800, Jason Wang wrote:

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

quoted

vdpa_get_config() and vdpa_set_config() now return the amount
of bytes read and written, so let's return them to the user space.

We also modify vhost_vdpa_config_validate() to return 0 (bytes read
or written) instead of an error, when the buffer length is 0.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 21eea2be5afa..b754c53171a7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -191,9 +191,6 @@ static ssize_t vhost_vdpa_config_validate(struct vhost_vdpa *v,
 	struct vdpa_device *vdpa = v->vdpa;
 	u32 size = vdpa->config->get_config_size(vdpa);
-	if (c->len == 0)
-		return -EINVAL;
-
 	return min(c->len, size);
 }

@@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	ssize_t config_size;
+	long ret;
 	u8 *buf;
 	if (copy_from_user(&config, c, size))

@@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,
 	if (!buf)
 		return -ENOMEM;
-	vdpa_get_config(vdpa, config.off, buf, config_size);
-
-	if (copy_to_user(c->buf, buf, config_size)) {
-		kvfree(buf);
-		return -EFAULT;
+	ret = vdpa_get_config(vdpa, config.off, buf, config_size);
+	if (ret < 0) {
+		ret = -EFAULT;
+		goto out;
 	}
+	if (copy_to_user(c->buf, buf, config_size))
+		ret = -EFAULT;
+
+out:
 	kvfree(buf);
-	return 0;
+	return ret;
 }
 static long vhost_vdpa_set_config(struct vhost_vdpa *v,

@@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	struct vhost_vdpa_config config;
 	unsigned long size = offsetof(struct vhost_vdpa_config, buf);
 	ssize_t config_size;
+	long ret;
 	u8 *buf;
 	if (copy_from_user(&config, c, size))

@@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,
 	if (IS_ERR(buf))
 		return PTR_ERR(buf);
-	vdpa_set_config(vdpa, config.off, buf, config_size);
+	ret = vdpa_set_config(vdpa, config.off, buf, config_size);
+	if (ret < 0)
+		ret = -EFAULT;
 	kvfree(buf);
-	return 0;
+	return ret;
 }

So I wonder whether it's worth to return the number of bytes since we 
can't propogate the result to driver or driver doesn't care about 
that.

Okay, but IIUC user space application that issue VHOST_VDPA_GET_CONFIG 
ioctl can use the return value.

Should we change also 'struct virtio_config_ops' to propagate this value 
also to virtio drivers?

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Tue, Mar 02, 2021 at 12:14:13PM +0800, Jason Wang wrote:

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

quoted

This new callback is used to get the size of the configuration space
of vDPA devices.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/linux/vdpa.h              | 4 ++++
 drivers/vdpa/ifcvf/ifcvf_main.c   | 6 ++++++
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 9 +++++++++
 4 files changed, 25 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 4ab5494503a8..fddf42b17573 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -150,6 +150,9 @@ struct vdpa_iova_range {
  * @set_status:			Set the device status
  *				@vdev: vdpa device
  *				@status: virtio device status
+ * @get_config_size:		Get the size of the configuration space
+ *				@vdev: vdpa device
+ *				Returns size_t: configuration size

Rethink about this, how much we could gain by introducing a dedicated 
ops here? E.g would it be simpler if we simply introduce a 
max_config_size to vdpa device?

Mainly because in this way we don't have to add new parameters to the 
vdpa_alloc_device() function.

We do the same for example for 'get_device_id', 'get_vendor_id', 
'get_vq_num_max'. All of these are usually static, but we have ops.
I think because it's easier to extend.

I don't know if it's worth adding a new structure for these static 
values at this point, like 'struct vdpa_config_params'.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On 2021/3/2 10:06 下午, Stefano Garzarella wrote:

On Tue, Mar 02, 2021 at 12:05:35PM +0800, Jason Wang wrote:

quoted

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

quoted

vdpa_get_config() and vdpa_set_config() now return the amount
of bytes read and written, so let's return them to the user space.

We also modify vhost_vdpa_config_validate() to return 0 (bytes read
or written) instead of an error, when the buffer length is 0.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 21eea2be5afa..b754c53171a7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -191,9 +191,6 @@ static ssize_t vhost_vdpa_config_validate(struct 

vhost_vdpa *v,
     struct vdpa_device *vdpa = v->vdpa;
     u32 size = vdpa->config->get_config_size(vdpa);
-    if (c->len == 0)
-        return -EINVAL;
-
     return min(c->len, size);
 }

@@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct 

vhost_vdpa *v,
     struct vhost_vdpa_config config;
     unsigned long size = offsetof(struct vhost_vdpa_config, buf);
     ssize_t config_size;
+    long ret;
     u8 *buf;
     if (copy_from_user(&config, c, size))

@@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct 

vhost_vdpa *v,
     if (!buf)
         return -ENOMEM;
-    vdpa_get_config(vdpa, config.off, buf, config_size);
-
-    if (copy_to_user(c->buf, buf, config_size)) {
-        kvfree(buf);
-        return -EFAULT;
+    ret = vdpa_get_config(vdpa, config.off, buf, config_size);
+    if (ret < 0) {
+        ret = -EFAULT;
+        goto out;
     }
+    if (copy_to_user(c->buf, buf, config_size))
+        ret = -EFAULT;
+
+out:
     kvfree(buf);
-    return 0;
+    return ret;
 }
 static long vhost_vdpa_set_config(struct vhost_vdpa *v,

@@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct 

vhost_vdpa *v,
     struct vhost_vdpa_config config;
     unsigned long size = offsetof(struct vhost_vdpa_config, buf);
     ssize_t config_size;
+    long ret;
     u8 *buf;
     if (copy_from_user(&config, c, size))

@@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct 

vhost_vdpa *v,
     if (IS_ERR(buf))
         return PTR_ERR(buf);
-    vdpa_set_config(vdpa, config.off, buf, config_size);
+    ret = vdpa_set_config(vdpa, config.off, buf, config_size);
+    if (ret < 0)
+        ret = -EFAULT;
     kvfree(buf);
-    return 0;
+    return ret;
 }

So I wonder whether it's worth to return the number of bytes since we 
can't propogate the result to driver or driver doesn't care about that.

Okay, but IIUC user space application that issue VHOST_VDPA_GET_CONFIG 
ioctl can use the return value.

Yes, but it looks to it's too late to change since it's a userspace 
noticble behaviour.

Should we change also 'struct virtio_config_ops' to propagate this 
value also to virtio drivers?

I think not, the reason is the driver doesn't expect the get()/set() can 
fail...

Thanks

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On 2021/3/2 10:15 下午, Stefano Garzarella wrote:

On Tue, Mar 02, 2021 at 12:14:13PM +0800, Jason Wang wrote:

quoted

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

quoted

This new callback is used to get the size of the configuration space
of vDPA devices.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 include/linux/vdpa.h              | 4 ++++
 drivers/vdpa/ifcvf/ifcvf_main.c   | 6 ++++++
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.c  | 9 +++++++++
 4 files changed, 25 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 4ab5494503a8..fddf42b17573 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -150,6 +150,9 @@ struct vdpa_iova_range {

  * @set_status:            Set the device status
  *                @vdev: vdpa device
  *                @status: virtio device status
+ * @get_config_size:        Get the size of the configuration space
+ *                @vdev: vdpa device
+ *                Returns size_t: configuration size

Rethink about this, how much we could gain by introducing a dedicated 
ops here? E.g would it be simpler if we simply introduce a 
max_config_size to vdpa device?

Mainly because in this way we don't have to add new parameters to the 
vdpa_alloc_device() function.

We do the same for example for 'get_device_id', 'get_vendor_id', 
'get_vq_num_max'. All of these are usually static, but we have ops.
I think because it's easier to extend.

I don't know if it's worth adding a new structure for these static 
values at this point, like 'struct vdpa_config_params'.

Yes, that's the point. I think for any static values, it should be set 
during device allocation.

I'm fine with both.

Thanks

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Thu, Mar 04, 2021 at 04:31:22PM +0800, Jason Wang wrote:

On 2021/3/2 10:06 下午, Stefano Garzarella wrote:

quoted

On Tue, Mar 02, 2021 at 12:05:35PM +0800, Jason Wang wrote:

quoted

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

quoted

vdpa_get_config() and vdpa_set_config() now return the amount
of bytes read and written, so let's return them to the user space.

We also modify vhost_vdpa_config_validate() to return 0 (bytes read
or written) instead of an error, when the buffer length is 0.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 21eea2be5afa..b754c53171a7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -191,9 +191,6 @@ static ssize_t 

vhost_vdpa_config_validate(struct vhost_vdpa *v,
    struct vdpa_device *vdpa = v->vdpa;
    u32 size = vdpa->config->get_config_size(vdpa);
-    if (c->len == 0)
-        return -EINVAL;
-
    return min(c->len, size);
}

@@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct 

vhost_vdpa *v,
    struct vhost_vdpa_config config;
    unsigned long size = offsetof(struct vhost_vdpa_config, buf);
    ssize_t config_size;
+    long ret;
    u8 *buf;
    if (copy_from_user(&config, c, size))

@@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct 

vhost_vdpa *v,
    if (!buf)
        return -ENOMEM;
-    vdpa_get_config(vdpa, config.off, buf, config_size);
-
-    if (copy_to_user(c->buf, buf, config_size)) {
-        kvfree(buf);
-        return -EFAULT;
+    ret = vdpa_get_config(vdpa, config.off, buf, config_size);
+    if (ret < 0) {
+        ret = -EFAULT;
+        goto out;
    }
+    if (copy_to_user(c->buf, buf, config_size))
+        ret = -EFAULT;
+
+out:
    kvfree(buf);
-    return 0;
+    return ret;
}
static long vhost_vdpa_set_config(struct vhost_vdpa *v,

@@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct 

vhost_vdpa *v,
    struct vhost_vdpa_config config;
    unsigned long size = offsetof(struct vhost_vdpa_config, buf);
    ssize_t config_size;
+    long ret;
    u8 *buf;
    if (copy_from_user(&config, c, size))

@@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct 

vhost_vdpa *v,
    if (IS_ERR(buf))
        return PTR_ERR(buf);
-    vdpa_set_config(vdpa, config.off, buf, config_size);
+    ret = vdpa_set_config(vdpa, config.off, buf, config_size);
+    if (ret < 0)
+        ret = -EFAULT;
    kvfree(buf);
-    return 0;
+    return ret;
}

So I wonder whether it's worth to return the number of bytes since 
we can't propogate the result to driver or driver doesn't care 
about that.

Okay, but IIUC user space application that issue 
VHOST_VDPA_GET_CONFIG ioctl can use the return value.

Yes, but it looks to it's too late to change since it's a userspace 
noticble behaviour.

Yeah, this is a good point.
I looked at QEMU and we only check if the value is not negative, so it 
should work, but for other applications it could be a real change.

Do we leave it as is?

quoted

Should we change also 'struct virtio_config_ops' to propagate this 
value also to virtio drivers?

I think not, the reason is the driver doesn't expect the get()/set() 
can fail...

Got it.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On Thu, Mar 04, 2021 at 04:34:52PM +0800, Jason Wang wrote:

On 2021/3/2 10:15 下午, Stefano Garzarella wrote:

quoted

On Tue, Mar 02, 2021 at 12:14:13PM +0800, Jason Wang wrote:

quoted

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

quoted

This new callback is used to get the size of the configuration space
of vDPA devices.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/vdpa.h              | 4 ++++
drivers/vdpa/ifcvf/ifcvf_main.c   | 6 ++++++
drivers/vdpa/mlx5/net/mlx5_vnet.c | 6 ++++++
drivers/vdpa/vdpa_sim/vdpa_sim.c  | 9 +++++++++
4 files changed, 25 insertions(+)

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 4ab5494503a8..fddf42b17573 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -150,6 +150,9 @@ struct vdpa_iova_range {
 * @set_status:            Set the device status
 *                @vdev: vdpa device
 *                @status: virtio device status
+ * @get_config_size:        Get the size of the configuration space
+ *                @vdev: vdpa device
+ *                Returns size_t: configuration size

Rethink about this, how much we could gain by introducing a 
dedicated ops here? E.g would it be simpler if we simply introduce 
a max_config_size to vdpa device?

Mainly because in this way we don't have to add new parameters to 
the vdpa_alloc_device() function.

We do the same for example for 'get_device_id', 'get_vendor_id', 
'get_vq_num_max'. All of these are usually static, but we have ops.
I think because it's easier to extend.

I don't know if it's worth adding a new structure for these static 
values at this point, like 'struct vdpa_config_params'.

Yes, that's the point. I think for any static values, it should be set 
during device allocation.

Yeah, I see.

I'm fine with both.

@Michael any thoughts?

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

On 2021/3/5 4:37 下午, Stefano Garzarella wrote:

On Thu, Mar 04, 2021 at 04:31:22PM +0800, Jason Wang wrote:

quoted

On 2021/3/2 10:06 下午, Stefano Garzarella wrote:

quoted

On Tue, Mar 02, 2021 at 12:05:35PM +0800, Jason Wang wrote:

quoted

On 2021/2/16 5:44 下午, Stefano Garzarella wrote:

quoted

vdpa_get_config() and vdpa_set_config() now return the amount
of bytes read and written, so let's return them to the user space.

We also modify vhost_vdpa_config_validate() to return 0 (bytes read
or written) instead of an error, when the buffer length is 0.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vhost/vdpa.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 21eea2be5afa..b754c53171a7 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c

@@ -191,9 +191,6 @@ static ssize_t 

vhost_vdpa_config_validate(struct vhost_vdpa *v,
     struct vdpa_device *vdpa = v->vdpa;
     u32 size = vdpa->config->get_config_size(vdpa);
-    if (c->len == 0)
-        return -EINVAL;
-
     return min(c->len, size);
 }

@@ -204,6 +201,7 @@ static long vhost_vdpa_get_config(struct 

vhost_vdpa *v,
     struct vhost_vdpa_config config;
     unsigned long size = offsetof(struct vhost_vdpa_config, buf);
     ssize_t config_size;
+    long ret;
     u8 *buf;
     if (copy_from_user(&config, c, size))

@@ -217,15 +215,18 @@ static long vhost_vdpa_get_config(struct 

vhost_vdpa *v,
     if (!buf)
         return -ENOMEM;
-    vdpa_get_config(vdpa, config.off, buf, config_size);
-
-    if (copy_to_user(c->buf, buf, config_size)) {
-        kvfree(buf);
-        return -EFAULT;
+    ret = vdpa_get_config(vdpa, config.off, buf, config_size);
+    if (ret < 0) {
+        ret = -EFAULT;
+        goto out;
     }
+    if (copy_to_user(c->buf, buf, config_size))
+        ret = -EFAULT;
+
+out:
     kvfree(buf);
-    return 0;
+    return ret;
 }
 static long vhost_vdpa_set_config(struct vhost_vdpa *v,

@@ -235,6 +236,7 @@ static long vhost_vdpa_set_config(struct 

vhost_vdpa *v,
     struct vhost_vdpa_config config;
     unsigned long size = offsetof(struct vhost_vdpa_config, buf);
     ssize_t config_size;
+    long ret;
     u8 *buf;
     if (copy_from_user(&config, c, size))

@@ -248,10 +250,12 @@ static long vhost_vdpa_set_config(struct 

vhost_vdpa *v,
     if (IS_ERR(buf))
         return PTR_ERR(buf);
-    vdpa_set_config(vdpa, config.off, buf, config_size);
+    ret = vdpa_set_config(vdpa, config.off, buf, config_size);
+    if (ret < 0)
+        ret = -EFAULT;
     kvfree(buf);
-    return 0;
+    return ret;
 }

So I wonder whether it's worth to return the number of bytes since 
we can't propogate the result to driver or driver doesn't care 
about that.

Okay, but IIUC user space application that issue 
VHOST_VDPA_GET_CONFIG ioctl can use the return value.

Yes, but it looks to it's too late to change since it's a userspace 
noticble behaviour.

Yeah, this is a good point.
I looked at QEMU and we only check if the value is not negative, so it 
should work, but for other applications it could be a real change.

Do we leave it as is?

Yes, I think we'd better be conservative here.

Thanks

quoted

quoted

Should we change also 'struct virtio_config_ops' to propagate this 
value also to virtio drivers?

I think not, the reason is the driver doesn't expect the get()/set() 
can fail...

Got it.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization