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

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