netdev - Re: [RFC 1/4] vDPA/ifcvf: add get/set_vq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <6be0490e-1f9c-3a7d-f5d3-8dd8e50b5fdc@intel.com>
Date:   Tue, 6 Sep 2022 10:12:44 +0800
From:   "Zhu, Lingshan" <lingshan.zhu@...el.com>
To:     Jason Wang <jasowang@...hat.com>, mst@...hat.com
Cc:     virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [RFC 1/4] vDPA/ifcvf: add get/set_vq_endian support for vDPA



On 9/5/2022 4:34 PM, Jason Wang wrote:
>
> 在 2022/9/1 18:15, Zhu Lingshan 写道:
>> This commit introuduces new config operatoions for vDPA:
>> vdpa_config_ops.get_vq_endian: set vq endian-ness
>> vdpa_config_ops.set_vq_endian: get vq endian-ness
>>
>> Because the endian-ness is a device wide attribute,
>> so seting a vq's endian-ness will result in changing
>> the device endian-ness, including all vqs and the config space.
>>
>> These two operations are implemented in ifcvf in this commit.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@...el.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 15 +++++++++++++++
>>   include/linux/vdpa.h            | 13 +++++++++++++
>>   3 files changed, 29 insertions(+)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index f5563f665cc6..640238b95033 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -19,6 +19,7 @@
>>   #include <uapi/linux/virtio_blk.h>
>>   #include <uapi/linux/virtio_config.h>
>>   #include <uapi/linux/virtio_pci.h>
>> +#include <uapi/linux/vhost.h>
>>     #define N3000_DEVICE_ID        0x1041
>>   #define N3000_SUBSYS_DEVICE_ID    0x001A
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index f9c0044c6442..270637d0f3a5 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -684,6 +684,19 @@ static struct vdpa_notification_area 
>> ifcvf_get_vq_notification(struct vdpa_devic
>>       return area;
>>   }
>>   +static u8 ifcvf_vdpa_get_vq_endian(struct vdpa_device *vdpa_dev, 
>> u16 idx)
>> +{
>> +    return VHOST_VRING_LITTLE_ENDIAN;
>> +}
>> +
>> +static int ifcvf_vdpa_set_vq_endian(struct vdpa_device *vdpa_dev, 
>> u16 idx, u8 endian)
>> +{
>> +    if (endian != VHOST_VRING_LITTLE_ENDIAN)
>> +        return -EFAULT;
>
>
> I'm worrying that this basically make the proposed API not much useful.
>
> For example, what would userspace do if it meet this failure?
This means the device only support LE, so if the user space does not 
work with LE,
I think it should explicitly fails on the device.

And we see the device only support LE, so no need to set anything to the 
device
if LE is to set.

Thanks,
Zhu Lingshan
>
> Thanks
>
>
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * IFCVF currently doesn't have on-chip IOMMU, so not
>>    * implemented set_map()/dma_map()/dma_unmap()
>> @@ -715,6 +728,8 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {
>>       .set_config    = ifcvf_vdpa_set_config,
>>       .set_config_cb  = ifcvf_vdpa_set_config_cb,
>>       .get_vq_notification = ifcvf_get_vq_notification,
>> +    .get_vq_endian    = ifcvf_vdpa_get_vq_endian,
>> +    .set_vq_endian    = ifcvf_vdpa_set_vq_endian,
>>   };
>>     static struct virtio_device_id id_table_net[] = {
>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>> index d282f464d2f1..5eb83453ba86 100644
>> --- a/include/linux/vdpa.h
>> +++ b/include/linux/vdpa.h
>> @@ -174,6 +174,17 @@ struct vdpa_map_file {
>>    *                @idx: virtqueue index
>>    *                Returns int: irq number of a virtqueue,
>>    *                negative number if no irq assigned.
>> + * @set_vq_endian:        set endian-ness for a virtqueue
>> + *                @vdev: vdpa device
>> + *                @idx: virtqueue index
>> + *                @endian: the endian-ness to set,
>> + *                can be VHOST_VRING_LITTLE_ENDIAN or 
>> VHOST_VRING_BIG_ENDIAN
>> + *                Returns integer: success (0) or error (< 0)
>> + * @get_vq_endian:        get the endian-ness of a virtqueue
>> + *                @vdev: vdpa device
>> + *                @idx: virtqueue index
>> + *                Returns u8, the endian-ness of the virtqueue,
>> + *                can be VHOST_VRING_LITTLE_ENDIAN or 
>> VHOST_VRING_BIG_ENDIAN
>>    * @get_vq_align:        Get the virtqueue align requirement
>>    *                for the device
>>    *                @vdev: vdpa device
>> @@ -306,6 +317,8 @@ struct vdpa_config_ops {
>>       (*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>       /* vq irq is not expected to be changed once DRIVER_OK is set */
>>       int (*get_vq_irq)(struct vdpa_device *vdev, u16 idx);
>> +    int (*set_vq_endian)(struct vdpa_device *vdev, u16 idx, u8 endian);
>> +    u8 (*get_vq_endian)(struct vdpa_device *vdev, u16 idx);
>>         /* Device ops */
>>       u32 (*get_vq_align)(struct vdpa_device *vdev);
>