This series enabled Intel FGPA SmartNIC C5000X-PL virtio-blk for vDPA.
This series requires:
Stefano's vdpa block patchset: https://lkml.org/lkml/2021/3/15/2113
my patchset to enable Intel FGPA SmartNIC C5000X-PL virtio-net for vDPA:
https://lkml.org/lkml/2021/3/17/432
Thanks!
Zhu Lingshan (3):
vDPA/ifcvf: deduce VIRTIO device ID when probe
vDPA/ifcvf: enable Intel C5000X-PL virtio-block for vDPA
vDPA/ifcvf: get_config_size should return dev specific config size
drivers/vdpa/ifcvf/ifcvf_base.h | 18 +++++++++++++-
drivers/vdpa/ifcvf/ifcvf_main.c | 43 ++++++++++++++++++++++-----------
2 files changed, 46 insertions(+), 15 deletions(-)
--
2.27.0
This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
for vDPA.
Signed-off-by: Zhu Lingshan <[email protected]>
---
drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..8b403522bf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -15,6 +15,7 @@
#include <linux/pci_regs.h>
#include <linux/vdpa.h>
#include <uapi/linux/virtio_net.h>
+#include <uapi/linux/virtio_blk.h>
#include <uapi/linux/virtio_config.h>
#include <uapi/linux/virtio_pci.h>
@@ -28,7 +29,12 @@
#define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
#define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
-#define IFCVF_SUPPORTED_FEATURES \
+#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
+#define C5000X_PL_BLK_DEVICE_ID 0x1001
+#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
+#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
+
+#define IFCVF_NET_SUPPORTED_FEATURES \
((1ULL << VIRTIO_NET_F_MAC) | \
(1ULL << VIRTIO_F_ANY_LAYOUT) | \
(1ULL << VIRTIO_F_VERSION_1) | \
@@ -37,6 +43,15 @@
(1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
(1ULL << VIRTIO_NET_F_MRG_RXBUF))
+#define IFCVF_BLK_SUPPORTED_FEATURES \
+ ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
+ (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
+ (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
+ (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
+ (1ULL << VIRTIO_BLK_F_MQ) | \
+ (1ULL << VIRTIO_F_VERSION_1) | \
+ (1ULL << VIRTIO_F_ACCESS_PLATFORM))
+
/* Only one queue pair for now. */
#define IFCVF_MAX_QUEUE_PAIRS 1
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 99b0a6b4c227..9b6a38b798fa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
u64 features;
- features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
+ if (vf->dev_type == VIRTIO_ID_NET)
+ features = ifcvf_get_features(vf) & IFCVF_NET_SUPPORTED_FEATURES;
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ features = ifcvf_get_features(vf) & IFCVF_BLK_SUPPORTED_FEATURES;
return features;
}
@@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
C5000X_PL_DEVICE_ID,
C5000X_PL_SUBSYS_VENDOR_ID,
C5000X_PL_SUBSYS_DEVICE_ID) },
+ { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
+ C5000X_PL_BLK_DEVICE_ID,
+ C5000X_PL_BLK_SUBSYS_VENDOR_ID,
+ C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
{ 0 },
};
--
2.27.0
get_config_size() should return the size based on the decected
device type.
Signed-off-by: Zhu Lingshan <[email protected]>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 9b6a38b798fa..b48b9789b69e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
{
- return sizeof(struct virtio_net_config);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ return size;
}
static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
--
2.27.0
This commit deduces VIRTIO device ID as device type when probe,
then ifcvf_vdpa_get_device_id() can simply return the ID.
ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
can work properly based on the device ID.
Signed-off-by: Zhu Lingshan <[email protected]>
---
drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index b2eeb16b9c2c..1c04cd256fa7 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -84,6 +84,7 @@ struct ifcvf_hw {
u32 notify_off_multiplier;
u64 req_features;
u64 hw_features;
+ u32 dev_type;
struct virtio_pci_common_cfg __iomem *common_cfg;
void __iomem *net_cfg;
struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 44d7586019da..99b0a6b4c227 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct vdpa_device *vdpa_dev)
static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
{
- struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
- struct pci_dev *pdev = adapter->pdev;
- u32 ret = -ENODEV;
-
- if (pdev->device < 0x1000 || pdev->device > 0x107f)
- return ret;
-
- if (pdev->device < 0x1040)
- ret = pdev->subsystem_device;
- else
- ret = pdev->device - 0x1040;
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
- return ret;
+ return vf->dev_type;
}
static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
@@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = &adapter->vf;
+ if (pdev->device < 0x1000 || pdev->device > 0x107f)
+ return -EOPNOTSUPP;
+
+ if (pdev->device < 0x1040)
+ vf->dev_type = pdev->subsystem_device;
+ else
+ vf->dev_type = pdev->device - 0x1040;
+
vf->base = pcim_iomap_table(pdev);
adapter->pdev = pdev;
--
2.27.0
?? 2021/4/14 ????5:18, Zhu Lingshan д??:
> This commit deduces VIRTIO device ID as device type when probe,
> then ifcvf_vdpa_get_device_id() can simply return the ID.
> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
> can work properly based on the device ID.
>
> Signed-off-by: Zhu Lingshan <[email protected]>
> ---
> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
> 2 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index b2eeb16b9c2c..1c04cd256fa7 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -84,6 +84,7 @@ struct ifcvf_hw {
> u32 notify_off_multiplier;
> u64 req_features;
> u64 hw_features;
> + u32 dev_type;
> struct virtio_pci_common_cfg __iomem *common_cfg;
> void __iomem *net_cfg;
> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 44d7586019da..99b0a6b4c227 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct vdpa_device *vdpa_dev)
>
> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
> {
> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
> - struct pci_dev *pdev = adapter->pdev;
> - u32 ret = -ENODEV;
> -
> - if (pdev->device < 0x1000 || pdev->device > 0x107f)
> - return ret;
> -
> - if (pdev->device < 0x1040)
> - ret = pdev->subsystem_device;
> - else
> - ret = pdev->device - 0x1040;
> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>
> - return ret;
> + return vf->dev_type;
> }
>
> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> pci_set_drvdata(pdev, adapter);
>
> vf = &adapter->vf;
> + if (pdev->device < 0x1000 || pdev->device > 0x107f)
> + return -EOPNOTSUPP;
> +
> + if (pdev->device < 0x1040)
> + vf->dev_type = pdev->subsystem_device;
> + else
> + vf->dev_type = pdev->device - 0x1040;
So a question here, is the device a transtional device or modern one?
If it's a transitonal one, can it swtich endianess automatically or not?
Thanks
> +
> vf->base = pcim_iomap_table(pdev);
>
> adapter->pdev = pdev;
?? 2021/4/14 ????5:18, Zhu Lingshan д??:
> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
> for vDPA.
>
> Signed-off-by: Zhu Lingshan <[email protected]>
> ---
> drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
> drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index 1c04cd256fa7..8b403522bf06 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -15,6 +15,7 @@
> #include <linux/pci_regs.h>
> #include <linux/vdpa.h>
> #include <uapi/linux/virtio_net.h>
> +#include <uapi/linux/virtio_blk.h>
> #include <uapi/linux/virtio_config.h>
> #include <uapi/linux/virtio_pci.h>
>
> @@ -28,7 +29,12 @@
> #define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
> #define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
>
> -#define IFCVF_SUPPORTED_FEATURES \
> +#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
> +#define C5000X_PL_BLK_DEVICE_ID 0x1001
> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
> +
> +#define IFCVF_NET_SUPPORTED_FEATURES \
> ((1ULL << VIRTIO_NET_F_MAC) | \
> (1ULL << VIRTIO_F_ANY_LAYOUT) | \
> (1ULL << VIRTIO_F_VERSION_1) | \
> @@ -37,6 +43,15 @@
> (1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
> (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>
> +#define IFCVF_BLK_SUPPORTED_FEATURES \
> + ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
> + (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
> + (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
> + (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
> + (1ULL << VIRTIO_BLK_F_MQ) | \
> + (1ULL << VIRTIO_F_VERSION_1) | \
> + (1ULL << VIRTIO_F_ACCESS_PLATFORM))
I think we've discussed this sometime in the past but what's the reason
for such whitelist consider there's already a get_features() implemention?
E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED?
Thanks
> +
> /* Only one queue pair for now. */
> #define IFCVF_MAX_QUEUE_PAIRS 1
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 99b0a6b4c227..9b6a38b798fa 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct vdpa_device *vdpa_dev)
> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> u64 features;
>
> - features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
> + if (vf->dev_type == VIRTIO_ID_NET)
> + features = ifcvf_get_features(vf) & IFCVF_NET_SUPPORTED_FEATURES;
> +
> + if (vf->dev_type == VIRTIO_ID_BLOCK)
> + features = ifcvf_get_features(vf) & IFCVF_BLK_SUPPORTED_FEATURES;
>
> return features;
> }
> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
> C5000X_PL_DEVICE_ID,
> C5000X_PL_SUBSYS_VENDOR_ID,
> C5000X_PL_SUBSYS_DEVICE_ID) },
> + { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
> + C5000X_PL_BLK_DEVICE_ID,
> + C5000X_PL_BLK_SUBSYS_VENDOR_ID,
> + C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>
> { 0 },
> };
?? 2021/4/14 ????5:18, Zhu Lingshan д??:
> get_config_size() should return the size based on the decected
> device type.
>
> Signed-off-by: Zhu Lingshan <[email protected]>
Acked-by: Jason Wang <[email protected]>
> ---
> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 9b6a38b798fa..b48b9789b69e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
>
> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
> {
> - return sizeof(struct virtio_net_config);
> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
> + size_t size;
> +
> + if (vf->dev_type == VIRTIO_ID_NET)
> + size = sizeof(struct virtio_net_config);
> +
> + if (vf->dev_type == VIRTIO_ID_BLOCK)
> + size = sizeof(struct virtio_blk_config);
> +
> + return size;
> }
>
> static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
On 4/15/2021 11:30 AM, Jason Wang wrote:
>
> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>> This commit deduces VIRTIO device ID as device type when probe,
>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>> can work properly based on the device ID.
>>
>> Signed-off-by: Zhu Lingshan <[email protected]>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>> 2 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index b2eeb16b9c2c..1c04cd256fa7 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>> u32 notify_off_multiplier;
>> u64 req_features;
>> u64 hw_features;
>> + u32 dev_type;
>> struct virtio_pci_common_cfg __iomem *common_cfg;
>> void __iomem *net_cfg;
>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 44d7586019da..99b0a6b4c227 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
>> vdpa_device *vdpa_dev)
>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>> {
>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>> - struct pci_dev *pdev = adapter->pdev;
>> - u32 ret = -ENODEV;
>> -
>> - if (pdev->device < 0x1000 || pdev->device > 0x107f)
>> - return ret;
>> -
>> - if (pdev->device < 0x1040)
>> - ret = pdev->subsystem_device;
>> - else
>> - ret = pdev->device - 0x1040;
>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> - return ret;
>> + return vf->dev_type;
>> }
>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
>> const struct pci_device_id *id)
>> pci_set_drvdata(pdev, adapter);
>> vf = &adapter->vf;
>> + if (pdev->device < 0x1000 || pdev->device > 0x107f)
>> + return -EOPNOTSUPP;
>> +
>> + if (pdev->device < 0x1040)
>> + vf->dev_type = pdev->subsystem_device;
>> + else
>> + vf->dev_type = pdev->device - 0x1040;
>
>
> So a question here, is the device a transtional device or modern one?
>
> If it's a transitonal one, can it swtich endianess automatically or not?
>
> Thanks
Hi Jason,
This driver should drive both modern and transitional devices as we
discussed before.
If it's a transitional one, it will act as a modern device by default,
legacy mode is a fail-over path.
For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must in
modern mode.
I think we don't need to worry about endianess for legacy mode.
Thanks
Zhu Lingshan
>
>
>> +
>> vf->base = pcim_iomap_table(pdev);
>> adapter->pdev = pdev;
>
On 4/15/2021 11:34 AM, Jason Wang wrote:
>
> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
>> for vDPA.
>>
>> Signed-off-by: Zhu Lingshan <[email protected]>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
>> drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>> 2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index 1c04cd256fa7..8b403522bf06 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>> @@ -15,6 +15,7 @@
>> #include <linux/pci_regs.h>
>> #include <linux/vdpa.h>
>> #include <uapi/linux/virtio_net.h>
>> +#include <uapi/linux/virtio_blk.h>
>> #include <uapi/linux/virtio_config.h>
>> #include <uapi/linux/virtio_pci.h>
>> @@ -28,7 +29,12 @@
>> #define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
>> #define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
>> -#define IFCVF_SUPPORTED_FEATURES \
>> +#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
>> +#define C5000X_PL_BLK_DEVICE_ID 0x1001
>> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
>> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
>> +
>> +#define IFCVF_NET_SUPPORTED_FEATURES \
>> ((1ULL << VIRTIO_NET_F_MAC) | \
>> (1ULL << VIRTIO_F_ANY_LAYOUT) | \
>> (1ULL << VIRTIO_F_VERSION_1) | \
>> @@ -37,6 +43,15 @@
>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
>> (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>> +#define IFCVF_BLK_SUPPORTED_FEATURES \
>> + ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
>> + (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
>> + (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
>> + (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
>> + (1ULL << VIRTIO_BLK_F_MQ) | \
>> + (1ULL << VIRTIO_F_VERSION_1) | \
>> + (1ULL << VIRTIO_F_ACCESS_PLATFORM))
>
>
> I think we've discussed this sometime in the past but what's the
> reason for such whitelist consider there's already a get_features()
> implemention?
>
> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or VIRTIO_F_RING_PACKED?
>
> Thanks
The reason is some feature bits are supported in the device but not
supported by the driver, e.g, for virtio-net, mq & cq implementation is
not ready in the driver.
Thanks!
>
>
>> +
>> /* Only one queue pair for now. */
>> #define IFCVF_MAX_QUEUE_PAIRS 1
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 99b0a6b4c227..9b6a38b798fa 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
>> vdpa_device *vdpa_dev)
>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> u64 features;
>> - features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
>> + if (vf->dev_type == VIRTIO_ID_NET)
>> + features = ifcvf_get_features(vf) &
>> IFCVF_NET_SUPPORTED_FEATURES;
>> +
>> + if (vf->dev_type == VIRTIO_ID_BLOCK)
>> + features = ifcvf_get_features(vf) &
>> IFCVF_BLK_SUPPORTED_FEATURES;
>> return features;
>> }
>> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>> C5000X_PL_DEVICE_ID,
>> C5000X_PL_SUBSYS_VENDOR_ID,
>> C5000X_PL_SUBSYS_DEVICE_ID) },
>> + { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
>> + C5000X_PL_BLK_DEVICE_ID,
>> + C5000X_PL_BLK_SUBSYS_VENDOR_ID,
>> + C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>> { 0 },
>> };
>
在 2021/4/15 下午1:55, Zhu Lingshan 写道:
>
>
> On 4/15/2021 11:34 AM, Jason Wang wrote:
>>
>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
>>> for vDPA.
>>>
>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>> ---
>>> drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
>>> drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>>> 2 files changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> index 1c04cd256fa7..8b403522bf06 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> @@ -15,6 +15,7 @@
>>> #include <linux/pci_regs.h>
>>> #include <linux/vdpa.h>
>>> #include <uapi/linux/virtio_net.h>
>>> +#include <uapi/linux/virtio_blk.h>
>>> #include <uapi/linux/virtio_config.h>
>>> #include <uapi/linux/virtio_pci.h>
>>> @@ -28,7 +29,12 @@
>>> #define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
>>> #define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
>>> -#define IFCVF_SUPPORTED_FEATURES \
>>> +#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
>>> +#define C5000X_PL_BLK_DEVICE_ID 0x1001
>>> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
>>> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
>>> +
>>> +#define IFCVF_NET_SUPPORTED_FEATURES \
>>> ((1ULL << VIRTIO_NET_F_MAC) | \
>>> (1ULL << VIRTIO_F_ANY_LAYOUT) | \
>>> (1ULL << VIRTIO_F_VERSION_1) | \
>>> @@ -37,6 +43,15 @@
>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>>> +#define IFCVF_BLK_SUPPORTED_FEATURES \
>>> + ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
>>> + (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
>>> + (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
>>> + (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
>>> + (1ULL << VIRTIO_BLK_F_MQ) | \
>>> + (1ULL << VIRTIO_F_VERSION_1) | \
>>> + (1ULL << VIRTIO_F_ACCESS_PLATFORM))
>>
>>
>> I think we've discussed this sometime in the past but what's the
>> reason for such whitelist consider there's already a get_features()
>> implemention?
>>
>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
>> VIRTIO_F_RING_PACKED?
>>
>> Thanks
> The reason is some feature bits are supported in the device but not
> supported by the driver, e.g, for virtio-net, mq & cq implementation
> is not ready in the driver.
I understand the case of virtio-net but I wonder why we need this for
block where we don't vq cvq.
Thanks
>
> Thanks!
>
>>
>>
>>> +
>>> /* Only one queue pair for now. */
>>> #define IFCVF_MAX_QUEUE_PAIRS 1
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index 99b0a6b4c227..9b6a38b798fa 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
>>> vdpa_device *vdpa_dev)
>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> u64 features;
>>> - features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
>>> + if (vf->dev_type == VIRTIO_ID_NET)
>>> + features = ifcvf_get_features(vf) &
>>> IFCVF_NET_SUPPORTED_FEATURES;
>>> +
>>> + if (vf->dev_type == VIRTIO_ID_BLOCK)
>>> + features = ifcvf_get_features(vf) &
>>> IFCVF_BLK_SUPPORTED_FEATURES;
>>> return features;
>>> }
>>> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>>> C5000X_PL_DEVICE_ID,
>>> C5000X_PL_SUBSYS_VENDOR_ID,
>>> C5000X_PL_SUBSYS_DEVICE_ID) },
>>> + { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
>>> + C5000X_PL_BLK_DEVICE_ID,
>>> + C5000X_PL_BLK_SUBSYS_VENDOR_ID,
>>> + C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>>> { 0 },
>>> };
>>
>
在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>
>
> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>
>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>> This commit deduces VIRTIO device ID as device type when probe,
>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>> can work properly based on the device ID.
>>>
>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>> ---
>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>> 2 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>> u32 notify_off_multiplier;
>>> u64 req_features;
>>> u64 hw_features;
>>> + u32 dev_type;
>>> struct virtio_pci_common_cfg __iomem *common_cfg;
>>> void __iomem *net_cfg;
>>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> index 44d7586019da..99b0a6b4c227 100644
>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
>>> vdpa_device *vdpa_dev)
>>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>>> {
>>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>> - struct pci_dev *pdev = adapter->pdev;
>>> - u32 ret = -ENODEV;
>>> -
>>> - if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>> - return ret;
>>> -
>>> - if (pdev->device < 0x1040)
>>> - ret = pdev->subsystem_device;
>>> - else
>>> - ret = pdev->device -0x1040;
>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>> - return ret;
>>> + return vf->dev_type;
>>> }
>>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>> const struct pci_device_id *id)
>>> pci_set_drvdata(pdev, adapter);
>>> vf = &adapter->vf;
>>> + if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>> + return -EOPNOTSUPP;
>>> +
>>> + if (pdev->device < 0x1040)
>>> + vf->dev_type = pdev->subsystem_device;
>>> + else
>>> + vf->dev_type = pdev->device - 0x1040;
>>
>>
>> So a question here, is the device a transtional device or modern one?
>>
>> If it's a transitonal one, can it swtich endianess automatically or not?
>>
>> Thanks
> Hi Jason,
>
> This driver should drive both modern and transitional devices as we
> discussed before.
> If it's a transitional one, it will act as a modern device by default,
> legacy mode is a fail-over path.
Note that legacy driver use native endian, support legacy driver
requires the device to know native endian which I'm not sure your device
can do that.
Thanks
> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
> in modern mode.
> I think we don't need to worry about endianess for legacy mode.
>
> Thanks
> Zhu Lingshan
>>
>>
>>> +
>>> vf->base = pcim_iomap_table(pdev);
>>> adapter->pdev = pdev;
>>
>
On 4/15/2021 2:30 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>>
>>
>> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>>
>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>> This commit deduces VIRTIO device ID as device type when probe,
>>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>>> can work properly based on the device ID.
>>>>
>>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>>> ---
>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>>> 2 files changed, 11 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>>> u32 notify_off_multiplier;
>>>> u64 req_features;
>>>> u64 hw_features;
>>>> + u32 dev_type;
>>>> struct virtio_pci_common_cfg __iomem *common_cfg;
>>>> void __iomem *net_cfg;
>>>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index 44d7586019da..99b0a6b4c227 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
>>>> vdpa_device *vdpa_dev)
>>>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>>>> {
>>>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>>> - struct pci_dev *pdev = adapter->pdev;
>>>> - u32 ret = -ENODEV;
>>>> -
>>>> - if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>> - return ret;
>>>> -
>>>> - if (pdev->device < 0x1040)
>>>> - ret = pdev->subsystem_device;
>>>> - else
>>>> - ret = pdev->device -0x1040;
>>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>> - return ret;
>>>> + return vf->dev_type;
>>>> }
>>>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>>> const struct pci_device_id *id)
>>>> pci_set_drvdata(pdev, adapter);
>>>> vf = &adapter->vf;
>>>> + if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>> + return -EOPNOTSUPP;
>>>> +
>>>> + if (pdev->device < 0x1040)
>>>> + vf->dev_type = pdev->subsystem_device;
>>>> + else
>>>> + vf->dev_type = pdev->device - 0x1040;
>>>
>>>
>>> So a question here, is the device a transtional device or modern one?
>>>
>>> If it's a transitonal one, can it swtich endianess automatically or
>>> not?
>>>
>>> Thanks
>> Hi Jason,
>>
>> This driver should drive both modern and transitional devices as we
>> discussed before.
>> If it's a transitional one, it will act as a modern device by
>> default, legacy mode is a fail-over path.
>
>
> Note that legacy driver use native endian, support legacy driver
> requires the device to know native endian which I'm not sure your
> device can do that.
>
> Thanks
Yes, legacy requires guest native endianess, I think we don't need to
worry about this because our transitional device should work in modern
mode by
default(legacy mode is the failover path we will never reach,
get_features will fail if no ACCESS_PLATFORM), we don't support legacy
device in vDPA.
Thanks
>
>
>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
>> in modern mode.
>> I think we don't need to worry about endianess for legacy mode.
>>
>> Thanks
>> Zhu Lingshan
>>>
>>>
>>>> +
>>>> vf->base = pcim_iomap_table(pdev);
>>>> adapter->pdev = pdev;
>>>
>>
>
On 4/15/2021 2:31 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午1:55, Zhu Lingshan 写道:
>>
>>
>> On 4/15/2021 11:34 AM, Jason Wang wrote:
>>>
>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>> This commit enabled Intel FPGA SmartNIC C5000X-PL virtio-block
>>>> for vDPA.
>>>>
>>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>>> ---
>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 17 ++++++++++++++++-
>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>>>> 2 files changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> index 1c04cd256fa7..8b403522bf06 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>> @@ -15,6 +15,7 @@
>>>> #include <linux/pci_regs.h>
>>>> #include <linux/vdpa.h>
>>>> #include <uapi/linux/virtio_net.h>
>>>> +#include <uapi/linux/virtio_blk.h>
>>>> #include <uapi/linux/virtio_config.h>
>>>> #include <uapi/linux/virtio_pci.h>
>>>> @@ -28,7 +29,12 @@
>>>> #define C5000X_PL_SUBSYS_VENDOR_ID 0x8086
>>>> #define C5000X_PL_SUBSYS_DEVICE_ID 0x0001
>>>> -#define IFCVF_SUPPORTED_FEATURES \
>>>> +#define C5000X_PL_BLK_VENDOR_ID 0x1AF4
>>>> +#define C5000X_PL_BLK_DEVICE_ID 0x1001
>>>> +#define C5000X_PL_BLK_SUBSYS_VENDOR_ID 0x8086
>>>> +#define C5000X_PL_BLK_SUBSYS_DEVICE_ID 0x0002
>>>> +
>>>> +#define IFCVF_NET_SUPPORTED_FEATURES \
>>>> ((1ULL << VIRTIO_NET_F_MAC) | \
>>>> (1ULL << VIRTIO_F_ANY_LAYOUT) | \
>>>> (1ULL << VIRTIO_F_VERSION_1) | \
>>>> @@ -37,6 +43,15 @@
>>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) | \
>>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF))
>>>> +#define IFCVF_BLK_SUPPORTED_FEATURES \
>>>> + ((1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
>>>> + (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
>>>> + (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
>>>> + (1ULL << VIRTIO_BLK_F_TOPOLOGY) | \
>>>> + (1ULL << VIRTIO_BLK_F_MQ) | \
>>>> + (1ULL << VIRTIO_F_VERSION_1) | \
>>>> + (1ULL << VIRTIO_F_ACCESS_PLATFORM))
>>>
>>>
>>> I think we've discussed this sometime in the past but what's the
>>> reason for such whitelist consider there's already a get_features()
>>> implemention?
>>>
>>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
>>> VIRTIO_F_RING_PACKED?
>>>
>>> Thanks
>> The reason is some feature bits are supported in the device but not
>> supported by the driver, e.g, for virtio-net, mq & cq implementation
>> is not ready in the driver.
>
>
> I understand the case of virtio-net but I wonder why we need this for
> block where we don't vq cvq.
>
> Thanks
This is still a subset of the feature bits read from hardware, I leave
it here to code consistently, and indicate what we support clearly.
Are you suggesting remove this feature bits list and just use what we
read from hardware?
Thansk
>
>
>>
>> Thanks!
>>
>>>
>>>
>>>> +
>>>> /* Only one queue pair for now. */
>>>> #define IFCVF_MAX_QUEUE_PAIRS 1
>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> index 99b0a6b4c227..9b6a38b798fa 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -171,7 +171,11 @@ static u64 ifcvf_vdpa_get_features(struct
>>>> vdpa_device *vdpa_dev)
>>>> struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>> u64 features;
>>>> - features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
>>>> + if (vf->dev_type == VIRTIO_ID_NET)
>>>> + features = ifcvf_get_features(vf) &
>>>> IFCVF_NET_SUPPORTED_FEATURES;
>>>> +
>>>> + if (vf->dev_type == VIRTIO_ID_BLOCK)
>>>> + features = ifcvf_get_features(vf) &
>>>> IFCVF_BLK_SUPPORTED_FEATURES;
>>>> return features;
>>>> }
>>>> @@ -509,6 +513,10 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>>>> C5000X_PL_DEVICE_ID,
>>>> C5000X_PL_SUBSYS_VENDOR_ID,
>>>> C5000X_PL_SUBSYS_DEVICE_ID) },
>>>> + { PCI_DEVICE_SUB(C5000X_PL_BLK_VENDOR_ID,
>>>> + C5000X_PL_BLK_DEVICE_ID,
>>>> + C5000X_PL_BLK_SUBSYS_VENDOR_ID,
>>>> + C5000X_PL_BLK_SUBSYS_DEVICE_ID) },
>>>> { 0 },
>>>> };
>>>
>>
>
在 2021/4/15 下午2:36, Zhu Lingshan 写道:
>
>
> On 4/15/2021 2:30 PM, Jason Wang wrote:
>>
>> 在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>>>
>>>
>>> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>>>
>>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>>> This commit deduces VIRTIO device ID as device type when probe,
>>>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>>>> can work properly based on the device ID.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>>>> ---
>>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>>>> 2 files changed, 11 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>>>> u32 notify_off_multiplier;
>>>>> u64 req_features;
>>>>> u64 hw_features;
>>>>> + u32 dev_type;
>>>>> struct virtio_pci_common_cfg __iomem *common_cfg;
>>>>> void __iomem *net_cfg;
>>>>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> index 44d7586019da..99b0a6b4c227 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
>>>>> vdpa_device *vdpa_dev)
>>>>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device *vdpa_dev)
>>>>> {
>>>>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>>>> - struct pci_dev *pdev = adapter->pdev;
>>>>> - u32 ret = -ENODEV;
>>>>> -
>>>>> - if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>> - return ret;
>>>>> -
>>>>> - if (pdev->device < 0x1040)
>>>>> - ret = pdev->subsystem_device;
>>>>> - else
>>>>> - ret = pdev->device-0x1040;
>>>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> - return ret;
>>>>> + return vf->dev_type;
>>>>> }
>>>>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device *vdpa_dev)
>>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>>>> const struct pci_device_id *id)
>>>>> pci_set_drvdata(pdev, adapter);
>>>>> vf = &adapter->vf;
>>>>> + if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>> + return -EOPNOTSUPP;
>>>>> +
>>>>> + if (pdev->device < 0x1040)
>>>>> + vf->dev_type = pdev->subsystem_device;
>>>>> + else
>>>>> + vf->dev_type = pdev->device - 0x1040;
>>>>
>>>>
>>>> So a question here, is the device a transtional device or modern one?
>>>>
>>>> If it's a transitonal one, can it swtich endianess automatically or
>>>> not?
>>>>
>>>> Thanks
>>> Hi Jason,
>>>
>>> This driver should drive both modern and transitional devices as we
>>> discussed before.
>>> If it's a transitional one, it will act as a modern device by
>>> default, legacy mode is a fail-over path.
>>
>>
>> Note that legacy driver use native endian, support legacy driver
>> requires the device to know native endian which I'm not sure your
>> device can do that.
>>
>> Thanks
> Yes, legacy requires guest native endianess, I think we don't need to
> worry about this because our transitional device should work in modern
> mode by
> default(legacy mode is the failover path we will never reach,
> get_features will fail if no ACCESS_PLATFORM), we don't support legacy
> device in vDPA.
>
> Thanks
Ok, so I think it's better to add a comment here.
Thanks
>>
>>
>>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it must
>>> in modern mode.
>>> I think we don't need to worry about endianess for legacy mode.
>>>
>>> Thanks
>>> Zhu Lingshan
>>>>
>>>>
>>>>> +
>>>>> vf->base = pcim_iomap_table(pdev);
>>>>> adapter->pdev = pdev;
>>>>
>>>
>>
>
在 2021/4/15 下午2:41, Zhu Lingshan 写道:
>>>>
>>>> I think we've discussed this sometime in the past but what's the
>>>> reason for such whitelist consider there's already a get_features()
>>>> implemention?
>>>>
>>>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
>>>> VIRTIO_F_RING_PACKED?
>>>>
>>>> Thanks
>>> The reason is some feature bits are supported in the device but not
>>> supported by the driver, e.g, for virtio-net, mq & cq implementation
>>> is not ready in the driver.
>>
>>
>> I understand the case of virtio-net but I wonder why we need this for
>> block where we don't vq cvq.
>>
>> Thanks
> This is still a subset of the feature bits read from hardware, I leave
> it here to code consistently, and indicate what we support clearly.
> Are you suggesting remove this feature bits list and just use what we
> read from hardware?
>
> Thansk
Yes, please do that.
The whiltelist doesn't help in this case I think.
Thanks
On 4/15/2021 3:17 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午2:41, Zhu Lingshan 写道:
>>>>>
>>>>> I think we've discussed this sometime in the past but what's the
>>>>> reason for such whitelist consider there's already a
>>>>> get_features() implemention?
>>>>>
>>>>> E.g Any reason to block VIRTIO_BLK_F_WRITE_ZEROS or
>>>>> VIRTIO_F_RING_PACKED?
>>>>>
>>>>> Thanks
>>>> The reason is some feature bits are supported in the device but not
>>>> supported by the driver, e.g, for virtio-net, mq & cq
>>>> implementation is not ready in the driver.
>>>
>>>
>>> I understand the case of virtio-net but I wonder why we need this
>>> for block where we don't vq cvq.
>>>
>>> Thanks
>> This is still a subset of the feature bits read from hardware, I
>> leave it here to code consistently, and indicate what we support
>> clearly.
>> Are you suggesting remove this feature bits list and just use what we
>> read from hardware?
>>
>> Thansk
>
>
> Yes, please do that.
>
> The whiltelist doesn't help in this case I think.
OK, will remove this in V2
Thanks
>
> Thanks
On 4/15/2021 3:16 PM, Jason Wang wrote:
>
> 在 2021/4/15 下午2:36, Zhu Lingshan 写道:
>>
>>
>> On 4/15/2021 2:30 PM, Jason Wang wrote:
>>>
>>> 在 2021/4/15 下午1:52, Zhu Lingshan 写道:
>>>>
>>>>
>>>> On 4/15/2021 11:30 AM, Jason Wang wrote:
>>>>>
>>>>> 在 2021/4/14 下午5:18, Zhu Lingshan 写道:
>>>>>> This commit deduces VIRTIO device ID as device type when probe,
>>>>>> then ifcvf_vdpa_get_device_id() can simply return the ID.
>>>>>> ifcvf_vdpa_get_features() and ifcvf_vdpa_get_config_size()
>>>>>> can work properly based on the device ID.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>>>>> ---
>>>>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 22 ++++++++++------------
>>>>>> 2 files changed, 11 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> index b2eeb16b9c2c..1c04cd256fa7 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>>> @@ -84,6 +84,7 @@ struct ifcvf_hw {
>>>>>> u32 notify_off_multiplier;
>>>>>> u64 req_features;
>>>>>> u64 hw_features;
>>>>>> + u32 dev_type;
>>>>>> struct virtio_pci_common_cfg __iomem *common_cfg;
>>>>>> void __iomem *net_cfg;
>>>>>> struct vring_info vring[IFCVF_MAX_QUEUE_PAIRS * 2];
>>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> index 44d7586019da..99b0a6b4c227 100644
>>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>>> @@ -323,19 +323,9 @@ static u32 ifcvf_vdpa_get_generation(struct
>>>>>> vdpa_device *vdpa_dev)
>>>>>> static u32 ifcvf_vdpa_get_device_id(struct vdpa_device
>>>>>> *vdpa_dev)
>>>>>> {
>>>>>> - struct ifcvf_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>>>>>> - struct pci_dev *pdev = adapter->pdev;
>>>>>> - u32 ret = -ENODEV;
>>>>>> -
>>>>>> - if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>>> - return ret;
>>>>>> -
>>>>>> - if (pdev->device < 0x1040)
>>>>>> - ret = pdev->subsystem_device;
>>>>>> - else
>>>>>> - ret = pdev->device-0x1040;
>>>>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>>> - return ret;
>>>>>> + return vf->dev_type;
>>>>>> }
>>>>>> static u32 ifcvf_vdpa_get_vendor_id(struct vdpa_device
>>>>>> *vdpa_dev)
>>>>>> @@ -466,6 +456,14 @@ static int ifcvf_probe(struct pci_dev *pdev,
>>>>>> const struct pci_device_id *id)
>>>>>> pci_set_drvdata(pdev, adapter);
>>>>>> vf = &adapter->vf;
>>>>>> + if (pdev->device < 0x1000 || pdev->device > 0x107f)
>>>>>> + return -EOPNOTSUPP;
>>>>>> +
>>>>>> + if (pdev->device < 0x1040)
>>>>>> + vf->dev_type = pdev->subsystem_device;
>>>>>> + else
>>>>>> + vf->dev_type = pdev->device - 0x1040;
>>>>>
>>>>>
>>>>> So a question here, is the device a transtional device or modern one?
>>>>>
>>>>> If it's a transitonal one, can it swtich endianess automatically
>>>>> or not?
>>>>>
>>>>> Thanks
>>>> Hi Jason,
>>>>
>>>> This driver should drive both modern and transitional devices as we
>>>> discussed before.
>>>> If it's a transitional one, it will act as a modern device by
>>>> default, legacy mode is a fail-over path.
>>>
>>>
>>> Note that legacy driver use native endian, support legacy driver
>>> requires the device to know native endian which I'm not sure your
>>> device can do that.
>>>
>>> Thanks
>> Yes, legacy requires guest native endianess, I think we don't need to
>> worry about this because our transitional device should work in
>> modern mode by
>> default(legacy mode is the failover path we will never reach,
>> get_features will fail if no ACCESS_PLATFORM), we don't support
>> legacy device in vDPA.
>>
>> Thanks
>
>
> Ok, so I think it's better to add a comment here.
sure, will add a comment in V2
Thanks
>
> Thanks
>
>
>>>
>>>
>>>> For vDPA, it has to support VIRTIO_1 and ACCESS_PLATFORM, so it
>>>> must in modern mode.
>>>> I think we don't need to worry about endianess for legacy mode.
>>>>
>>>> Thanks
>>>> Zhu Lingshan
>>>>>
>>>>>
>>>>>> +
>>>>>> vf->base = pcim_iomap_table(pdev);
>>>>>> adapter->pdev = pdev;
>>>>>
>>>>
>>>
>>
>
On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
>get_config_size() should return the size based on the decected
>device type.
>
>Signed-off-by: Zhu Lingshan <[email protected]>
>---
> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>index 9b6a38b798fa..b48b9789b69e 100644
>--- a/drivers/vdpa/ifcvf/ifcvf_main.c
>+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>@@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct vdpa_device *vdpa_dev)
>
> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
> {
>- return sizeof(struct virtio_net_config);
>+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>+ size_t size;
>+
>+ if (vf->dev_type == VIRTIO_ID_NET)
>+ size = sizeof(struct virtio_net_config);
>+
>+ if (vf->dev_type == VIRTIO_ID_BLOCK)
>+ size = sizeof(struct virtio_blk_config);
>+
>+ return size;
I'm not familiar with the ifcvf details, but can it happen that the
device is not block or net?
Should we set `size` to 0 by default to handle this case or are we sure
it's one of the two?
Maybe we should add a comment or a warning message in this case, to
prevent some analysis tool or compiler from worrying that `size` might
be uninitialized.
I was thinking something like this:
switch(vf->dev_type) {
case VIRTIO_ID_NET:
size = sizeof(struct virtio_net_config);
break;
case VIRTIO_ID_BLOCK:
size = sizeof(struct virtio_blk_config);
break;
default:
/* or WARN(1, "") if dev_warn() not apply */
dev_warn(... , "virtio ID [0x%x] not supported\n")
size = 0;
}
Thanks,
Stefano
在 2021/4/15 下午4:12, Stefano Garzarella 写道:
> On Wed, Apr 14, 2021 at 05:18:32PM +0800, Zhu Lingshan wrote:
>> get_config_size() should return the size based on the decected
>> device type.
>>
>> Signed-off-by: Zhu Lingshan <[email protected]>
>> ---
>> drivers/vdpa/ifcvf/ifcvf_main.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 9b6a38b798fa..b48b9789b69e 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -347,7 +347,16 @@ static u32 ifcvf_vdpa_get_vq_align(struct
>> vdpa_device *vdpa_dev)
>>
>> static size_t ifcvf_vdpa_get_config_size(struct vdpa_device *vdpa_dev)
>> {
>> - return sizeof(struct virtio_net_config);
>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> + size_t size;
>> +
>> + if (vf->dev_type == VIRTIO_ID_NET)
>> + size = sizeof(struct virtio_net_config);
>> +
>> + if (vf->dev_type == VIRTIO_ID_BLOCK)
>> + size = sizeof(struct virtio_blk_config);
>> +
>> + return size;
>
> I'm not familiar with the ifcvf details, but can it happen that the
> device is not block or net?
>
> Should we set `size` to 0 by default to handle this case or are we
> sure it's one of the two?
>
> Maybe we should add a comment or a warning message in this case, to
> prevent some analysis tool or compiler from worrying that `size` might
> be uninitialized.
>
> I was thinking something like this:
>
> switch(vf->dev_type) {
> case VIRTIO_ID_NET:
> size = sizeof(struct virtio_net_config);
> break;
> case VIRTIO_ID_BLOCK:
> size = sizeof(struct virtio_blk_config);
> break;
> default:
> /* or WARN(1, "") if dev_warn() not apply */
> dev_warn(... , "virtio ID [0x%x] not supported\n")
> size = 0;
>
> }
>
Yes, I agree.
Thanks
> Thanks,
> Stefano
>