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

[Jason Wang <jasowang@redhat.com>]

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@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