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 <[email protected]>
---
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
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-net
for vDPA
Signed-off-by: Zhu Lingshan <[email protected]>
---
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
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 <[email protected]>
---
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 removes the version number string, using kernel
version is enough.
Signed-off-by: Zhu Lingshan <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]>
> ---
> 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)
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 <[email protected]>
> ---
> 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,
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 <[email protected]>
>> ---
>> 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
> [email protected]
> 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:
>> 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 <[email protected]>
>> ---
>> 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)
>
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 <[email protected]>
>> ---
>> 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,
>
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 <[email protected]>
>>> ---
>>> 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)
>>
>
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 <[email protected]>
>>> ---
>>> 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,
>>
>
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 <[email protected]>
>>> ---
>>> 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
>> [email protected]
>> 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:
>>
>>
>> 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 <[email protected]>
>>>> ---
>>>> 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
>>> [email protected]
>>> 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:
>>
>>
>> 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 <[email protected]>
>>>> ---
>>>> 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)
>>>
>>
>
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 <[email protected]>
>>>>> ---
>>>>> 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
>>>> [email protected]
>>>> 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:
>>
>>
>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
>>>>> [email protected]
>>>>> 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:
>>
>> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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
>>>>>> [email protected]
>>>>>> 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:
>>
>>
>> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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
>>>>>>> [email protected]
>>>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>>>>>>
>>>>>
>>>>
>>>
>>
>