RE: [PATCH] ifcvf/vDPA: ifcvf drives standard virtio pci devices

[Parav Pandit <parav@nvidia.com>]

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