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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

On 2021/3/2 10:06 下午, Stefano Garzarella wrote:
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@xxxxxxxxxx>
---
 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?




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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux