Re: [PATCH V3 4/6] vDPA/ifcvf: remove the version number string

Re: [PATCH V3 4/6] vDPA/ifcvf: remove the version number string

[Leon Romanovsky]

On Wed, Mar 10, 2021 at 05:00:50PM +0800, Zhu Lingshan wrote:
> This commit removes the version number string, using kernel
> version is enough.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_main.c | 2 --
>  1 file changed, 2 deletions(-)
>

I already added my ROB, but will add again.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3 4/6] vDPA/ifcvf: remove the version number string

[Zhu Lingshan]

Thanks Leon, I will include your ROB if there is a V4.

On 3/10/2021 5:16 PM, Leon Romanovsky wrote:
> On Wed, Mar 10, 2021 at 05:00:50PM +0800, Zhu Lingshan wrote:
>> This commit removes the version number string, using kernel
>> version is enough.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
> I already added my ROB, but will add again.
>
> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> _______________________________________________
> 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 V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA

[Jason Wang]

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
> examines this when set features.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>   3 files changed, 14 insertions(+)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index ea6a78791c9b..58f47fdce385 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>   	return hw->hw_features;
>   }
>   
> +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
> +{
> +	if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>   			   void *dst, int length)
>   {
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index dbb8c10aa3b1..91c5735d4dc9 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, u32 *hi);
>   void ifcvf_reset(struct ifcvf_hw *hw);
>   u64 ifcvf_get_features(struct ifcvf_hw *hw);
>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>   int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>   struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 25fb9dfe23f0..f624f202447d 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
>   static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, u64 features)
>   {
>   	struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> +	int ret;
> +
> +	ret = ifcvf_verify_min_features(vf);


So this validate device features instead of driver which is the one we 
really want to check?

Thanks


> +	if (ret)
> +		return ret;
>   
>   	vf->req_features = features;
>   

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

Re: [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id

[Jason Wang]

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
> In this commit, ifcvf_get_vendor_id() will return
> a device specific vendor id of the probed pci device
> than a hard code.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index fa1af301cf55..e501ee07de17 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -324,7 +324,10 @@ static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>   
>   static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>   {
> -	return IFCVF_SUBSYS_VENDOR_ID;
> +	struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
> +	struct pci_dev *pdev = adapter->pdev;
> +
> +	return pdev->subsystem_vendor;
>   }


While at this, I wonder if we can do something similar in 
get_device_id() if it could be simple deduced from some simple math from 
the pci device id?

Thanks


>   
>   static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)

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

Re: [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids

[Jason Wang]

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
> IFCVF driver probes multiple types of devices now,
> to distinguish the original device driven by IFCVF
> from others, it is renamed as "N3000".
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>   drivers/vdpa/ifcvf/ifcvf_base.h | 8 ++++----
>   drivers/vdpa/ifcvf/ifcvf_main.c | 8 ++++----
>   2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index 75d9a8052039..794d1505d857 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -18,10 +18,10 @@
>   #include <uapi/linux/virtio_config.h>
>   #include <uapi/linux/virtio_pci.h>
>   
> -#define IFCVF_VENDOR_ID		0x1AF4
> -#define IFCVF_DEVICE_ID		0x1041
> -#define IFCVF_SUBSYS_VENDOR_ID	0x8086
> -#define IFCVF_SUBSYS_DEVICE_ID	0x001A
> +#define N3000_VENDOR_ID		0x1AF4
> +#define N3000_DEVICE_ID		0x1041
> +#define N3000_SUBSYS_VENDOR_ID	0x8086
> +#define N3000_SUBSYS_DEVICE_ID	0x001A
>   
>   #define C5000X_PL_VENDOR_ID		0x1AF4
>   #define C5000X_PL_DEVICE_ID		0x1000
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 26a2dab7ca66..fd5befc5cbcc 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -480,10 +480,10 @@ static void ifcvf_remove(struct pci_dev *pdev)
>   }
>   
>   static struct pci_device_id ifcvf_pci_ids[] = {
> -	{ PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
> -		IFCVF_DEVICE_ID,
> -		IFCVF_SUBSYS_VENDOR_ID,
> -		IFCVF_SUBSYS_DEVICE_ID) },
> +	{ PCI_DEVICE_SUB(N3000_VENDOR_ID,
> +			 N3000_DEVICE_ID,


I am not sure the plan for Intel but I wonder if we can simply use 
PCI_ANY_ID for device id here. Otherewise you need to maintain a very 
long list of ids here.

Thanks


> +			 N3000_SUBSYS_VENDOR_ID,
> +			 N3000_SUBSYS_DEVICE_ID) },
>   	{ PCI_DEVICE_SUB(C5000X_PL_VENDOR_ID,
>   			 C5000X_PL_DEVICE_ID,
>   			 C5000X_PL_SUBSYS_VENDOR_ID,

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

Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA

[Zhu Lingshan]

On 3/11/2021 11:20 AM, Jason Wang wrote:
>
> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
>> examines this when set features.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>>   3 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index ea6a78791c9b..58f47fdce385 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>>       return hw->hw_features;
>>   }
>>   +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
>> +{
>> +    if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>> +        return -EINVAL;
>> +
>> +    return 0;
>> +}
>> +
>>   void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>>                  void *dst, int length)
>>   {
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index dbb8c10aa3b1..91c5735d4dc9 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, u32 *hi);
>>   void ifcvf_reset(struct ifcvf_hw *hw);
>>   u64 ifcvf_get_features(struct ifcvf_hw *hw);
>>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>>   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>>   int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>>   struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 25fb9dfe23f0..f624f202447d 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>> vdpa_device *vdpa_dev)
>>   static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, 
>> u64 features)
>>   {
>>       struct ifcvf_hw *vf = vdpa_to_intersectionvf(vdpa_dev);
>> +    int ret;
>> +
>> +    ret = ifcvf_verify_min_features(vf);
>
>
> So this validate device features instead of driver which is the one we 
> really want to check?
>
> Thanks
Hi Jason,

Here we check device feature bits to make sure the device support 
ACCESS_PLATFORM. In get_features(),
it will return a intersection of device features bit and driver 
supported features bits(which includes ACCESS_PLATFORM).
Other components like QEMU should not set features bits more than this 
intersection of bits. so we can make sure if this
ifcvf_verify_min_features() passed, both device and driver support 
ACCESS_PLATFORM.

Are you suggesting check driver feature bits in 
ifcvf_verify_min_features() in the meantime as well?

Thanks！
>
>
>> +    if (ret)
>> +        return ret;
>>         vf->req_features = features;
>
> _______________________________________________
> 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 V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA

[Zhu Lingshan]

On 3/11/2021 11:20 AM, Jason Wang wrote:
>
> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
>> examines this when set features.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>>   3 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index ea6a78791c9b..58f47fdce385 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>>       return hw->hw_features;
>>   }
>>   +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
>> +{
>> +    if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>> +        return -EINVAL;
>> +
>> +    return 0;
>> +}
>> +
>>   void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>>                  void *dst, int length)
>>   {
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index dbb8c10aa3b1..91c5735d4dc9 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, u32 *hi);
>>   void ifcvf_reset(struct ifcvf_hw *hw);
>>   u64 ifcvf_get_features(struct ifcvf_hw *hw);
>>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>>   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>>   int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>>   struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 25fb9dfe23f0..f624f202447d 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>> vdpa_device *vdpa_dev)
>>   static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, 
>> u64 features)
>>   {
>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> +    int ret;
>> +
>> +    ret = ifcvf_verify_min_features(vf);
>
>
> So this validate device features instead of driver which is the one we 
> really want to check?
>
> Thanks

Hi Jason,

Here we check device feature bits to make sure the device support 
ACCESS_PLATFORM. In get_features(),
it will return a intersection of device features bit and driver 
supported features bits(which includes ACCESS_PLATFORM).
Other components like QEMU should not set features bits more than this 
intersection of bits. so we can make sure if this
ifcvf_verify_min_features() passed, both device and driver support 
ACCESS_PLATFORM.

Are you suggesting check driver feature bits in 
ifcvf_verify_min_features() in the meantime as well?

Thanks！
>
>
>> +    if (ret)
>> +        return ret;
>>         vf->req_features = features;
>
> _______________________________________________
> 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 V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id

[Zhu Lingshan]

On 3/11/2021 11:23 AM, Jason Wang wrote:
>
> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>> In this commit, ifcvf_get_vendor_id() will return
>> a device specific vendor id of the probed pci device
>> than a hard code.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index fa1af301cf55..e501ee07de17 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -324,7 +324,10 @@ static u32 ifcvf_vdpa_get_device_id(struct 
>> vdpa_device *vdpa_dev)
>>     static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>   {
>> -    return IFCVF_SUBSYS_VENDOR_ID;
>> +    struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>> +    struct pci_dev *pdev = adapter->pdev;
>> +
>> +    return pdev->subsystem_vendor;
>>   }
>
>
> While at this, I wonder if we can do something similar in 
> get_device_id() if it could be simple deduced from some simple math 
> from the pci device id?
>
> Thanks
Hi Jason,

IMHO, this implementation is just some memory read ops, I think other 
implementations may not save many cpu cycles, an if cost at least three 
cpu cycles.

Thanks!
>
>
>>     static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
>

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

Re: [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids

[Zhu Lingshan]

On 3/11/2021 11:25 AM, Jason Wang wrote:
>
> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>> IFCVF driver probes multiple types of devices now,
>> to distinguish the original device driven by IFCVF
>> from others, it is renamed as "N3000".
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 8 ++++----
>>   drivers/vdpa/ifcvf/ifcvf_main.c | 8 ++++----
>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index 75d9a8052039..794d1505d857 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -18,10 +18,10 @@
>>   #include <uapi/linux/virtio_config.h>
>>   #include <uapi/linux/virtio_pci.h>
>>   -#define IFCVF_VENDOR_ID        0x1AF4
>> -#define IFCVF_DEVICE_ID        0x1041
>> -#define IFCVF_SUBSYS_VENDOR_ID    0x8086
>> -#define IFCVF_SUBSYS_DEVICE_ID    0x001A
>> +#define N3000_VENDOR_ID        0x1AF4
>> +#define N3000_DEVICE_ID        0x1041
>> +#define N3000_SUBSYS_VENDOR_ID    0x8086
>> +#define N3000_SUBSYS_DEVICE_ID    0x001A
>>     #define C5000X_PL_VENDOR_ID        0x1AF4
>>   #define C5000X_PL_DEVICE_ID        0x1000
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 26a2dab7ca66..fd5befc5cbcc 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -480,10 +480,10 @@ static void ifcvf_remove(struct pci_dev *pdev)
>>   }
>>     static struct pci_device_id ifcvf_pci_ids[] = {
>> -    { PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
>> -        IFCVF_DEVICE_ID,
>> -        IFCVF_SUBSYS_VENDOR_ID,
>> -        IFCVF_SUBSYS_DEVICE_ID) },
>> +    { PCI_DEVICE_SUB(N3000_VENDOR_ID,
>> +             N3000_DEVICE_ID,
>
>
> I am not sure the plan for Intel but I wonder if we can simply use 
> PCI_ANY_ID for device id here. Otherewise you need to maintain a very 
> long list of ids here.
>
> Thanks
Hi Jason,

Thanks! but maybe if we present a very simple and clear list like what 
e1000 does can help the users understand what we support easily.

Thanks!
>
>
>> + N3000_SUBSYS_VENDOR_ID,
>> +             N3000_SUBSYS_DEVICE_ID) },
>>       { PCI_DEVICE_SUB(C5000X_PL_VENDOR_ID,
>>                C5000X_PL_DEVICE_ID,
>>                C5000X_PL_SUBSYS_VENDOR_ID,
>

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

Re: [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id

[Jason Wang]

On 2021/3/11 12:21 下午, Zhu Lingshan wrote:
>
>
> On 3/11/2021 11:23 AM, Jason Wang wrote:
>>
>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>>> In this commit, ifcvf_get_vendor_id() will return
>>> a device specific vendor id of the probed pci device
>>> than a hard code.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index fa1af301cf55..e501ee07de17 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -324,7 +324,10 @@ static u32 ifcvf_vdpa_get_device_id(struct 
>>> vdpa_device *vdpa_dev)
>>>     static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>>   {
>>> -    return IFCVF_SUBSYS_VENDOR_ID;
>>> +    struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>> +    struct pci_dev *pdev = adapter->pdev;
>>> +
>>> +    return pdev->subsystem_vendor;
>>>   }
>>
>>
>> While at this, I wonder if we can do something similar in 
>> get_device_id() if it could be simple deduced from some simple math 
>> from the pci device id?
>>
>> Thanks
> Hi Jason,
>
> IMHO, this implementation is just some memory read ops, I think other 
> implementations may not save many cpu cycles, an if cost at least 
> three cpu cycles.
>
> Thanks!


Well, I meant whehter you can deduce virtio device id from 
pdev->subsystem_device.

Thanks


>>
>>
>>>     static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
>>
>

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

Re: [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids

[Jason Wang]

On 2021/3/11 12:23 下午, Zhu Lingshan wrote:
>
>
> On 3/11/2021 11:25 AM, Jason Wang wrote:
>>
>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>>> IFCVF driver probes multiple types of devices now,
>>> to distinguish the original device driven by IFCVF
>>> from others, it is renamed as "N3000".
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 8 ++++----
>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 8 ++++----
>>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> index 75d9a8052039..794d1505d857 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> @@ -18,10 +18,10 @@
>>>   #include <uapi/linux/virtio_config.h>
>>>   #include <uapi/linux/virtio_pci.h>
>>>   -#define IFCVF_VENDOR_ID        0x1AF4
>>> -#define IFCVF_DEVICE_ID        0x1041
>>> -#define IFCVF_SUBSYS_VENDOR_ID    0x8086
>>> -#define IFCVF_SUBSYS_DEVICE_ID    0x001A
>>> +#define N3000_VENDOR_ID        0x1AF4
>>> +#define N3000_DEVICE_ID        0x1041
>>> +#define N3000_SUBSYS_VENDOR_ID    0x8086
>>> +#define N3000_SUBSYS_DEVICE_ID    0x001A
>>>     #define C5000X_PL_VENDOR_ID        0x1AF4
>>>   #define C5000X_PL_DEVICE_ID        0x1000
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index 26a2dab7ca66..fd5befc5cbcc 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -480,10 +480,10 @@ static void ifcvf_remove(struct pci_dev *pdev)
>>>   }
>>>     static struct pci_device_id ifcvf_pci_ids[] = {
>>> -    { PCI_DEVICE_SUB(IFCVF_VENDOR_ID,
>>> -        IFCVF_DEVICE_ID,
>>> -        IFCVF_SUBSYS_VENDOR_ID,
>>> -        IFCVF_SUBSYS_DEVICE_ID) },
>>> +    { PCI_DEVICE_SUB(N3000_VENDOR_ID,
>>> +             N3000_DEVICE_ID,
>>
>>
>> I am not sure the plan for Intel but I wonder if we can simply use 
>> PCI_ANY_ID for device id here. Otherewise you need to maintain a very 
>> long list of ids here.
>>
>> Thanks
> Hi Jason,
>
> Thanks! but maybe if we present a very simple and clear list like what 
> e1000 does can help the users understand what we support easily.
>
> Thanks!


That's fine.

Thanks


>>
>>
>>> + N3000_SUBSYS_VENDOR_ID,
>>> +             N3000_SUBSYS_DEVICE_ID) },
>>>       { PCI_DEVICE_SUB(C5000X_PL_VENDOR_ID,
>>>                C5000X_PL_DEVICE_ID,
>>>                C5000X_PL_SUBSYS_VENDOR_ID,
>>
>

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

Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA

[Jason Wang]

On 2021/3/11 12:16 下午, Zhu Lingshan wrote:
>
>
> On 3/11/2021 11:20 AM, Jason Wang wrote:
>>
>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
>>> examines this when set features.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>>>   3 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>> index ea6a78791c9b..58f47fdce385 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>>>       return hw->hw_features;
>>>   }
>>>   +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
>>> +{
>>> +    if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>> +        return -EINVAL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>>>                  void *dst, int length)
>>>   {
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> index dbb8c10aa3b1..91c5735d4dc9 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, u32 *hi);
>>>   void ifcvf_reset(struct ifcvf_hw *hw);
>>>   u64 ifcvf_get_features(struct ifcvf_hw *hw);
>>>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>>>   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>>>   int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>>>   struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index 25fb9dfe23f0..f624f202447d 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>>> vdpa_device *vdpa_dev)
>>>   static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, 
>>> u64 features)
>>>   {
>>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> +    int ret;
>>> +
>>> +    ret = ifcvf_verify_min_features(vf);
>>
>>
>> So this validate device features instead of driver which is the one 
>> we really want to check?
>>
>> Thanks
>
> Hi Jason,
>
> Here we check device feature bits to make sure the device support 
> ACCESS_PLATFORM. 


If you want to check device features, you need to do that during probe() 
and fail the probing if without the feature. But I think you won't ship 
cards without ACCESS_PLATFORM.


> In get_features(),
> it will return a intersection of device features bit and driver 
> supported features bits(which includes ACCESS_PLATFORM).
> Other components like QEMU should not set features bits more than this 
> intersection of bits. so we can make sure if this
> ifcvf_verify_min_features() passed, both device and driver support 
> ACCESS_PLATFORM.
>
> Are you suggesting check driver feature bits in 
> ifcvf_verify_min_features() in the meantime as well?


So it really depends on your hardware. If you hardware can always offer 
ACCESS_PLATFORM, you just need to check driver features. This is how 
vdpa_sim and mlx5_vdpa work.

Thanks


>
> Thanks！
>>
>>
>>> +    if (ret)
>>> +        return ret;
>>>         vf->req_features = features;
>>
>> _______________________________________________
>> 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 V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA

[Jason Wang]

On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:
>
>
> On 3/11/2021 2:20 PM, Jason Wang wrote:
>>
>> On 2021/3/11 12:16 下午, Zhu Lingshan wrote:
>>>
>>>
>>> On 3/11/2021 11:20 AM, Jason Wang wrote:
>>>>
>>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
>>>>> examines this when set features.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---
>>>>>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>>>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>>>>>   3 files changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> index ea6a78791c9b..58f47fdce385 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>>>>>       return hw->hw_features;
>>>>>   }
>>>>>   +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
>>>>> +{
>>>>> +    if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>>>>>                  void *dst, int length)
>>>>>   {
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> index dbb8c10aa3b1..91c5735d4dc9 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, u32 
>>>>> *hi);
>>>>>   void ifcvf_reset(struct ifcvf_hw *hw);
>>>>>   u64 ifcvf_get_features(struct ifcvf_hw *hw);
>>>>>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>>>>>   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>>>>>   int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>>>>>   struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> index 25fb9dfe23f0..f624f202447d 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>>>>> vdpa_device *vdpa_dev)
>>>>>   static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, 
>>>>> u64 features)
>>>>>   {
>>>>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = ifcvf_verify_min_features(vf);
>>>>
>>>>
>>>> So this validate device features instead of driver which is the one 
>>>> we really want to check?
>>>>
>>>> Thanks
>>>
>>> Hi Jason,
>>>
>>> Here we check device feature bits to make sure the device support 
>>> ACCESS_PLATFORM. 
>>
>>
>> If you want to check device features, you need to do that during 
>> probe() and fail the probing if without the feature. But I think you 
>> won't ship cards without ACCESS_PLATFORM.
> Yes, there are no reasons ship a card without ACCESS_PLATFORM
>>
>>
>>> In get_features(),
>>> it will return a intersection of device features bit and driver 
>>> supported features bits(which includes ACCESS_PLATFORM).
>>> Other components like QEMU should not set features bits more than 
>>> this intersection of bits. so we can make sure if this
>>> ifcvf_verify_min_features() passed, both device and driver support 
>>> ACCESS_PLATFORM.
>>>
>>> Are you suggesting check driver feature bits in 
>>> ifcvf_verify_min_features() in the meantime as well?
>>
>>
>> So it really depends on your hardware. If you hardware can always 
>> offer ACCESS_PLATFORM, you just need to check driver features. This 
>> is how vdpa_sim and mlx5_vdpa work.
> Yes, we always support ACCESS_PLATFORM, so it is hard coded in the 
> macro IFCVF_SUPPORTED_FEATURES.


That's not what I read from the code:

         features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;


> Now we check whether device support this feature bit as a double 
> conformation, are you suggesting we should check whether 
> ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES
> in set_features() as well?


If we know device will always offer ACCESS_PLATFORM, there's no need to 
check it again. What we should check if whether driver set that, and if 
it doesn't we need to fail set_features(). I think there's little chance 
that IFCVF can work when IOMMU_PLATFORM is not negotiated.


> I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more 
> reliable.


So again, if you want to check device features, set_features() is not 
the proper place. We need to fail the probe in this case.

Thanks


>
> Thanks!
>>
>> Thanks
>>
>>
>>>
>>> Thanks！
>>>>
>>>>
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>>         vf->req_features = features;
>>>>
>>>> _______________________________________________
>>>> 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 V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA

[Jason Wang]

On 2021/3/12 2:40 下午, Zhu, Lingshan wrote:
>
>
> On 3/12/2021 1:52 PM, Jason Wang wrote:
>>
>> On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:
>>>
>>>
>>> On 3/11/2021 2:20 PM, Jason Wang wrote:
>>>>
>>>> On 2021/3/11 12:16 下午, Zhu Lingshan wrote:
>>>>>
>>>>>
>>>>> On 3/11/2021 11:20 AM, Jason Wang wrote:
>>>>>>
>>>>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>>>>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
>>>>>>> examines this when set features.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>> ---
>>>>>>>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>>>>>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>>>>>>>   3 files changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>>> index ea6a78791c9b..58f47fdce385 100644
>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>>>>>>>       return hw->hw_features;
>>>>>>>   }
>>>>>>>   +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
>>>>>>> +{
>>>>>>> +    if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>   void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>>>>>>>                  void *dst, int length)
>>>>>>>   {
>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>>> index dbb8c10aa3b1..91c5735d4dc9 100644
>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>>> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, 
>>>>>>> u32 *hi);
>>>>>>>   void ifcvf_reset(struct ifcvf_hw *hw);
>>>>>>>   u64 ifcvf_get_features(struct ifcvf_hw *hw);
>>>>>>>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>>>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>>>>>>>   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>>>>>>>   int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>>>>>>>   struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> index 25fb9dfe23f0..f624f202447d 100644
>>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>>>>>>> vdpa_device *vdpa_dev)
>>>>>>>   static int ifcvf_vdpa_set_features(struct vdpa_device 
>>>>>>> *vdpa_dev, u64 features)
>>>>>>>   {
>>>>>>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ret = ifcvf_verify_min_features(vf);
>>>>>>
>>>>>>
>>>>>> So this validate device features instead of driver which is the 
>>>>>> one we really want to check?
>>>>>>
>>>>>> Thanks
>>>>>
>>>>> Hi Jason,
>>>>>
>>>>> Here we check device feature bits to make sure the device support 
>>>>> ACCESS_PLATFORM. 
>>>>
>>>>
>>>> If you want to check device features, you need to do that during 
>>>> probe() and fail the probing if without the feature. But I think 
>>>> you won't ship cards without ACCESS_PLATFORM.
>>> Yes, there are no reasons ship a card without ACCESS_PLATFORM
>>>>
>>>>
>>>>> In get_features(),
>>>>> it will return a intersection of device features bit and driver 
>>>>> supported features bits(which includes ACCESS_PLATFORM).
>>>>> Other components like QEMU should not set features bits more than 
>>>>> this intersection of bits. so we can make sure if this
>>>>> ifcvf_verify_min_features() passed, both device and driver support 
>>>>> ACCESS_PLATFORM.
>>>>>
>>>>> Are you suggesting check driver feature bits in 
>>>>> ifcvf_verify_min_features() in the meantime as well?
>>>>
>>>>
>>>> So it really depends on your hardware. If you hardware can always 
>>>> offer ACCESS_PLATFORM, you just need to check driver features. This 
>>>> is how vdpa_sim and mlx5_vdpa work.
>>> Yes, we always support ACCESS_PLATFORM, so it is hard coded in the 
>>> macro IFCVF_SUPPORTED_FEATURES.
>>
>>
>> That's not what I read from the code:
>>
>>         features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
> ifcvf_get_features() reads device feature bits(which should always has 
> ACCSSS_PLATFORM) and IFCVF_SUPPORTED_FEATURES is the driver supported 
> feature bits 


For "driver" you probably mean IFCVF. So there's some misunderstanding 
before, what I meant for "driver" is virtio driver that do feature 
negotaitation with the device.

I wonder what features are supported by the device but not the IFCVF driver?

Thanks


> which hard coded ACCESS_PLATFORM, so the intersection should include 
> ACCESS_PLATFORM.
> the intersection "features" is returned in get_features(), qemu should 
> set features according to it.
>>
>>
>>> Now we check whether device support this feature bit as a double 
>>> conformation, are you suggesting we should check whether 
>>> ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES
>>> in set_features() as well?
>>
>>
>> If we know device will always offer ACCESS_PLATFORM, there's no need 
>> to check it again. What we should check if whether driver set that, 
>> and if it doesn't we need to fail set_features(). I think there's 
>> little chance that IFCVF can work when IOMMU_PLATFORM is not negotiated.
> Agree, will check the features bit to set instead of device feature 
> bits. Thanks!
>>
>>
>>
>>> I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more 
>>> reliable.
>>
>>
>> So again, if you want to check device features, set_features() is not 
>> the proper place. We need to fail the probe in this case.
>>
>> Thanks
>>
>>
>>>
>>> Thanks!
>>>>
>>>> Thanks
>>>>
>>>>
>>>>>
>>>>> Thanks！
>>>>>>
>>>>>>
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>>>         vf->req_features = features;
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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