* [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
@ 2024-05-13 21:27 Zhu Lingshan
2024-05-13 13:36 ` Parav Pandit
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Zhu Lingshan @ 2024-05-13 21:27 UTC (permalink / raw)
To: jasowang, mst; +Cc: virtualization, Zhu Lingshan
Most of ifcvf code work for standard virtio pci,
except for bar4. This commit makes bar4 only effective
for Intel products, thereby turning ifcvf into
a standard virtio pci driver.
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
drivers/vdpa/Kconfig | 2 +-
drivers/vdpa/ifcvf/ifcvf_base.c | 17 ++++++++++++-----
drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
index 656c1cb541de..dbf09f35bcef 100644
--- a/drivers/vdpa/Kconfig
+++ b/drivers/vdpa/Kconfig
@@ -44,7 +44,7 @@ config VDPA_USER
in a userspace program.
config IFCVF
- tristate "Intel IFC VF vDPA driver"
+ tristate "Intel IFC VF and standard virtio-pci vDPA driver"
depends on PCI_MSI
help
This kernel module can drive Intel IFC VF NIC to offload
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 472daa588a9d..47bc9b11c678 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -178,7 +178,8 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
hw->vring[i].irq = -EINVAL;
}
- hw->lm_cfg = hw->base[IFCVF_LM_BAR];
+ if (hw->pdev->subsystem_vendor == PCI_VENDOR_ID_INTEL)
+ hw->lm_cfg = hw->base[IFCVF_LM_BAR];
IFCVF_DBG(pdev,
"PCI capability mapping: common cfg: %p, notify base: %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n",
@@ -330,18 +331,24 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
u16 last_avail_idx;
- last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
+ if (lm_cfg) {
+ last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
+ return last_avail_idx;
+ }
- return last_avail_idx;
+ return -EOPNOTSUPP;
}
int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
{
struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
- vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
+ if (lm_cfg) {
+ vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
+ return 0;
+ }
- return 0;
+ return -EOPNOTSUPP;
}
void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 80d0a0460885..e89fd4d96ff1 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
* Intel IFC VF NIC driver for virtio dataplane offloading
+ * This driver also drives standard virtio-pci hardwares
*
* Copyright (C) 2020 Intel Corporation.
*
@@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
VIRTIO_TRANS_ID_BLOCK,
PCI_VENDOR_ID_INTEL,
VIRTIO_ID_BLOCK) },
-
+ /* standard virtio pci devices */
+ { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
+ PCI_ANY_ID) },
{ 0 },
};
MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* RE: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-13 21:27 [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices Zhu Lingshan
@ 2024-05-13 13:36 ` Parav Pandit
2024-05-14 1:27 ` Zhu, Lingshan
2024-05-13 13:38 ` Michael S. Tsirkin
2024-05-14 13:44 ` Michael S. Tsirkin
2 siblings, 1 reply; 9+ messages in thread
From: Parav Pandit @ 2024-05-13 13:36 UTC (permalink / raw)
To: Zhu Lingshan, jasowang@redhat.com, mst@redhat.com
Cc: virtualization@lists.linux.dev
Hi Lingshan,
> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Tuesday, May 14, 2024 2:57 AM
> To: jasowang@redhat.com; mst@redhat.com
> Cc: virtualization@lists.linux.dev; Zhu Lingshan <lingshan.zhu@intel.com>
> Subject: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
>
> Most of ifcvf code work for standard virtio pci, except for bar4. This commit
> makes bar4 only effective for Intel products, thereby turning ifcvf into a
> standard virtio pci driver.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/vdpa/Kconfig | 2 +-
> drivers/vdpa/ifcvf/ifcvf_base.c | 17 ++++++++++++-----
> drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
> 3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig index
> 656c1cb541de..dbf09f35bcef 100644
> --- a/drivers/vdpa/Kconfig
> +++ b/drivers/vdpa/Kconfig
> @@ -44,7 +44,7 @@ config VDPA_USER
> in a userspace program.
>
> config IFCVF
> - tristate "Intel IFC VF vDPA driver"
> + tristate "Intel IFC VF and standard virtio-pci vDPA driver"
The virtio specification limits the subsystem vendor and device id usage for informational purposes only.
Hence this change/claim here for standard is not good.
I don't have objection to have per vendor creativity in the spec, but for that let OASIS commit to ratify the spec to use subsystem vendor and device id for non-informational purposes.
And amend the spec " (for informational purposes by the driver).".
> depends on PCI_MSI
> help
> This kernel module can drive Intel IFC VF NIC to offload diff --git
> a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c index
> 472daa588a9d..47bc9b11c678 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -178,7 +178,8 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev
> *pdev)
> hw->vring[i].irq = -EINVAL;
> }
>
> - hw->lm_cfg = hw->base[IFCVF_LM_BAR];
> + if (hw->pdev->subsystem_vendor == PCI_VENDOR_ID_INTEL)
> + hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>
> IFCVF_DBG(pdev,
> "PCI capability mapping: common cfg: %p, notify base:
> %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n", @@ -330,18 +331,24 @@
> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
> u16 last_avail_idx;
>
> - last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
> + if (lm_cfg) {
> + last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid
> * 2);
> + return last_avail_idx;
> + }
>
> - return last_avail_idx;
> + return -EOPNOTSUPP;
> }
>
> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num) {
> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>
> - vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
> + if (lm_cfg) {
> + vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
> + return 0;
> + }
>
This is more than informational and requires OASIS virtio committee to add section to indicate per subsystem vendor to allow its modifications.
> - return 0;
> + return -EOPNOTSUPP;
> }
>
> void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num) diff --git
> a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index
> 80d0a0460885..e89fd4d96ff1 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> * Intel IFC VF NIC driver for virtio dataplane offloading
> + * This driver also drives standard virtio-pci hardwares
> *
> * Copyright (C) 2020 Intel Corporation.
> *
> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
> VIRTIO_TRANS_ID_BLOCK,
> PCI_VENDOR_ID_INTEL,
> VIRTIO_ID_BLOCK) },
> -
> + /* standard virtio pci devices */
> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
> + PCI_ANY_ID) },
> { 0 },
> };
> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-13 13:36 ` Parav Pandit
@ 2024-05-14 1:27 ` Zhu, Lingshan
2024-05-14 3:33 ` Parav Pandit
0 siblings, 1 reply; 9+ messages in thread
From: Zhu, Lingshan @ 2024-05-14 1:27 UTC (permalink / raw)
To: Parav Pandit, jasowang@redhat.com, mst@redhat.com
Cc: virtualization@lists.linux.dev
On 5/13/2024 9:36 PM, Parav Pandit wrote:
> Hi Lingshan,
>
>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>> Sent: Tuesday, May 14, 2024 2:57 AM
>> To: jasowang@redhat.com; mst@redhat.com
>> Cc: virtualization@lists.linux.dev; Zhu Lingshan <lingshan.zhu@intel.com>
>> Subject: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
>>
>> Most of ifcvf code work for standard virtio pci, except for bar4. This commit
>> makes bar4 only effective for Intel products, thereby turning ifcvf into a
>> standard virtio pci driver.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/Kconfig | 2 +-
>> drivers/vdpa/ifcvf/ifcvf_base.c | 17 ++++++++++++-----
>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
>> 3 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig index
>> 656c1cb541de..dbf09f35bcef 100644
>> --- a/drivers/vdpa/Kconfig
>> +++ b/drivers/vdpa/Kconfig
>> @@ -44,7 +44,7 @@ config VDPA_USER
>> in a userspace program.
>>
>> config IFCVF
>> - tristate "Intel IFC VF vDPA driver"
>> + tristate "Intel IFC VF and standard virtio-pci vDPA driver"
> The virtio specification limits the subsystem vendor and device id usage for informational purposes only.
> Hence this change/claim here for standard is not good.
>
> I don't have objection to have per vendor creativity in the spec, but for that let OASIS commit to ratify the spec to use subsystem vendor and device id for non-informational purposes.
> And amend the spec " (for informational purposes by the driver).".
The spec says:
The PCI Subsystem Vendor ID and the PCI Subsystem Device ID MAY reflect
the PCI Vendor and Device
ID of the environment (for informational purposes by the driver).
I don't see how this change conflicts with the spec.
>
>> depends on PCI_MSI
>> help
>> This kernel module can drive Intel IFC VF NIC to offload diff --git
>> a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c index
>> 472daa588a9d..47bc9b11c678 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -178,7 +178,8 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev
>> *pdev)
>> hw->vring[i].irq = -EINVAL;
>> }
>>
>> - hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>> + if (hw->pdev->subsystem_vendor == PCI_VENDOR_ID_INTEL)
>> + hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>>
>> IFCVF_DBG(pdev,
>> "PCI capability mapping: common cfg: %p, notify base:
>> %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n", @@ -330,18 +331,24 @@
>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>> u16 last_avail_idx;
>>
>> - last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
>> + if (lm_cfg) {
>> + last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid
>> * 2);
>> + return last_avail_idx;
>> + }
>>
>> - return last_avail_idx;
>> + return -EOPNOTSUPP;
>> }
>>
>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num) {
>> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>>
>> - vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
>> + if (lm_cfg) {
>> + vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
>> + return 0;
>> + }
>>
> This is more than informational and requires OASIS virtio committee to add section to indicate per subsystem vendor to allow its modifications.
This is Intel Specific implementation.
>
>> - return 0;
>> + return -EOPNOTSUPP;
>> }
>>
>> void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num) diff --git
>> a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index
>> 80d0a0460885..e89fd4d96ff1 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -1,6 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> * Intel IFC VF NIC driver for virtio dataplane offloading
>> + * This driver also drives standard virtio-pci hardwares
>> *
>> * Copyright (C) 2020 Intel Corporation.
>> *
>> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>> VIRTIO_TRANS_ID_BLOCK,
>> PCI_VENDOR_ID_INTEL,
>> VIRTIO_ID_BLOCK) },
>> -
>> + /* standard virtio pci devices */
>> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
>> + PCI_ANY_ID) },
>> { 0 },
>> };
>> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
>> --
>> 2.43.0
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* RE: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-14 1:27 ` Zhu, Lingshan
@ 2024-05-14 3:33 ` Parav Pandit
2024-05-14 5:25 ` Zhu, Lingshan
0 siblings, 1 reply; 9+ messages in thread
From: Parav Pandit @ 2024-05-14 3:33 UTC (permalink / raw)
To: Zhu, Lingshan, jasowang@redhat.com, mst@redhat.com
Cc: virtualization@lists.linux.dev
> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Tuesday, May 14, 2024 6:58 AM
>
> On 5/13/2024 9:36 PM, Parav Pandit wrote:
> > Hi Lingshan,
> >
> >> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Sent: Tuesday, May 14, 2024 2:57 AM
> >> To: jasowang@redhat.com; mst@redhat.com
> >> Cc: virtualization@lists.linux.dev; Zhu Lingshan
> >> <lingshan.zhu@intel.com>
> >> Subject: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
> >>
> >> Most of ifcvf code work for standard virtio pci, except for bar4.
> >> This commit makes bar4 only effective for Intel products, thereby
> >> turning ifcvf into a standard virtio pci driver.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >> drivers/vdpa/Kconfig | 2 +-
> >> drivers/vdpa/ifcvf/ifcvf_base.c | 17 ++++++++++++-----
> >> drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
> >> 3 files changed, 17 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig index
> >> 656c1cb541de..dbf09f35bcef 100644
> >> --- a/drivers/vdpa/Kconfig
> >> +++ b/drivers/vdpa/Kconfig
> >> @@ -44,7 +44,7 @@ config VDPA_USER
> >> in a userspace program.
> >>
> >> config IFCVF
> >> - tristate "Intel IFC VF vDPA driver"
> >> + tristate "Intel IFC VF and standard virtio-pci vDPA driver"
> > The virtio specification limits the subsystem vendor and device id usage for
> informational purposes only.
> > Hence this change/claim here for standard is not good.
> >
> > I don't have objection to have per vendor creativity in the spec, but for that
> let OASIS commit to ratify the spec to use subsystem vendor and device id for
> non-informational purposes.
> > And amend the spec " (for informational purposes by the driver).".
> The spec says:
> The PCI Subsystem Vendor ID and the PCI Subsystem Device ID MAY reflect
> the PCI Vendor and Device ID of the environment (for informational purposes
> by the driver).
>
> I don't see how this change conflicts with the spec.
It does. It is used beyond the _informational_ purposes as described in the previous email.
Let vp_vdpa driver handle the standard device as Michael pointed out.
> >
> >> depends on PCI_MSI
> >> help
> >> This kernel module can drive Intel IFC VF NIC to offload diff
> >> --git a/drivers/vdpa/ifcvf/ifcvf_base.c
> >> b/drivers/vdpa/ifcvf/ifcvf_base.c index
> >> 472daa588a9d..47bc9b11c678 100644
> >> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> >> @@ -178,7 +178,8 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct
> >> pci_dev
> >> *pdev)
> >> hw->vring[i].irq = -EINVAL;
> >> }
> >>
> >> - hw->lm_cfg = hw->base[IFCVF_LM_BAR];
> >> + if (hw->pdev->subsystem_vendor == PCI_VENDOR_ID_INTEL)
> >> + hw->lm_cfg = hw->base[IFCVF_LM_BAR];
> >>
> >> IFCVF_DBG(pdev,
> >> "PCI capability mapping: common cfg: %p, notify base:
> >> %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n", @@ -330,18
> >> +331,24 @@
> >> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
> >> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
> >> u16 last_avail_idx;
> >>
> >> - last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
> >> + if (lm_cfg) {
> >> + last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid
> >> * 2);
> >> + return last_avail_idx;
> >> + }
> >>
> >> - return last_avail_idx;
> >> + return -EOPNOTSUPP;
> >> }
> >>
> >> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num) {
> >> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
> >>
> >> - vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
> >> + if (lm_cfg) {
> >> + vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
> >> + return 0;
> >> + }
> >>
> > This is more than informational and requires OASIS virtio committee to add
> section to indicate per subsystem vendor to allow its modifications.
> This is Intel Specific implementation.
Sure that is fine. So keep it as _intel_ driver.
No need to add "and standard" and things are just fine.
> >
> >> - return 0;
> >> + return -EOPNOTSUPP;
> >> }
> >>
> >> void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num) diff
> >> --git a/drivers/vdpa/ifcvf/ifcvf_main.c
> >> b/drivers/vdpa/ifcvf/ifcvf_main.c index
> >> 80d0a0460885..e89fd4d96ff1 100644
> >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> >> @@ -1,6 +1,7 @@
> >> // SPDX-License-Identifier: GPL-2.0-only
> >> /*
> >> * Intel IFC VF NIC driver for virtio dataplane offloading
> >> + * This driver also drives standard virtio-pci hardwares
> >> *
> >> * Copyright (C) 2020 Intel Corporation.
> >> *
> >> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
> >> VIRTIO_TRANS_ID_BLOCK,
> >> PCI_VENDOR_ID_INTEL,
> >> VIRTIO_ID_BLOCK) },
> >> -
> >> + /* standard virtio pci devices */
> >> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
> >> + PCI_ANY_ID) },
> >> { 0 },
> >> };
> >> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
> >> --
> >> 2.43.0
> >>
> >
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-14 3:33 ` Parav Pandit
@ 2024-05-14 5:25 ` Zhu, Lingshan
0 siblings, 0 replies; 9+ messages in thread
From: Zhu, Lingshan @ 2024-05-14 5:25 UTC (permalink / raw)
To: Parav Pandit, jasowang@redhat.com, mst@redhat.com
Cc: virtualization@lists.linux.dev
On 5/14/2024 11:33 AM, Parav Pandit wrote:
>
>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>> Sent: Tuesday, May 14, 2024 6:58 AM
>>
>> On 5/13/2024 9:36 PM, Parav Pandit wrote:
>>> Hi Lingshan,
>>>
>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Sent: Tuesday, May 14, 2024 2:57 AM
>>>> To: jasowang@redhat.com; mst@redhat.com
>>>> Cc: virtualization@lists.linux.dev; Zhu Lingshan
>>>> <lingshan.zhu@intel.com>
>>>> Subject: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
>>>>
>>>> Most of ifcvf code work for standard virtio pci, except for bar4.
>>>> This commit makes bar4 only effective for Intel products, thereby
>>>> turning ifcvf into a standard virtio pci driver.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>> drivers/vdpa/Kconfig | 2 +-
>>>> drivers/vdpa/ifcvf/ifcvf_base.c | 17 ++++++++++++-----
>>>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
>>>> 3 files changed, 17 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig index
>>>> 656c1cb541de..dbf09f35bcef 100644
>>>> --- a/drivers/vdpa/Kconfig
>>>> +++ b/drivers/vdpa/Kconfig
>>>> @@ -44,7 +44,7 @@ config VDPA_USER
>>>> in a userspace program.
>>>>
>>>> config IFCVF
>>>> - tristate "Intel IFC VF vDPA driver"
>>>> + tristate "Intel IFC VF and standard virtio-pci vDPA driver"
>>> The virtio specification limits the subsystem vendor and device id usage for
>> informational purposes only.
>>> Hence this change/claim here for standard is not good.
>>>
>>> I don't have objection to have per vendor creativity in the spec, but for that
>> let OASIS commit to ratify the spec to use subsystem vendor and device id for
>> non-informational purposes.
>>> And amend the spec " (for informational purposes by the driver).".
>> The spec says:
>> The PCI Subsystem Vendor ID and the PCI Subsystem Device ID MAY reflect
>> the PCI Vendor and Device ID of the environment (for informational purposes
>> by the driver).
>>
>> I don't see how this change conflicts with the spec.
> It does. It is used beyond the _informational_ purposes as described in the previous email.
> Let vp_vdpa driver handle the standard device as Michael pointed out.
Informational means the information should be used by someone or some
program, and every vendor
has its own sud-vendor-id.
>
>>>> depends on PCI_MSI
>>>> help
>>>> This kernel module can drive Intel IFC VF NIC to offload diff
>>>> --git a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c index
>>>> 472daa588a9d..47bc9b11c678 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>> @@ -178,7 +178,8 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct
>>>> pci_dev
>>>> *pdev)
>>>> hw->vring[i].irq = -EINVAL;
>>>> }
>>>>
>>>> - hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>>>> + if (hw->pdev->subsystem_vendor == PCI_VENDOR_ID_INTEL)
>>>> + hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>>>>
>>>> IFCVF_DBG(pdev,
>>>> "PCI capability mapping: common cfg: %p, notify base:
>>>> %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n", @@ -330,18
>>>> +331,24 @@
>>>> u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>>>> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>>>> u16 last_avail_idx;
>>>>
>>>> - last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
>>>> + if (lm_cfg) {
>>>> + last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid
>>>> * 2);
>>>> + return last_avail_idx;
>>>> + }
>>>>
>>>> - return last_avail_idx;
>>>> + return -EOPNOTSUPP;
>>>> }
>>>>
>>>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num) {
>>>> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>>>>
>>>> - vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
>>>> + if (lm_cfg) {
>>>> + vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
>>>> + return 0;
>>>> + }
>>>>
>>> This is more than informational and requires OASIS virtio committee to add
>> section to indicate per subsystem vendor to allow its modifications.
>> This is Intel Specific implementation.
> Sure that is fine. So keep it as _intel_ driver.
> No need to add "and standard" and things are just fine.
bar4 is still Intel only.
>
>>>> - return 0;
>>>> + return -EOPNOTSUPP;
>>>> }
>>>>
>>>> void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num) diff
>>>> --git a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c index
>>>> 80d0a0460885..e89fd4d96ff1 100644
>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>> @@ -1,6 +1,7 @@
>>>> // SPDX-License-Identifier: GPL-2.0-only
>>>> /*
>>>> * Intel IFC VF NIC driver for virtio dataplane offloading
>>>> + * This driver also drives standard virtio-pci hardwares
>>>> *
>>>> * Copyright (C) 2020 Intel Corporation.
>>>> *
>>>> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>>>> VIRTIO_TRANS_ID_BLOCK,
>>>> PCI_VENDOR_ID_INTEL,
>>>> VIRTIO_ID_BLOCK) },
>>>> -
>>>> + /* standard virtio pci devices */
>>>> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
>>>> + PCI_ANY_ID) },
>>>> { 0 },
>>>> };
>>>> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
>>>> --
>>>> 2.43.0
>>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-13 21:27 [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices Zhu Lingshan
2024-05-13 13:36 ` Parav Pandit
@ 2024-05-13 13:38 ` Michael S. Tsirkin
2024-05-14 1:22 ` Zhu, Lingshan
2024-05-14 13:44 ` Michael S. Tsirkin
2 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2024-05-13 13:38 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: jasowang, virtualization
On Tue, May 14, 2024 at 05:27:07AM +0800, Zhu Lingshan wrote:
> Most of ifcvf code work for standard virtio pci,
> except for bar4. This commit makes bar4 only effective
> for Intel products, thereby turning ifcvf into
> a standard virtio pci driver.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/vdpa/Kconfig | 2 +-
> drivers/vdpa/ifcvf/ifcvf_base.c | 17 ++++++++++++-----
> drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
> 3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
> index 656c1cb541de..dbf09f35bcef 100644
> --- a/drivers/vdpa/Kconfig
> +++ b/drivers/vdpa/Kconfig
> @@ -44,7 +44,7 @@ config VDPA_USER
> in a userspace program.
>
> config IFCVF
> - tristate "Intel IFC VF vDPA driver"
> + tristate "Intel IFC VF and standard virtio-pci vDPA driver"
> depends on PCI_MSI
> help
> This kernel module can drive Intel IFC VF NIC to offload
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 472daa588a9d..47bc9b11c678 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -178,7 +178,8 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
> hw->vring[i].irq = -EINVAL;
> }
>
> - hw->lm_cfg = hw->base[IFCVF_LM_BAR];
> + if (hw->pdev->subsystem_vendor == PCI_VENDOR_ID_INTEL)
> + hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>
> IFCVF_DBG(pdev,
> "PCI capability mapping: common cfg: %p, notify base: %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n",
> @@ -330,18 +331,24 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
> u16 last_avail_idx;
>
> - last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
> + if (lm_cfg) {
> + last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
> + return last_avail_idx;
> + }
>
> - return last_avail_idx;
> + return -EOPNOTSUPP;
> }
>
> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
> {
> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>
> - vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
> + if (lm_cfg) {
> + vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
> + return 0;
> + }
>
> - return 0;
> + return -EOPNOTSUPP;
> }
>
> void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num)
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 80d0a0460885..e89fd4d96ff1 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> * Intel IFC VF NIC driver for virtio dataplane offloading
> + * This driver also drives standard virtio-pci hardwares
> *
> * Copyright (C) 2020 Intel Corporation.
> *
> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
> VIRTIO_TRANS_ID_BLOCK,
> PCI_VENDOR_ID_INTEL,
> VIRTIO_ID_BLOCK) },
> -
> + /* standard virtio pci devices */
> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
> + PCI_ANY_ID) },
> { 0 },
> };
> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
Will make it conflict with the normal virtio driver, won't it?
What are you trying to achieve here?
> --
> 2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-13 13:38 ` Michael S. Tsirkin
@ 2024-05-14 1:22 ` Zhu, Lingshan
0 siblings, 0 replies; 9+ messages in thread
From: Zhu, Lingshan @ 2024-05-14 1:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, virtualization
On 5/13/2024 9:38 PM, Michael S. Tsirkin wrote:
> On Tue, May 14, 2024 at 05:27:07AM +0800, Zhu Lingshan wrote:
>> Most of ifcvf code work for standard virtio pci,
>> except for bar4. This commit makes bar4 only effective
>> for Intel products, thereby turning ifcvf into
>> a standard virtio pci driver.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/Kconfig | 2 +-
>> drivers/vdpa/ifcvf/ifcvf_base.c | 17 ++++++++++++-----
>> drivers/vdpa/ifcvf/ifcvf_main.c | 5 ++++-
>> 3 files changed, 17 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
>> index 656c1cb541de..dbf09f35bcef 100644
>> --- a/drivers/vdpa/Kconfig
>> +++ b/drivers/vdpa/Kconfig
>> @@ -44,7 +44,7 @@ config VDPA_USER
>> in a userspace program.
>>
>> config IFCVF
>> - tristate "Intel IFC VF vDPA driver"
>> + tristate "Intel IFC VF and standard virtio-pci vDPA driver"
>> depends on PCI_MSI
>> help
>> This kernel module can drive Intel IFC VF NIC to offload
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 472daa588a9d..47bc9b11c678 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -178,7 +178,8 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
>> hw->vring[i].irq = -EINVAL;
>> }
>>
>> - hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>> + if (hw->pdev->subsystem_vendor == PCI_VENDOR_ID_INTEL)
>> + hw->lm_cfg = hw->base[IFCVF_LM_BAR];
>>
>> IFCVF_DBG(pdev,
>> "PCI capability mapping: common cfg: %p, notify base: %p\n, isr cfg: %p, device cfg: %p, multiplier: %u\n",
>> @@ -330,18 +331,24 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid)
>> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>> u16 last_avail_idx;
>>
>> - last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
>> + if (lm_cfg) {
>> + last_avail_idx = vp_ioread16(&lm_cfg->vq_state_region + qid * 2);
>> + return last_avail_idx;
>> + }
>>
>> - return last_avail_idx;
>> + return -EOPNOTSUPP;
>> }
>>
>> int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
>> {
>> struct ifcvf_lm_cfg __iomem *lm_cfg = hw->lm_cfg;
>>
>> - vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
>> + if (lm_cfg) {
>> + vp_iowrite16(num, &lm_cfg->vq_state_region + qid * 2);
>> + return 0;
>> + }
>>
>> - return 0;
>> + return -EOPNOTSUPP;
>> }
>>
>> void ifcvf_set_vq_num(struct ifcvf_hw *hw, u16 qid, u32 num)
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>> index 80d0a0460885..e89fd4d96ff1 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -1,6 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> * Intel IFC VF NIC driver for virtio dataplane offloading
>> + * This driver also drives standard virtio-pci hardwares
>> *
>> * Copyright (C) 2020 Intel Corporation.
>> *
>> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>> VIRTIO_TRANS_ID_BLOCK,
>> PCI_VENDOR_ID_INTEL,
>> VIRTIO_ID_BLOCK) },
>> -
>> + /* standard virtio pci devices */
>> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
>> + PCI_ANY_ID) },
>> { 0 },
>> };
>> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
> Will make it conflict with the normal virtio driver, won't it?
>
> What are you trying to achieve here?
The confliction exists before, in my setup, virtio-pci driver always
probe virtio-pci devices
before ifcvf.
Since ifcvf drives is all standard virtio-pci except for bar4, so this
patch intends to maximize
the use of the ifcvf driver and allow developers without Intel hardware
to also develop this driver.
Thanks
Zhu Lingshan
>
>
>
>> --
>> 2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-13 21:27 [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices Zhu Lingshan
2024-05-13 13:36 ` Parav Pandit
2024-05-13 13:38 ` Michael S. Tsirkin
@ 2024-05-14 13:44 ` Michael S. Tsirkin
2024-05-15 7:28 ` Zhu, Lingshan
2 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2024-05-14 13:44 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: jasowang, virtualization
On Tue, May 14, 2024 at 05:27:07AM +0800, Zhu Lingshan wrote:
> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
> VIRTIO_TRANS_ID_BLOCK,
> PCI_VENDOR_ID_INTEL,
> VIRTIO_ID_BLOCK) },
> -
> + /* standard virtio pci devices */
> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
> + PCI_ANY_ID) },
> { 0 },
To summarize, I don't think ifcvf should drive standard virtio devices.
If it can then it sounds more like a reason to try to reuse more code
from virtio core.
> };
> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
> --
> 2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices
2024-05-14 13:44 ` Michael S. Tsirkin
@ 2024-05-15 7:28 ` Zhu, Lingshan
0 siblings, 0 replies; 9+ messages in thread
From: Zhu, Lingshan @ 2024-05-15 7:28 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: jasowang, virtualization
On 5/14/2024 9:44 PM, Michael S. Tsirkin wrote:
> On Tue, May 14, 2024 at 05:27:07AM +0800, Zhu Lingshan wrote:
>> @@ -880,7 +881,9 @@ static struct pci_device_id ifcvf_pci_ids[] = {
>> VIRTIO_TRANS_ID_BLOCK,
>> PCI_VENDOR_ID_INTEL,
>> VIRTIO_ID_BLOCK) },
>> -
>> + /* standard virtio pci devices */
>> + { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET,
>> + PCI_ANY_ID) },
>> { 0 },
>
> To summarize, I don't think ifcvf should drive standard virtio devices.
> If it can then it sounds more like a reason to try to reuse more code
> from virtio core.
Yeah, OK.
By the way, I send out a V2 of MAINTAINER patch only changing my
email address to kernel.org.
>
>
>> };
>> MODULE_DEVICE_TABLE(pci, ifcvf_pci_ids);
>> --
>> 2.43.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-05-15 7:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-13 21:27 [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices Zhu Lingshan
2024-05-13 13:36 ` Parav Pandit
2024-05-14 1:27 ` Zhu, Lingshan
2024-05-14 3:33 ` Parav Pandit
2024-05-14 5:25 ` Zhu, Lingshan
2024-05-13 13:38 ` Michael S. Tsirkin
2024-05-14 1:22 ` Zhu, Lingshan
2024-05-14 13:44 ` Michael S. Tsirkin
2024-05-15 7:28 ` Zhu, Lingshan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).