Re: [PATCH] powernv/pci: Fix m64 checks for SR-IOV and window alignment
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2016-09-14 08:32:23
On Wed, 2016-09-14 at 16:37 +1000, Russell Currey wrote:
Commit 5958d19a143e checks for prefetchable m64 BARs by comparing the addresses instead of using resource flags. This broke SR-IOV as the m64 check in pnv_pci_ioda_fixup_iov_resources() fails. The condition in pnv_pci_window_alignment() also changed to checking only IORESOURCE_MEM_64 instead of both IORESOURCE_MEM_64 and IORESOURCE_PREFETCH.
CC'ing Gavin who might have some insight in the matter. Why do we check for prefetch ? On PCIe, any 64-bit BAR can live under a prefetchable region afaik... Gavin, any idea ? Also:
quoted hunk ↗ jump to hunk
Revert these cases to the previous behaviour, adding a new helper function to do so. This is named pnv_pci_is_m64_flags() to make it clear this function is only looking at resource flags and should not be relied on for non-SRIOV resources. Fixes: 5958d19a143e ("Fix incorrect PE reservation attempt on some 64-bit BARs") Reported-by: Alexey Kardashevskiy <redacted> Signed-off-by: Russell Currey <redacted> --- arch/powerpc/platforms/powernv/pci-ioda.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)diff --git a/arch/powerpc/platforms/powernv/pci-ioda.cb/arch/powerpc/platforms/powernv/pci-ioda.c index c16d790..2f25622 100644--- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c@@ -124,6 +124,13 @@ static inline bool pnv_pci_is_m64(struct pnv_phb*phb, struct resource *r) r->start < (phb->ioda.m64_base + phb-quoted
ioda.m64_size));} +static inline bool pnv_pci_is_m64_flags(unsigned long resource_flags) +{ + unsigned long flags = (IORESOURCE_MEM_64 | IORESOURCE_PREFETCH); + + return (resource_flags & flags) == flags; +}
I don't agree. See below.
quoted hunk ↗ jump to hunk
static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, int pe_no) { phb->ioda.pe_array[pe_no].phb = phb;@@ -2871,7 +2878,7 @@ static voidpnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) res = &pdev->resource[i + PCI_IOV_RESOURCES]; if (!res->flags || res->parent) continue; - if (!pnv_pci_is_m64(phb, res)) { + if (!pnv_pci_is_m64_flags(res->flags)) { dev_warn(&pdev->dev, "Don't support SR-IOV with" " non M64 VF BAR%d: %pR. \n", i, res);
What is that function actually doing ? Having IORESOURCE_64 and PREFETCHABLE is completely orthogonal to being in the M64 region. This is the bug my original patch was fixing in fact as it's possible for the allocator to put a 64-bit resource in the M32 region.
quoted hunk ↗ jump to hunk
@@ -3096,7 +3103,7 @@ static resource_size_tpnv_pci_window_alignment(struct pci_bus *bus, * alignment for any 64-bit resource, PCIe doesn't care and * bridges only do 64-bit prefetchable anyway. */ - if (phb->ioda.m64_segsize && (type & IORESOURCE_MEM_64)) + if (phb->ioda.m64_segsize && pnv_pci_is_m64_flags(type)) return phb->ioda.m64_segsize;
I disagree similarly. 64-bit non-prefetchable resources should live in the M64 space as well.
if (type & IORESOURCE_MEM) return phb->ioda.m32_segsize;
Something seems to be deeply wrong here and this patch looks to me that it's just papering over the problem in way that could bring back the bugs I've seen if the generic allocator decides to put things in the M32 window. We need to look at this more closely and understand WTF that code intends means to do. Cheers, Ben.