Re: [RFC PATCH 10/10] vhost/vdpa: return configuration bytes read and written to user space

[Stefano Garzarella <sgarzare@redhat.com>]

On Tue, Mar 02, 2021 at 12:05:35PM +0800, Jason Wang wrote:
>
>On 2021/2/16 5:44 下午, Stefano Garzarella wrote:
>>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