On Wed, Apr 30, 2025 at 02:31:59PM +0200, Niklas Cassel wrote:
> While the parameter 'interrupts' to the functions pci_epc_set_msi() and
> pci_epc_set_msix() represent the actual number of interrupts, and
> pci_epc_get_msi() and pci_epc_get_msix() return the actual number of
> interrupts.
>
> These endpoint library functions just mentioned will however supply
> "interrupts - 1" to the EPC callback functions pci_epc_ops->set_msi() and
> pci_epc_ops->set_msix(), and likewise add 1 to return value from
> pci_epc_ops->get_msi() and pci_epc_ops->get_msix(),
Only {get/set}_msix() callbacks were having this behavior, right?
> even though the
> parameter name for the callback function is also named 'interrupts'.
>
> While the set_msix() callback function in pcie-designware-ep writes the
> Table Size field correctly (N-1), the calculation of the PBA offset
> is wrong because it calculates space for (N-1) entries instead of N.
>
> This results in e.g. the following error when using QEMU with PCI
> passthrough on a device which relies on the PCI endpoint subsystem:
> failed to add PCI capability 0x11[0x50]@0xb0: table & pba overlap, or they don't fit in BARs, or don't align
>
> Fix the calculation of PBA offset in the MSI-X capability.
>
Thanks for the fix! We should also fix the API discrepancy w.r.t interrupts as
it is causing much of a headache. One more example is the interrupt vector. API
expects the vectors to be 1-based, but in reality, vectors start from 0. So the
callers of raise_irq() has to increment the vector and the implementation has to
decrement it.
If you want to fix it up too, let me know. Otherwise, I may do it at some point.
> Fixes: 83153d9f36e2 ("PCI: endpoint: Fix ->set_msix() to take BIR and offset as arguments")
This doesn't seem like the correct fixes commit.
- Mani
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
> drivers/pci/controller/dwc/pcie-designware-ep.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 1a0bf9341542..24026f3f3413 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -585,6 +585,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> struct dw_pcie_ep_func *ep_func;
> u32 val, reg;
> + u16 actual_interrupts = interrupts + 1;
>
> ep_func = dw_pcie_ep_get_func_from_ep(ep, func_no);
> if (!ep_func || !ep_func->msix_cap)
> @@ -595,7 +596,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> reg = ep_func->msix_cap + PCI_MSIX_FLAGS;
> val = dw_pcie_ep_readw_dbi(ep, func_no, reg);
> val &= ~PCI_MSIX_FLAGS_QSIZE;
> - val |= interrupts;
> + val |= interrupts; /* 0's based value */
> dw_pcie_writew_dbi(pci, reg, val);
>
> reg = ep_func->msix_cap + PCI_MSIX_TABLE;
> @@ -603,7 +604,7 @@ static int dw_pcie_ep_set_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
>
> reg = ep_func->msix_cap + PCI_MSIX_PBA;
> - val = (offset + (interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
> + val = (offset + (actual_interrupts * PCI_MSIX_ENTRY_SIZE)) | bir;
> dw_pcie_ep_writel_dbi(ep, func_no, reg, val);
>
> dw_pcie_dbi_ro_wr_dis(pci);
> --
> 2.49.0
>
--
மணிவண்ணன் சதாசிவம்
