Re: [PATCH v2 1/2] PCI: dwc: ep: Flush the cached MSI address before unmap

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The cached MSI iATU mapping logic in dw_pcie_ep_raise_msi_irq() omits checking func_no, causing MSIs from subsequent functions to be sent with the wrong Requester ID.
- [High] Missing flush of posted MSI writes before unmapping the ATU window in dw_pcie_ep_stop().
- [High] Hardware address conflict and ATU window leak due to shared ep->msi_mem_phys between MSI and MSI-X.
--

--- Patch [1]: [PATCH v2 1/2] PCI: dwc: ep: Flush the cached MSI address before unmap ---
commit 61e06ab5c5d18de814a8a431d661886fcd30c6d7
Author: Niklas Cassel <[email protected]>

PCI: dwc: ep: Flush the cached MSI address before unmap

While we do a flush before unmap in dw_pcie_ep_raise_msix_irq(), we do
not do a flush before unmap in dw_pcie_ep_raise_msi_irq(), if the cached
MSI target address changed. Fix this by doing a flush before unmap.

Link: https://lore.kernel.org/linux-pci/[email protected]/

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1032,6 +1032,8 @@ int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep, u8 func_no,
>  		 * there is no unified way to check if we have operations in
>  		 * flight, thus we don't know if we should WARN() or not.
>  		 */
> +		/* flush posted write before unmap */
> +		readl(ep->msi_mem + ep->msi_iatu_mapped_offset);

[Severity: High]
This is a pre-existing issue, but since this patch adds a flush here to
drain posted writes, is a similar flush needed in dw_pcie_ep_stop()?

During device stop, dw_pcie_ep_stop() unmaps the MSI ATU window:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_stop() {
    ...
	if (ep->msi_iatu_mapped) {
		dw_pcie_ep_unmap_addr(epc, 0, 0, ep->msi_mem_phys);
		ep->msi_iatu_mapped = false;
	}
    ...
}

Would stopping the controller and unmapping the window without a dummy
read to drain the AXI bridge cause in-flight MSI writes to be dropped?

>  		dw_pcie_ep_unmap_addr(epc, func_no, 0, ep->msi_mem_phys);

[Severity: High]
This isn't a bug introduced by this patch, but does the MSI iATU mapping
logic need to track the function number?

Currently, struct dw_pcie_ep lacks a field to track which function number
is mapped. The caching logic in dw_pcie_ep_raise_msi_irq() checks the
target address and size, but omits the function number.

If Function 0 raises an MSI, the ATU is programmed with Function 0's
Requester ID. If Function 1 then raises an MSI to the identical target
address, the cache check succeeds and bypasses ATU reprogramming.

Might this cause Function 1's MSIs to be transmitted via the ATU window
still configured with Function 0's Requester ID, leading to dropped
interrupts by the host IOMMU?


[Severity: High]
This is a pre-existing issue, but does sharing ep->msi_mem_phys between
MSI and MSI-X cause hardware address conflicts?

If dw_pcie_ep_raise_msi_irq() maps an ATU window for ep->msi_mem_phys,
and subsequently dw_pcie_ep_raise_msix_irq() unconditionally allocates
and maps a new ATU window for the exact same physical address, overlapping
translation windows are created.

When dw_pcie_ep_unmap_addr() is called, it searches for the physical address
and unmaps the first matching window:

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_find_index() {
    ...
	for_each_set_bit(index, ep->ob_window_map, pci->num_ob_windows) {
		if (ep->outbound_addr[index] != addr)
			continue;
		*atu_index = index;
		return 0;
	}
    ...
}

Would this incorrectly return the MSI window's index, unmapping the MSI
window instead of the MSI-X window, and permanently leaking the newly
allocated MSI-X ATU window?

>  		ep->msi_iatu_mapped = false;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1