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

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

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

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

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

Signed-off-by: Zhu Lingshan <redacted>
---
 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

This commit removes the version number string, using kernel
version is enough.

Signed-off-by: Zhu Lingshan <redacted>
---
 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

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

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
 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

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

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

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;
  

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

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)

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

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,

On 3/11/2021 11:20 AM, Jason Wang wrote:

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
  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！

quoted

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

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

On 3/11/2021 11:23 AM, Jason Wang wrote:

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

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

quoted

    static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)

On 3/11/2021 11:25 AM, Jason Wang wrote:

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

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

quoted

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

On 2021/3/11 12:21 下午, Zhu Lingshan wrote:

On 3/11/2021 11:23 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

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

quoted

quoted

    static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)

On 2021/3/11 12:23 下午, Zhu Lingshan wrote:

On 3/11/2021 11:25 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

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

quoted

quoted

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

On 2021/3/11 12:16 下午, Zhu Lingshan wrote:

On 3/11/2021 11:20 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
  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！

quoted

quoted

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

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

On 3/11/2021 2:20 PM, Jason Wang wrote:

On 2021/3/11 12:16 下午, Zhu Lingshan wrote:

quoted

On 3/11/2021 11:20 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
  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

quoted

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

quoted

Thanks！

quoted

quoted

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

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

On 3/11/2021 2:13 PM, Jason Wang wrote:

On 2021/3/11 12:21 下午, Zhu Lingshan wrote:

quoted

On 3/11/2021 11:23 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

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

quoted

quoted

quoted

    static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)

On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:

On 3/11/2021 2:20 PM, Jason Wang wrote:

quoted

On 2021/3/11 12:16 下午, Zhu Lingshan wrote:

quoted

On 3/11/2021 11:20 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
  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

quoted

quoted

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!

quoted

Thanks

quoted

Thanks！

quoted

quoted

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

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

On 3/12/2021 1:52 PM, Jason Wang wrote:

On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:

quoted

On 3/11/2021 2:20 PM, Jason Wang wrote:

quoted

On 2021/3/11 12:16 下午, Zhu Lingshan wrote:

quoted

On 3/11/2021 11:20 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
  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

quoted

quoted

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.

quoted

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!

quoted

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

quoted

Thanks!

quoted

Thanks

quoted

Thanks！

quoted

quoted

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

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

On 2021/3/12 2:40 下午, Zhu, Lingshan wrote:

On 3/12/2021 1:52 PM, Jason Wang wrote:

quoted

On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:

quoted

On 3/11/2021 2:20 PM, Jason Wang wrote:

quoted

On 2021/3/11 12:16 下午, Zhu Lingshan wrote:

quoted

On 3/11/2021 11:20 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
  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

quoted

quoted

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.

quoted

quoted

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!

quoted

quoted

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

quoted

Thanks!

quoted

Thanks

quoted

Thanks！

quoted

quoted

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

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

On 3/12/2021 3:00 PM, Jason Wang wrote:

On 2021/3/12 2:40 下午, Zhu, Lingshan wrote:

quoted

On 3/12/2021 1:52 PM, Jason Wang wrote:

quoted

On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:

quoted

On 3/11/2021 2:20 PM, Jason Wang wrote:

quoted

On 2021/3/11 12:16 下午, Zhu Lingshan wrote:

quoted

On 3/11/2021 11:20 AM, Jason Wang wrote:

quoted

On 2021/3/10 5:00 下午, Zhu Lingshan wrote:

quoted

vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
examines this when set features.

Signed-off-by: Zhu Lingshan <redacted>
---
  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

quoted

quoted

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!

quoted

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.

quoted

quoted

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!

quoted

quoted

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

quoted

Thanks!

quoted

Thanks

quoted

Thanks！

quoted

quoted

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

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