[PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net
@ 2021-03-10  9:00 Zhu Lingshan
  2021-03-10  9:00 ` [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id Zhu Lingshan
                   ` (5 more replies)
  0 siblings, 6 replies; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-10  9:00 UTC (permalink / raw)
  To: jasowang, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel, Zhu Lingshan

This series enabled Intel FGPA SmartNIC C5000X-PL
virtio-net for vDPA.
vDPA requires VIRTIO_F_ACCESS_PLATFORM as a must, this series
verify this feature bit when set features.

Changes from V2:
verify VIRTIO_F_ACCESS_PLATFORM when set features(Jason)

Changes from V1:
remove version number string(Leon)
add new device ids and remove original device ids in
separate patches(Jason)

Zhu Lingshan (6):
  vDPA/ifcvf: get_vendor_id returns a device specific vendor id
  vDPA/ifcvf: enable Intel C5000X-PL virtio-net for vDPA
  vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids
  vDPA/ifcvf: remove the version number string
  vDPA/ifcvf: fetch device feature bits when probe
  vDPA/ifcvf: verify mandatory feature bits for vDPA

 drivers/vdpa/ifcvf/ifcvf_base.c | 20 ++++++++++++++++++--
 drivers/vdpa/ifcvf/ifcvf_base.h | 16 ++++++++++++----
 drivers/vdpa/ifcvf/ifcvf_main.c | 27 ++++++++++++++++++++-------
 3 files changed, 50 insertions(+), 13 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id
  2021-03-10  9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
@ 2021-03-10  9:00 ` Zhu Lingshan
  2021-03-11  3:23   ` Jason Wang
  2021-03-10  9:00 ` [PATCH V3 2/6] vDPA/ifcvf: enable Intel C5000X-PL virtio-net for vDPA Zhu Lingshan
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-10  9:00 UTC (permalink / raw)
  To: jasowang, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel, Zhu Lingshan

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;
 }
 
 static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id
  2021-03-10  9:00 ` [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id Zhu Lingshan
@ 2021-03-11  3:23   ` Jason Wang
  2021-03-11  4:21     ` Zhu Lingshan
  0 siblings, 1 reply; 23+ messages in thread
From: Jason Wang @ 2021-03-11  3:23 UTC (permalink / raw)
  To: Zhu Lingshan, mst, lulu, leonro; +Cc: virtualization, netdev, kvm, linux-kernel


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)


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id
  2021-03-11  3:23   ` Jason Wang
@ 2021-03-11  4:21     ` Zhu Lingshan
  2021-03-11  6:13       ` Jason Wang
  0 siblings, 1 reply; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-11  4:21 UTC (permalink / raw)
  To: Jason Wang, Zhu Lingshan, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel



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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id
  2021-03-11  4:21     ` Zhu Lingshan
@ 2021-03-11  6:13       ` Jason Wang
  2021-03-11  7:34         ` Zhu, Lingshan
  0 siblings, 1 reply; 23+ messages in thread
From: Jason Wang @ 2021-03-11  6:13 UTC (permalink / raw)
  To: Zhu Lingshan, Zhu Lingshan, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id
  2021-03-11  6:13       ` Jason Wang
@ 2021-03-11  7:34         ` Zhu, Lingshan
  0 siblings, 0 replies; 23+ messages in thread
From: Zhu, Lingshan @ 2021-03-11  7:34 UTC (permalink / raw)
  To: Jason Wang, Zhu Lingshan, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel



On 3/11/2021 2:13 PM, Jason Wang wrote:
>
> 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
Oh, sure, I get you
>
>
>>>
>>>
>>>>     static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
>>>
>>
>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH V3 2/6] vDPA/ifcvf: enable Intel C5000X-PL virtio-net for vDPA
  2021-03-10  9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
  2021-03-10  9:00 ` [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id Zhu Lingshan
@ 2021-03-10  9:00 ` Zhu Lingshan
  2021-03-10  9:00 ` [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids Zhu Lingshan
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-10  9:00 UTC (permalink / raw)
  To: jasowang, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel, Zhu Lingshan

This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-net
for vDPA

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.h | 5 +++++
 drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 64696d63fe07..75d9a8052039 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -23,6 +23,11 @@
 #define IFCVF_SUBSYS_VENDOR_ID	0x8086
 #define IFCVF_SUBSYS_DEVICE_ID	0x001A
 
+#define C5000X_PL_VENDOR_ID		0x1AF4
+#define C5000X_PL_DEVICE_ID		0x1000
+#define C5000X_PL_SUBSYS_VENDOR_ID	0x8086
+#define C5000X_PL_SUBSYS_DEVICE_ID	0x0001
+
 #define IFCVF_SUPPORTED_FEATURES \
 		((1ULL << VIRTIO_NET_F_MAC)			| \
 		 (1ULL << VIRTIO_F_ANY_LAYOUT)			| \
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index e501ee07de17..26a2dab7ca66 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -484,6 +484,11 @@ static struct pci_device_id ifcvf_pci_ids[] = {
 		IFCVF_DEVICE_ID,
 		IFCVF_SUBSYS_VENDOR_ID,
 		IFCVF_SUBSYS_DEVICE_ID) },
+	{ PCI_DEVICE_SUB(C5000X_PL_VENDOR_ID,
+			 C5000X_PL_DEVICE_ID,
+			 C5000X_PL_SUBSYS_VENDOR_ID,
+			 C5000X_PL_SUBSYS_DEVICE_ID) },
+
 	{ 0 },
 };
 MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids
  2021-03-10  9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
  2021-03-10  9:00 ` [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id Zhu Lingshan
  2021-03-10  9:00 ` [PATCH V3 2/6] vDPA/ifcvf: enable Intel C5000X-PL virtio-net for vDPA Zhu Lingshan
@ 2021-03-10  9:00 ` Zhu Lingshan
  2021-03-11  3:25   ` Jason Wang
  2021-03-10  9:00 ` [PATCH V3 4/6] vDPA/ifcvf: remove the version number string Zhu Lingshan
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-10  9:00 UTC (permalink / raw)
  To: jasowang, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel, Zhu Lingshan

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,
+			 N3000_SUBSYS_VENDOR_ID,
+			 N3000_SUBSYS_DEVICE_ID) },
 	{ PCI_DEVICE_SUB(C5000X_PL_VENDOR_ID,
 			 C5000X_PL_DEVICE_ID,
 			 C5000X_PL_SUBSYS_VENDOR_ID,
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids
  2021-03-10  9:00 ` [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids Zhu Lingshan
@ 2021-03-11  3:25   ` Jason Wang
  2021-03-11  4:23     ` Zhu Lingshan
  0 siblings, 1 reply; 23+ messages in thread
From: Jason Wang @ 2021-03-11  3:25 UTC (permalink / raw)
  To: Zhu Lingshan, mst, lulu, leonro; +Cc: virtualization, netdev, kvm, linux-kernel


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,


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids
  2021-03-11  3:25   ` Jason Wang
@ 2021-03-11  4:23     ` Zhu Lingshan
  2021-03-11  6:14       ` Jason Wang
  0 siblings, 1 reply; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-11  4:23 UTC (permalink / raw)
  To: Jason Wang, Zhu Lingshan, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel



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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids
  2021-03-11  4:23     ` Zhu Lingshan
@ 2021-03-11  6:14       ` Jason Wang
  0 siblings, 0 replies; 23+ messages in thread
From: Jason Wang @ 2021-03-11  6:14 UTC (permalink / raw)
  To: Zhu Lingshan, Zhu Lingshan, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH V3 4/6] vDPA/ifcvf: remove the version number string
  2021-03-10  9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
                   ` (2 preceding siblings ...)
  2021-03-10  9:00 ` [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids Zhu Lingshan
@ 2021-03-10  9:00 ` Zhu Lingshan
  2021-03-10  9:16   ` Leon Romanovsky
  2021-03-10  9:00 ` [PATCH V3 5/6] vDPA/ifcvf: fetch device feature bits when probe Zhu Lingshan
  2021-03-10  9:00 ` [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA Zhu Lingshan
  5 siblings, 1 reply; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-10  9:00 UTC (permalink / raw)
  To: jasowang, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel, Zhu Lingshan

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(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index fd5befc5cbcc..c34e1eec6b6c 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -14,7 +14,6 @@
 #include <linux/sysfs.h>
 #include "ifcvf_base.h"
 
-#define VERSION_STRING  "0.1"
 #define DRIVER_AUTHOR   "Intel Corporation"
 #define IFCVF_DRIVER_NAME       "ifcvf"
 
@@ -503,4 +502,3 @@ static struct pci_driver ifcvf_driver = {
 module_pci_driver(ifcvf_driver);
 
 MODULE_LICENSE("GPL v2");
-MODULE_VERSION(VERSION_STRING);
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 4/6] vDPA/ifcvf: remove the version number string
  2021-03-10  9:00 ` [PATCH V3 4/6] vDPA/ifcvf: remove the version number string Zhu Lingshan
@ 2021-03-10  9:16   ` Leon Romanovsky
  0 siblings, 0 replies; 23+ messages in thread
From: Leon Romanovsky @ 2021-03-10  9:16 UTC (permalink / raw)
  To: Zhu Lingshan
  Cc: jasowang, mst, lulu, virtualization, netdev, kvm, linux-kernel

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>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH V3 5/6] vDPA/ifcvf: fetch device feature bits when probe
  2021-03-10  9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
                   ` (3 preceding siblings ...)
  2021-03-10  9:00 ` [PATCH V3 4/6] vDPA/ifcvf: remove the version number string Zhu Lingshan
@ 2021-03-10  9:00 ` Zhu Lingshan
  2021-03-10  9:00 ` [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA Zhu Lingshan
  5 siblings, 0 replies; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-10  9:00 UTC (permalink / raw)
  To: jasowang, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel, Zhu Lingshan

This commit would read and store device feature
bits when probe.

rename ifcvf_get_features() to ifcvf_get_hw_features(),
it reads and stores features of the probed device.

new ifcvf_get_features() simply returns stored
feature bits.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 12 ++++++++++--
 drivers/vdpa/ifcvf/ifcvf_base.h |  2 ++
 drivers/vdpa/ifcvf/ifcvf_main.c |  2 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index f2a128e56de5..ea6a78791c9b 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -202,10 +202,11 @@ static void ifcvf_add_status(struct ifcvf_hw *hw, u8 status)
 	ifcvf_get_status(hw);
 }
 
-u64 ifcvf_get_features(struct ifcvf_hw *hw)
+u64 ifcvf_get_hw_features(struct ifcvf_hw *hw)
 {
 	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
 	u32 features_lo, features_hi;
+	u64 features;
 
 	ifc_iowrite32(0, &cfg->device_feature_select);
 	features_lo = ifc_ioread32(&cfg->device_feature);
@@ -213,7 +214,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
 	ifc_iowrite32(1, &cfg->device_feature_select);
 	features_hi = ifc_ioread32(&cfg->device_feature);
 
-	return ((u64)features_hi << 32) | features_lo;
+	features = ((u64)features_hi << 32) | features_lo;
+
+	return features;
+}
+
+u64 ifcvf_get_features(struct ifcvf_hw *hw)
+{
+	return hw->hw_features;
 }
 
 void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 794d1505d857..dbb8c10aa3b1 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -83,6 +83,7 @@ struct ifcvf_hw {
 	void __iomem *notify_base;
 	u32 notify_off_multiplier;
 	u64 req_features;
+	u64 hw_features;
 	struct virtio_pci_common_cfg __iomem *common_cfg;
 	void __iomem *net_cfg;
 	struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
@@ -121,6 +122,7 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status);
 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);
 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 c34e1eec6b6c..25fb9dfe23f0 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -458,6 +458,8 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	for (i = 0; i < IFCVF_MAX_QUEUE_PAIRS * 2; i++)
 		vf->vring[i].irq = -EINVAL;
 
+	vf->hw_features = ifcvf_get_hw_features(vf);
+
 	ret = vdpa_register_device(&adapter->vdpa);
 	if (ret) {
 		IFCVF_ERR(pdev, "Failed to register ifcvf to vdpa bus");
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-10  9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
                   ` (4 preceding siblings ...)
  2021-03-10  9:00 ` [PATCH V3 5/6] vDPA/ifcvf: fetch device feature bits when probe Zhu Lingshan
@ 2021-03-10  9:00 ` Zhu Lingshan
  2021-03-11  3:20   ` Jason Wang
  5 siblings, 1 reply; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-10  9:00 UTC (permalink / raw)
  To: jasowang, mst, lulu, leonro
  Cc: virtualization, netdev, kvm, linux-kernel, Zhu Lingshan

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);
+	if (ret)
+		return ret;
 
 	vf->req_features = features;
 
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-10  9:00 ` [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA Zhu Lingshan
@ 2021-03-11  3:20   ` Jason Wang
  2021-03-11  4:16     ` Zhu Lingshan
  0 siblings, 1 reply; 23+ messages in thread
From: Jason Wang @ 2021-03-11  3:20 UTC (permalink / raw)
  To: Zhu Lingshan, mst, lulu, leonro; +Cc: virtualization, netdev, kvm, linux-kernel


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-11  3:20   ` Jason Wang
@ 2021-03-11  4:16     ` Zhu Lingshan
  2021-03-11  6:20       ` Jason Wang
  0 siblings, 1 reply; 23+ messages in thread
From: Zhu Lingshan @ 2021-03-11  4:16 UTC (permalink / raw)
  To: Jason Wang, Zhu Lingshan, mst, lulu, leonro
  Cc: netdev, linux-kernel, kvm, virtualization



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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-11  4:16     ` Zhu Lingshan
@ 2021-03-11  6:20       ` Jason Wang
  2021-03-11  7:19         ` Zhu, Lingshan
  0 siblings, 1 reply; 23+ messages in thread
From: Jason Wang @ 2021-03-11  6:20 UTC (permalink / raw)
  To: Zhu Lingshan, Zhu Lingshan, mst, lulu, leonro
  Cc: netdev, linux-kernel, kvm, virtualization


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
>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-11  6:20       ` Jason Wang
@ 2021-03-11  7:19         ` Zhu, Lingshan
  2021-03-12  5:52           ` Jason Wang
  0 siblings, 1 reply; 23+ messages in thread
From: Zhu, Lingshan @ 2021-03-11  7:19 UTC (permalink / raw)
  To: Jason Wang, Zhu Lingshan, mst, lulu, leonro
  Cc: netdev, linux-kernel, kvm, virtualization



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.
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? I prefer check both device and 
IFCVF_SUPPORTED_FEATURES both, more reliable.

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-11  7:19         ` Zhu, Lingshan
@ 2021-03-12  5:52           ` Jason Wang
  2021-03-12  6:40             ` Zhu, Lingshan
  0 siblings, 1 reply; 23+ messages in thread
From: Jason Wang @ 2021-03-12  5:52 UTC (permalink / raw)
  To: Zhu, Lingshan, Zhu Lingshan, mst, lulu, leonro
  Cc: netdev, linux-kernel, kvm, virtualization


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-12  5:52           ` Jason Wang
@ 2021-03-12  6:40             ` Zhu, Lingshan
  2021-03-12  7:00               ` Jason Wang
  0 siblings, 1 reply; 23+ messages in thread
From: Zhu, Lingshan @ 2021-03-12  6:40 UTC (permalink / raw)
  To: Jason Wang, Zhu Lingshan, mst, lulu, leonro
  Cc: netdev, linux-kernel, kvm, virtualization



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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-12  6:40             ` Zhu, Lingshan
@ 2021-03-12  7:00               ` Jason Wang
  2021-03-12  7:08                 ` Zhu, Lingshan
  0 siblings, 1 reply; 23+ messages in thread
From: Jason Wang @ 2021-03-12  7:00 UTC (permalink / raw)
  To: Zhu, Lingshan, Zhu Lingshan, mst, lulu, leonro
  Cc: netdev, linux-kernel, kvm, virtualization


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
  2021-03-12  7:00               ` Jason Wang
@ 2021-03-12  7:08                 ` Zhu, Lingshan
  0 siblings, 0 replies; 23+ messages in thread
From: Zhu, Lingshan @ 2021-03-12  7:08 UTC (permalink / raw)
  To: Jason Wang, Zhu Lingshan, mst, lulu, leonro
  Cc: netdev, linux-kernel, kvm, virtualization



On 3/12/2021 3:00 PM, Jason Wang wrote:
>
> 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
we did not use TSO hardware feature bits in IFCVF driver for now. 
Anyway, we will check the features bits to set in set_features than 
hw/ifcvf driver feature bits.

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2021-03-12  7:09 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10  9:00 [PATCH V3 0/6] vDPA/ifcvf: enables Intel C5000X-PL virtio-net Zhu Lingshan
2021-03-10  9:00 ` [PATCH V3 1/6] vDPA/ifcvf: get_vendor_id returns a device specific vendor id Zhu Lingshan
2021-03-11  3:23   ` Jason Wang
2021-03-11  4:21     ` Zhu Lingshan
2021-03-11  6:13       ` Jason Wang
2021-03-11  7:34         ` Zhu, Lingshan
2021-03-10  9:00 ` [PATCH V3 2/6] vDPA/ifcvf: enable Intel C5000X-PL virtio-net for vDPA Zhu Lingshan
2021-03-10  9:00 ` [PATCH V3 3/6] vDPA/ifcvf: rename original IFCVF dev ids to N3000 ids Zhu Lingshan
2021-03-11  3:25   ` Jason Wang
2021-03-11  4:23     ` Zhu Lingshan
2021-03-11  6:14       ` Jason Wang
2021-03-10  9:00 ` [PATCH V3 4/6] vDPA/ifcvf: remove the version number string Zhu Lingshan
2021-03-10  9:16   ` Leon Romanovsky
2021-03-10  9:00 ` [PATCH V3 5/6] vDPA/ifcvf: fetch device feature bits when probe Zhu Lingshan
2021-03-10  9:00 ` [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA Zhu Lingshan
2021-03-11  3:20   ` Jason Wang
2021-03-11  4:16     ` Zhu Lingshan
2021-03-11  6:20       ` Jason Wang
2021-03-11  7:19         ` Zhu, Lingshan
2021-03-12  5:52           ` Jason Wang
2021-03-12  6:40             ` Zhu, Lingshan
2021-03-12  7:00               ` Jason Wang
2021-03-12  7:08                 ` Zhu, Lingshan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).