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

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

[Parav Pandit]

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
> 

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

[Michael S. Tsirkin]

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

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

[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

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

[Zhu, Lingshan]

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

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

[Zhu, Lingshan]

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

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

[Parav Pandit]

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

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

[Zhu, Lingshan]

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

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

[Michael S. Tsirkin]

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

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

[Zhu, Lingshan]

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
>