[PATCH 0/7] Supoort shared irq for virtqueues

[PATCH 0/7] Supoort shared irq for virtqueues

[Zhu Lingshan]

On some platforms, it has been observed that a device may fail to
allocate enough MSI-X vectors, under such circumstances, the vqs have
to share a irq/vector.

This series extends irq requester/handlers abilities to deal with:
(granted nvectors, and max_intr = total vq number + 1(config interrupt) )

1)nvectors = max_intr: each vq has its own vector/irq,
config interrupt is enabled, normal case
2)max_intr > nvectors >= 2: vqs share one irq/vector, config interrupt is
enabled
3)nvectors = 1, vqs share one irq/vector, config interrupt is disabled.
Otherwise it fails.

This series also made necessary changes to irq cleaners and related
helpers.

Pleaase help reivew.

Thanks!
Zhu Lingshan

Zhu Lingshan (7):
  vDPA/ifcvf: implement IO read/write helpers in the header file
  vDPA/ifcvf: introduce new helpers to set config vector and vq vectors
  vDPA/ifcvf: implement device MSIX vector allocation helper
  vDPA/ifcvf: implement shared irq handlers for vqs
  vDPA/ifcvf: irq request helpers for both shared and per_vq irq
  vDPA/ifcvf: implement config interrupt request helper
  vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

 drivers/vdpa/ifcvf/ifcvf_base.c |  65 ++++--------
 drivers/vdpa/ifcvf/ifcvf_base.h |  45 +++++++-
 drivers/vdpa/ifcvf/ifcvf_main.c | 179 +++++++++++++++++++++++++++-----
 3 files changed, 215 insertions(+), 74 deletions(-)

-- 
2.27.0

[PATCH 1/7] vDPA/ifcvf: implement IO read/write helpers in the header file

[Zhu Lingshan]

re-implement IO read/write helpers in the header file, so that
they can be utilized among modules.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 36 --------------------------------
 drivers/vdpa/ifcvf/ifcvf_base.h | 37 +++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 2808f1ba9f7b..0b5df4cfaf06 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -10,42 +10,6 @@
 
 #include "ifcvf_base.h"
 
-static inline u8 ifc_ioread8(u8 __iomem *addr)
-{
-	return ioread8(addr);
-}
-static inline u16 ifc_ioread16 (__le16 __iomem *addr)
-{
-	return ioread16(addr);
-}
-
-static inline u32 ifc_ioread32(__le32 __iomem *addr)
-{
-	return ioread32(addr);
-}
-
-static inline void ifc_iowrite8(u8 value, u8 __iomem *addr)
-{
-	iowrite8(value, addr);
-}
-
-static inline void ifc_iowrite16(u16 value, __le16 __iomem *addr)
-{
-	iowrite16(value, addr);
-}
-
-static inline void ifc_iowrite32(u32 value, __le32 __iomem *addr)
-{
-	iowrite32(value, addr);
-}
-
-static void ifc_iowrite64_twopart(u64 val,
-				  __le32 __iomem *lo, __le32 __iomem *hi)
-{
-	ifc_iowrite32((u32)val, lo);
-	ifc_iowrite32(val >> 32, hi);
-}
-
 struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw)
 {
 	return container_of(hw, struct ifcvf_adapter, vf);
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 09918af3ecf8..c924a7673afb 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -42,6 +42,43 @@
 #define ifcvf_private_to_vf(adapter) \
 	(&((struct ifcvf_adapter *)adapter)->vf)
 
+static inline u8 ifc_ioread8(u8 __iomem *addr)
+{
+	return ioread8(addr);
+}
+
+static inline u16 ifc_ioread16(__le16 __iomem *addr)
+{
+	return ioread16(addr);
+}
+
+static inline u32 ifc_ioread32(__le32 __iomem *addr)
+{
+	return ioread32(addr);
+}
+
+static inline void ifc_iowrite8(u8 value, u8 __iomem *addr)
+{
+	iowrite8(value, addr);
+}
+
+static inline void ifc_iowrite16(u16 value, __le16 __iomem *addr)
+{
+	iowrite16(value, addr);
+}
+
+static inline void ifc_iowrite32(u32 value, __le32 __iomem *addr)
+{
+	iowrite32(value, addr);
+}
+
+static inline void ifc_iowrite64_twopart(u64 val,
+				  __le32 __iomem *lo, __le32 __iomem *hi)
+{
+	ifc_iowrite32((u32)val, lo);
+	ifc_iowrite32(val >> 32, hi);
+}
+
 struct vring_info {
 	u64 desc;
 	u64 avail;
-- 
2.27.0

[PATCH 2/7] vDPA/ifcvf: introduce new helpers to set config vector and vq vectors

[Zhu Lingshan]

This commit introduces new helpers to set config vector
and vq vectors in virtio common config space.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 30 ++++++++++++++++++++++++++++++
 drivers/vdpa/ifcvf/ifcvf_base.h |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 0b5df4cfaf06..696a41560eaa 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -15,6 +15,36 @@ struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw)
 	return container_of(hw, struct ifcvf_adapter, vf);
 }
 
+int ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
+	struct ifcvf_adapter *ifcvf = vf_to_adapter(hw);
+
+	ifc_iowrite16(qid, &cfg->queue_select);
+	ifc_iowrite16(vector, &cfg->queue_msix_vector);
+	if (ifc_ioread16(&cfg->queue_msix_vector) == VIRTIO_MSI_NO_VECTOR) {
+		IFCVF_ERR(ifcvf->pdev, "No msix vector for queue %u\n", qid);
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
+int ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector)
+{
+	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
+	struct ifcvf_adapter *ifcvf = vf_to_adapter(hw);
+
+	cfg = hw->common_cfg;
+	ifc_iowrite16(vector,  &cfg->msix_config);
+	if (ifc_ioread16(&cfg->msix_config) == VIRTIO_MSI_NO_VECTOR) {
+		IFCVF_ERR(ifcvf->pdev, "No msix vector for device config\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static void __iomem *get_cap_addr(struct ifcvf_hw *hw,
 				  struct virtio_pci_cap *cap)
 {
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index c924a7673afb..1d5431040d7d 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -157,4 +157,6 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
 int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
 struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
 int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
+int ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector);
+int ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector);
 #endif /* _IFCVF_H_ */
-- 
2.27.0

Re: [PATCH 2/7] vDPA/ifcvf: introduce new helpers to set config vector and vq vectors

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:46PM +0800, Zhu Lingshan wrote:
> This commit introduces new helpers to set config vector
> and vq vectors in virtio common config space.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_base.c | 30 ++++++++++++++++++++++++++++++
>  drivers/vdpa/ifcvf/ifcvf_base.h |  2 ++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 0b5df4cfaf06..696a41560eaa 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -15,6 +15,36 @@ struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw)
>  	return container_of(hw, struct ifcvf_adapter, vf);
>  }
>  
> +int ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector)
> +{
> +	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> +	struct ifcvf_adapter *ifcvf = vf_to_adapter(hw);
> +
> +	ifc_iowrite16(qid, &cfg->queue_select);
> +	ifc_iowrite16(vector, &cfg->queue_msix_vector);
> +	if (ifc_ioread16(&cfg->queue_msix_vector) == VIRTIO_MSI_NO_VECTOR) {
> +		IFCVF_ERR(ifcvf->pdev, "No msix vector for queue %u\n", qid);
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector)
> +{
> +	struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg;
> +	struct ifcvf_adapter *ifcvf = vf_to_adapter(hw);
> +
> +	cfg = hw->common_cfg;
> +	ifc_iowrite16(vector,  &cfg->msix_config);
> +	if (ifc_ioread16(&cfg->msix_config) == VIRTIO_MSI_NO_VECTOR) {
> +		IFCVF_ERR(ifcvf->pdev, "No msix vector for device config\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

The messages are confusing. Something like "unable to set" would be
clearer. And maybe supply the info which vector and which vq to set?

>  static void __iomem *get_cap_addr(struct ifcvf_hw *hw,
>  				  struct virtio_pci_cap *cap)
>  {
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index c924a7673afb..1d5431040d7d 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -157,4 +157,6 @@ u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>  int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>  struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>  int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
> +int ifcvf_set_vq_vector(struct ifcvf_hw *hw, u16 qid, int vector);
> +int ifcvf_set_config_vector(struct ifcvf_hw *hw, int vector);
>  #endif /* _IFCVF_H_ */
> -- 
> 2.27.0

[PATCH 3/7] vDPA/ifcvf: implement device MSIX vector allocation helper

[Zhu Lingshan]

This commit implements a MSIX vector allocation helper

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_main.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 6dc75ca70b37..64fc78eaa1a9 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -58,6 +58,30 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
 	ifcvf_free_irq_vectors(pdev);
 }
 
+static int ifcvf_alloc_vectors(struct ifcvf_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct ifcvf_hw *vf = &adapter->vf;
+	u16 max_intr = 0;
+	u16 ret = 0;
+
+	/* all queues and config interrupt  */
+	max_intr = vf->nr_vring + 1;
+	ret = pci_alloc_irq_vectors(pdev, 1, max_intr, PCI_IRQ_MSIX|PCI_IRQ_AFFINITY);
+
+	if (ret < 0) {
+		IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
+		return ret;
+	}
+
+	if (ret < max_intr)
+		IFCVF_INFO(pdev,
+			   "Requested %u vectors, however only %u allocated, lower performance\n",
+			   max_intr, ret);
+
+	return ret;
+}
+
 static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
 {
 	struct pci_dev *pdev = adapter->pdev;
-- 
2.27.0

Re: [PATCH 3/7] vDPA/ifcvf: implement device MSIX vector allocation helper

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:47PM +0800, Zhu Lingshan wrote:
> This commit implements a MSIX vector allocation helper
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_main.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 6dc75ca70b37..64fc78eaa1a9 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -58,6 +58,30 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
>  	ifcvf_free_irq_vectors(pdev);
>  }
>  
> +static int ifcvf_alloc_vectors(struct ifcvf_adapter *adapter)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct ifcvf_hw *vf = &adapter->vf;
> +	u16 max_intr = 0;
> +	u16 ret = 0;

just declare these where they are inited. this = 0 is pointless.

> +
> +	/* all queues and config interrupt  */
> +	max_intr = vf->nr_vring + 1;
> +	ret = pci_alloc_irq_vectors(pdev, 1, max_intr, PCI_IRQ_MSIX|PCI_IRQ_AFFINITY);


coding style violation.

> +
> +	if (ret < 0) {
> +		IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
> +		return ret;
> +	}
> +
> +	if (ret < max_intr)
> +		IFCVF_INFO(pdev,
> +			   "Requested %u vectors, however only %u allocated, lower performance\n",
> +			   max_intr, ret);
> +
> +	return ret;
> +}
> +
>  static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>  {
>  	struct pci_dev *pdev = adapter->pdev;

this kind of split up does not help review. just include
it with the code that uses it.


> -- 
> 2.27.0

[PATCH 4/7] vDPA/ifcvf: implement shared irq handlers for vqs

[Zhu Lingshan]

It has observed that a device may fail to alloc enough vectors on
some platforms, e.g., requires 16 vectors, but only 2 or 4 vector
slots allocated. The virt queues have to share a vector/irq under
such circumstances.

This irq handlers has to kick every queue because it is not
possible to tell which queue triggers the interrupt.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_main.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 64fc78eaa1a9..19e1d1cd71a3 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -37,6 +37,21 @@ static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t ifcvf_shared_intr_handler(int irq, void *arg)
+{
+	struct ifcvf_hw *vf = arg;
+	struct vring_info *vring;
+	int i;
+
+	for (i = 0; i < vf->nr_vring; i++) {
+		vring = &vf->vring[i];
+		if (vring->cb.callback)
+			vf->vring->cb.callback(vring->cb.private);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static void ifcvf_free_irq_vectors(void *data)
 {
 	pci_free_irq_vectors(data);
-- 
2.27.0

Re: [PATCH 4/7] vDPA/ifcvf: implement shared irq handlers for vqs

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:48PM +0800, Zhu Lingshan wrote:
> It has observed that a device may fail to alloc enough vectors on
> some platforms, e.g., requires 16 vectors, but only 2 or 4 vector
> slots allocated. The virt queues have to share a vector/irq under
> such circumstances.
> 
> This irq handlers has to kick every queue because it is not
> possible to tell which queue triggers the interrupt.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_main.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 64fc78eaa1a9..19e1d1cd71a3 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -37,6 +37,21 @@ static irqreturn_t ifcvf_intr_handler(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t ifcvf_shared_intr_handler(int irq, void *arg)
> +{
> +	struct ifcvf_hw *vf = arg;
> +	struct vring_info *vring;
> +	int i;
> +
> +	for (i = 0; i < vf->nr_vring; i++) {
> +		vring = &vf->vring[i];
> +		if (vring->cb.callback)
> +			vf->vring->cb.callback(vring->cb.private);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static void ifcvf_free_irq_vectors(void *data)
>  {
>  	pci_free_irq_vectors(data);


A static function with no caller. surprised gcc does not warn.

> -- 
> 2.27.0

[PATCH 5/7] vDPA/ifcvf: irq request helpers for both shared and per_vq irq

[Zhu Lingshan]

This commit implements new irq request helpers:
ifcvf_request_shared_vq_irq() for a shared irq, in this case,
all virtqueues would share one irq/vector. This can help the
device work on some platforms that can not provide enough
MSIX vectors

ifcvf_request_per_vq_irq() for per vq irqs, in this case,
every virtqueue has its own irq/vector

ifcvf_request_vq_irq() calls either of the above two, depends on
the number of allocated vectors.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c |  9 -----
 drivers/vdpa/ifcvf/ifcvf_main.c | 66 +++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 696a41560eaa..fc496d10cf8d 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -349,15 +349,6 @@ static int ifcvf_hw_enable(struct ifcvf_hw *hw)
 		ifc_iowrite64_twopart(hw->vring[i].used, &cfg->queue_used_lo,
 				     &cfg->queue_used_hi);
 		ifc_iowrite16(hw->vring[i].size, &cfg->queue_size);
-		ifc_iowrite16(i + IFCVF_MSI_QUEUE_OFF, &cfg->queue_msix_vector);
-
-		if (ifc_ioread16(&cfg->queue_msix_vector) ==
-		    VIRTIO_MSI_NO_VECTOR) {
-			IFCVF_ERR(ifcvf->pdev,
-				  "No msix vector for queue %u\n", i);
-			return -EINVAL;
-		}
-
 		ifcvf_set_vq_state(hw, i, hw->vring[i].last_avail_idx);
 		ifc_iowrite16(1, &cfg->queue_enable);
 	}
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 19e1d1cd71a3..ce2fbc429fbe 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -97,6 +97,72 @@ static int ifcvf_alloc_vectors(struct ifcvf_adapter *adapter)
 	return ret;
 }
 
+static int ifcvf_request_per_vq_irq(struct ifcvf_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct ifcvf_hw *vf = &adapter->vf;
+	int i, vector, ret, irq;
+
+	for (i = 0; i < vf->nr_vring; i++) {
+		snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n", pci_name(pdev), i);
+		vector = i;
+		irq = pci_irq_vector(pdev, vector);
+		ret = devm_request_irq(&pdev->dev, irq,
+				       ifcvf_intr_handler, 0,
+				       vf->vring[i].msix_name,
+				       &vf->vring[i]);
+		if (ret) {
+			IFCVF_ERR(pdev, "Failed to request irq for vq %d\n", i);
+			ifcvf_free_irq(adapter, i);
+		} else {
+			vf->vring[i].irq = irq;
+			ifcvf_set_vq_vector(vf, i, vector);
+		}
+	}
+
+	return 0;
+}
+
+static int ifcvf_request_shared_vq_irq(struct ifcvf_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct ifcvf_hw *vf = &adapter->vf;
+	int i, vector, ret, irq;
+
+	vector = 0;
+	irq = pci_irq_vector(pdev, vector);
+	ret = devm_request_irq(&pdev->dev, irq,
+			       ifcvf_shared_intr_handler, 0,
+			       "ifcvf_shared_irq",
+			       vf);
+	if (ret) {
+		IFCVF_ERR(pdev, "Failed to request shared irq for vf\n");
+
+		return ret;
+	}
+
+	for (i = 0; i < vf->nr_vring; i++) {
+		vf->vring[i].irq = irq;
+		ifcvf_set_vq_vector(vf, i, vector);
+	}
+
+	return 0;
+
+}
+
+static int ifcvf_request_vq_irq(struct ifcvf_adapter *adapter, u8 vector_per_vq)
+{
+	int ret;
+
+	if (vector_per_vq)
+		ret = ifcvf_request_per_vq_irq(adapter);
+	else
+		ret = ifcvf_request_shared_vq_irq(adapter);
+
+	return ret;
+}
+
+
 static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
 {
 	struct pci_dev *pdev = adapter->pdev;
-- 
2.27.0

Re: [PATCH 5/7] vDPA/ifcvf: irq request helpers for both shared and per_vq irq

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:49PM +0800, Zhu Lingshan wrote:
> This commit implements new irq request helpers:
> ifcvf_request_shared_vq_irq() for a shared irq, in this case,
> all virtqueues would share one irq/vector. This can help the
> device work on some platforms that can not provide enough
> MSIX vectors
> 
> ifcvf_request_per_vq_irq() for per vq irqs, in this case,
> every virtqueue has its own irq/vector
> 
> ifcvf_request_vq_irq() calls either of the above two, depends on
> the number of allocated vectors.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_base.c |  9 -----
>  drivers/vdpa/ifcvf/ifcvf_main.c | 66 +++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 696a41560eaa..fc496d10cf8d 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -349,15 +349,6 @@ static int ifcvf_hw_enable(struct ifcvf_hw *hw)
>  		ifc_iowrite64_twopart(hw->vring[i].used, &cfg->queue_used_lo,
>  				     &cfg->queue_used_hi);
>  		ifc_iowrite16(hw->vring[i].size, &cfg->queue_size);
> -		ifc_iowrite16(i + IFCVF_MSI_QUEUE_OFF, &cfg->queue_msix_vector);
> -
> -		if (ifc_ioread16(&cfg->queue_msix_vector) ==
> -		    VIRTIO_MSI_NO_VECTOR) {
> -			IFCVF_ERR(ifcvf->pdev,
> -				  "No msix vector for queue %u\n", i);
> -			return -EINVAL;
> -		}
> -
>  		ifcvf_set_vq_state(hw, i, hw->vring[i].last_avail_idx);
>  		ifc_iowrite16(1, &cfg->queue_enable);
>  	}
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 19e1d1cd71a3..ce2fbc429fbe 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -97,6 +97,72 @@ static int ifcvf_alloc_vectors(struct ifcvf_adapter *adapter)
>  	return ret;
>  }
>  
> +static int ifcvf_request_per_vq_irq(struct ifcvf_adapter *adapter)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct ifcvf_hw *vf = &adapter->vf;
> +	int i, vector, ret, irq;
> +
> +	for (i = 0; i < vf->nr_vring; i++) {
> +		snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n", pci_name(pdev), i);
> +		vector = i;
> +		irq = pci_irq_vector(pdev, vector);
> +		ret = devm_request_irq(&pdev->dev, irq,
> +				       ifcvf_intr_handler, 0,
> +				       vf->vring[i].msix_name,
> +				       &vf->vring[i]);
> +		if (ret) {
> +			IFCVF_ERR(pdev, "Failed to request irq for vq %d\n", i);
> +			ifcvf_free_irq(adapter, i);
> +		} else {
> +			vf->vring[i].irq = irq;
> +			ifcvf_set_vq_vector(vf, i, vector);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ifcvf_request_shared_vq_irq(struct ifcvf_adapter *adapter)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct ifcvf_hw *vf = &adapter->vf;
> +	int i, vector, ret, irq;
> +
> +	vector = 0;
> +	irq = pci_irq_vector(pdev, vector);
> +	ret = devm_request_irq(&pdev->dev, irq,
> +			       ifcvf_shared_intr_handler, 0,
> +			       "ifcvf_shared_irq",
> +			       vf);
> +	if (ret) {
> +		IFCVF_ERR(pdev, "Failed to request shared irq for vf\n");
> +
> +		return ret;
> +	}
> +
> +	for (i = 0; i < vf->nr_vring; i++) {
> +		vf->vring[i].irq = irq;
> +		ifcvf_set_vq_vector(vf, i, vector);
> +	}
> +
> +	return 0;
> +
> +}
> +
> +static int ifcvf_request_vq_irq(struct ifcvf_adapter *adapter, u8 vector_per_vq)
> +{
> +	int ret;
> +
> +	if (vector_per_vq)
> +		ret = ifcvf_request_per_vq_irq(adapter);
> +	else
> +		ret = ifcvf_request_shared_vq_irq(adapter);
> +
> +	return ret;
> +}
> +
> +
>  static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>  {
>  	struct pci_dev *pdev = adapter->pdev;


this moves code from init function to static ones which
are never called. I guess until patch 7? You can't
split up patches like this, git bisect won't work if you do -
code needs to work after each patch is applied.

> -- 
> 2.27.0

[PATCH 6/7] vDPA/ifcvf: implement config interrupt request helper

[Zhu Lingshan]

This commit implements new helper to request config interrupt by
a given MSIX vector.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c |  6 ------
 drivers/vdpa/ifcvf/ifcvf_main.c | 26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index fc496d10cf8d..38f91dc6481f 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -330,12 +330,6 @@ static int ifcvf_hw_enable(struct ifcvf_hw *hw)
 
 	ifcvf = vf_to_adapter(hw);
 	cfg = hw->common_cfg;
-	ifc_iowrite16(IFCVF_MSI_CONFIG_OFF, &cfg->msix_config);
-
-	if (ifc_ioread16(&cfg->msix_config) == VIRTIO_MSI_NO_VECTOR) {
-		IFCVF_ERR(ifcvf->pdev, "No msix vector for device config\n");
-		return -EINVAL;
-	}
 
 	for (i = 0; i < hw->nr_vring; i++) {
 		if (!hw->vring[i].ready)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index ce2fbc429fbe..414b5dfd04ca 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -162,6 +162,32 @@ static int ifcvf_request_vq_irq(struct ifcvf_adapter *adapter, u8 vector_per_vq)
 	return ret;
 }
 
+static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_vector)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct ifcvf_hw *vf = &adapter->vf;
+	int ret;
+
+	if (!config_vector) {
+		IFCVF_INFO(pdev, "No config interrupt because of no vectors\n");
+		vf->config_irq = -EINVAL;
+		return 0;
+	}
+
+	snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
+		 pci_name(pdev));
+	vf->config_irq = pci_irq_vector(pdev, config_vector);
+	ret = devm_request_irq(&pdev->dev, vf->config_irq,
+			       ifcvf_config_changed, 0,
+			       vf->config_msix_name, vf);
+	if (ret) {
+		IFCVF_ERR(pdev, "Failed to request config irq\n");
+		return ret;
+	}
+		ifcvf_set_config_vector(vf, config_vector);
+
+	return 0;
+}
 
 static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
 {
-- 
2.27.0

Re: [PATCH 6/7] vDPA/ifcvf: implement config interrupt request helper

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:50PM +0800, Zhu Lingshan wrote:
> This commit implements new helper to request config interrupt by
> a given MSIX vector.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_base.c |  6 ------
>  drivers/vdpa/ifcvf/ifcvf_main.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index fc496d10cf8d..38f91dc6481f 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -330,12 +330,6 @@ static int ifcvf_hw_enable(struct ifcvf_hw *hw)
>  
>  	ifcvf = vf_to_adapter(hw);
>  	cfg = hw->common_cfg;
> -	ifc_iowrite16(IFCVF_MSI_CONFIG_OFF, &cfg->msix_config);
> -
> -	if (ifc_ioread16(&cfg->msix_config) == VIRTIO_MSI_NO_VECTOR) {
> -		IFCVF_ERR(ifcvf->pdev, "No msix vector for device config\n");
> -		return -EINVAL;
> -	}
>  
>  	for (i = 0; i < hw->nr_vring; i++) {
>  		if (!hw->vring[i].ready)


this drops code presumably breaking it. I guess follow up patch
restores it back? pls squash there.

> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index ce2fbc429fbe..414b5dfd04ca 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -162,6 +162,32 @@ static int ifcvf_request_vq_irq(struct ifcvf_adapter *adapter, u8 vector_per_vq)
>  	return ret;
>  }
>  
> +static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_vector)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct ifcvf_hw *vf = &adapter->vf;
> +	int ret;
> +
> +	if (!config_vector) {
> +		IFCVF_INFO(pdev, "No config interrupt because of no vectors\n");
> +		vf->config_irq = -EINVAL;
> +		return 0;
> +	}
> +
> +	snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
> +		 pci_name(pdev));
> +	vf->config_irq = pci_irq_vector(pdev, config_vector);
> +	ret = devm_request_irq(&pdev->dev, vf->config_irq,
> +			       ifcvf_config_changed, 0,
> +			       vf->config_msix_name, vf);
> +	if (ret) {
> +		IFCVF_ERR(pdev, "Failed to request config irq\n");
> +		return ret;
> +	}
> +		ifcvf_set_config_vector(vf, config_vector);
> +
> +	return 0;
> +}
>  

So this adds a static function with no caller, I guess the
caller is in the next patch - pls squash there.

>  static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>  {
> -- 
> 2.27.0

[PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Zhu Lingshan]

This commit expends irq requester abilities to handle per vq irq,
shared irq and config irq.

On some platforms, the device can not get enough vectors for every
virtqueue and config interrupt, the device needs to work under such
circumstances.

Normally a device can get enough vectors, so every virtqueue and
config interrupt can have its own vector/irq. If the total vector
number is less than all virtqueues + 1(config interrupt), all
virtqueues need to share a vector/irq and config interrupt is
enabled. If the total vector number < 2, all vitequeues share
a vector/irq, and config interrupt is disabled. Otherwise it will
fail if allocation for vectors fails.

This commit also made necessary chages to the irq cleaner to
free per vq irq/shared irq and config irq.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
 drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
 2 files changed, 38 insertions(+), 46 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 1d5431040d7d..1d0afb63f06c 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -27,8 +27,6 @@
 
 #define IFCVF_QUEUE_ALIGNMENT	PAGE_SIZE
 #define IFCVF_QUEUE_MAX		32768
-#define IFCVF_MSI_CONFIG_OFF	0
-#define IFCVF_MSI_QUEUE_OFF	1
 #define IFCVF_PCI_MAX_RESOURCE	6
 
 #define IFCVF_LM_CFG_SIZE		0x40
@@ -102,11 +100,13 @@ struct ifcvf_hw {
 	u8 notify_bar;
 	/* Notificaiton bar address */
 	void __iomem *notify_base;
+	u8 vector_per_vq;
+	u16 padding;
 	phys_addr_t notify_base_pa;
 	u32 notify_off_multiplier;
+	u32 dev_type;
 	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_QUEUES];
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 414b5dfd04ca..ec76e342bd7e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -17,6 +17,8 @@
 #define DRIVER_AUTHOR   "Intel Corporation"
 #define IFCVF_DRIVER_NAME       "ifcvf"
 
+static struct vdpa_config_ops ifc_vdpa_ops;
+
 static irqreturn_t ifcvf_config_changed(int irq, void *arg)
 {
 	struct ifcvf_hw *vf = arg;
@@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
 	struct ifcvf_hw *vf = &adapter->vf;
 	int i;
 
+	if (vf->vector_per_vq)
+		for (i = 0; i < queues; i++) {
+			devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
+			vf->vring[i].irq = -EINVAL;
+		}
+	else
+		devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
 
-	for (i = 0; i < queues; i++) {
-		devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
-		vf->vring[i].irq = -EINVAL;
+
+	if (vf->config_irq != -EINVAL) {
+		devm_free_irq(&pdev->dev, vf->config_irq, vf);
+		vf->config_irq = -EINVAL;
 	}
 
-	devm_free_irq(&pdev->dev, vf->config_irq, vf);
 	ifcvf_free_irq_vectors(pdev);
 }
 
@@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
 
 static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
 {
-	struct pci_dev *pdev = adapter->pdev;
 	struct ifcvf_hw *vf = &adapter->vf;
-	int vector, i, ret, irq;
-	u16 max_intr;
+	u16 nvectors, max_vectors;
+	int config_vector, ret;
 
-	/* all queues and config interrupt  */
-	max_intr = vf->nr_vring + 1;
+	nvectors = ifcvf_alloc_vectors(adapter);
+	if (nvectors < 0)
+		return nvectors;
 
-	ret = pci_alloc_irq_vectors(pdev, max_intr,
-				    max_intr, PCI_IRQ_MSIX);
-	if (ret < 0) {
-		IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
-		return ret;
-	}
+	vf->vector_per_vq = true;
+	max_vectors = vf->nr_vring + 1;
+	config_vector = vf->nr_vring;
 
-	snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
-		 pci_name(pdev));
-	vector = 0;
-	vf->config_irq = pci_irq_vector(pdev, vector);
-	ret = devm_request_irq(&pdev->dev, vf->config_irq,
-			       ifcvf_config_changed, 0,
-			       vf->config_msix_name, vf);
-	if (ret) {
-		IFCVF_ERR(pdev, "Failed to request config irq\n");
-		return ret;
+	if (nvectors < max_vectors) {
+		vf->vector_per_vq = false;
+		config_vector = 1;
+		ifc_vdpa_ops.get_vq_irq = NULL;
 	}
 
-	for (i = 0; i < vf->nr_vring; i++) {
-		snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
-			 pci_name(pdev), i);
-		vector = i + IFCVF_MSI_QUEUE_OFF;
-		irq = pci_irq_vector(pdev, vector);
-		ret = devm_request_irq(&pdev->dev, irq,
-				       ifcvf_intr_handler, 0,
-				       vf->vring[i].msix_name,
-				       &vf->vring[i]);
-		if (ret) {
-			IFCVF_ERR(pdev,
-				  "Failed to request irq for vq %d\n", i);
-			ifcvf_free_irq(adapter, i);
+	if (nvectors < 2)
+		config_vector = 0;
 
-			return ret;
-		}
+	ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
+	if (ret)
+		return ret;
 
-		vf->vring[i].irq = irq;
-	}
+	ret = ifcvf_request_config_irq(adapter, config_vector);
+
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
  * IFCVF currently does't have on-chip IOMMU, so not
  * implemented set_map()/dma_map()/dma_unmap()
  */
-static const struct vdpa_config_ops ifc_vdpa_ops = {
+static struct vdpa_config_ops ifc_vdpa_ops = {
 	.get_features	= ifcvf_vdpa_get_features,
 	.set_features	= ifcvf_vdpa_set_features,
 	.get_status	= ifcvf_vdpa_get_status,
-- 
2.27.0

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
> This commit expends irq requester abilities to handle per vq irq,
> shared irq and config irq.
> 
> On some platforms, the device can not get enough vectors for every
> virtqueue and config interrupt, the device needs to work under such
> circumstances.
> 
> Normally a device can get enough vectors, so every virtqueue and
> config interrupt can have its own vector/irq. If the total vector
> number is less than all virtqueues + 1(config interrupt), all
> virtqueues need to share a vector/irq and config interrupt is
> enabled. If the total vector number < 2, all vitequeues share
> a vector/irq, and config interrupt is disabled. Otherwise it will
> fail if allocation for vectors fails.
> 
> This commit also made necessary chages to the irq cleaner to
> free per vq irq/shared irq and config irq.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>


In this case, shouldn't you also check VIRTIO_PCI_ISR_CONFIG?
doing that will skip the need 


> ---
>  drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
>  drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
>  2 files changed, 38 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index 1d5431040d7d..1d0afb63f06c 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -27,8 +27,6 @@
>  
>  #define IFCVF_QUEUE_ALIGNMENT	PAGE_SIZE
>  #define IFCVF_QUEUE_MAX		32768
> -#define IFCVF_MSI_CONFIG_OFF	0
> -#define IFCVF_MSI_QUEUE_OFF	1
>  #define IFCVF_PCI_MAX_RESOURCE	6
>  
>  #define IFCVF_LM_CFG_SIZE		0x40
> @@ -102,11 +100,13 @@ struct ifcvf_hw {
>  	u8 notify_bar;
>  	/* Notificaiton bar address */
>  	void __iomem *notify_base;
> +	u8 vector_per_vq;
> +	u16 padding;

What is this padding doing?

>  	phys_addr_t notify_base_pa;
>  	u32 notify_off_multiplier;
> +	u32 dev_type;
>  	u64 req_features;
>  	u64 hw_features;
> -	u32 dev_type;

moving things around ... optimization? split out.

>  	struct virtio_pci_common_cfg __iomem *common_cfg;
>  	void __iomem *net_cfg;
>  	struct vring_info vring[IFCVF_MAX_QUEUES];
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 414b5dfd04ca..ec76e342bd7e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -17,6 +17,8 @@
>  #define DRIVER_AUTHOR   "Intel Corporation"
>  #define IFCVF_DRIVER_NAME       "ifcvf"
>  
> +static struct vdpa_config_ops ifc_vdpa_ops;
> +

there can be multiple devices thinkably.
reusing a global ops does not sound reasonable.


>  static irqreturn_t ifcvf_config_changed(int irq, void *arg)
>  {
>  	struct ifcvf_hw *vf = arg;
> @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
>  	struct ifcvf_hw *vf = &adapter->vf;
>  	int i;
>  
> +	if (vf->vector_per_vq)
> +		for (i = 0; i < queues; i++) {
> +			devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> +			vf->vring[i].irq = -EINVAL;
> +		}
> +	else
> +		devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
>  
> -	for (i = 0; i < queues; i++) {
> -		devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> -		vf->vring[i].irq = -EINVAL;
> +
> +	if (vf->config_irq != -EINVAL) {
> +		devm_free_irq(&pdev->dev, vf->config_irq, vf);
> +		vf->config_irq = -EINVAL;
>  	}

what about other error types?

>  
> -	devm_free_irq(&pdev->dev, vf->config_irq, vf);
>  	ifcvf_free_irq_vectors(pdev);
>  }
>  
> @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
>  
>  static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>  {
> -	struct pci_dev *pdev = adapter->pdev;
>  	struct ifcvf_hw *vf = &adapter->vf;
> -	int vector, i, ret, irq;
> -	u16 max_intr;
> +	u16 nvectors, max_vectors;
> +	int config_vector, ret;
>  
> -	/* all queues and config interrupt  */
> -	max_intr = vf->nr_vring + 1;
> +	nvectors = ifcvf_alloc_vectors(adapter);
> +	if (nvectors < 0)
> +		return nvectors;
>  
> -	ret = pci_alloc_irq_vectors(pdev, max_intr,
> -				    max_intr, PCI_IRQ_MSIX);
> -	if (ret < 0) {
> -		IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
> -		return ret;
> -	}
> +	vf->vector_per_vq = true;
> +	max_vectors = vf->nr_vring + 1;
> +	config_vector = vf->nr_vring;
>  
> -	snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
> -		 pci_name(pdev));
> -	vector = 0;
> -	vf->config_irq = pci_irq_vector(pdev, vector);
> -	ret = devm_request_irq(&pdev->dev, vf->config_irq,
> -			       ifcvf_config_changed, 0,
> -			       vf->config_msix_name, vf);
> -	if (ret) {
> -		IFCVF_ERR(pdev, "Failed to request config irq\n");
> -		return ret;
> +	if (nvectors < max_vectors) {
> +		vf->vector_per_vq = false;
> +		config_vector = 1;
> +		ifc_vdpa_ops.get_vq_irq = NULL;
>  	}
>  
> -	for (i = 0; i < vf->nr_vring; i++) {
> -		snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
> -			 pci_name(pdev), i);
> -		vector = i + IFCVF_MSI_QUEUE_OFF;
> -		irq = pci_irq_vector(pdev, vector);
> -		ret = devm_request_irq(&pdev->dev, irq,
> -				       ifcvf_intr_handler, 0,
> -				       vf->vring[i].msix_name,
> -				       &vf->vring[i]);
> -		if (ret) {
> -			IFCVF_ERR(pdev,
> -				  "Failed to request irq for vq %d\n", i);
> -			ifcvf_free_irq(adapter, i);
> +	if (nvectors < 2)
> +		config_vector = 0;
>  
> -			return ret;
> -		}
> +	ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
> +	if (ret)
> +		return ret;
>  
> -		vf->vring[i].irq = irq;
> -	}
> +	ret = ifcvf_request_config_irq(adapter, config_vector);
> +
> +	if (ret)
> +		return ret;

here on error we need to cleanup vq irq we requested, need we not?

>  
>  	return 0;
>  }
> @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>   * IFCVF currently does't have on-chip IOMMU, so not
>   * implemented set_map()/dma_map()/dma_unmap()
>   */
> -static const struct vdpa_config_ops ifc_vdpa_ops = {
> +static struct vdpa_config_ops ifc_vdpa_ops = {
>  	.get_features	= ifcvf_vdpa_get_features,
>  	.set_features	= ifcvf_vdpa_set_features,
>  	.get_status	= ifcvf_vdpa_get_status,
> -- 
> 2.27.0

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 04:17:29PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 1/11/2022 3:11 PM, Zhu, Lingshan wrote:
> 
> 
> 
>     On 1/10/2022 2:04 PM, Michael S. Tsirkin wrote:
> 
>         On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
> 
>             This commit expends irq requester abilities to handle per vq irq,
>             shared irq and config irq.
> 
>             On some platforms, the device can not get enough vectors for every
>             virtqueue and config interrupt, the device needs to work under such
>             circumstances.
> 
>             Normally a device can get enough vectors, so every virtqueue and
>             config interrupt can have its own vector/irq. If the total vector
>             number is less than all virtqueues + 1(config interrupt), all
>             virtqueues need to share a vector/irq and config interrupt is
>             enabled. If the total vector number < 2, all vitequeues share
>             a vector/irq, and config interrupt is disabled. Otherwise it will
>             fail if allocation for vectors fails.
> 
>             This commit also made necessary chages to the irq cleaner to
>             free per vq irq/shared irq and config irq.
> 
>             Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> 
>         In this case, shouldn't you also check VIRTIO_PCI_ISR_CONFIG?
>         doing that will skip the need
> 
>     Hello Michael,
> 
>     When insufficient MSIX vectors granted:
>     If num_vectors >=2, there will be a vector for the config interrupt, and all vqs share one vector.
>     If num_vectors =1, all vqs share the only one vector, and config interrupt is disabled.

ATM linux falls back to INTX in that case, shared by config and vqs.

> 
>     currently vqs and config interrupt don't share vectors, so IMHO, no need to check VIRTIO_PCI_ISR_CONFIG.

IMHO it does not matter much that current Linux drivers do not use it,
the spec explicitly allows this option. If such hardware
becomes more common (and you seem to want to improve support
for managing interrupts so maybe yes) we'll add it in Linux.

>     I will send a V2 patch address your comments.
> 
>     Thanks,
>     Zhu Lingshan
> 
>             ---
>              drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
>              drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
>              2 files changed, 38 insertions(+), 46 deletions(-)
> 
>             diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>             index 1d5431040d7d..1d0afb63f06c 100644
>             --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>             +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>             @@ -27,8 +27,6 @@
> 
>              #define IFCVF_QUEUE_ALIGNMENT  PAGE_SIZE
>              #define IFCVF_QUEUE_MAX                32768
>             -#define IFCVF_MSI_CONFIG_OFF   0
>             -#define IFCVF_MSI_QUEUE_OFF    1
>              #define IFCVF_PCI_MAX_RESOURCE 6
> 
>              #define IFCVF_LM_CFG_SIZE              0x40
>             @@ -102,11 +100,13 @@ struct ifcvf_hw {
>                     u8 notify_bar;
>                     /* Notificaiton bar address */
>                     void __iomem *notify_base;
>             +       u8 vector_per_vq;
>             +       u16 padding;
> 
>         What is this padding doing?
> 
> for cacheline alignment
> 
> 
> 
>                     phys_addr_t notify_base_pa;
>                     u32 notify_off_multiplier;
>             +       u32 dev_type;
>                     u64 req_features;
>                     u64 hw_features;
>             -       u32 dev_type;
> 
>         moving things around ... optimization? split out.
> 
> sure
> 
> 
> 
>                     struct virtio_pci_common_cfg __iomem *common_cfg;
>                     void __iomem *net_cfg;
>                     struct vring_info vring[IFCVF_MAX_QUEUES];
>             diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>             index 414b5dfd04ca..ec76e342bd7e 100644
>             --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>             +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>             @@ -17,6 +17,8 @@
>              #define DRIVER_AUTHOR   "Intel Corporation"
>              #define IFCVF_DRIVER_NAME       "ifcvf"
> 
>             +static struct vdpa_config_ops ifc_vdpa_ops;
>             +
> 
>         there can be multiple devices thinkably.
>         reusing a global ops does not sound reasonable.
> 
> OK, I will set vq irq number to -EINVAL when vqs share irq,
> then we can disable irq_bypass when see irq = -EINVAL,
> no need to set get_vq_irq = NULL.
> 
> 
> 
> 
>              static irqreturn_t ifcvf_config_changed(int irq, void *arg)
>              {
>                     struct ifcvf_hw *vf = arg;
>             @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
>                     struct ifcvf_hw *vf = &adapter->vf;
>                     int i;
> 
>             +       if (vf->vector_per_vq)
>             +               for (i = 0; i < queues; i++) {
>             +                       devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>             +                       vf->vring[i].irq = -EINVAL;
>             +               }
>             +       else
>             +               devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
> 
>             -       for (i = 0; i < queues; i++) {
>             -               devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>             -               vf->vring[i].irq = -EINVAL;
>             +
>             +       if (vf->config_irq != -EINVAL) {
>             +               devm_free_irq(&pdev->dev, vf->config_irq, vf);
>             +               vf->config_irq = -EINVAL;
>                     }
> 
>         what about other error types?
> 
> vf->config_irq is set to -EINVAL in ifcvf_request_config_irq(),
> if no config irq(vector) is granted, or it should be a valid irq number,
> so there can be no other error numbers. But I can change it
> to  if (vf->config_irq < 0) for sure
> 
> 
> 
> 
>             -       devm_free_irq(&pdev->dev, vf->config_irq, vf);
>                     ifcvf_free_irq_vectors(pdev);
>              }
> 
>             @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
> 
>              static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>              {
>             -       struct pci_dev *pdev = adapter->pdev;
>                     struct ifcvf_hw *vf = &adapter->vf;
>             -       int vector, i, ret, irq;
>             -       u16 max_intr;
>             +       u16 nvectors, max_vectors;
>             +       int config_vector, ret;
> 
>             -       /* all queues and config interrupt  */
>             -       max_intr = vf->nr_vring + 1;
>             +       nvectors = ifcvf_alloc_vectors(adapter);
>             +       if (nvectors < 0)
>             +               return nvectors;
> 
>             -       ret = pci_alloc_irq_vectors(pdev, max_intr,
>             -                                   max_intr, PCI_IRQ_MSIX);
>             -       if (ret < 0) {
>             -               IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
>             -               return ret;
>             -       }
>             +       vf->vector_per_vq = true;
>             +       max_vectors = vf->nr_vring + 1;
>             +       config_vector = vf->nr_vring;
> 
>             -       snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
>             -                pci_name(pdev));
>             -       vector = 0;
>             -       vf->config_irq = pci_irq_vector(pdev, vector);
>             -       ret = devm_request_irq(&pdev->dev, vf->config_irq,
>             -                              ifcvf_config_changed, 0,
>             -                              vf->config_msix_name, vf);
>             -       if (ret) {
>             -               IFCVF_ERR(pdev, "Failed to request config irq\n");
>             -               return ret;
>             +       if (nvectors < max_vectors) {
>             +               vf->vector_per_vq = false;
>             +               config_vector = 1;
>             +               ifc_vdpa_ops.get_vq_irq = NULL;
>                     }
> 
>             -       for (i = 0; i < vf->nr_vring; i++) {
>             -               snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
>             -                        pci_name(pdev), i);
>             -               vector = i + IFCVF_MSI_QUEUE_OFF;
>             -               irq = pci_irq_vector(pdev, vector);
>             -               ret = devm_request_irq(&pdev->dev, irq,
>             -                                      ifcvf_intr_handler, 0,
>             -                                      vf->vring[i].msix_name,
>             -                                      &vf->vring[i]);
>             -               if (ret) {
>             -                       IFCVF_ERR(pdev,
>             -                                 "Failed to request irq for vq %d\n", i);
>             -                       ifcvf_free_irq(adapter, i);
>             +       if (nvectors < 2)
>             +               config_vector = 0;
> 
>             -                       return ret;
>             -               }
>             +       ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
>             +       if (ret)
>             +               return ret;
> 
>             -               vf->vring[i].irq = irq;
>             -       }
>             +       ret = ifcvf_request_config_irq(adapter, config_vector);
>             +
>             +       if (ret)
>             +               return ret;
> 
>         here on error we need to cleanup vq irq we requested, need we not?
> 
> I think it may not be needed, it can work without config interrupt, though lame
> 
> Thanks for your comments!
> Zhu Lingshan
> 
> 
> 
> 
>                     return 0;
>              }
>             @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>               * IFCVF currently does't have on-chip IOMMU, so not
>               * implemented set_map()/dma_map()/dma_unmap()
>               */
>             -static const struct vdpa_config_ops ifc_vdpa_ops = {
>             +static struct vdpa_config_ops ifc_vdpa_ops = {
>                     .get_features   = ifcvf_vdpa_get_features,
>                     .set_features   = ifcvf_vdpa_set_features,
>                     .get_status     = ifcvf_vdpa_get_status,
>             --
>             2.27.0
> 
> 
> 
> 

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Zhu, Lingshan]

On 1/13/2022 5:52 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 13, 2022 at 04:17:29PM +0800, Zhu, Lingshan wrote:
>>
>> On 1/11/2022 3:11 PM, Zhu, Lingshan wrote:
>>
>>
>>
>>      On 1/10/2022 2:04 PM, Michael S. Tsirkin wrote:
>>
>>          On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
>>
>>              This commit expends irq requester abilities to handle per vq irq,
>>              shared irq and config irq.
>>
>>              On some platforms, the device can not get enough vectors for every
>>              virtqueue and config interrupt, the device needs to work under such
>>              circumstances.
>>
>>              Normally a device can get enough vectors, so every virtqueue and
>>              config interrupt can have its own vector/irq. If the total vector
>>              number is less than all virtqueues + 1(config interrupt), all
>>              virtqueues need to share a vector/irq and config interrupt is
>>              enabled. If the total vector number < 2, all vitequeues share
>>              a vector/irq, and config interrupt is disabled. Otherwise it will
>>              fail if allocation for vectors fails.
>>
>>              This commit also made necessary chages to the irq cleaner to
>>              free per vq irq/shared irq and config irq.
>>
>>              Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>
>>          In this case, shouldn't you also check VIRTIO_PCI_ISR_CONFIG?
>>          doing that will skip the need
>>
>>      Hello Michael,
>>
>>      When insufficient MSIX vectors granted:
>>      If num_vectors >=2, there will be a vector for the config interrupt, and all vqs share one vector.
>>      If num_vectors =1, all vqs share the only one vector, and config interrupt is disabled.
> ATM linux falls back to INTX in that case, shared by config and vqs.
Yes, the same result. However this driver needs to drive VFs too, and 
VFs do not support INTX,
so we need it to send msix dma.
>
>>      currently vqs and config interrupt don't share vectors, so IMHO, no need to check .
> IMHO it does not matter much that current Linux drivers do not use it,
> the spec explicitly allows this option. If such hardware
> becomes more common (and you seem to want to improve support
> for managing interrupts so maybe yes) we'll add it in Linux.
(just see your another email coming), so I think I should implement a 
irq handler
for num_vectors=1 case, a handler checks VIRTIO_PCI_ISR_CONFIG to tell 
whether
it is a vq interrupt or config interrupt, then handle it(not disabling 
config interrupt).

Thanks,
Zhu Lingshan
>
>>      I will send a V2 patch address your comments.
>>
>>      Thanks,
>>      Zhu Lingshan
>>
>>              ---
>>               drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
>>               drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
>>               2 files changed, 38 insertions(+), 46 deletions(-)
>>
>>              diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>>              index 1d5431040d7d..1d0afb63f06c 100644
>>              --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>              +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>              @@ -27,8 +27,6 @@
>>
>>               #define IFCVF_QUEUE_ALIGNMENT  PAGE_SIZE
>>               #define IFCVF_QUEUE_MAX                32768
>>              -#define IFCVF_MSI_CONFIG_OFF   0
>>              -#define IFCVF_MSI_QUEUE_OFF    1
>>               #define IFCVF_PCI_MAX_RESOURCE 6
>>
>>               #define IFCVF_LM_CFG_SIZE              0x40
>>              @@ -102,11 +100,13 @@ struct ifcvf_hw {
>>                      u8 notify_bar;
>>                      /* Notificaiton bar address */
>>                      void __iomem *notify_base;
>>              +       u8 vector_per_vq;
>>              +       u16 padding;
>>
>>          What is this padding doing?
>>
>> for cacheline alignment
>>
>>
>>
>>                      phys_addr_t notify_base_pa;
>>                      u32 notify_off_multiplier;
>>              +       u32 dev_type;
>>                      u64 req_features;
>>                      u64 hw_features;
>>              -       u32 dev_type;
>>
>>          moving things around ... optimization? split out.
>>
>> sure
>>
>>
>>
>>                      struct virtio_pci_common_cfg __iomem *common_cfg;
>>                      void __iomem *net_cfg;
>>                      struct vring_info vring[IFCVF_MAX_QUEUES];
>>              diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>>              index 414b5dfd04ca..ec76e342bd7e 100644
>>              --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>              +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>              @@ -17,6 +17,8 @@
>>               #define DRIVER_AUTHOR   "Intel Corporation"
>>               #define IFCVF_DRIVER_NAME       "ifcvf"
>>
>>              +static struct vdpa_config_ops ifc_vdpa_ops;
>>              +
>>
>>          there can be multiple devices thinkably.
>>          reusing a global ops does not sound reasonable.
>>
>> OK, I will set vq irq number to -EINVAL when vqs share irq,
>> then we can disable irq_bypass when see irq = -EINVAL,
>> no need to set get_vq_irq = NULL.
>>
>>
>>
>>
>>               static irqreturn_t ifcvf_config_changed(int irq, void *arg)
>>               {
>>                      struct ifcvf_hw *vf = arg;
>>              @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
>>                      struct ifcvf_hw *vf = &adapter->vf;
>>                      int i;
>>
>>              +       if (vf->vector_per_vq)
>>              +               for (i = 0; i < queues; i++) {
>>              +                       devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>>              +                       vf->vring[i].irq = -EINVAL;
>>              +               }
>>              +       else
>>              +               devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
>>
>>              -       for (i = 0; i < queues; i++) {
>>              -               devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>>              -               vf->vring[i].irq = -EINVAL;
>>              +
>>              +       if (vf->config_irq != -EINVAL) {
>>              +               devm_free_irq(&pdev->dev, vf->config_irq, vf);
>>              +               vf->config_irq = -EINVAL;
>>                      }
>>
>>          what about other error types?
>>
>> vf->config_irq is set to -EINVAL in ifcvf_request_config_irq(),
>> if no config irq(vector) is granted, or it should be a valid irq number,
>> so there can be no other error numbers. But I can change it
>> to  if (vf->config_irq < 0) for sure
>>
>>
>>
>>
>>              -       devm_free_irq(&pdev->dev, vf->config_irq, vf);
>>                      ifcvf_free_irq_vectors(pdev);
>>               }
>>
>>              @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
>>
>>               static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>>               {
>>              -       struct pci_dev *pdev = adapter->pdev;
>>                      struct ifcvf_hw *vf = &adapter->vf;
>>              -       int vector, i, ret, irq;
>>              -       u16 max_intr;
>>              +       u16 nvectors, max_vectors;
>>              +       int config_vector, ret;
>>
>>              -       /* all queues and config interrupt  */
>>              -       max_intr = vf->nr_vring + 1;
>>              +       nvectors = ifcvf_alloc_vectors(adapter);
>>              +       if (nvectors < 0)
>>              +               return nvectors;
>>
>>              -       ret = pci_alloc_irq_vectors(pdev, max_intr,
>>              -                                   max_intr, PCI_IRQ_MSIX);
>>              -       if (ret < 0) {
>>              -               IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
>>              -               return ret;
>>              -       }
>>              +       vf->vector_per_vq = true;
>>              +       max_vectors = vf->nr_vring + 1;
>>              +       config_vector = vf->nr_vring;
>>
>>              -       snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
>>              -                pci_name(pdev));
>>              -       vector = 0;
>>              -       vf->config_irq = pci_irq_vector(pdev, vector);
>>              -       ret = devm_request_irq(&pdev->dev, vf->config_irq,
>>              -                              ifcvf_config_changed, 0,
>>              -                              vf->config_msix_name, vf);
>>              -       if (ret) {
>>              -               IFCVF_ERR(pdev, "Failed to request config irq\n");
>>              -               return ret;
>>              +       if (nvectors < max_vectors) {
>>              +               vf->vector_per_vq = false;
>>              +               config_vector = 1;
>>              +               ifc_vdpa_ops.get_vq_irq = NULL;
>>                      }
>>
>>              -       for (i = 0; i < vf->nr_vring; i++) {
>>              -               snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
>>              -                        pci_name(pdev), i);
>>              -               vector = i + IFCVF_MSI_QUEUE_OFF;
>>              -               irq = pci_irq_vector(pdev, vector);
>>              -               ret = devm_request_irq(&pdev->dev, irq,
>>              -                                      ifcvf_intr_handler, 0,
>>              -                                      vf->vring[i].msix_name,
>>              -                                      &vf->vring[i]);
>>              -               if (ret) {
>>              -                       IFCVF_ERR(pdev,
>>              -                                 "Failed to request irq for vq %d\n", i);
>>              -                       ifcvf_free_irq(adapter, i);
>>              +       if (nvectors < 2)
>>              +               config_vector = 0;
>>
>>              -                       return ret;
>>              -               }
>>              +       ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
>>              +       if (ret)
>>              +               return ret;
>>
>>              -               vf->vring[i].irq = irq;
>>              -       }
>>              +       ret = ifcvf_request_config_irq(adapter, config_vector);
>>              +
>>              +       if (ret)
>>              +               return ret;
>>
>>          here on error we need to cleanup vq irq we requested, need we not?
>>
>> I think it may not be needed, it can work without config interrupt, though lame
>>
>> Thanks for your comments!
>> Zhu Lingshan
>>
>>
>>
>>
>>                      return 0;
>>               }
>>              @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>>                * IFCVF currently does't have on-chip IOMMU, so not
>>                * implemented set_map()/dma_map()/dma_unmap()
>>                */
>>              -static const struct vdpa_config_ops ifc_vdpa_ops = {
>>              +static struct vdpa_config_ops ifc_vdpa_ops = {
>>                      .get_features   = ifcvf_vdpa_get_features,
>>                      .set_features   = ifcvf_vdpa_set_features,
>>                      .get_status     = ifcvf_vdpa_get_status,
>>              --
>>              2.27.0
>>
>>
>>
>>

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 06:10:15PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 1/13/2022 5:52 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 13, 2022 at 04:17:29PM +0800, Zhu, Lingshan wrote:
> > > 
> > > On 1/11/2022 3:11 PM, Zhu, Lingshan wrote:
> > > 
> > > 
> > > 
> > >      On 1/10/2022 2:04 PM, Michael S. Tsirkin wrote:
> > > 
> > >          On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
> > > 
> > >              This commit expends irq requester abilities to handle per vq irq,
> > >              shared irq and config irq.
> > > 
> > >              On some platforms, the device can not get enough vectors for every
> > >              virtqueue and config interrupt, the device needs to work under such
> > >              circumstances.
> > > 
> > >              Normally a device can get enough vectors, so every virtqueue and
> > >              config interrupt can have its own vector/irq. If the total vector
> > >              number is less than all virtqueues + 1(config interrupt), all
> > >              virtqueues need to share a vector/irq and config interrupt is
> > >              enabled. If the total vector number < 2, all vitequeues share
> > >              a vector/irq, and config interrupt is disabled. Otherwise it will
> > >              fail if allocation for vectors fails.
> > > 
> > >              This commit also made necessary chages to the irq cleaner to
> > >              free per vq irq/shared irq and config irq.
> > > 
> > >              Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > 
> > >          In this case, shouldn't you also check VIRTIO_PCI_ISR_CONFIG?
> > >          doing that will skip the need
> > > 
> > >      Hello Michael,
> > > 
> > >      When insufficient MSIX vectors granted:
> > >      If num_vectors >=2, there will be a vector for the config interrupt, and all vqs share one vector.
> > >      If num_vectors =1, all vqs share the only one vector, and config interrupt is disabled.
> > ATM linux falls back to INTX in that case, shared by config and vqs.
> Yes, the same result. However this driver needs to drive VFs too, and VFs do
> not support INTX,
> so we need it to send msix dma.
> > 
> > >      currently vqs and config interrupt don't share vectors, so IMHO, no need to check .
> > IMHO it does not matter much that current Linux drivers do not use it,
> > the spec explicitly allows this option. If such hardware
> > becomes more common (and you seem to want to improve support
> > for managing interrupts so maybe yes) we'll add it in Linux.
> (just see your another email coming), so I think I should implement a irq
> handler
> for num_vectors=1 case, a handler checks VIRTIO_PCI_ISR_CONFIG to tell
> whether
> it is a vq interrupt or config interrupt, then handle it(not disabling
> config interrupt).
> 
> Thanks,
> Zhu Lingshan

Right. To be more exact, if status is bit 2 is not set you call
vq interrupt, if set you call both vq and config interrupt,
since with MSI only config interrupts have a status bit.

> > 
> > >      I will send a V2 patch address your comments.
> > > 
> > >      Thanks,
> > >      Zhu Lingshan
> > > 
> > >              ---
> > >               drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
> > >               drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
> > >               2 files changed, 38 insertions(+), 46 deletions(-)
> > > 
> > >              diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> > >              index 1d5431040d7d..1d0afb63f06c 100644
> > >              --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> > >              +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> > >              @@ -27,8 +27,6 @@
> > > 
> > >               #define IFCVF_QUEUE_ALIGNMENT  PAGE_SIZE
> > >               #define IFCVF_QUEUE_MAX                32768
> > >              -#define IFCVF_MSI_CONFIG_OFF   0
> > >              -#define IFCVF_MSI_QUEUE_OFF    1
> > >               #define IFCVF_PCI_MAX_RESOURCE 6
> > > 
> > >               #define IFCVF_LM_CFG_SIZE              0x40
> > >              @@ -102,11 +100,13 @@ struct ifcvf_hw {
> > >                      u8 notify_bar;
> > >                      /* Notificaiton bar address */
> > >                      void __iomem *notify_base;
> > >              +       u8 vector_per_vq;
> > >              +       u16 padding;
> > > 
> > >          What is this padding doing?
> > > 
> > > for cacheline alignment
> > > 
> > > 
> > > 
> > >                      phys_addr_t notify_base_pa;
> > >                      u32 notify_off_multiplier;
> > >              +       u32 dev_type;
> > >                      u64 req_features;
> > >                      u64 hw_features;
> > >              -       u32 dev_type;
> > > 
> > >          moving things around ... optimization? split out.
> > > 
> > > sure
> > > 
> > > 
> > > 
> > >                      struct virtio_pci_common_cfg __iomem *common_cfg;
> > >                      void __iomem *net_cfg;
> > >                      struct vring_info vring[IFCVF_MAX_QUEUES];
> > >              diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> > >              index 414b5dfd04ca..ec76e342bd7e 100644
> > >              --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> > >              +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> > >              @@ -17,6 +17,8 @@
> > >               #define DRIVER_AUTHOR   "Intel Corporation"
> > >               #define IFCVF_DRIVER_NAME       "ifcvf"
> > > 
> > >              +static struct vdpa_config_ops ifc_vdpa_ops;
> > >              +
> > > 
> > >          there can be multiple devices thinkably.
> > >          reusing a global ops does not sound reasonable.
> > > 
> > > OK, I will set vq irq number to -EINVAL when vqs share irq,
> > > then we can disable irq_bypass when see irq = -EINVAL,
> > > no need to set get_vq_irq = NULL.
> > > 
> > > 
> > > 
> > > 
> > >               static irqreturn_t ifcvf_config_changed(int irq, void *arg)
> > >               {
> > >                      struct ifcvf_hw *vf = arg;
> > >              @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
> > >                      struct ifcvf_hw *vf = &adapter->vf;
> > >                      int i;
> > > 
> > >              +       if (vf->vector_per_vq)
> > >              +               for (i = 0; i < queues; i++) {
> > >              +                       devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> > >              +                       vf->vring[i].irq = -EINVAL;
> > >              +               }
> > >              +       else
> > >              +               devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
> > > 
> > >              -       for (i = 0; i < queues; i++) {
> > >              -               devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> > >              -               vf->vring[i].irq = -EINVAL;
> > >              +
> > >              +       if (vf->config_irq != -EINVAL) {
> > >              +               devm_free_irq(&pdev->dev, vf->config_irq, vf);
> > >              +               vf->config_irq = -EINVAL;
> > >                      }
> > > 
> > >          what about other error types?
> > > 
> > > vf->config_irq is set to -EINVAL in ifcvf_request_config_irq(),
> > > if no config irq(vector) is granted, or it should be a valid irq number,
> > > so there can be no other error numbers. But I can change it
> > > to  if (vf->config_irq < 0) for sure
> > > 
> > > 
> > > 
> > > 
> > >              -       devm_free_irq(&pdev->dev, vf->config_irq, vf);
> > >                      ifcvf_free_irq_vectors(pdev);
> > >               }
> > > 
> > >              @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
> > > 
> > >               static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
> > >               {
> > >              -       struct pci_dev *pdev = adapter->pdev;
> > >                      struct ifcvf_hw *vf = &adapter->vf;
> > >              -       int vector, i, ret, irq;
> > >              -       u16 max_intr;
> > >              +       u16 nvectors, max_vectors;
> > >              +       int config_vector, ret;
> > > 
> > >              -       /* all queues and config interrupt  */
> > >              -       max_intr = vf->nr_vring + 1;
> > >              +       nvectors = ifcvf_alloc_vectors(adapter);
> > >              +       if (nvectors < 0)
> > >              +               return nvectors;
> > > 
> > >              -       ret = pci_alloc_irq_vectors(pdev, max_intr,
> > >              -                                   max_intr, PCI_IRQ_MSIX);
> > >              -       if (ret < 0) {
> > >              -               IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
> > >              -               return ret;
> > >              -       }
> > >              +       vf->vector_per_vq = true;
> > >              +       max_vectors = vf->nr_vring + 1;
> > >              +       config_vector = vf->nr_vring;
> > > 
> > >              -       snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
> > >              -                pci_name(pdev));
> > >              -       vector = 0;
> > >              -       vf->config_irq = pci_irq_vector(pdev, vector);
> > >              -       ret = devm_request_irq(&pdev->dev, vf->config_irq,
> > >              -                              ifcvf_config_changed, 0,
> > >              -                              vf->config_msix_name, vf);
> > >              -       if (ret) {
> > >              -               IFCVF_ERR(pdev, "Failed to request config irq\n");
> > >              -               return ret;
> > >              +       if (nvectors < max_vectors) {
> > >              +               vf->vector_per_vq = false;
> > >              +               config_vector = 1;
> > >              +               ifc_vdpa_ops.get_vq_irq = NULL;
> > >                      }
> > > 
> > >              -       for (i = 0; i < vf->nr_vring; i++) {
> > >              -               snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
> > >              -                        pci_name(pdev), i);
> > >              -               vector = i + IFCVF_MSI_QUEUE_OFF;
> > >              -               irq = pci_irq_vector(pdev, vector);
> > >              -               ret = devm_request_irq(&pdev->dev, irq,
> > >              -                                      ifcvf_intr_handler, 0,
> > >              -                                      vf->vring[i].msix_name,
> > >              -                                      &vf->vring[i]);
> > >              -               if (ret) {
> > >              -                       IFCVF_ERR(pdev,
> > >              -                                 "Failed to request irq for vq %d\n", i);
> > >              -                       ifcvf_free_irq(adapter, i);
> > >              +       if (nvectors < 2)
> > >              +               config_vector = 0;
> > > 
> > >              -                       return ret;
> > >              -               }
> > >              +       ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
> > >              +       if (ret)
> > >              +               return ret;
> > > 
> > >              -               vf->vring[i].irq = irq;
> > >              -       }
> > >              +       ret = ifcvf_request_config_irq(adapter, config_vector);
> > >              +
> > >              +       if (ret)
> > >              +               return ret;
> > > 
> > >          here on error we need to cleanup vq irq we requested, need we not?
> > > 
> > > I think it may not be needed, it can work without config interrupt, though lame
> > > 
> > > Thanks for your comments!
> > > Zhu Lingshan
> > > 
> > > 
> > > 
> > > 
> > >                      return 0;
> > >               }
> > >              @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
> > >                * IFCVF currently does't have on-chip IOMMU, so not
> > >                * implemented set_map()/dma_map()/dma_unmap()
> > >                */
> > >              -static const struct vdpa_config_ops ifc_vdpa_ops = {
> > >              +static struct vdpa_config_ops ifc_vdpa_ops = {
> > >                      .get_features   = ifcvf_vdpa_get_features,
> > >                      .set_features   = ifcvf_vdpa_set_features,
> > >                      .get_status     = ifcvf_vdpa_get_status,
> > >              --
> > >              2.27.0
> > > 
> > > 
> > > 
> > > 

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Michael S. Tsirkin]

On Fri, Jan 14, 2022 at 08:32:24PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 1/13/2022 6:29 PM, Michael S. Tsirkin wrote:
> 
>     On Thu, Jan 13, 2022 at 06:10:15PM +0800, Zhu, Lingshan wrote:
> 
> 
>         On 1/13/2022 5:52 PM, Michael S. Tsirkin wrote:
> 
>             On Thu, Jan 13, 2022 at 04:17:29PM +0800, Zhu, Lingshan wrote:
> 
>                 On 1/11/2022 3:11 PM, Zhu, Lingshan wrote:
> 
> 
> 
>                      On 1/10/2022 2:04 PM, Michael S. Tsirkin wrote:
> 
>                          On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
> 
>                              This commit expends irq requester abilities to handle per vq irq,
>                              shared irq and config irq.
> 
>                              On some platforms, the device can not get enough vectors for every
>                              virtqueue and config interrupt, the device needs to work under such
>                              circumstances.
> 
>                              Normally a device can get enough vectors, so every virtqueue and
>                              config interrupt can have its own vector/irq. If the total vector
>                              number is less than all virtqueues + 1(config interrupt), all
>                              virtqueues need to share a vector/irq and config interrupt is
>                              enabled. If the total vector number < 2, all vitequeues share
>                              a vector/irq, and config interrupt is disabled. Otherwise it will
>                              fail if allocation for vectors fails.
> 
>                              This commit also made necessary chages to the irq cleaner to
>                              free per vq irq/shared irq and config irq.
> 
>                              Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> 
>                          In this case, shouldn't you also check VIRTIO_PCI_ISR_CONFIG?
>                          doing that will skip the need
> 
>                      Hello Michael,
> 
>                      When insufficient MSIX vectors granted:
>                      If num_vectors >=2, there will be a vector for the config interrupt, and all vqs share one vector.
>                      If num_vectors =1, all vqs share the only one vector, and config interrupt is disabled.
> 
>             ATM linux falls back to INTX in that case, shared by config and vqs.
> 
>         Yes, the same result. However this driver needs to drive VFs too, and VFs do
>         not support INTX,
>         so we need it to send msix dma.
> 
>                      currently vqs and config interrupt don't share vectors, so IMHO, no need to check .
> 
>             IMHO it does not matter much that current Linux drivers do not use it,
>             the spec explicitly allows this option. If such hardware
>             becomes more common (and you seem to want to improve support
>             for managing interrupts so maybe yes) we'll add it in Linux.
> 
>         (just see your another email coming), so I think I should implement a irq
>         handler
>         for num_vectors=1 case, a handler checks VIRTIO_PCI_ISR_CONFIG to tell
>         whether
>         it is a vq interrupt or config interrupt, then handle it(not disabling
>         config interrupt).
> 
>         Thanks,
>         Zhu Lingshan
> 
>     Right. To be more exact, if status is bit 2 is not set you call
>     vq interrupt, if set you call both vq and config interrupt,
>     since with MSI only config interrupts have a status bit.
> 
> Thanks! I guess I should call both vq and config interrupt handlers when they
> share only one vector, because the spec says VIRTIO_PCI_CAP_ISR_CFG
> is for INTx, but VFs don't support INTx as SRIOV spec required, so
> isr may always be zero.
> 
> Thanks,
> Zhu Lingshan
> 

Yes. But on the other hand, the spec says:

The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­
ration change notification to the driver.
If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a
virtqueue notification to the driver.

which to me implies that the Device Configuration Interrupt bit
is set unconditionally.

And yes it says:
...to be used for INT#x interrupt handling
but it does not say "exclusively".

It is unfortunate that it does not copy this requirement in more places, but
I think that device does have to set Device Configuration Interrupt bit
unconditionally.


What exactly does ifcvf do? Does it ever trigger config change
interrupts? If it does, does it set the Device Configuration Interrupt
when MSI is used?

I will report a spec defect for the apparent inconsistency.

> 
>                      I will send a V2 patch address your comments.
> 
>                      Thanks,
>                      Zhu Lingshan
> 
>                              ---
>                               drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
>                               drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
>                               2 files changed, 38 insertions(+), 46 deletions(-)
> 
>                              diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>                              index 1d5431040d7d..1d0afb63f06c 100644
>                              --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>                              +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>                              @@ -27,8 +27,6 @@
> 
>                               #define IFCVF_QUEUE_ALIGNMENT  PAGE_SIZE
>                               #define IFCVF_QUEUE_MAX                32768
>                              -#define IFCVF_MSI_CONFIG_OFF   0
>                              -#define IFCVF_MSI_QUEUE_OFF    1
>                               #define IFCVF_PCI_MAX_RESOURCE 6
> 
>                               #define IFCVF_LM_CFG_SIZE              0x40
>                              @@ -102,11 +100,13 @@ struct ifcvf_hw {
>                                      u8 notify_bar;
>                                      /* Notificaiton bar address */
>                                      void __iomem *notify_base;
>                              +       u8 vector_per_vq;
>                              +       u16 padding;
> 
>                          What is this padding doing?
> 
>                 for cacheline alignment
> 
> 
> 
>                                      phys_addr_t notify_base_pa;
>                                      u32 notify_off_multiplier;
>                              +       u32 dev_type;
>                                      u64 req_features;
>                                      u64 hw_features;
>                              -       u32 dev_type;
> 
>                          moving things around ... optimization? split out.
> 
>                 sure
> 
> 
> 
>                                      struct virtio_pci_common_cfg __iomem *common_cfg;
>                                      void __iomem *net_cfg;
>                                      struct vring_info vring[IFCVF_MAX_QUEUES];
>                              diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>                              index 414b5dfd04ca..ec76e342bd7e 100644
>                              --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>                              +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>                              @@ -17,6 +17,8 @@
>                               #define DRIVER_AUTHOR   "Intel Corporation"
>                               #define IFCVF_DRIVER_NAME       "ifcvf"
> 
>                              +static struct vdpa_config_ops ifc_vdpa_ops;
>                              +
> 
>                          there can be multiple devices thinkably.
>                          reusing a global ops does not sound reasonable.
> 
>                 OK, I will set vq irq number to -EINVAL when vqs share irq,
>                 then we can disable irq_bypass when see irq = -EINVAL,
>                 no need to set get_vq_irq = NULL.
> 
> 
> 
> 
>                               static irqreturn_t ifcvf_config_changed(int irq, void *arg)
>                               {
>                                      struct ifcvf_hw *vf = arg;
>                              @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
>                                      struct ifcvf_hw *vf = &adapter->vf;
>                                      int i;
> 
>                              +       if (vf->vector_per_vq)
>                              +               for (i = 0; i < queues; i++) {
>                              +                       devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>                              +                       vf->vring[i].irq = -EINVAL;
>                              +               }
>                              +       else
>                              +               devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
> 
>                              -       for (i = 0; i < queues; i++) {
>                              -               devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>                              -               vf->vring[i].irq = -EINVAL;
>                              +
>                              +       if (vf->config_irq != -EINVAL) {
>                              +               devm_free_irq(&pdev->dev, vf->config_irq, vf);
>                              +               vf->config_irq = -EINVAL;
>                                      }
> 
>                          what about other error types?
> 
>                 vf->config_irq is set to -EINVAL in ifcvf_request_config_irq(),
>                 if no config irq(vector) is granted, or it should be a valid irq number,
>                 so there can be no other error numbers. But I can change it
>                 to  if (vf->config_irq < 0) for sure
> 
> 
> 
> 
>                              -       devm_free_irq(&pdev->dev, vf->config_irq, vf);
>                                      ifcvf_free_irq_vectors(pdev);
>                               }
> 
>                              @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
> 
>                               static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>                               {
>                              -       struct pci_dev *pdev = adapter->pdev;
>                                      struct ifcvf_hw *vf = &adapter->vf;
>                              -       int vector, i, ret, irq;
>                              -       u16 max_intr;
>                              +       u16 nvectors, max_vectors;
>                              +       int config_vector, ret;
> 
>                              -       /* all queues and config interrupt  */
>                              -       max_intr = vf->nr_vring + 1;
>                              +       nvectors = ifcvf_alloc_vectors(adapter);
>                              +       if (nvectors < 0)
>                              +               return nvectors;
> 
>                              -       ret = pci_alloc_irq_vectors(pdev, max_intr,
>                              -                                   max_intr, PCI_IRQ_MSIX);
>                              -       if (ret < 0) {
>                              -               IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
>                              -               return ret;
>                              -       }
>                              +       vf->vector_per_vq = true;
>                              +       max_vectors = vf->nr_vring + 1;
>                              +       config_vector = vf->nr_vring;
> 
>                              -       snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
>                              -                pci_name(pdev));
>                              -       vector = 0;
>                              -       vf->config_irq = pci_irq_vector(pdev, vector);
>                              -       ret = devm_request_irq(&pdev->dev, vf->config_irq,
>                              -                              ifcvf_config_changed, 0,
>                              -                              vf->config_msix_name, vf);
>                              -       if (ret) {
>                              -               IFCVF_ERR(pdev, "Failed to request config irq\n");
>                              -               return ret;
>                              +       if (nvectors < max_vectors) {
>                              +               vf->vector_per_vq = false;
>                              +               config_vector = 1;
>                              +               ifc_vdpa_ops.get_vq_irq = NULL;
>                                      }
> 
>                              -       for (i = 0; i < vf->nr_vring; i++) {
>                              -               snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
>                              -                        pci_name(pdev), i);
>                              -               vector = i + IFCVF_MSI_QUEUE_OFF;
>                              -               irq = pci_irq_vector(pdev, vector);
>                              -               ret = devm_request_irq(&pdev->dev, irq,
>                              -                                      ifcvf_intr_handler, 0,
>                              -                                      vf->vring[i].msix_name,
>                              -                                      &vf->vring[i]);
>                              -               if (ret) {
>                              -                       IFCVF_ERR(pdev,
>                              -                                 "Failed to request irq for vq %d\n", i);
>                              -                       ifcvf_free_irq(adapter, i);
>                              +       if (nvectors < 2)
>                              +               config_vector = 0;
> 
>                              -                       return ret;
>                              -               }
>                              +       ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
>                              +       if (ret)
>                              +               return ret;
> 
>                              -               vf->vring[i].irq = irq;
>                              -       }
>                              +       ret = ifcvf_request_config_irq(adapter, config_vector);
>                              +
>                              +       if (ret)
>                              +               return ret;
> 
>                          here on error we need to cleanup vq irq we requested, need we not?
> 
>                 I think it may not be needed, it can work without config interrupt, though lame
> 
>                 Thanks for your comments!
>                 Zhu Lingshan
> 
> 
> 
> 
>                                      return 0;
>                               }
>                              @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>                                * IFCVF currently does't have on-chip IOMMU, so not
>                                * implemented set_map()/dma_map()/dma_unmap()
>                                */
>                              -static const struct vdpa_config_ops ifc_vdpa_ops = {
>                              +static struct vdpa_config_ops ifc_vdpa_ops = {
>                                      .get_features   = ifcvf_vdpa_get_features,
>                                      .set_features   = ifcvf_vdpa_set_features,
>                                      .get_status     = ifcvf_vdpa_get_status,
>                              --
>                              2.27.0
> 
> 
> 
> 
> 
> 

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Zhu, Lingshan]

On 1/14/2022 9:36 PM, Michael S. Tsirkin wrote:
> On Fri, Jan 14, 2022 at 08:32:24PM +0800, Zhu, Lingshan wrote:
>>
>> On 1/13/2022 6:29 PM, Michael S. Tsirkin wrote:
>>
>>      On Thu, Jan 13, 2022 at 06:10:15PM +0800, Zhu, Lingshan wrote:
>>
>>
>>          On 1/13/2022 5:52 PM, Michael S. Tsirkin wrote:
>>
>>              On Thu, Jan 13, 2022 at 04:17:29PM +0800, Zhu, Lingshan wrote:
>>
>>                  On 1/11/2022 3:11 PM, Zhu, Lingshan wrote:
>>
>>
>>
>>                       On 1/10/2022 2:04 PM, Michael S. Tsirkin wrote:
>>
>>                           On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
>>
>>                               This commit expends irq requester abilities to handle per vq irq,
>>                               shared irq and config irq.
>>
>>                               On some platforms, the device can not get enough vectors for every
>>                               virtqueue and config interrupt, the device needs to work under such
>>                               circumstances.
>>
>>                               Normally a device can get enough vectors, so every virtqueue and
>>                               config interrupt can have its own vector/irq. If the total vector
>>                               number is less than all virtqueues + 1(config interrupt), all
>>                               virtqueues need to share a vector/irq and config interrupt is
>>                               enabled. If the total vector number < 2, all vitequeues share
>>                               a vector/irq, and config interrupt is disabled. Otherwise it will
>>                               fail if allocation for vectors fails.
>>
>>                               This commit also made necessary chages to the irq cleaner to
>>                               free per vq irq/shared irq and config irq.
>>
>>                               Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>
>>                           In this case, shouldn't you also check VIRTIO_PCI_ISR_CONFIG?
>>                           doing that will skip the need
>>
>>                       Hello Michael,
>>
>>                       When insufficient MSIX vectors granted:
>>                       If num_vectors >=2, there will be a vector for the config interrupt, and all vqs share one vector.
>>                       If num_vectors =1, all vqs share the only one vector, and config interrupt is disabled.
>>
>>              ATM linux falls back to INTX in that case, shared by config and vqs.
>>
>>          Yes, the same result. However this driver needs to drive VFs too, and VFs do
>>          not support INTX,
>>          so we need it to send msix dma.
>>
>>                       currently vqs and config interrupt don't share vectors, so IMHO, no need to check .
>>
>>              IMHO it does not matter much that current Linux drivers do not use it,
>>              the spec explicitly allows this option. If such hardware
>>              becomes more common (and you seem to want to improve support
>>              for managing interrupts so maybe yes) we'll add it in Linux.
>>
>>          (just see your another email coming), so I think I should implement a irq
>>          handler
>>          for num_vectors=1 case, a handler checks VIRTIO_PCI_ISR_CONFIG to tell
>>          whether
>>          it is a vq interrupt or config interrupt, then handle it(not disabling
>>          config interrupt).
>>
>>          Thanks,
>>          Zhu Lingshan
>>
>>      Right. To be more exact, if status is bit 2 is not set you call
>>      vq interrupt, if set you call both vq and config interrupt,
>>      since with MSI only config interrupts have a status bit.
>>
>> Thanks! I guess I should call both vq and config interrupt handlers when they
>> share only one vector, because the spec says VIRTIO_PCI_CAP_ISR_CFG
>> is for INTx, but VFs don't support INTx as SRIOV spec required, so
>> isr may always be zero.
>>
>> Thanks,
>> Zhu Lingshan
>>
> Yes. But on the other hand, the spec says:
>
> The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
> The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­
> ration change notification to the driver.
> If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a
> virtqueue notification to the driver.
>
> which to me implies that the Device Configuration Interrupt bit
> is set unconditionally.
>
> And yes it says:
> ...to be used for INT#x interrupt handling
> but it does not say "exclusively".
>
> It is unfortunate that it does not copy this requirement in more places, but
> I think that device does have to set Device Configuration Interrupt bit
> unconditionally.
sorry for the late reply, I totally agree on expanding ISR cap to 
MSI(and MSIX) usage.
>
>
> What exactly does ifcvf do? Does it ever trigger config change
> interrupts? If it does, does it set the Device Configuration Interrupt
> when MSI is used?
It triggers config interrupt upon config changes. However, the spec says:
"If MSI-X capability is enabled, the driver SHOULD NOT access ISR status 
upon detecting a Queue Interrupt.",
so for a VF(only MSIX, no INTx), when a vq interrupt is triggered, we 
see isr == 0;
So I think it would be nice to make slight changes to the spec: 
consistently describes ISR cap usage, and
remove this (ambiguous) limitation of ISR usage(which implies only for 
INTx).

For the driver which drivers both VF and PF, I think currently we should 
ignore ISR cap, means if the all device vqs and config interrupt
share the only one vector/IRQ, just kick them all. I agree we should 
work out a way to tell the device type in the future.
Does this sounds reasonable?

Thanks,
Zhu Lingshan
>
> I will report a spec defect for the apparent inconsistency.
>
>>                       I will send a V2 patch address your comments.
>>
>>                       Thanks,
>>                       Zhu Lingshan
>>
>>                               ---
>>                                drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
>>                                drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
>>                                2 files changed, 38 insertions(+), 46 deletions(-)
>>
>>                               diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
>>                               index 1d5431040d7d..1d0afb63f06c 100644
>>                               --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>                               +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>                               @@ -27,8 +27,6 @@
>>
>>                                #define IFCVF_QUEUE_ALIGNMENT  PAGE_SIZE
>>                                #define IFCVF_QUEUE_MAX                32768
>>                               -#define IFCVF_MSI_CONFIG_OFF   0
>>                               -#define IFCVF_MSI_QUEUE_OFF    1
>>                                #define IFCVF_PCI_MAX_RESOURCE 6
>>
>>                                #define IFCVF_LM_CFG_SIZE              0x40
>>                               @@ -102,11 +100,13 @@ struct ifcvf_hw {
>>                                       u8 notify_bar;
>>                                       /* Notificaiton bar address */
>>                                       void __iomem *notify_base;
>>                               +       u8 vector_per_vq;
>>                               +       u16 padding;
>>
>>                           What is this padding doing?
>>
>>                  for cacheline alignment
>>
>>
>>
>>                                       phys_addr_t notify_base_pa;
>>                                       u32 notify_off_multiplier;
>>                               +       u32 dev_type;
>>                                       u64 req_features;
>>                                       u64 hw_features;
>>                               -       u32 dev_type;
>>
>>                           moving things around ... optimization? split out.
>>
>>                  sure
>>
>>
>>
>>                                       struct virtio_pci_common_cfg __iomem *common_cfg;
>>                                       void __iomem *net_cfg;
>>                                       struct vring_info vring[IFCVF_MAX_QUEUES];
>>                               diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
>>                               index 414b5dfd04ca..ec76e342bd7e 100644
>>                               --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>                               +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>                               @@ -17,6 +17,8 @@
>>                                #define DRIVER_AUTHOR   "Intel Corporation"
>>                                #define IFCVF_DRIVER_NAME       "ifcvf"
>>
>>                               +static struct vdpa_config_ops ifc_vdpa_ops;
>>                               +
>>
>>                           there can be multiple devices thinkably.
>>                           reusing a global ops does not sound reasonable.
>>
>>                  OK, I will set vq irq number to -EINVAL when vqs share irq,
>>                  then we can disable irq_bypass when see irq = -EINVAL,
>>                  no need to set get_vq_irq = NULL.
>>
>>
>>
>>
>>                                static irqreturn_t ifcvf_config_changed(int irq, void *arg)
>>                                {
>>                                       struct ifcvf_hw *vf = arg;
>>                               @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
>>                                       struct ifcvf_hw *vf = &adapter->vf;
>>                                       int i;
>>
>>                               +       if (vf->vector_per_vq)
>>                               +               for (i = 0; i < queues; i++) {
>>                               +                       devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>>                               +                       vf->vring[i].irq = -EINVAL;
>>                               +               }
>>                               +       else
>>                               +               devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
>>
>>                               -       for (i = 0; i < queues; i++) {
>>                               -               devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
>>                               -               vf->vring[i].irq = -EINVAL;
>>                               +
>>                               +       if (vf->config_irq != -EINVAL) {
>>                               +               devm_free_irq(&pdev->dev, vf->config_irq, vf);
>>                               +               vf->config_irq = -EINVAL;
>>                                       }
>>
>>                           what about other error types?
>>
>>                  vf->config_irq is set to -EINVAL in ifcvf_request_config_irq(),
>>                  if no config irq(vector) is granted, or it should be a valid irq number,
>>                  so there can be no other error numbers. But I can change it
>>                  to  if (vf->config_irq < 0) for sure
>>
>>
>>
>>
>>                               -       devm_free_irq(&pdev->dev, vf->config_irq, vf);
>>                                       ifcvf_free_irq_vectors(pdev);
>>                                }
>>
>>                               @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
>>
>>                                static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>>                                {
>>                               -       struct pci_dev *pdev = adapter->pdev;
>>                                       struct ifcvf_hw *vf = &adapter->vf;
>>                               -       int vector, i, ret, irq;
>>                               -       u16 max_intr;
>>                               +       u16 nvectors, max_vectors;
>>                               +       int config_vector, ret;
>>
>>                               -       /* all queues and config interrupt  */
>>                               -       max_intr = vf->nr_vring + 1;
>>                               +       nvectors = ifcvf_alloc_vectors(adapter);
>>                               +       if (nvectors < 0)
>>                               +               return nvectors;
>>
>>                               -       ret = pci_alloc_irq_vectors(pdev, max_intr,
>>                               -                                   max_intr, PCI_IRQ_MSIX);
>>                               -       if (ret < 0) {
>>                               -               IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
>>                               -               return ret;
>>                               -       }
>>                               +       vf->vector_per_vq = true;
>>                               +       max_vectors = vf->nr_vring + 1;
>>                               +       config_vector = vf->nr_vring;
>>
>>                               -       snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
>>                               -                pci_name(pdev));
>>                               -       vector = 0;
>>                               -       vf->config_irq = pci_irq_vector(pdev, vector);
>>                               -       ret = devm_request_irq(&pdev->dev, vf->config_irq,
>>                               -                              ifcvf_config_changed, 0,
>>                               -                              vf->config_msix_name, vf);
>>                               -       if (ret) {
>>                               -               IFCVF_ERR(pdev, "Failed to request config irq\n");
>>                               -               return ret;
>>                               +       if (nvectors < max_vectors) {
>>                               +               vf->vector_per_vq = false;
>>                               +               config_vector = 1;
>>                               +               ifc_vdpa_ops.get_vq_irq = NULL;
>>                                       }
>>
>>                               -       for (i = 0; i < vf->nr_vring; i++) {
>>                               -               snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
>>                               -                        pci_name(pdev), i);
>>                               -               vector = i + IFCVF_MSI_QUEUE_OFF;
>>                               -               irq = pci_irq_vector(pdev, vector);
>>                               -               ret = devm_request_irq(&pdev->dev, irq,
>>                               -                                      ifcvf_intr_handler, 0,
>>                               -                                      vf->vring[i].msix_name,
>>                               -                                      &vf->vring[i]);
>>                               -               if (ret) {
>>                               -                       IFCVF_ERR(pdev,
>>                               -                                 "Failed to request irq for vq %d\n", i);
>>                               -                       ifcvf_free_irq(adapter, i);
>>                               +       if (nvectors < 2)
>>                               +               config_vector = 0;
>>
>>                               -                       return ret;
>>                               -               }
>>                               +       ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
>>                               +       if (ret)
>>                               +               return ret;
>>
>>                               -               vf->vring[i].irq = irq;
>>                               -       }
>>                               +       ret = ifcvf_request_config_irq(adapter, config_vector);
>>                               +
>>                               +       if (ret)
>>                               +               return ret;
>>
>>                           here on error we need to cleanup vq irq we requested, need we not?
>>
>>                  I think it may not be needed, it can work without config interrupt, though lame
>>
>>                  Thanks for your comments!
>>                  Zhu Lingshan
>>
>>
>>
>>
>>                                       return 0;
>>                                }
>>                               @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>>                                 * IFCVF currently does't have on-chip IOMMU, so not
>>                                 * implemented set_map()/dma_map()/dma_unmap()
>>                                 */
>>                               -static const struct vdpa_config_ops ifc_vdpa_ops = {
>>                               +static struct vdpa_config_ops ifc_vdpa_ops = {
>>                                       .get_features   = ifcvf_vdpa_get_features,
>>                                       .set_features   = ifcvf_vdpa_set_features,
>>                                       .get_status     = ifcvf_vdpa_get_status,
>>                               --
>>                               2.27.0
>>
>>
>>
>>
>>
>>

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Michael S. Tsirkin]

On Tue, Jan 18, 2022 at 11:07:52AM +0800, Zhu, Lingshan wrote:
> 
> 
> On 1/14/2022 9:36 PM, Michael S. Tsirkin wrote:
> > On Fri, Jan 14, 2022 at 08:32:24PM +0800, Zhu, Lingshan wrote:
> > > 
> > > On 1/13/2022 6:29 PM, Michael S. Tsirkin wrote:
> > > 
> > >      On Thu, Jan 13, 2022 at 06:10:15PM +0800, Zhu, Lingshan wrote:
> > > 
> > > 
> > >          On 1/13/2022 5:52 PM, Michael S. Tsirkin wrote:
> > > 
> > >              On Thu, Jan 13, 2022 at 04:17:29PM +0800, Zhu, Lingshan wrote:
> > > 
> > >                  On 1/11/2022 3:11 PM, Zhu, Lingshan wrote:
> > > 
> > > 
> > > 
> > >                       On 1/10/2022 2:04 PM, Michael S. Tsirkin wrote:
> > > 
> > >                           On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
> > > 
> > >                               This commit expends irq requester abilities to handle per vq irq,
> > >                               shared irq and config irq.
> > > 
> > >                               On some platforms, the device can not get enough vectors for every
> > >                               virtqueue and config interrupt, the device needs to work under such
> > >                               circumstances.
> > > 
> > >                               Normally a device can get enough vectors, so every virtqueue and
> > >                               config interrupt can have its own vector/irq. If the total vector
> > >                               number is less than all virtqueues + 1(config interrupt), all
> > >                               virtqueues need to share a vector/irq and config interrupt is
> > >                               enabled. If the total vector number < 2, all vitequeues share
> > >                               a vector/irq, and config interrupt is disabled. Otherwise it will
> > >                               fail if allocation for vectors fails.
> > > 
> > >                               This commit also made necessary chages to the irq cleaner to
> > >                               free per vq irq/shared irq and config irq.
> > > 
> > >                               Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > 
> > >                           In this case, shouldn't you also check VIRTIO_PCI_ISR_CONFIG?
> > >                           doing that will skip the need
> > > 
> > >                       Hello Michael,
> > > 
> > >                       When insufficient MSIX vectors granted:
> > >                       If num_vectors >=2, there will be a vector for the config interrupt, and all vqs share one vector.
> > >                       If num_vectors =1, all vqs share the only one vector, and config interrupt is disabled.
> > > 
> > >              ATM linux falls back to INTX in that case, shared by config and vqs.
> > > 
> > >          Yes, the same result. However this driver needs to drive VFs too, and VFs do
> > >          not support INTX,
> > >          so we need it to send msix dma.
> > > 
> > >                       currently vqs and config interrupt don't share vectors, so IMHO, no need to check .
> > > 
> > >              IMHO it does not matter much that current Linux drivers do not use it,
> > >              the spec explicitly allows this option. If such hardware
> > >              becomes more common (and you seem to want to improve support
> > >              for managing interrupts so maybe yes) we'll add it in Linux.
> > > 
> > >          (just see your another email coming), so I think I should implement a irq
> > >          handler
> > >          for num_vectors=1 case, a handler checks VIRTIO_PCI_ISR_CONFIG to tell
> > >          whether
> > >          it is a vq interrupt or config interrupt, then handle it(not disabling
> > >          config interrupt).
> > > 
> > >          Thanks,
> > >          Zhu Lingshan
> > > 
> > >      Right. To be more exact, if status is bit 2 is not set you call
> > >      vq interrupt, if set you call both vq and config interrupt,
> > >      since with MSI only config interrupts have a status bit.
> > > 
> > > Thanks! I guess I should call both vq and config interrupt handlers when they
> > > share only one vector, because the spec says VIRTIO_PCI_CAP_ISR_CFG
> > > is for INTx, but VFs don't support INTx as SRIOV spec required, so
> > > isr may always be zero.
> > > 
> > > Thanks,
> > > Zhu Lingshan
> > > 
> > Yes. But on the other hand, the spec says:
> > 
> > The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
> > The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­
> > ration change notification to the driver.
> > If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a
> > virtqueue notification to the driver.
> > 
> > which to me implies that the Device Configuration Interrupt bit
> > is set unconditionally.
> > 
> > And yes it says:
> > ...to be used for INT#x interrupt handling
> > but it does not say "exclusively".
> > 
> > It is unfortunate that it does not copy this requirement in more places, but
> > I think that device does have to set Device Configuration Interrupt bit
> > unconditionally.
> sorry for the late reply, I totally agree on expanding ISR cap to MSI(and
> MSIX) usage.
> > 
> > 
> > What exactly does ifcvf do? Does it ever trigger config change
> > interrupts? If it does, does it set the Device Configuration Interrupt
> > when MSI is used?
> It triggers config interrupt upon config changes. However, the spec says:
> "If MSI-X capability is enabled, the driver SHOULD NOT access ISR status
> upon detecting a Queue Interrupt.",
> so for a VF(only MSIX, no INTx), when a vq interrupt is triggered, we see
> isr == 0;

Exactly. So vq callback unconditionally invoked, config callback
only if ISR is set.

> So I think it would be nice to make slight changes to the spec: consistently
> describes ISR cap usage, and
> remove this (ambiguous) limitation of ISR usage(which implies only for
> INTx).
> 
> For the driver which drivers both VF and PF, I think currently we should
> ignore ISR cap, means if the all device vqs and config interrupt
> share the only one vector/IRQ, just kick them all. I agree we should work
> out a way to tell the device type in the future.
> Does this sounds reasonable?
> 
> Thanks,
> Zhu Lingshan
> > 
> > I will report a spec defect for the apparent inconsistency.
> > 
> > >                       I will send a V2 patch address your comments.
> > > 
> > >                       Thanks,
> > >                       Zhu Lingshan
> > > 
> > >                               ---
> > >                                drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
> > >                                drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
> > >                                2 files changed, 38 insertions(+), 46 deletions(-)
> > > 
> > >                               diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> > >                               index 1d5431040d7d..1d0afb63f06c 100644
> > >                               --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> > >                               +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> > >                               @@ -27,8 +27,6 @@
> > > 
> > >                                #define IFCVF_QUEUE_ALIGNMENT  PAGE_SIZE
> > >                                #define IFCVF_QUEUE_MAX                32768
> > >                               -#define IFCVF_MSI_CONFIG_OFF   0
> > >                               -#define IFCVF_MSI_QUEUE_OFF    1
> > >                                #define IFCVF_PCI_MAX_RESOURCE 6
> > > 
> > >                                #define IFCVF_LM_CFG_SIZE              0x40
> > >                               @@ -102,11 +100,13 @@ struct ifcvf_hw {
> > >                                       u8 notify_bar;
> > >                                       /* Notificaiton bar address */
> > >                                       void __iomem *notify_base;
> > >                               +       u8 vector_per_vq;
> > >                               +       u16 padding;
> > > 
> > >                           What is this padding doing?
> > > 
> > >                  for cacheline alignment
> > > 
> > > 
> > > 
> > >                                       phys_addr_t notify_base_pa;
> > >                                       u32 notify_off_multiplier;
> > >                               +       u32 dev_type;
> > >                                       u64 req_features;
> > >                                       u64 hw_features;
> > >                               -       u32 dev_type;
> > > 
> > >                           moving things around ... optimization? split out.
> > > 
> > >                  sure
> > > 
> > > 
> > > 
> > >                                       struct virtio_pci_common_cfg __iomem *common_cfg;
> > >                                       void __iomem *net_cfg;
> > >                                       struct vring_info vring[IFCVF_MAX_QUEUES];
> > >                               diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> > >                               index 414b5dfd04ca..ec76e342bd7e 100644
> > >                               --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> > >                               +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> > >                               @@ -17,6 +17,8 @@
> > >                                #define DRIVER_AUTHOR   "Intel Corporation"
> > >                                #define IFCVF_DRIVER_NAME       "ifcvf"
> > > 
> > >                               +static struct vdpa_config_ops ifc_vdpa_ops;
> > >                               +
> > > 
> > >                           there can be multiple devices thinkably.
> > >                           reusing a global ops does not sound reasonable.
> > > 
> > >                  OK, I will set vq irq number to -EINVAL when vqs share irq,
> > >                  then we can disable irq_bypass when see irq = -EINVAL,
> > >                  no need to set get_vq_irq = NULL.
> > > 
> > > 
> > > 
> > > 
> > >                                static irqreturn_t ifcvf_config_changed(int irq, void *arg)
> > >                                {
> > >                                       struct ifcvf_hw *vf = arg;
> > >                               @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
> > >                                       struct ifcvf_hw *vf = &adapter->vf;
> > >                                       int i;
> > > 
> > >                               +       if (vf->vector_per_vq)
> > >                               +               for (i = 0; i < queues; i++) {
> > >                               +                       devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> > >                               +                       vf->vring[i].irq = -EINVAL;
> > >                               +               }
> > >                               +       else
> > >                               +               devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
> > > 
> > >                               -       for (i = 0; i < queues; i++) {
> > >                               -               devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> > >                               -               vf->vring[i].irq = -EINVAL;
> > >                               +
> > >                               +       if (vf->config_irq != -EINVAL) {
> > >                               +               devm_free_irq(&pdev->dev, vf->config_irq, vf);
> > >                               +               vf->config_irq = -EINVAL;
> > >                                       }
> > > 
> > >                           what about other error types?
> > > 
> > >                  vf->config_irq is set to -EINVAL in ifcvf_request_config_irq(),
> > >                  if no config irq(vector) is granted, or it should be a valid irq number,
> > >                  so there can be no other error numbers. But I can change it
> > >                  to  if (vf->config_irq < 0) for sure
> > > 
> > > 
> > > 
> > > 
> > >                               -       devm_free_irq(&pdev->dev, vf->config_irq, vf);
> > >                                       ifcvf_free_irq_vectors(pdev);
> > >                                }
> > > 
> > >                               @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
> > > 
> > >                                static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
> > >                                {
> > >                               -       struct pci_dev *pdev = adapter->pdev;
> > >                                       struct ifcvf_hw *vf = &adapter->vf;
> > >                               -       int vector, i, ret, irq;
> > >                               -       u16 max_intr;
> > >                               +       u16 nvectors, max_vectors;
> > >                               +       int config_vector, ret;
> > > 
> > >                               -       /* all queues and config interrupt  */
> > >                               -       max_intr = vf->nr_vring + 1;
> > >                               +       nvectors = ifcvf_alloc_vectors(adapter);
> > >                               +       if (nvectors < 0)
> > >                               +               return nvectors;
> > > 
> > >                               -       ret = pci_alloc_irq_vectors(pdev, max_intr,
> > >                               -                                   max_intr, PCI_IRQ_MSIX);
> > >                               -       if (ret < 0) {
> > >                               -               IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
> > >                               -               return ret;
> > >                               -       }
> > >                               +       vf->vector_per_vq = true;
> > >                               +       max_vectors = vf->nr_vring + 1;
> > >                               +       config_vector = vf->nr_vring;
> > > 
> > >                               -       snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
> > >                               -                pci_name(pdev));
> > >                               -       vector = 0;
> > >                               -       vf->config_irq = pci_irq_vector(pdev, vector);
> > >                               -       ret = devm_request_irq(&pdev->dev, vf->config_irq,
> > >                               -                              ifcvf_config_changed, 0,
> > >                               -                              vf->config_msix_name, vf);
> > >                               -       if (ret) {
> > >                               -               IFCVF_ERR(pdev, "Failed to request config irq\n");
> > >                               -               return ret;
> > >                               +       if (nvectors < max_vectors) {
> > >                               +               vf->vector_per_vq = false;
> > >                               +               config_vector = 1;
> > >                               +               ifc_vdpa_ops.get_vq_irq = NULL;
> > >                                       }
> > > 
> > >                               -       for (i = 0; i < vf->nr_vring; i++) {
> > >                               -               snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
> > >                               -                        pci_name(pdev), i);
> > >                               -               vector = i + IFCVF_MSI_QUEUE_OFF;
> > >                               -               irq = pci_irq_vector(pdev, vector);
> > >                               -               ret = devm_request_irq(&pdev->dev, irq,
> > >                               -                                      ifcvf_intr_handler, 0,
> > >                               -                                      vf->vring[i].msix_name,
> > >                               -                                      &vf->vring[i]);
> > >                               -               if (ret) {
> > >                               -                       IFCVF_ERR(pdev,
> > >                               -                                 "Failed to request irq for vq %d\n", i);
> > >                               -                       ifcvf_free_irq(adapter, i);
> > >                               +       if (nvectors < 2)
> > >                               +               config_vector = 0;
> > > 
> > >                               -                       return ret;
> > >                               -               }
> > >                               +       ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
> > >                               +       if (ret)
> > >                               +               return ret;
> > > 
> > >                               -               vf->vring[i].irq = irq;
> > >                               -       }
> > >                               +       ret = ifcvf_request_config_irq(adapter, config_vector);
> > >                               +
> > >                               +       if (ret)
> > >                               +               return ret;
> > > 
> > >                           here on error we need to cleanup vq irq we requested, need we not?
> > > 
> > >                  I think it may not be needed, it can work without config interrupt, though lame
> > > 
> > >                  Thanks for your comments!
> > >                  Zhu Lingshan
> > > 
> > > 
> > > 
> > > 
> > >                                       return 0;
> > >                                }
> > >                               @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
> > >                                 * IFCVF currently does't have on-chip IOMMU, so not
> > >                                 * implemented set_map()/dma_map()/dma_unmap()
> > >                                 */
> > >                               -static const struct vdpa_config_ops ifc_vdpa_ops = {
> > >                               +static struct vdpa_config_ops ifc_vdpa_ops = {
> > >                                       .get_features   = ifcvf_vdpa_get_features,
> > >                                       .set_features   = ifcvf_vdpa_set_features,
> > >                                       .get_status     = ifcvf_vdpa_get_status,
> > >                               --
> > >                               2.27.0
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 

Re: [PATCH 7/7] vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:51PM +0800, Zhu Lingshan wrote:
> This commit expends irq requester abilities to handle per vq irq,
> shared irq and config irq.
> 
> On some platforms, the device can not get enough vectors for every
> virtqueue and config interrupt, the device needs to work under such
> circumstances.
> 
> Normally a device can get enough vectors, so every virtqueue and
> config interrupt can have its own vector/irq. If the total vector
> number is less than all virtqueues + 1(config interrupt), all
> virtqueues need to share a vector/irq and config interrupt is
> enabled. If the total vector number < 2, all vitequeues share
> a vector/irq, and config interrupt is disabled.

disabling config interrupt breaks link status updates and announcement
support. looks like link will never go up.

> Otherwise it will
> fail if allocation for vectors fails.
> 
> This commit also made necessary chages to the irq cleaner to
> free per vq irq/shared irq and config irq.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/ifcvf/ifcvf_base.h |  6 +--
>  drivers/vdpa/ifcvf/ifcvf_main.c | 78 +++++++++++++++------------------
>  2 files changed, 38 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
> index 1d5431040d7d..1d0afb63f06c 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
> @@ -27,8 +27,6 @@
>  
>  #define IFCVF_QUEUE_ALIGNMENT	PAGE_SIZE
>  #define IFCVF_QUEUE_MAX		32768
> -#define IFCVF_MSI_CONFIG_OFF	0
> -#define IFCVF_MSI_QUEUE_OFF	1
>  #define IFCVF_PCI_MAX_RESOURCE	6
>  
>  #define IFCVF_LM_CFG_SIZE		0x40
> @@ -102,11 +100,13 @@ struct ifcvf_hw {
>  	u8 notify_bar;
>  	/* Notificaiton bar address */
>  	void __iomem *notify_base;
> +	u8 vector_per_vq;
> +	u16 padding;
>  	phys_addr_t notify_base_pa;
>  	u32 notify_off_multiplier;
> +	u32 dev_type;
>  	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_QUEUES];
> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> index 414b5dfd04ca..ec76e342bd7e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -17,6 +17,8 @@
>  #define DRIVER_AUTHOR   "Intel Corporation"
>  #define IFCVF_DRIVER_NAME       "ifcvf"
>  
> +static struct vdpa_config_ops ifc_vdpa_ops;
> +
>  static irqreturn_t ifcvf_config_changed(int irq, void *arg)
>  {
>  	struct ifcvf_hw *vf = arg;
> @@ -63,13 +65,20 @@ static void ifcvf_free_irq(struct ifcvf_adapter *adapter, int queues)
>  	struct ifcvf_hw *vf = &adapter->vf;
>  	int i;
>  
> +	if (vf->vector_per_vq)
> +		for (i = 0; i < queues; i++) {
> +			devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> +			vf->vring[i].irq = -EINVAL;
> +		}
> +	else
> +		devm_free_irq(&pdev->dev, vf->vring[0].irq, vf);
>  
> -	for (i = 0; i < queues; i++) {
> -		devm_free_irq(&pdev->dev, vf->vring[i].irq, &vf->vring[i]);
> -		vf->vring[i].irq = -EINVAL;
> +
> +	if (vf->config_irq != -EINVAL) {
> +		devm_free_irq(&pdev->dev, vf->config_irq, vf);
> +		vf->config_irq = -EINVAL;
>  	}
>  
> -	devm_free_irq(&pdev->dev, vf->config_irq, vf);
>  	ifcvf_free_irq_vectors(pdev);
>  }
>  
> @@ -191,52 +200,35 @@ static int ifcvf_request_config_irq(struct ifcvf_adapter *adapter, int config_ve
>  
>  static int ifcvf_request_irq(struct ifcvf_adapter *adapter)
>  {
> -	struct pci_dev *pdev = adapter->pdev;
>  	struct ifcvf_hw *vf = &adapter->vf;
> -	int vector, i, ret, irq;
> -	u16 max_intr;
> +	u16 nvectors, max_vectors;
> +	int config_vector, ret;
>  
> -	/* all queues and config interrupt  */
> -	max_intr = vf->nr_vring + 1;
> +	nvectors = ifcvf_alloc_vectors(adapter);
> +	if (nvectors < 0)
> +		return nvectors;
>  
> -	ret = pci_alloc_irq_vectors(pdev, max_intr,
> -				    max_intr, PCI_IRQ_MSIX);
> -	if (ret < 0) {
> -		IFCVF_ERR(pdev, "Failed to alloc IRQ vectors\n");
> -		return ret;
> -	}
> +	vf->vector_per_vq = true;
> +	max_vectors = vf->nr_vring + 1;
> +	config_vector = vf->nr_vring;
>  
> -	snprintf(vf->config_msix_name, 256, "ifcvf[%s]-config\n",
> -		 pci_name(pdev));
> -	vector = 0;
> -	vf->config_irq = pci_irq_vector(pdev, vector);
> -	ret = devm_request_irq(&pdev->dev, vf->config_irq,
> -			       ifcvf_config_changed, 0,
> -			       vf->config_msix_name, vf);
> -	if (ret) {
> -		IFCVF_ERR(pdev, "Failed to request config irq\n");
> -		return ret;
> +	if (nvectors < max_vectors) {
> +		vf->vector_per_vq = false;
> +		config_vector = 1;
> +		ifc_vdpa_ops.get_vq_irq = NULL;
>  	}
>  
> -	for (i = 0; i < vf->nr_vring; i++) {
> -		snprintf(vf->vring[i].msix_name, 256, "ifcvf[%s]-%d\n",
> -			 pci_name(pdev), i);
> -		vector = i + IFCVF_MSI_QUEUE_OFF;
> -		irq = pci_irq_vector(pdev, vector);
> -		ret = devm_request_irq(&pdev->dev, irq,
> -				       ifcvf_intr_handler, 0,
> -				       vf->vring[i].msix_name,
> -				       &vf->vring[i]);
> -		if (ret) {
> -			IFCVF_ERR(pdev,
> -				  "Failed to request irq for vq %d\n", i);
> -			ifcvf_free_irq(adapter, i);
> +	if (nvectors < 2)
> +		config_vector = 0;
>  
> -			return ret;
> -		}
> +	ret = ifcvf_request_vq_irq(adapter, vf->vector_per_vq);
> +	if (ret)
> +		return ret;
>  
> -		vf->vring[i].irq = irq;
> -	}
> +	ret = ifcvf_request_config_irq(adapter, config_vector);
> +
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }
> @@ -573,7 +565,7 @@ static struct vdpa_notification_area ifcvf_get_vq_notification(struct vdpa_devic
>   * IFCVF currently does't have on-chip IOMMU, so not
>   * implemented set_map()/dma_map()/dma_unmap()
>   */
> -static const struct vdpa_config_ops ifc_vdpa_ops = {
> +static struct vdpa_config_ops ifc_vdpa_ops = {
>  	.get_features	= ifcvf_vdpa_get_features,
>  	.set_features	= ifcvf_vdpa_set_features,
>  	.get_status	= ifcvf_vdpa_get_status,
> -- 
> 2.27.0

Re: [PATCH 0/7] Supoort shared irq for virtqueues

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:44PM +0800, Zhu Lingshan wrote:
> On some platforms, it has been observed that a device may fail to
> allocate enough MSI-X vectors, under such circumstances, the vqs have
> to share a irq/vector.
> 
> This series extends irq requester/handlers abilities to deal with:
> (granted nvectors, and max_intr = total vq number + 1(config interrupt) )
> 
> 1)nvectors = max_intr: each vq has its own vector/irq,
> config interrupt is enabled, normal case
> 2)max_intr > nvectors >= 2: vqs share one irq/vector, config interrupt is
> enabled
> 3)nvectors = 1, vqs share one irq/vector, config interrupt is disabled.
> Otherwise it fails.
> 
> This series also made necessary changes to irq cleaners and related
> helpers.
> 
> Pleaase help reivew.


A bunch of typos in commit logs, please spell-check them.

> Thanks!
> Zhu Lingshan
> 
> Zhu Lingshan (7):
>   vDPA/ifcvf: implement IO read/write helpers in the header file
>   vDPA/ifcvf: introduce new helpers to set config vector and vq vectors
>   vDPA/ifcvf: implement device MSIX vector allocation helper
>   vDPA/ifcvf: implement shared irq handlers for vqs
>   vDPA/ifcvf: irq request helpers for both shared and per_vq irq
>   vDPA/ifcvf: implement config interrupt request helper
>   vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq
> 
>  drivers/vdpa/ifcvf/ifcvf_base.c |  65 ++++--------
>  drivers/vdpa/ifcvf/ifcvf_base.h |  45 +++++++-
>  drivers/vdpa/ifcvf/ifcvf_main.c | 179 +++++++++++++++++++++++++++-----
>  3 files changed, 215 insertions(+), 74 deletions(-)
> 
> -- 
> 2.27.0

Re: [PATCH 0/7] Supoort shared irq for virtqueues

[Michael S. Tsirkin]

On Mon, Jan 10, 2022 at 01:18:44PM +0800, Zhu Lingshan wrote:
> On some platforms, it has been observed that a device may fail to
> allocate enough MSI-X vectors, under such circumstances, the vqs have
> to share a irq/vector.
> 
> This series extends irq requester/handlers abilities to deal with:
> (granted nvectors, and max_intr = total vq number + 1(config interrupt) )
> 
> 1)nvectors = max_intr: each vq has its own vector/irq,
> config interrupt is enabled, normal case
> 2)max_intr > nvectors >= 2: vqs share one irq/vector, config interrupt is
> enabled
> 3)nvectors = 1, vqs share one irq/vector, config interrupt is disabled.
> Otherwise it fails.
> 
> This series also made necessary changes to irq cleaners and related
> helpers.


BTW you should copy the virtio mailing list, too.
Not just netdev.

> Pleaase help reivew.
> 
> Thanks!
> Zhu Lingshan
> 
> Zhu Lingshan (7):
>   vDPA/ifcvf: implement IO read/write helpers in the header file
>   vDPA/ifcvf: introduce new helpers to set config vector and vq vectors
>   vDPA/ifcvf: implement device MSIX vector allocation helper
>   vDPA/ifcvf: implement shared irq handlers for vqs
>   vDPA/ifcvf: irq request helpers for both shared and per_vq irq
>   vDPA/ifcvf: implement config interrupt request helper
>   vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq
> 
>  drivers/vdpa/ifcvf/ifcvf_base.c |  65 ++++--------
>  drivers/vdpa/ifcvf/ifcvf_base.h |  45 +++++++-
>  drivers/vdpa/ifcvf/ifcvf_main.c | 179 +++++++++++++++++++++++++++-----
>  3 files changed, 215 insertions(+), 74 deletions(-)
> 
> -- 
> 2.27.0

[PATCH 0/7] Supoort shared irq for virtqueues

[Zhu Lingshan]

On some platforms, it has been observed that a device may fail to
allocate enough MSI-X vectors, under such circumstances, the vqs have
to share a irq/vector.

This series extends irq requester/handlers abilities to deal with:
(granted nvectors, and max_intr = total vq number + 1(config interrupt) )

1)nvectors = max_intr: each vq has its own vector/irq,
config interrupt is enabled, normal case
2)max_intr > nvectors >= 2: vqs share one irq/vector, config interrupt is
enabled
3)nvectors = 1, vqs share one irq/vector, config interrupt is disabled.
Otherwise it fails.

This series also made necessary changes to irq cleaners and related
helpers.

Pleaase help reivew.

Thanks!
Zhu Lingshan

Zhu Lingshan (7):
  vDPA/ifcvf: implement IO read/write helpers in the header file
  vDPA/ifcvf: introduce new helpers to set config vector and vq vectors
  vDPA/ifcvf: implement device MSIX vector allocation helper
  vDPA/ifcvf: implement shared irq handlers for vqs
  vDPA/ifcvf: irq request helpers for both shared and per_vq irq
  vDPA/ifcvf: implement config interrupt request helper
  vDPA/ifcvf: improve irq requester, to handle per_vq/shared/config irq

 drivers/vdpa/ifcvf/ifcvf_base.c |  65 ++++--------
 drivers/vdpa/ifcvf/ifcvf_base.h |  45 +++++++-
 drivers/vdpa/ifcvf/ifcvf_main.c | 179 +++++++++++++++++++++++++++-----
 3 files changed, 215 insertions(+), 74 deletions(-)

-- 
2.27.0