Virtualization - Mar 2021 - [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:
>>
>> 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 at 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
>

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 at 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?
>
>
>>
>>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