Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe

Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe

[Jason Wang]

在 2021/4/14 下午5:18, Zhu Lingshan 写道:
> This commit deduces VIRTIO device ID as device type when probe,
> then ifcvf_vdpa_get_device_id() can simply return the ID.
> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
> can work properly based on the device ID.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>   drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>   2 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index b2eeb16b9c2c..1c04cd256fa7 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>   	u32 notify_off_multiplier;
>   	u64 req_features;
>   	u64 hw_features;
> +	u32 dev_type;
>   	struct virtio_pci_common_cfg __iomem *common_cfg;
>   	void __iomem *net_cfg;
>   	struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 44d7586019da..99b0a6b4c227 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct vdpa_device *vdpa_dev)
>   
>   static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>   {
> -	struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
> -	struct pci_dev *pdev = adapter->pdev;
> -	u32 ret = -ENODEV;
> -
> -	if (pdev->device < 0x1000 || pdev->device > 0x107f)
> -		return ret;
> -
> -	if (pdev->device < 0x1040)
> -		ret =  pdev->subsystem_device;
> -	else
> -		ret =  pdev->device - 0x1040;
> +	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>   
> -	return ret;
> +	return vf->dev_type;
>   }
>   
>   static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	pci_set_drvdata(pdev, adapter);
>   
>   	vf = &adapter->vf;
> +	if (pdev->device < 0x1000 || pdev->device > 0x107f)
> +		return -EOPNOTSUPP;
> +
> +	if (pdev->device < 0x1040)
> +		vf->dev_type =  pdev->subsystem_device;
> +	else
> +		vf->dev_type =  pdev->device - 0x1040;


So a question here, is the device a transtional device or modern one?

If it's a transitonal one, can it swtich endianess automatically or not?

Thanks


> +
>   	vf->base = pcim_iomap_table(pdev);
>   
>   	adapter->pdev = pdev;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe

[Zhu Lingshan]

On 4/15/2021 11:30 AM, Jason Wang wrote:
>
> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>> This commit deduces VIRTIO device ID as device type when probe,
>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>> can work properly based on the device ID.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>   2 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index b2eeb16b9c2c..1c04cd256fa7 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>       u32 notify_off_multiplier;
>>       u64 req_features;
>>       u64 hw_features;
>> +    u32 dev_type;
>>       struct virtio_pci_common_cfg __iomem *common_cfg;
>>       void __iomem *net_cfg;
>>       struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 44d7586019da..99b0a6b4c227 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct 
>> vdpa_device *vdpa_dev)
>>     static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>>   {
>> -    struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>> -    struct pci_dev *pdev = adapter->pdev;
>> -    u32 ret = -ENODEV;
>> -
>> -    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>> -        return ret;
>> -
>> -    if (pdev->device < 0x1040)
>> -        ret =  pdev->subsystem_device;
>> -    else
>> -        ret =  pdev->device - 0x1040;
>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>   -    return ret;
>> +    return vf->dev_type;
>>   }
>>     static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>       pci_set_drvdata(pdev, adapter);
>>         vf = &adapter->vf;
>> +    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>> +        return -EOPNOTSUPP;
>> +
>> +    if (pdev->device < 0x1040)
>> +        vf->dev_type =  pdev->subsystem_device;
>> +    else
>> +        vf->dev_type =  pdev->device - 0x1040;
>
>
> So a question here, is the device a transtional device or modern one?
>
> If it's a transitonal one, can it swtich endianess automatically or not?
>
> Thanks
Hi Jason,

This driver should drive both modern and transitional devices as we 
discussed before.
If it's a transitional one, it will act as a modern device by default, 
legacy mode is a fail-over path.
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must in 
modern mode.
I think we don't need to worry about endianess for legacy mode.

Thanks
Zhu Lingshan
>
>
>> +
>>       vf->base = pcim_iomap_table(pdev);
>>         adapter->pdev = pdev;
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe

[Jason Wang]

在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>
>
> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>
>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>> This commit deduces VIRTIO device ID as device type when probe,
>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>> can work properly based on the device ID.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>>   2 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>>       u32 notify_off_multiplier;
>>>       u64 req_features;
>>>       u64 hw_features;
>>> +    u32 dev_type;
>>>       struct virtio_pci_common_cfg __iomem *common_cfg;
>>>       void __iomem *net_cfg;
>>>       struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index 44d7586019da..99b0a6b4c227 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct 
>>> vdpa_device *vdpa_dev)
>>>     static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>>>   {
>>> -    struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>> -    struct pci_dev *pdev = adapter->pdev;
>>> -    u32 ret = -ENODEV;
>>> -
>>> -    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>> -        return ret;
>>> -
>>> -    if (pdev->device < 0x1040)
>>> -        ret =  pdev->subsystem_device;
>>> -    else
>>> -        ret =  pdev->device -0x1040;
>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>   -    return ret;
>>> +    return vf->dev_type;
>>>   }
>>>     static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, 
>>> const struct pci_device_id *id)
>>>       pci_set_drvdata(pdev, adapter);
>>>         vf = &adapter->vf;
>>> +    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    if (pdev->device < 0x1040)
>>> +        vf->dev_type =  pdev->subsystem_device;
>>> +    else
>>> +        vf->dev_type =  pdev->device - 0x1040;
>>
>>
>> So a question here, is the device a transtional device or modern one?
>>
>> If it's a transitonal one, can it swtich endianess automatically or not?
>>
>> Thanks
> Hi Jason,
>
> This driver should drive both modern and transitional devices as we 
> discussed before.
> If it's a transitional one, it will act as a modern device by default, 
> legacy mode is a fail-over path.


Note that legacy driver use native endian, support legacy driver 
requires the device to know native endian which I'm not sure your device 
can do that.

Thanks


> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must 
> in modern mode.
> I think we don't need to worry about endianess for legacy mode.
>
> Thanks
> Zhu Lingshan
>>
>>
>>> +
>>>       vf->base = pcim_iomap_table(pdev);
>>>         adapter->pdev = pdev;
>>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe

[Zhu Lingshan]

On 4/15/2021 2:30 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>>
>>
>> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>>
>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>> This commit deduces VIRTIO device ID as device type when probe,
>>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>>> can work properly based on the device ID.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>>>   2 files changed, 11 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>>>       u32 notify_off_multiplier;
>>>>       u64 req_features;
>>>>       u64 hw_features;
>>>> +    u32 dev_type;
>>>>       struct virtio_pci_common_cfg __iomem *common_cfg;
>>>>       void __iomem *net_cfg;
>>>>       struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index 44d7586019da..99b0a6b4c227 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct 
>>>> vdpa_device *vdpa_dev)
>>>>     static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>>>>   {
>>>> -    struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>>> -    struct pci_dev *pdev = adapter->pdev;
>>>> -    u32 ret = -ENODEV;
>>>> -
>>>> -    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>> -        return ret;
>>>> -
>>>> -    if (pdev->device < 0x1040)
>>>> -        ret =  pdev->subsystem_device;
>>>> -    else
>>>> -        ret =  pdev->device -0x1040;
>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>   -    return ret;
>>>> +    return vf->dev_type;
>>>>   }
>>>>     static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, 
>>>> const struct pci_device_id *id)
>>>>       pci_set_drvdata(pdev, adapter);
>>>>         vf = &adapter->vf;
>>>> +    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>> +        return -EOPNOTSUPP;
>>>> +
>>>> +    if (pdev->device < 0x1040)
>>>> +        vf->dev_type =  pdev->subsystem_device;
>>>> +    else
>>>> +        vf->dev_type =  pdev->device - 0x1040;
>>>
>>>
>>> So a question here, is the device a transtional device or modern one?
>>>
>>> If it's a transitonal one, can it swtich endianess automatically or 
>>> not?
>>>
>>> Thanks
>> Hi Jason,
>>
>> This driver should drive both modern and transitional devices as we 
>> discussed before.
>> If it's a transitional one, it will act as a modern device by 
>> default, legacy mode is a fail-over path.
>
>
> Note that legacy driver use native endian, support legacy driver 
> requires the device to know native endian which I'm not sure your 
> device can do that.
>
> Thanks
Yes, legacy requires guest native endianess, I think we don't need to 
worry about this because our transitional device should work in modern 
mode by
default(legacy mode is the failover path we will never reach, 
get_features will fail if no ACCESS_PLATFORM), we don't support legacy 
device in vDPA.

Thanks
>
>
>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must 
>> in modern mode.
>> I think we don't need to worry about endianess for legacy mode.
>>
>> Thanks
>> Zhu Lingshan
>>>
>>>
>>>> +
>>>>       vf->base = pcim_iomap_table(pdev);
>>>>         adapter->pdev = pdev;
>>>
>>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe

[Jason Wang]

在 2021/4/15 下午2:36, Zhu Lingshan 写道:
>
>
> On 4/15/2021 2:30 PM, Jason Wang wrote:
>>
>> 在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>>>
>>>
>>> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>>>
>>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>>> This commit deduces VIRTIO device ID as device type when probe,
>>>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>>>> can work properly based on the device ID.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---
>>>>>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>>>>   2 files changed, 11 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>>>>       u32 notify_off_multiplier;
>>>>>       u64 req_features;
>>>>>       u64 hw_features;
>>>>> +    u32 dev_type;
>>>>>       struct virtio_pci_common_cfg __iomem *common_cfg;
>>>>>       void __iomem *net_cfg;
>>>>>       struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> index 44d7586019da..99b0a6b4c227 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct 
>>>>> vdpa_device *vdpa_dev)
>>>>>     static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>>>>>   {
>>>>> -    struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>>>> -    struct pci_dev *pdev = adapter->pdev;
>>>>> -    u32 ret = -ENODEV;
>>>>> -
>>>>> -    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>> -        return ret;
>>>>> -
>>>>> -    if (pdev->device < 0x1040)
>>>>> -        ret =  pdev->subsystem_device;
>>>>> -    else
>>>>> -        ret =  pdev->device-0x1040;
>>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>   -    return ret;
>>>>> +    return vf->dev_type;
>>>>>   }
>>>>>     static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, 
>>>>> const struct pci_device_id *id)
>>>>>       pci_set_drvdata(pdev, adapter);
>>>>>         vf = &adapter->vf;
>>>>> +    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>> +        return -EOPNOTSUPP;
>>>>> +
>>>>> +    if (pdev->device < 0x1040)
>>>>> +        vf->dev_type =  pdev->subsystem_device;
>>>>> +    else
>>>>> +        vf->dev_type =  pdev->device - 0x1040;
>>>>
>>>>
>>>> So a question here, is the device a transtional device or modern one?
>>>>
>>>> If it's a transitonal one, can it swtich endianess automatically or 
>>>> not?
>>>>
>>>> Thanks
>>> Hi Jason,
>>>
>>> This driver should drive both modern and transitional devices as we 
>>> discussed before.
>>> If it's a transitional one, it will act as a modern device by 
>>> default, legacy mode is a fail-over path.
>>
>>
>> Note that legacy driver use native endian, support legacy driver 
>> requires the device to know native endian which I'm not sure your 
>> device can do that.
>>
>> Thanks
> Yes, legacy requires guest native endianess, I think we don't need to 
> worry about this because our transitional device should work in modern 
> mode by
> default(legacy mode is the failover path we will never reach, 
> get_features will fail if no ACCESS_PLATFORM), we don't support legacy 
> device in vDPA.
>
> Thanks


Ok, so I think it's better to add a comment here.

Thanks


>>
>>
>>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must 
>>> in modern mode.
>>> I think we don't need to worry about endianess for legacy mode.
>>>
>>> Thanks
>>> Zhu Lingshan
>>>>
>>>>
>>>>> +
>>>>>       vf->base = pcim_iomap_table(pdev);
>>>>>         adapter->pdev = pdev;
>>>>
>>>
>>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vDPA/ifcvf: deduce VIRTIO device ID when probe

[Zhu Lingshan]

On 4/15/2021 3:16 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午2:36, Zhu Lingshan 写道:
>>
>>
>> On 4/15/2021 2:30 PM, Jason Wang wrote:
>>>
>>> 在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>>>>
>>>>
>>>> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>>>>
>>>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>>>> This commit deduces VIRTIO device ID as device type when probe,
>>>>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>>>>> can work properly based on the device ID.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>>>>>>   drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
>>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>>>>>   2 files changed, 11 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>>>>>       u32 notify_off_multiplier;
>>>>>>       u64 req_features;
>>>>>>       u64 hw_features;
>>>>>> +    u32 dev_type;
>>>>>>       struct virtio_pci_common_cfg __iomem *common_cfg;
>>>>>>       void __iomem *net_cfg;
>>>>>>       struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> index 44d7586019da..99b0a6b4c227 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct 
>>>>>> vdpa_device *vdpa_dev)
>>>>>>     static u32 ifcvf_vdpa_get_device_id(struct vdpa_device 
>>>>>> *vdpa_dev)
>>>>>>   {
>>>>>> -    struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>>>>> -    struct pci_dev *pdev = adapter->pdev;
>>>>>> -    u32 ret = -ENODEV;
>>>>>> -
>>>>>> -    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>>> -        return ret;
>>>>>> -
>>>>>> -    if (pdev->device < 0x1040)
>>>>>> -        ret =  pdev->subsystem_device;
>>>>>> -    else
>>>>>> -        ret =  pdev->device-0x1040;
>>>>>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>>   -    return ret;
>>>>>> +    return vf->dev_type;
>>>>>>   }
>>>>>>     static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device 
>>>>>> *vdpa_dev)
>>>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, 
>>>>>> const struct pci_device_id *id)
>>>>>>       pci_set_drvdata(pdev, adapter);
>>>>>>         vf = &adapter->vf;
>>>>>> +    if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>>> +        return -EOPNOTSUPP;
>>>>>> +
>>>>>> +    if (pdev->device < 0x1040)
>>>>>> +        vf->dev_type =  pdev->subsystem_device;
>>>>>> +    else
>>>>>> +        vf->dev_type =  pdev->device - 0x1040;
>>>>>
>>>>>
>>>>> So a question here, is the device a transtional device or modern one?
>>>>>
>>>>> If it's a transitonal one, can it swtich endianess automatically 
>>>>> or not?
>>>>>
>>>>> Thanks
>>>> Hi Jason,
>>>>
>>>> This driver should drive both modern and transitional devices as we 
>>>> discussed before.
>>>> If it's a transitional one, it will act as a modern device by 
>>>> default, legacy mode is a fail-over path.
>>>
>>>
>>> Note that legacy driver use native endian, support legacy driver 
>>> requires the device to know native endian which I'm not sure your 
>>> device can do that.
>>>
>>> Thanks
>> Yes, legacy requires guest native endianess, I think we don't need to 
>> worry about this because our transitional device should work in 
>> modern mode by
>> default(legacy mode is the failover path we will never reach, 
>> get_features will fail if no ACCESS_PLATFORM), we don't support 
>> legacy device in vDPA.
>>
>> Thanks
>
>
> Ok, so I think it's better to add a comment here.
sure, will add a comment in V2

Thanks
>
> Thanks
>
>
>>>
>>>
>>>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it 
>>>> must in modern mode.
>>>> I think we don't need to worry about endianess for legacy mode.
>>>>
>>>> Thanks
>>>> Zhu Lingshan
>>>>>
>>>>>
>>>>>> +
>>>>>>       vf->base = pcim_iomap_table(pdev);
>>>>>>         adapter->pdev = pdev;
>>>>>
>>>>
>>>
>>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA

[Jason Wang]

在 2021/4/14 下午5:18, Zhu Lingshan 写道:
> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
> for vDPA.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
>   drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>   2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index 1c04cd256fa7..8b403522bf06 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -15,6 +15,7 @@
>   #include <linux/pci_regs.h>
>   #include <linux/vdpa.h>
>   #include <uapi/linux/virtio_net.h>
> +#include <uapi/linux/virtio_blk.h>
>   #include <uapi/linux/virtio_config.h>
>   #include <uapi/linux/virtio_pci.h>
>   
> @@ -28,7 +29,12 @@
>   #define C5000X_PL_SUBSYS_VENDOR_ID	0x8086
>   #define C5000X_PL_SUBSYS_DEVICE_ID	0x0001
>   
> -#define IFCVF_SUPPORTED_FEATURES \
> +#define C5000X_PL_BLK_VENDOR_ID		0x1AF4
> +#define C5000X_PL_BLK_DEVICE_ID		0x1001
> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID	0x8086
> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID	0x0002
> +
> +#define IFCVF_NET_SUPPORTED_FEATURES \
>   		((1ULL << VIRTIO_NET_F_MAC)			| \
>   		 (1ULL << VIRTIO_F_ANY_LAYOUT)			| \
>   		 (1ULL << VIRTIO_F_VERSION_1)			| \
> @@ -37,6 +43,15 @@
>   		 (1ULL << VIRTIO_F_ACCESS_PLATFORM)		| \
>   		 (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>   
> +#define IFCVF_BLK_SUPPORTED_FEATURES \
> +		((1ULL << VIRTIO_BLK_F_SIZE_MAX)		| \
> +		 (1ULL << VIRTIO_BLK_F_SEG_MAX)			| \
> +		 (1ULL << VIRTIO_BLK_F_BLK_SIZE)		| \
> +		 (1ULL << VIRTIO_BLK_F_TOPOLOGY)		| \
> +		 (1ULL << VIRTIO_BLK_F_MQ)			| \
> +		 (1ULL << VIRTIO_F_VERSION_1)			| \
> +		 (1ULL << VIRTIO_F_ACCESS_PLATFORM))


I think we've discussed this sometime in the past but what's the reason 
for such whitelist consider there's already a get_features() implemention?

E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED?

Thanks


> +
>   /* Only one queue pair for now. */
>   #define IFCVF_MAX_QUEUE_PAIRS	1
>   
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 99b0a6b4c227..9b6a38b798fa 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
>   	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>   	u64 features;
>   
> -	features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
> +	if (vf->dev_type == VIRTIO_ID_NET)
> +		features = ifcvf_get_features(vf) & IFCVF_NET_SUPPORTED_FEATURES;
> +
> +	if (vf->dev_type == VIRTIO_ID_BLOCK)
> +		features = ifcvf_get_features(vf) & IFCVF_BLK_SUPPORTED_FEATURES;
>   
>   	return features;
>   }
> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>   			 C5000X_PL_DEVICE_ID,
>   			 C5000X_PL_SUBSYS_VENDOR_ID,
>   			 C5000X_PL_SUBSYS_DEVICE_ID) },
> +	{ PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
> +			 C5000X_PL_BLK_DEVICE_ID,
> +			 C5000X_PL_BLK_SUBSYS_VENDOR_ID,
> +			 C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>   
>   	{ 0 },
>   };

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA

[Zhu Lingshan]

On 4/15/2021 11:34 AM, Jason Wang wrote:
>
> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
>> for vDPA.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>>   2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index 1c04cd256fa7..8b403522bf06 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -15,6 +15,7 @@
>>   #include <linux/pci_regs.h>
>>   #include <linux/vdpa.h>
>>   #include <uapi/linux/virtio_net.h>
>> +#include <uapi/linux/virtio_blk.h>
>>   #include <uapi/linux/virtio_config.h>
>>   #include <uapi/linux/virtio_pci.h>
>>   @@ -28,7 +29,12 @@
>>   #define C5000X_PL_SUBSYS_VENDOR_ID    0x8086
>>   #define C5000X_PL_SUBSYS_DEVICE_ID    0x0001
>>   -#define IFCVF_SUPPORTED_FEATURES \
>> +#define C5000X_PL_BLK_VENDOR_ID        0x1AF4
>> +#define C5000X_PL_BLK_DEVICE_ID        0x1001
>> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID    0x8086
>> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID    0x0002
>> +
>> +#define IFCVF_NET_SUPPORTED_FEATURES \
>>           ((1ULL << VIRTIO_NET_F_MAC)            | \
>>            (1ULL << VIRTIO_F_ANY_LAYOUT)            | \
>>            (1ULL << VIRTIO_F_VERSION_1)            | \
>> @@ -37,6 +43,15 @@
>>            (1ULL << VIRTIO_F_ACCESS_PLATFORM)        | \
>>            (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>>   +#define IFCVF_BLK_SUPPORTED_FEATURES \
>> +        ((1ULL << VIRTIO_BLK_F_SIZE_MAX)        | \
>> +         (1ULL << VIRTIO_BLK_F_SEG_MAX)            | \
>> +         (1ULL << VIRTIO_BLK_F_BLK_SIZE)        | \
>> +         (1ULL << VIRTIO_BLK_F_TOPOLOGY)        | \
>> +         (1ULL << VIRTIO_BLK_F_MQ)            | \
>> +         (1ULL << VIRTIO_F_VERSION_1)            | \
>> +         (1ULL << VIRTIO_F_ACCESS_PLATFORM))
>
>
> I think we've discussed this sometime in the past but what's the 
> reason for such whitelist consider there's already a get_features() 
> implemention?
>
> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED?
>
> Thanks
The reason is some feature bits are supported in the device but not 
supported by the driver, e.g, for virtio-net, mq & cq implementation is 
not ready in the driver.

Thanks!

>
>
>> +
>>   /* Only one queue pair for now. */
>>   #define IFCVF_MAX_QUEUE_PAIRS    1
>>   diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 99b0a6b4c227..9b6a38b798fa 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>> vdpa_device *vdpa_dev)
>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>       u64 features;
>>   -    features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
>> +    if (vf->dev_type == VIRTIO_ID_NET)
>> +        features = ifcvf_get_features(vf) & 
>> IFCVF_NET_SUPPORTED_FEATURES;
>> +
>> +    if (vf->dev_type == VIRTIO_ID_BLOCK)
>> +        features = ifcvf_get_features(vf) & 
>> IFCVF_BLK_SUPPORTED_FEATURES;
>>         return features;
>>   }
>> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>>                C5000X_PL_DEVICE_ID,
>>                C5000X_PL_SUBSYS_VENDOR_ID,
>>                C5000X_PL_SUBSYS_DEVICE_ID) },
>> +    { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
>> +             C5000X_PL_BLK_DEVICE_ID,
>> +             C5000X_PL_BLK_SUBSYS_VENDOR_ID,
>> +             C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>>         { 0 },
>>   };
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA

[Jason Wang]

在 2021/4/15 下午1:55, Zhu Lingshan 写道:
>
>
> On 4/15/2021 11:34 AM, Jason Wang wrote:
>>
>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
>>> for vDPA.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>>>   2 files changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> index 1c04cd256fa7..8b403522bf06 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> @@ -15,6 +15,7 @@
>>>   #include <linux/pci_regs.h>
>>>   #include <linux/vdpa.h>
>>>   #include <uapi/linux/virtio_net.h>
>>> +#include <uapi/linux/virtio_blk.h>
>>>   #include <uapi/linux/virtio_config.h>
>>>   #include <uapi/linux/virtio_pci.h>
>>>   @@ -28,7 +29,12 @@
>>>   #define C5000X_PL_SUBSYS_VENDOR_ID    0x8086
>>>   #define C5000X_PL_SUBSYS_DEVICE_ID    0x0001
>>>   -#define IFCVF_SUPPORTED_FEATURES \
>>> +#define C5000X_PL_BLK_VENDOR_ID        0x1AF4
>>> +#define C5000X_PL_BLK_DEVICE_ID        0x1001
>>> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID    0x8086
>>> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID    0x0002
>>> +
>>> +#define IFCVF_NET_SUPPORTED_FEATURES \
>>>           ((1ULL << VIRTIO_NET_F_MAC)            | \
>>>            (1ULL << VIRTIO_F_ANY_LAYOUT)            | \
>>>            (1ULL << VIRTIO_F_VERSION_1)            | \
>>> @@ -37,6 +43,15 @@
>>>            (1ULL << VIRTIO_F_ACCESS_PLATFORM)        | \
>>>            (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>>>   +#define IFCVF_BLK_SUPPORTED_FEATURES \
>>> +        ((1ULL << VIRTIO_BLK_F_SIZE_MAX)        | \
>>> +         (1ULL << VIRTIO_BLK_F_SEG_MAX)            | \
>>> +         (1ULL << VIRTIO_BLK_F_BLK_SIZE)        | \
>>> +         (1ULL << VIRTIO_BLK_F_TOPOLOGY)        | \
>>> +         (1ULL << VIRTIO_BLK_F_MQ)            | \
>>> +         (1ULL << VIRTIO_F_VERSION_1)            | \
>>> +         (1ULL << VIRTIO_F_ACCESS_PLATFORM))
>>
>>
>> I think we've discussed this sometime in the past but what's the 
>> reason for such whitelist consider there's already a get_features() 
>> implemention?
>>
>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or 
>> VIRTIO_F_RING_PACKED?
>>
>> Thanks
> The reason is some feature bits are supported in the device but not 
> supported by the driver, e.g, for virtio-net, mq & cq implementation 
> is not ready in the driver.


I understand the case of virtio-net but I wonder why we need this for 
block where we don't vq cvq.

Thanks


>
> Thanks!
>
>>
>>
>>> +
>>>   /* Only one queue pair for now. */
>>>   #define IFCVF_MAX_QUEUE_PAIRS    1
>>>   diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index 99b0a6b4c227..9b6a38b798fa 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>>> vdpa_device *vdpa_dev)
>>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>       u64 features;
>>>   -    features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
>>> +    if (vf->dev_type == VIRTIO_ID_NET)
>>> +        features = ifcvf_get_features(vf) & 
>>> IFCVF_NET_SUPPORTED_FEATURES;
>>> +
>>> +    if (vf->dev_type == VIRTIO_ID_BLOCK)
>>> +        features = ifcvf_get_features(vf) & 
>>> IFCVF_BLK_SUPPORTED_FEATURES;
>>>         return features;
>>>   }
>>> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>>>                C5000X_PL_DEVICE_ID,
>>>                C5000X_PL_SUBSYS_VENDOR_ID,
>>>                C5000X_PL_SUBSYS_DEVICE_ID) },
>>> +    { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
>>> +             C5000X_PL_BLK_DEVICE_ID,
>>> +             C5000X_PL_BLK_SUBSYS_VENDOR_ID,
>>> +             C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>>>         { 0 },
>>>   };
>>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA

[Zhu Lingshan]

On 4/15/2021 2:31 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午1:55, Zhu Lingshan 写道:
>>
>>
>> On 4/15/2021 11:34 AM, Jason Wang wrote:
>>>
>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
>>>> for vDPA.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>>>>   2 files changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> index 1c04cd256fa7..8b403522bf06 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> @@ -15,6 +15,7 @@
>>>>   #include <linux/pci_regs.h>
>>>>   #include <linux/vdpa.h>
>>>>   #include <uapi/linux/virtio_net.h>
>>>> +#include <uapi/linux/virtio_blk.h>
>>>>   #include <uapi/linux/virtio_config.h>
>>>>   #include <uapi/linux/virtio_pci.h>
>>>>   @@ -28,7 +29,12 @@
>>>>   #define C5000X_PL_SUBSYS_VENDOR_ID    0x8086
>>>>   #define C5000X_PL_SUBSYS_DEVICE_ID    0x0001
>>>>   -#define IFCVF_SUPPORTED_FEATURES \
>>>> +#define C5000X_PL_BLK_VENDOR_ID        0x1AF4
>>>> +#define C5000X_PL_BLK_DEVICE_ID        0x1001
>>>> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID    0x8086
>>>> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID    0x0002
>>>> +
>>>> +#define IFCVF_NET_SUPPORTED_FEATURES \
>>>>           ((1ULL << VIRTIO_NET_F_MAC)            | \
>>>>            (1ULL << VIRTIO_F_ANY_LAYOUT) | \
>>>>            (1ULL << VIRTIO_F_VERSION_1)            | \
>>>> @@ -37,6 +43,15 @@
>>>>            (1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
>>>>            (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>>>>   +#define IFCVF_BLK_SUPPORTED_FEATURES \
>>>> +        ((1ULL << VIRTIO_BLK_F_SIZE_MAX)        | \
>>>> +         (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
>>>> +         (1ULL << VIRTIO_BLK_F_BLK_SIZE)        | \
>>>> +         (1ULL << VIRTIO_BLK_F_TOPOLOGY)        | \
>>>> +         (1ULL << VIRTIO_BLK_F_MQ)            | \
>>>> +         (1ULL << VIRTIO_F_VERSION_1)            | \
>>>> +         (1ULL << VIRTIO_F_ACCESS_PLATFORM))
>>>
>>>
>>> I think we've discussed this sometime in the past but what's the 
>>> reason for such whitelist consider there's already a get_features() 
>>> implemention?
>>>
>>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or 
>>> VIRTIO_F_RING_PACKED?
>>>
>>> Thanks
>> The reason is some feature bits are supported in the device but not 
>> supported by the driver, e.g, for virtio-net, mq & cq implementation 
>> is not ready in the driver.
>
>
> I understand the case of virtio-net but I wonder why we need this for 
> block where we don't vq cvq.
>
> Thanks
This is still a subset of the feature bits read from hardware, I leave 
it here to code consistently, and indicate what we support clearly.
Are you suggesting remove this feature bits list and just use what we 
read from hardware?

Thansk
>
>
>>
>> Thanks!
>>
>>>
>>>
>>>> +
>>>>   /* Only one queue pair for now. */
>>>>   #define IFCVF_MAX_QUEUE_PAIRS    1
>>>>   diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index 99b0a6b4c227..9b6a38b798fa 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>>>> vdpa_device *vdpa_dev)
>>>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>       u64 features;
>>>>   -    features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
>>>> +    if (vf->dev_type == VIRTIO_ID_NET)
>>>> +        features = ifcvf_get_features(vf) & 
>>>> IFCVF_NET_SUPPORTED_FEATURES;
>>>> +
>>>> +    if (vf->dev_type == VIRTIO_ID_BLOCK)
>>>> +        features = ifcvf_get_features(vf) & 
>>>> IFCVF_BLK_SUPPORTED_FEATURES;
>>>>         return features;
>>>>   }
>>>> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>>>>                C5000X_PL_DEVICE_ID,
>>>>                C5000X_PL_SUBSYS_VENDOR_ID,
>>>>                C5000X_PL_SUBSYS_DEVICE_ID) },
>>>> +    { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
>>>> +             C5000X_PL_BLK_DEVICE_ID,
>>>> +             C5000X_PL_BLK_SUBSYS_VENDOR_ID,
>>>> +             C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>>>>         { 0 },
>>>>   };
>>>
>>
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA

[Jason Wang]

在 2021/4/15 下午2:41, Zhu Lingshan 写道:
>>>>
>>>> I think we've discussed this sometime in the past but what's the 
>>>> reason for such whitelist consider there's already a get_features() 
>>>> implemention?
>>>>
>>>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or 
>>>> VIRTIO_F_RING_PACKED?
>>>>
>>>> Thanks
>>> The reason is some feature bits are supported in the device but not 
>>> supported by the driver, e.g, for virtio-net, mq & cq implementation 
>>> is not ready in the driver.
>>
>>
>> I understand the case of virtio-net but I wonder why we need this for 
>> block where we don't vq cvq.
>>
>> Thanks
> This is still a subset of the feature bits read from hardware, I leave 
> it here to code consistently, and indicate what we support clearly.
> Are you suggesting remove this feature bits list and just use what we 
> read from hardware?
>
> Thansk 


Yes, please do that.

The whiltelist doesn't help in this case I think.

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA

[Zhu Lingshan]

On 4/15/2021 3:17 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午2:41, Zhu Lingshan 写道:
>>>>>
>>>>> I think we've discussed this sometime in the past but what's the 
>>>>> reason for such whitelist consider there's already a 
>>>>> get_features() implemention?
>>>>>
>>>>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or 
>>>>> VIRTIO_F_RING_PACKED?
>>>>>
>>>>> Thanks
>>>> The reason is some feature bits are supported in the device but not 
>>>> supported by the driver, e.g, for virtio-net, mq & cq 
>>>> implementation is not ready in the driver.
>>>
>>>
>>> I understand the case of virtio-net but I wonder why we need this 
>>> for block where we don't vq cvq.
>>>
>>> Thanks
>> This is still a subset of the feature bits read from hardware, I 
>> leave it here to code consistently, and indicate what we support 
>> clearly.
>> Are you suggesting remove this feature bits list and just use what we 
>> read from hardware?
>>
>> Thansk 
>
>
> Yes, please do that.
>
> The whiltelist doesn't help in this case I think.
OK, will remove this in V2

Thanks
>
> Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size

[Jason Wang]

在 2021/4/14 下午5:18, Zhu Lingshan 写道:
> get_config_size() should return the size based on the decected
> device type.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 9b6a38b798fa..b48b9789b69e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
>   
>   static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
>   {
> -	return sizeof(struct virtio_net_config);
> +	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> +	size_t size;
> +
> +	if (vf->dev_type == VIRTIO_ID_NET)
> +		size = sizeof(struct virtio_net_config);
> +
> +	if (vf->dev_type == VIRTIO_ID_BLOCK)
> +		size = sizeof(struct virtio_blk_config);
> +
> +	return size;
>   }
>   
>   static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size

[Stefano Garzarella]

On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
>get_config_size() should return the size based on the decected
>device type.
>
>Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>---
> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>index 9b6a38b798fa..b48b9789b69e 100644
>--- a/drivers/vdpa/ifcvf/ifcvf_main.c
>+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
>
> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
> {
>-	return sizeof(struct virtio_net_config);
>+	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>+	size_t size;
>+
>+	if (vf->dev_type == VIRTIO_ID_NET)
>+		size = sizeof(struct virtio_net_config);
>+
>+	if (vf->dev_type == VIRTIO_ID_BLOCK)
>+		size = sizeof(struct virtio_blk_config);
>+
>+	return size;

I'm not familiar with the ifcvf details, but can it happen that the 
device is not block or net?

Should we set `size` to 0 by default to handle this case or are we sure 
it's one of the two?

Maybe we should add a comment or a warning message in this case, to 
prevent some analysis tool or compiler from worrying that `size` might 
be uninitialized.

I was thinking something like this:

	switch(vf->dev_type) {
	case VIRTIO_ID_NET:
		size = sizeof(struct virtio_net_config);
		break;
	case VIRTIO_ID_BLOCK:
		size = sizeof(struct virtio_blk_config);
		break;
	default:
		/* or WARN(1, "") if dev_warn() not apply */
		dev_warn(... , "virtio ID [0x%x] not supported\n")
		size = 0;

	}

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size

[Jason Wang]

在 2021/4/15 下午4:12, Stefano Garzarella 写道:
> On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
>> get_config_size() should return the size based on the decected
>> device type.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 9b6a38b798fa..b48b9789b69e 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct 
>> vdpa_device *vdpa_dev)
>>
>> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
>> {
>> -    return sizeof(struct virtio_net_config);
>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> +    size_t size;
>> +
>> +    if (vf->dev_type == VIRTIO_ID_NET)
>> +        size = sizeof(struct virtio_net_config);
>> +
>> +    if (vf->dev_type == VIRTIO_ID_BLOCK)
>> +        size = sizeof(struct virtio_blk_config);
>> +
>> +    return size;
>
> I'm not familiar with the ifcvf details, but can it happen that the 
> device is not block or net?
>
> Should we set `size` to 0 by default to handle this case or are we 
> sure it's one of the two?
>
> Maybe we should add a comment or a warning message in this case, to 
> prevent some analysis tool or compiler from worrying that `size` might 
> be uninitialized.
>
> I was thinking something like this:
>
>     switch(vf->dev_type) {
>     case VIRTIO_ID_NET:
>         size = sizeof(struct virtio_net_config);
>         break;
>     case VIRTIO_ID_BLOCK:
>         size = sizeof(struct virtio_blk_config);
>         break;
>     default:
>         /* or WARN(1, "") if dev_warn() not apply */
>         dev_warn(... , "virtio ID [0x%x] not supported\n")
>         size = 0;
>
>     }
>

Yes, I agree.

Thanks


> Thanks,
> Stefano
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size

[Zhu Lingshan]

On 4/15/2021 4:12 PM, Stefano Garzarella wrote:
> On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
>> get_config_size() should return the size based on the decected
>> device type.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 9b6a38b798fa..b48b9789b69e 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct 
>> vdpa_device *vdpa_dev)
>>
>> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
>> {
>> -    return sizeof(struct virtio_net_config);
>> +    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> +    size_t size;
>> +
>> +    if (vf->dev_type == VIRTIO_ID_NET)
>> +        size = sizeof(struct virtio_net_config);
>> +
>> +    if (vf->dev_type == VIRTIO_ID_BLOCK)
>> +        size = sizeof(struct virtio_blk_config);
>> +
>> +    return size;
>
> I'm not familiar with the ifcvf details, but can it happen that the 
> device is not block or net?
>
> Should we set `size` to 0 by default to handle this case or are we 
> sure it's one of the two?
>
> Maybe we should add a comment or a warning message in this case, to 
> prevent some analysis tool or compiler from worrying that `size` might 
> be uninitialized.
>
> I was thinking something like this:
>
>     switch(vf->dev_type) {
>     case VIRTIO_ID_NET:
>         size = sizeof(struct virtio_net_config);
>         break;
>     case VIRTIO_ID_BLOCK:
>         size = sizeof(struct virtio_blk_config);
>         break;
>     default:
>         /* or WARN(1, "") if dev_warn() not apply */
>         dev_warn(... , "virtio ID [0x%x] not supported\n")
>         size = 0;
>
>     }
>
> Thanks,
> Stefano
agree, will add this in V2

Thanks
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vDPA/ifcvf: get_config_size should return dev specific config size

[Stefano Garzarella]

On Thu, Apr 15, 2021 at 04:23:15PM +0800, Zhu Lingshan wrote:
>
>
>On 4/15/2021 4:12 PM, Stefano Garzarella wrote:
>>On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
>>>get_config_size() should return the size based on the decected
>>>device type.
>>>
>>>Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>---
>>>drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
>>>1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>index 9b6a38b798fa..b48b9789b69e 100644
>>>--- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct 
>>>vdpa_device *vdpa_dev)
>>>
>>>static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
>>>{
>>>-    return sizeof(struct virtio_net_config);
>>>+    struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>+    size_t size;
>>>+
>>>+    if (vf->dev_type == VIRTIO_ID_NET)
>>>+        size = sizeof(struct virtio_net_config);
>>>+
>>>+    if (vf->dev_type == VIRTIO_ID_BLOCK)
>>>+        size = sizeof(struct virtio_blk_config);
>>>+
>>>+    return size;
>>
>>I'm not familiar with the ifcvf details, but can it happen that the 
>>device is not block or net?
>>
>>Should we set `size` to 0 by default to handle this case or are we 
>>sure it's one of the two?
>>
>>Maybe we should add a comment or a warning message in this case, to 
>>prevent some analysis tool or compiler from worrying that `size` 
>>might be uninitialized.
>>
>>I was thinking something like this:
>>
>>    switch(vf->dev_type) {
>>    case VIRTIO_ID_NET:
>>        size = sizeof(struct virtio_net_config);
>>        break;
>>    case VIRTIO_ID_BLOCK:
>>        size = sizeof(struct virtio_blk_config);
>>        break;
>>    default:
>>        /* or WARN(1, "") if dev_warn() not apply */
>>        dev_warn(... , "virtio ID [0x%x] not supported\n")
>>        size = 0;
>>
>>    }
>>
>>Thanks,
>>Stefano
>agree, will add this in V2

Great, maybe something similar also in patch 2/3 for `features` in 
ifcvf_vdpa_get_features().

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization