PCI: dwc: ep: Cache MSI outbound iATU mapping

From: Koichiro Den <[email protected]>

dw_pcie_ep_raise_msi_irq() currently programs an outbound iATU window for the MSI target address on every interrupt and tears it down again via dw_pcie_ep_unmap_addr().

On systems that heavily use the AXI bridge interface (for example when the integrated eDMA engine is active), this means the outbound iATU registers are updated while traffic is in flight. The DesignWare endpoint databook 5.40a - "3.10.6.1 iATU Outbound Programming Overview" warns that updating iATU registers in this situation is not supported, and the behavior is undefined.

Under high MSI and eDMA load this pattern results in occasional bogus outbound transactions and IOMMU faults, on the RC side, such as:

   ipmmu-vmsa eed40000.iommu: Unhandled fault: status 0x00001502 iova 0xfe000000

followed by the system becoming unresponsive. This is the actual output observed on Renesas R-Car S4, with its ipmmu_hc used with PCIe ch0.

There is no need to reprogram the iATU region used for MSI on every interrupt. The host-provided MSI address is stable while MSI is enabled, and the endpoint driver already dedicates a scratch buffer for MSI generation.

Cache the aligned MSI address and map size, program the outbound iATU once, and keep the window enabled. Subsequent interrupts only perform a write to the MSI scratch buffer, avoiding dynamic iATU reprogramming in the hot path and fixing the lockups seen under load.

Signed-off-by Koichiro Den <[email protected]>

[cassel: do same change for dw_pcie_ep_raise_msix_irq()]

Signed-off-by Niklas Cassel <[email protected]>

---
Notes: Without this patch, I also see IOMMU errors on the host side, when running drivers/nvme/target/pci-epf.c with a large queue depth.

pci_epc_raise_irq() does take a mutex, so the calls to these functions are serialized, so it really appears that the DWC controller does not handle iATU reprogramming while there are outstanding transactions.

Just like Koichiro describes here:
https://lore.kernel.org/linux-pci/ddriorsgyjs6klcb6d7pi2u3ah3wxlzku7v2dpyjlo6tmalvfw@yj5dczlkggt6/

I also see the iova faulting on the RC side to be the start of "addr_space" on the EP, so it appears that a transaction has gone through untranslated. (Most likely because the DWC controller does handle the iATU table being modified while there are outstanding transactions.)

This patch has been tested using pci-epf-test and nvmet-pci-epf on rock5b.

pci-epf-test does change between MSI and MSI-X without calling dw_pcie_ep_stop(), however, the msg_addr address written by the host will be the same address, at least when using a Linux host using a DWC based controller. If another host ends up using different msg_addr for MSI and MSI-X, then I think that we will need to modify pci-epf-test to call a function when changing IRQ type, such that pcie-designware-ep.c can tear down the MSI/MSI-X mapping.

  .../pci/controller/dwc/pcie-designware-ep.c   | 82 ++++++++++++++++---
  drivers/pci/controller/dwc/pcie-designware.h  |  5 ++
  2 files changed, 75 insertions(+), 12 deletions(-)

drivers/pci/controller/dwc/pcie-designware-ep.c +39 -10

index 7f2112c2fb21..2bbeddaa73d4 100644
@@ -601,6 +601,16 @@ static void dw_pcie_ep_stop(struct pci_epc *epc)
  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
 
+	/*
+	 * Tear down the dedicated outbound window used for MSI
+	 * generation. This avoids leaking an iATU window across
+	 * endpoint stop/start cycles.
+	 */
+	if (ep->msi_iatu_mapped) {
+		dw_pcie_ep_unmap_addr(epc, 0, 0, ep->msi_mem_phys);
+		ep->msi_iatu_mapped = false;
+	}
+
  	dw_pcie_stop_link(pci);
  }
 

@@ -702,14 +712,37 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
  	msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower;
 
  	msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset);
-	ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
-				  map_size);
-	if (ret)
-		return ret;
 
-	writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset);
+	/*
+	 * Program the outbound iATU once and keep it enabled.
+	 *
+	 * The spec warns that updating iATU registers while there are
+	 * operations in flight on the AXI bridge interface is not
+	 * supported, so we avoid reprogramming the region on every MSI,
+	 * specifically unmapping immediately after writel().
+	 */
+	if (!ep->msi_iatu_mapped) {
+		ret = dw_pcie_ep_map_addr(epc, func_no, 0,
+					  ep->msi_mem_phys, msg_addr,
+					  map_size);
+		if (ret)
+			return ret;
+
+		ep->msi_iatu_mapped = true;
+		ep->msi_msg_addr = msg_addr;
+		ep->msi_map_size = map_size;
+	} else if (WARN_ON_ONCE(ep->msi_msg_addr != msg_addr ||
+				ep->msi_map_size != map_size)) {
+		/*
+		 * The host changed the MSI target address or the required
+		 * mapping size changed. Reprogramming the iATU at runtime is
+		 * unsafe on this controller, so bail out instead of trying to
+		 * update the existing region.
+		 */
+		return -EINVAL;
+	}
 
-	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys);
+	writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset);
 
  	return 0;
  }

@@ -786,14 +819,36 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
  	}
 
  	msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset);
-	ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
-				  map_size);
-	if (ret)
-		return ret;
 

drivers/pci/controller/dwc/pcie-designware-ep.c +22 -1

+		ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
+					  map_size);
+		if (ret)
+			return ret;
+
+		ep->msi_iatu_mapped = true;
+		ep->msi_msg_addr = msg_addr;
+		ep->msi_map_size = map_size;
+	} else if (WARN_ON_ONCE(ep->msi_msg_addr != msg_addr ||
+				ep->msi_map_size != map_size)) {
+		/*
+		 * The host changed the MSI-X target address or the required
+		 * mapping size changed. Reprogramming the iATU at runtime is
+		 * unsafe on this controller, so bail out instead of trying to
+		 * update the existing region.
+		 */
+		return -EINVAL;
+	}
 
-	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys);
+	writel(msg_data, ep->msi_mem + offset);
 
  	return 0;
  }

@@ -1086,6 +1141,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
  	struct device *dev = pci->dev;
 
  	INIT_LIST_HEAD(&ep->func_list);
+	ep->msi_iatu_mapped = false;
+	ep->msi_msg_addr = 0;
+	ep->msi_map_size = 0;
 
  	epc = devm_pci_epc_create(dev, &epc_ops);
  	if (IS_ERR(epc)) {

drivers/pci/controller/dwc/pcie-designware.h +5 -0

index a31bd93490dc..1093c622826d 100644
@@ -472,6 +472,11 @@ struct dw_pcie_ep {
  	void __iomem		*msi_mem;
  	phys_addr_t		msi_mem_phys;
  	struct pci_epf_bar	*epf_bar[PCI_STD_NUM_BARS];
+
+	/* MSI outbound iATU state */
+	bool			msi_iatu_mapped;
+	u64			msi_msg_addr;
+	size_t			msi_map_size;
  };
 
  struct dw_pcie_ops {

Quoted 14 lines · Niklas Cassel

drivers/pci/controller/dwc/pcie-designware-ep.c +6 -5

@@ -786,14 +819,36 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
  	}
  	msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset);
-	ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
-				  map_size);
-	if (ret)
-		return ret;
-	writel(msg_data, ep->msi_mem + offset);
+	/*
+	 * Program the outbound iATU once and keep it enabled.
+	 *
+	 * The spec warns that updating iATU registers while there are
+	 * operations in flight on the AXI bridge interface is not
+	 * supported, so we avoid reprogramming the region on every MSI-X,

drivers/pci/controller/dwc/pcie-designware-ep.c int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, · line 724

Quoted 3 lines

drivers/pci/controller/dwc/pcie-designware-ep.c +3 -0

+	 * specifically unmapping immediately after writel().
+	 */
+	if (!ep->msi_iatu_mapped) {

This is wrong, in MSIX each vector can give you different address, you can't expect same address for
all the vectors in MSIX table. In ARM based system you might see only single address for X86 this will
change.

And also we see in MSIX the address are getting updated at runtime with x86 windows host machines.

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

Quoted 15 lines · Krishna Chaitanya Chundru

Quoted 14 lines · Niklas Cassel

drivers/pci/controller/dwc/pcie-designware-ep.c +6 -5

@@ -786,14 +819,36 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
   	}
   	msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset);
-	ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
-				  map_size);
-	if (ret)
-		return ret;
-	writel(msg_data, ep->msi_mem + offset);
+	/*
+	 * Program the outbound iATU once and keep it enabled.
+	 *
+	 * The spec warns that updating iATU registers while there are
+	 * operations in flight on the AXI bridge interface is not
+	 * supported, so we avoid reprogramming the region on every MSI-X,

Quoted 8 lines

drivers/pci/controller/dwc/pcie-designware-ep.c int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no, · line 724

Quoted 3 lines

drivers/pci/controller/dwc/pcie-designware-ep.c +3 -0

+	 * specifically unmapping immediately after writel().
+	 */
+	if (!ep->msi_iatu_mapped) {

This is wrong, in MSIX each vector can give you different address, you can't expect same address for
all the vectors in MSIX table. In ARM based system you might see only single address for X86 this will
change.

Ok, thank you. I did not know.

Quoted 2 lines

And also we see in MSIX the address are getting updated at runtime with x86 windows host machines.

My idea was to add a pci_epc_set_currently_enabled_irq_type() API, and then let the EPF driver call that if it wants to change the IRQ type.

But... if the msg_addr can change at runtime, even when the IRQ type does not change, then a pci_epc_set_currently_enabled_irq_type() API will not help.

I guess we will need to come up with something else for the MSI-X case.

Quoted 2 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

AFAICT, right now, the only driver ever calling this function is:
drivers/pci/controller/dwc/pci-layerscape-ep.c

Are you suggesting that we somehow change all the other DWC based EPC drivers' .raise_irq() callback to call dw_pcie_ep_raise_msix_irq_doorbell() instead of dw_pcie_ep_raise_msix_irq() for case PCI_IRQ_MSIX ?

That sounds like a big change that would need to be tested and verified for each DWC based EPC driver.

Kind regards,
Niklas

Quoted 8 lines

I guess we will need to come up with something else for the MSI-X case.

Quoted 2 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

AFAICT, right now, the only driver ever calling this function is:
drivers/pci/controller/dwc/pci-layerscape-ep.c

Quoted 3 lines

Are you suggesting that we somehow change all the other DWC based EPC drivers' .raise_irq() callback to call dw_pcie_ep_raise_msix_irq_doorbell() instead of dw_pcie_ep_raise_msix_irq() for case PCI_IRQ_MSIX ?

Yes.

Quoted 2 lines

That sounds like a big change that would need to be tested and verified for each DWC based EPC driver.

I agree, but this will be clean solution to avoid iATU for MSIX.

Quoted 26 lines · Niklas Cassel

From: Koichiro Den <[email protected]>

dw_pcie_ep_raise_msi_irq() currently programs an outbound iATU window for the MSI target address on every interrupt and tears it down again via dw_pcie_ep_unmap_addr().

On systems that heavily use the AXI bridge interface (for example when the integrated eDMA engine is active), this means the outbound iATU registers are updated while traffic is in flight. The DesignWare endpoint databook 5.40a - "3.10.6.1 iATU Outbound Programming Overview" warns that updating iATU registers in this situation is not supported, and the behavior is undefined.

Under high MSI and eDMA load this pattern results in occasional bogus outbound transactions and IOMMU faults, on the RC side, such as:

   ipmmu-vmsa eed40000.iommu: Unhandled fault: status 0x00001502 iova 0xfe000000

followed by the system becoming unresponsive. This is the actual output observed on Renesas R-Car S4, with its ipmmu_hc used with PCIe ch0.

There is no need to reprogram the iATU region used for MSI on every interrupt. The host-provided MSI address is stable while MSI is enabled, and the endpoint driver already dedicates a scratch buffer for MSI generation.

Quoted 8 lines

Cache the aligned MSI address and map size, program the outbound iATU once, and keep the window enabled. Subsequent interrupts only perform a write to the MSI scratch buffer, avoiding dynamic iATU reprogramming in the hot path and fixing the lockups seen under load.

Signed-off-by Koichiro Den <[email protected]>

[cassel: do same change for dw_pcie_ep_raise_msix_irq()]

Signed-off-by Niklas Cassel <[email protected]>

Quoted 96 lines

---
Notes: Without this patch, I also see IOMMU errors on the host side, when running drivers/nvme/target/pci-epf.c with a large queue depth.

pci_epc_raise_irq() does take a mutex, so the calls to these functions are serialized, so it really appears that the DWC controller does not handle iATU reprogramming while there are outstanding transactions.

Just like Koichiro describes here:
https://lore.kernel.org/linux-pci/ddriorsgyjs6klcb6d7pi2u3ah3wxlzku7v2dpyjlo6tmalvfw@yj5dczlkggt6/

I also see the iova faulting on the RC side to be the start of "addr_space" on the EP, so it appears that a transaction has gone through untranslated. (Most likely because the DWC controller does handle the iATU table being modified while there are outstanding transactions.)

This patch has been tested using pci-epf-test and nvmet-pci-epf on rock5b.

pci-epf-test does change between MSI and MSI-X without calling dw_pcie_ep_stop(), however, the msg_addr address written by the host will be the same address, at least when using a Linux host using a DWC based controller. If another host ends up using different msg_addr for MSI and MSI-X, then I think that we will need to modify pci-epf-test to call a function when changing IRQ type, such that pcie-designware-ep.c can tear down the MSI/MSI-X mapping.

  .../pci/controller/dwc/pcie-designware-ep.c   | 82 ++++++++++++++++---
  drivers/pci/controller/dwc/pcie-designware.h  |  5 ++
  2 files changed, 75 insertions(+), 12 deletions(-)

drivers/pci/controller/dwc/pcie-designware-ep.c +39 -10

index 7f2112c2fb21..2bbeddaa73d4 100644
@@ -601,6 +601,16 @@ static void dw_pcie_ep_stop(struct pci_epc *epc)
  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	/*
+	 * Tear down the dedicated outbound window used for MSI
+	 * generation. This avoids leaking an iATU window across
+	 * endpoint stop/start cycles.
+	 */
+	if (ep->msi_iatu_mapped) {
+		dw_pcie_ep_unmap_addr(epc, 0, 0, ep->msi_mem_phys);
+		ep->msi_iatu_mapped = false;
+	}
+
  	dw_pcie_stop_link(pci);
  }

@@ -702,14 +712,37 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
  	msg_addr = ((u64)msg_addr_upper) << 32 | msg_addr_lower;
  	msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset);
-	ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
-				  map_size);
-	if (ret)
-		return ret;
-	writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset);
+	/*
+	 * Program the outbound iATU once and keep it enabled.
+	 *
+	 * The spec warns that updating iATU registers while there are
+	 * operations in flight on the AXI bridge interface is not
+	 * supported, so we avoid reprogramming the region on every MSI,
+	 * specifically unmapping immediately after writel().
+	 */
+	if (!ep->msi_iatu_mapped) {
+		ret = dw_pcie_ep_map_addr(epc, func_no, 0,
+					  ep->msi_mem_phys, msg_addr,
+					  map_size);
+		if (ret)
+			return ret;
+
+		ep->msi_iatu_mapped = true;
+		ep->msi_msg_addr = msg_addr;
+		ep->msi_map_size = map_size;
+	} else if (WARN_ON_ONCE(ep->msi_msg_addr != msg_addr ||
+				ep->msi_map_size != map_size)) {
+		/*
+		 * The host changed the MSI target address or the required
+		 * mapping size changed. Reprogramming the iATU at runtime is
+		 * unsafe on this controller, so bail out instead of trying to
+		 * update the existing region.
+		 */
+		return -EINVAL;
+	}
-	dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys);
+	writel(msg_data | (interrupt_num - 1), ep->msi_mem + offset);
  	return 0;
  }

@@ -786,14 +819,36 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
  	}
  	msg_addr = dw_pcie_ep_align_addr(epc, msg_addr, &map_size, &offset);
-	ret = dw_pcie_ep_map_addr(epc, func_no, 0, ep->msi_mem_phys, msg_addr,
-				  map_size);
-	if (ret)
-		return ret;

drivers/pci/controller/dwc/pcie-designware-ep.c int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no, · line 829

Quoted 10 lines

drivers/pci/controller/dwc/pcie-designware-ep.c +9 -1

-	writel(msg_data, ep->msi_mem + offset);
+	/*
+	 * Program the outbound iATU once and keep it enabled.
+	 *
+	 * The spec warns that updating iATU registers while there are
+	 * operations in flight on the AXI bridge interface is not
+	 * supported, so we avoid reprogramming the region on every MSI-X,
+	 * specifically unmapping immediately after writel().
+	 */
+	if (!ep->msi_iatu_mapped) {

This is wrong, in MSIX each vector can give you different address, you can't expect same address for
all the vectors in MSIX table. In ARM based system you might see only single address for X86 this will
change.

And also we see in MSIX the address are getting updated at runtime with x86 windows host machines.

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

Quoted 6 lines

This is wrong, in MSIX each vector can give you different address, you can't expect same address for
all the vectors in MSIX table. In ARM based system you might see only single address for X86 this will
change.

Quoted 3 lines

And also we see in MSIX the address are getting updated at runtime with x86 windows host machines.

The spec allows changing the MSI-X address/data pair when the corresponding vector is *masked*, and classifies the behavior as undefined if address/data pair gets changed while the vector is *unmasked*.

If we were to enable caching, then we need to take note of the above point. But the EP implementation doesn't get any notification when the vector gets masked/unmasked dynamically, which makes it trickier to implement the caching in SW.

Quoted 3 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

Quoted 4 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

Quoted 11 lines · Manivannan Sadhasivam

Quoted 4 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

FWIW, I did try to simply change:

drivers/pci/controller/dwc/pcie-dw-rockchip.c +2 -1

index 8f2cc1ef25e3..00770f9786e3 100644
@@ -319,7 +319,8 @@ static int rockchip_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
        case PCI_IRQ_MSI:
                return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
        case PCI_IRQ_MSIX:
-               return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+               return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no,
+                                                         interrupt_num);
        default:
                dev_err(pci->dev, "UNKNOWN IRQ type\n");
        }

For the pcie-dw-rockchip driver, but it is not working:

[  130.042849] nvme nvme0: I/O tag 0 (1000) QID 0 timeout, completion polled

Without this change, things work.

Perhaps this feature is not an optional one, but at least we will require more changes than a simple one liner.

Quoted 9 lines · Manivannan Sadhasivam

Quoted 3 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

FWIW, I did try to simply change:

drivers/pci/controller/dwc/pcie-dw-rockchip.c +2 -1

index 8f2cc1ef25e3..00770f9786e3 100644
@@ -319,7 +319,8 @@ static int rockchip_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
         case PCI_IRQ_MSI:
                 return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
         case PCI_IRQ_MSIX:
-               return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+               return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no,
+                                                         interrupt_num);
         default:
                 dev_err(pci->dev, "UNKNOWN IRQ type\n");
         }

For the pcie-dw-rockchip driver, but it is not working:

[  130.042849] nvme nvme0: I/O tag 0 (1000) QID 0 timeout, completion polled

Without this change, things work.

Perhaps this feature is not an optional one, but at least we will require more changes than a simple one liner.

Kind regards,
Niklas

Quoted 28 lines · Niklas Cassel

Quoted 9 lines · Manivannan Sadhasivam

Quoted 3 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

FWIW, I did try to simply change:

drivers/pci/controller/dwc/pcie-dw-rockchip.c +2 -1

index 8f2cc1ef25e3..00770f9786e3 100644
@@ -319,7 +319,8 @@ static int rockchip_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
         case PCI_IRQ_MSI:
                 return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
         case PCI_IRQ_MSIX:
-               return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+               return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no,
+                                                         interrupt_num);
         default:
                 dev_err(pci->dev, "UNKNOWN IRQ type\n");
         }

Quoted 7 lines

For the pcie-dw-rockchip driver, but it is not working:

[  130.042849] nvme nvme0: I/O tag 0 (1000) QID 0 timeout, completion polled

Without this change, things work.

Perhaps this feature is not an optional one, but at least we will require more changes than a simple one liner.

Hi Niklas,

It should be automatic only, no extra configurations should be required, I believe your
HW doesn't support this feature, from spec 6..0a, sec 3.9.1.3
iMSIX-TX: Integrated MSI-X Transmit (USP)
I believe your HW is not generated with MSIX_TABLE_EN =1. In that case you can't use this feature.

+ Shawn

Quoted 40 lines · Krishna Chaitanya Chundru

Quoted 28 lines · Niklas Cassel

Quoted 9 lines · Manivannan Sadhasivam

Quoted 3 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

FWIW, I did try to simply change:

drivers/pci/controller/dwc/pcie-dw-rockchip.c +2 -1

index 8f2cc1ef25e3..00770f9786e3 100644
@@ -319,7 +319,8 @@ static int rockchip_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
          case PCI_IRQ_MSI:
                  return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
          case PCI_IRQ_MSIX:
-               return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+               return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no,
+                                                         interrupt_num);
          default:
                  dev_err(pci->dev, "UNKNOWN IRQ type\n");
          }

Quoted 7 lines

For the pcie-dw-rockchip driver, but it is not working:

[  130.042849] nvme nvme0: I/O tag 0 (1000) QID 0 timeout, completion polled

Without this change, things work.

Perhaps this feature is not an optional one, but at least we will require more changes than a simple one liner.

Hi Niklas,

Quoted 6 lines

It should be automatic only, no extra configurations should be required, I believe your
HW doesn't support this feature, from spec 6..0a, sec 3.9.1.3
iMSIX-TX: Integrated MSI-X Transmit (USP)
I believe your HW is not generated with MSIX_TABLE_EN =1. In that case you can't use this feature.

Looking at the RK3588 TRM, it does have register:
USP_PCIE_PL_MSIX_DOORBELL_OFF
Address: Operational Base + offset (0x0248)

Port Logic registers start at offset 0x700 on this SoC, so 0x700 + 0x248 == 0x948, which matches:
drivers/pci/controller/dwc/pcie-designware.h:#define PCIE_MSIX_DOORBELL 0x948

I don't think the TRM would include this register if the DWC coere was not generated with MSIX_TABLE_EN=1.

Shawn, any suggestions?
For full thread, see:
https://lore.kernel.org/linux-pci/[email protected]/T/#t

FWIW, Krishna Chaitanya, I did try the dw_pcie_ep_raise_msix_irq_doorbell() change above also with the pci-epf-test EPF driver, and it also caused the pci-epf-test driver to stop working.

Kind regards,
Niklas

Quoted 56 lines · Niklas Cassel

+ Shawn

Quoted 42 lines · Krishna Chaitanya Chundru

Quoted 30 lines · Niklas Cassel

Quoted 9 lines · Manivannan Sadhasivam

Quoted 3 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

FWIW, I did try to simply change:

drivers/pci/controller/dwc/pcie-dw-rockchip.c +0 -0

index 8f2cc1ef25e3..00770f9786e3 100644
@@ -319,7 +319,8 @@ static int rockchip_pcie_raise_irq(struct 
 dw_pcie_ep *ep, u8 func_no,

          case PCI_IRQ_MSI:
                  return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); case PCI_IRQ_MSIX:
-               return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+               return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no,
+ interrupt_num);
          default:
                  dev_err(pci->dev, "UNKNOWN IRQ type\n");
          }

Quoted 7 lines

For the pcie-dw-rockchip driver, but it is not working:
[  130.042849] nvme nvme0: I/O tag 0 (1000) QID 0 timeout, completion polled

Without this change, things work.

Perhaps this feature is not an optional one, but at least we will require more changes than a simple one liner.

Hi Niklas,

Quoted 6 lines

It should be automatic only, no extra configurations should be required, I believe your
HW doesn't support this feature, from spec 6..0a, sec 3.9.1.3
iMSIX-TX: Integrated MSI-X Transmit (USP)
I believe your HW is not generated with MSIX_TABLE_EN =1. In that case you can't use this feature.

Looking at the RK3588 TRM, it does have register:
USP_PCIE_PL_MSIX_DOORBELL_OFF
Address: Operational Base + offset (0x0248)

Quoted 7 lines

Port Logic registers start at offset 0x700 on this SoC,
so 0x700 + 0x248 == 0x948, which matches:
drivers/pci/controller/dwc/pcie-designware.h:#define PCIE_MSIX_DOORBELL         0x948

I don't think the TRM would include this register if the DWC coere was not generated with MSIX_TABLE_EN=1.

I checked the IP configurtion parameters for RK3588 DM controller for sure, it sets MSIX_TABLE_EN=1.

Looking into dw_pcie_ep_raise_msix_irq_doorbell(), it doesn't seem to
match the dwc databook. No matter for non-AXI mode or AXI access mode,
shouldn't we need to generate a MSI-X table RAM with data/address/vector/TC in advanced? Am I missing anything because I didn't look

too much regarding to the EPC side?

Quoted 14 lines

Shawn, any suggestions?
For full thread, see:
https://lore.kernel.org/linux-pci/[email protected]/T/#t

FWIW, Krishna Chaitanya, I did try the dw_pcie_ep_raise_msix_irq_doorbell() change above also with the pci-epf-test EPF driver, and it also caused the pci-epf-test driver to stop working.

Kind regards,
Niklas

Quoted 65 lines · Shawn Lin

Quoted 63 lines · Niklas Cassel

+ Shawn

Quoted 42 lines · Krishna Chaitanya Chundru

Quoted 30 lines · Niklas Cassel

Quoted 9 lines · Manivannan Sadhasivam

Quoted 3 lines

Use the MSIX doorbell method which will not use iATU at all, dw_pcie_ep_raise_msix_irq_doorbell().

I think this is the safe bet since this feature doesn't seem like an optional one.

Niklas, if you can just fix MSI in this patch and leave out MSI-X for the vendor drivers to transition to doorbell, I'm OK to merge it. Otherwise, I don't know how you can reliably fix MSI-X generation with AXI slave interface.

FWIW, I did try to simply change:

drivers/pci/controller/dwc/pcie-dw-rockchip.c +0 -0

index 8f2cc1ef25e3..00770f9786e3 100644
@@ -319,7 +319,8 @@ static int rockchip_pcie_raise_irq(struct 
 dw_pcie_ep *ep, u8 func_no,

          case PCI_IRQ_MSI:
                  return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num); case PCI_IRQ_MSIX:
-               return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+               return dw_pcie_ep_raise_msix_irq_doorbell(ep, func_no,
+ interrupt_num);
          default:
                  dev_err(pci->dev, "UNKNOWN IRQ type\n");
          }

Quoted 7 lines

For the pcie-dw-rockchip driver, but it is not working:
[  130.042849] nvme nvme0: I/O tag 0 (1000) QID 0 timeout, completion polled

Without this change, things work.

Perhaps this feature is not an optional one, but at least we will require more changes than a simple one liner.

Hi Niklas,

Quoted 6 lines

It should be automatic only, no extra configurations should be required, I believe your
HW doesn't support this feature, from spec 6..0a, sec 3.9.1.3
iMSIX-TX: Integrated MSI-X Transmit (USP)
I believe your HW is not generated with MSIX_TABLE_EN =1. In that case you can't use this feature.

Looking at the RK3588 TRM, it does have register:
USP_PCIE_PL_MSIX_DOORBELL_OFF
Address: Operational Base + offset (0x0248)

Port Logic registers start at offset 0x700 on this SoC,
so 0x700 + 0x248 == 0x948, which matches:
drivers/pci/controller/dwc/pcie-designware.h:#define PCIE_MSIX_DOORBELL         0x948

I don't think the TRM would include this register if the DWC coere was not generated with MSIX_TABLE_EN=1.

Quoted 6 lines

I checked the IP configurtion parameters for RK3588 DM controller for sure, it sets MSIX_TABLE_EN=1.

Looking into dw_pcie_ep_raise_msix_irq_doorbell(), it doesn't seem to
match the dwc databook. No matter for non-AXI mode or AXI access mode,
shouldn't we need to generate a MSI-X table RAM with data/address/ vector/TC in advanced? Am I missing anything because I didn't look

The MSI-X table will updated automatically when host updates the MSI-X table, when MSI-X is enabled by host.

- Krishna Chaitanya.

Quoted 16 lines

too much regarding to the EPC side?

Quoted 13 lines

Shawn, any suggestions?
For full thread, see:
https://lore.kernel.org/linux-pci/3b34aa66- [email protected]/T/#t

FWIW, Krishna Chaitanya, I did try the dw_pcie_ep_raise_msix_irq_doorbell() change above also with the pci-epf-test EPF driver, and it also caused the pci-epf-test driver to stop working.

Kind regards,
Niklas

Quoted 8 lines · Shawn Lin

Quoted 7 lines

I checked the IP configurtion parameters for RK3588 DM controller for sure, it sets MSIX_TABLE_EN=1.

Looking into dw_pcie_ep_raise_msix_irq_doorbell(), it doesn't seem to match the dwc databook. No matter for non-AXI mode or AXI access mode, shouldn't we need to generate a MSI-X table RAM with data/address/ vector/TC in advanced? Am I missing anything because I didn't look

Quoted 3 lines

The MSI-X table will updated automatically when host updates the MSI-X table, when MSI-X is enabled
by host.

Thanks for these details.
I re-read the databook, especially "Figure 3-49 iMSIX-TX: MSIX Transmit ". Yes, it's updated automatically when host write access it, which either comes from RX or local DBI(for debug purpose).

Quoted 7 lines

Quoted 7 lines

I checked the IP configurtion parameters for RK3588 DM controller for sure, it sets MSIX_TABLE_EN=1.

Looking into dw_pcie_ep_raise_msix_irq_doorbell(), it doesn't seem to match the dwc databook. No matter for non-AXI mode or AXI access mode, shouldn't we need to generate a MSI-X table RAM with data/address/ vector/TC in advanced? Am I missing anything because I didn't look

Quoted 3 lines

The MSI-X table will updated automatically when host updates the MSI-X table, when MSI-X is enabled
by host.

Thanks for these details.
I re-read the databook, especially "Figure 3-49 iMSIX-TX: MSIX Transmit ". Yes, it's updated automatically when host write access it, which either comes from RX or local DBI(for debug purpose).

Hello all,

Quoted 7 lines · Shawn Lin

Quoted 7 lines

Quoted 7 lines

I checked the IP configurtion parameters for RK3588 DM controller for sure, it sets MSIX_TABLE_EN=1.

Looking into dw_pcie_ep_raise_msix_irq_doorbell(), it doesn't seem to match the dwc databook. No matter for non-AXI mode or AXI access mode, shouldn't we need to generate a MSI-X table RAM with data/address/ vector/TC in advanced? Am I missing anything because I didn't look

Quoted 8 lines

Quoted 3 lines

The MSI-X table will updated automatically when host updates the MSI-X table, when MSI-X is enabled
by host.

Thanks for these details.
I re-read the databook, especially "Figure 3-49 iMSIX-TX: MSIX Transmit ". Yes, it's updated automatically when host write access it, which either comes from RX or local DBI(for debug purpose).

With the help of Shawn, I managed to get dw_pcie_ep_raise_msix_irq_doorbell() to work on RK3588.

By default, on RK3588, the MSI-X table is stored in BAR4 at offset 0x4000.

There seems to be a limitation that the iMSIX-TX doorbell feature only works when the MSI-X table is stored in this default location.

As you know, by default, pci-epf-test stores the MSI-X table in BAR0, after the registers defined by pci-epf-test itself.
This works fine when triggering a MSI-X using dw_pcie_ep_raise_msix_irq(), but does not work when using dw_pcie_ep_raise_msix_irq_doorbell() (at least not or RK3588).

Additionally, on RK3588, at offset 0x0 in BAR4, the eDMA registers are mapped, so we cannot simply use BAR4 as test_reg_bar, as that would conflict with the registers defined by pci-epf-test-itself.

The solution here is most likely to let EPC drivers define a
"HW limitation" of the MSI-X table (BAR number and offset), and create a new API in the PCI endpoint framework that exposes this, and modify all EPF drivers to use this API before calling pci_epc_set_msix().

Personally, I'm too busy to work on this right now.
For our use case with the NVMe PCI EPF, we can simply force it to only use MSI (instead of MSI-X).

But... I could submit a WARN() in dw_pcie_ep_raise_msix_irq() that explains that this function is broken, and that dw_pcie_ep_raise_msix_irq_doorbell() should be used instead, on platforms that support it.

Or, if it is preferred, we could simply modify all DWC based drivers that is not using dw_pcie_ep_raise_msix_irq_doorbell() (i.e. all except layerspace) to simply not set "msix_capable = true" in epc_features, as we know that all drivers that are using dw_pcie_ep_raise_msix_irq() are broken.

Thoughts?

Kind regards,
Niklas