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

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