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
changes from V1:
(1)add comments to explain this driver drives virtio modern devices
and transitional devices in modern mode.(Jason)
(2)remove IFCVF_BLK_SUPPORTED_FEATURES, use hardware feature bits
directly(Jason)
(3)add error handling and message in get_config_size(Stefano)
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 | 9 ++++-
drivers/vdpa/ifcvf/ifcvf_main.c | 58 +++++++++++++++++++++++++--------
2 files changed, 52 insertions(+), 15 deletions(-)
--
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 | 30 ++++++++++++++++++------------
2 files changed, 19 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..469a9b5737b7 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,22 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
pci_set_drvdata(pdev, adapter);
vf = &adapter->vf;
+
+ /* This drirver drives both modern virtio devices and transitional
+ * devices in modern mode.
+ * vDPA requires feature bit VIRTIO_F_ACCESS_PLATFORM,
+ * so legacy devices and transitional devices in legacy
+ * mode will not work for vDPA, this driver will not
+ * drive devices with legacy interface.
+ */
+ 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
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 | 8 +++++++-
drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1c04cd256fa7..0111bfdeb342 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) | \
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 469a9b5737b7..cea1313b1a3f 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);
return features;
}
@@ -517,6 +521,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 | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index cea1313b1a3f..6844c49fe1de 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -347,7 +347,23 @@ 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_adapter *adapter = vdpa_to_adapter(vdpa_dev);
+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
+ struct pci_dev *pdev = adapter->pdev;
+ size_t size;
+
+ if (vf->dev_type == VIRTIO_ID_NET)
+ size = sizeof(struct virtio_net_config);
+
+ else if (vf->dev_type == VIRTIO_ID_BLOCK)
+ size = sizeof(struct virtio_blk_config);
+
+ else {
+ size = 0;
+ IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
+ }
+
+ return size;
}
static void ifcvf_vdpa_get_config(struct vdpa_device *vdpa_dev,
--
2.27.0
On Thu, Apr 15, 2021 at 05:53:35PM +0800, Zhu Lingshan wrote:
>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 | 8 +++++++-
> drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
> 2 files changed, 16 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>index 1c04cd256fa7..0111bfdeb342 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) | \
>diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>index 469a9b5737b7..cea1313b1a3f 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);
>
Should we put a warning here too otherwise feature could be seen
unassigned?
Thanks,
Stefano
> return features;
> }
>@@ -517,6 +521,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
>
On Thu, Apr 15, 2021 at 05:53:36PM +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 | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>index cea1313b1a3f..6844c49fe1de 100644
>--- a/drivers/vdpa/ifcvf/ifcvf_main.c
>+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>@@ -347,7 +347,23 @@ 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_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>+ struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>+ struct pci_dev *pdev = adapter->pdev;
>+ size_t size;
>+
>+ if (vf->dev_type == VIRTIO_ID_NET)
>+ size = sizeof(struct virtio_net_config);
>+
>+ else if (vf->dev_type == VIRTIO_ID_BLOCK)
>+ size = sizeof(struct virtio_blk_config);
>+
>+ else {
>+ size = 0;
>+ IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
>+ }
I slightly prefer the switch, but I don't have a strong opinion.
However, if we want to use if/else, we should follow
`Documentation/process/coding-style.rst` line 166:
Note that the closing brace is empty on a line of its own, **except** in
the cases where it is followed by a continuation of the same statement,
ie a ``while`` in a do-statement or an ``else`` in an if-statement, like
also `scripts/checkpatch.pl --strict` complains:
CHECK: braces {} should be used on all arms of this statement
#209: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:355:
+ if (vf->dev_type == VIRTIO_ID_NET)
[...]
+ else if (vf->dev_type == VIRTIO_ID_BLOCK)
[...]
+ else {
[...]
CHECK: Unbalanced braces around else statement
#215: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:361:
+ else {
Thanks,
Stefano
On 4/15/2021 9:41 PM, Stefano Garzarella wrote:
> On Thu, Apr 15, 2021 at 05:53:35PM +0800, Zhu Lingshan wrote:
>> 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 | 8 +++++++-
>> drivers/vdpa/ifcvf/ifcvf_main.c | 10 +++++++++-
>> 2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h
>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>> index 1c04cd256fa7..0111bfdeb342 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) | \
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 469a9b5737b7..cea1313b1a3f 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);
>>
>
> Should we put a warning here too otherwise feature could be seen
> unassigned?
Thanks, it will be a switch code block too.
>
> Thanks,
> Stefano
>
>> return features;
>> }
>> @@ -517,6 +521,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
>>
>
On 4/15/2021 9:48 PM, Stefano Garzarella wrote:
> On Thu, Apr 15, 2021 at 05:53:36PM +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 | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index cea1313b1a3f..6844c49fe1de 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -347,7 +347,23 @@ 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_adapter *adapter = vdpa_to_adapter(vdpa_dev);
>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>> + struct pci_dev *pdev = adapter->pdev;
>> + size_t size;
>> +
>> + if (vf->dev_type == VIRTIO_ID_NET)
>> + size = sizeof(struct virtio_net_config);
>> +
>> + else if (vf->dev_type == VIRTIO_ID_BLOCK)
>> + size = sizeof(struct virtio_blk_config);
>> +
>> + else {
>> + size = 0;
>> + IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", vf->dev_type);
>> + }
>
> I slightly prefer the switch, but I don't have a strong opinion.
>
> However, if we want to use if/else, we should follow
> `Documentation/process/coding-style.rst` line 166:
> Note that the closing brace is empty on a line of its own,
> **except** in
> the cases where it is followed by a continuation of the same
> statement,
> ie a ``while`` in a do-statement or an ``else`` in an if-statement,
> like
>
> also `scripts/checkpatch.pl --strict` complains:
>
> CHECK: braces {} should be used on all arms of this statement
> #209: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:355:
> + if (vf->dev_type == VIRTIO_ID_NET)
> [...]
> + else if (vf->dev_type == VIRTIO_ID_BLOCK)
> [...]
> + else {
> [...]
>
> CHECK: Unbalanced braces around else statement
> #215: FILE: drivers/vdpa/ifcvf/ifcvf_main.c:361:
> + else {
Thanks Stefano, the reason is we only have one line code after if, so
looks like {} is unnecessary, I agree switch can clear up
code style confusions. I will add this in v3.
Thanks!
>
> Thanks,
> Stefano
>
> _______________________________________________
> Virtualization mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization