On Mon, 2016-09-19 at 16:37 +1000, Russell Currey wrote:

On Wed, 2016-09-14 at 21:30 +1000, Gavin Shan wrote:

quoted

On Wed, Sep 14, 2016 at 05:51:08PM +1000, Benjamin Herrenschmidt wrote:

quoted

On Wed, 2016-09-14 at 16:37 +1000, Russell Currey wrote:

quoted

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 ?

Ben, what I understood for long time: non-prefetchable BAR cannot live under
a prefetchable region (window), but any BAR can live under non-prefetchable
region (window).

That is actually no longer true on PCIe I think. I need to double check but I
believe PCIe allows it because PCIe bridges aren't allowed to prefetch. 

That being said, our alignment hook is for bridge regions, and in that case, well,
the only 64-bit window is prefetchable...

quoted

quoted

quoted

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")

quoted

quoted

quoted

quoted

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.c

b/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)

quoted

quoted

quoted

quoted

 		r->start < (phb->ioda.m64_base + phb-

ioda.m64_size));

 }
 
+static inline bool pnv_pci_is_m64_flags(unsigned long
resource_flags)
+{

quoted

quoted

quoted

quoted

+	unsigned long flags = (IORESOURCE_MEM_64 |

IORESOURCE_PREFETCH);
+

quoted

quoted

quoted

quoted

+	return (resource_flags & flags) == flags;

+}

I don't agree. See below.

quoted

 static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, int
pe_no)
 {

quoted

quoted

quoted

quoted

 	phb->ioda.pe_array[pe_no].phb = phb;

@@ -2871,7 +2878,7 @@ static void

pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)

quoted

quoted

quoted

quoted

 		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"

quoted

quoted

quoted

quoted

 					" non M64 VF BAR%d: %pR.

\n",

quoted

quoted

quoted

quoted

 				 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.

This function is called before the resoureces are resized and assigned.
So using the resource's start/end addresses to judge it's in M64 or M32
windows are not reliable. Currently, all IOV BARs is required to have
(IORESOURCE_64 | PREFETCHABLE) which is covered by bridge's M64 window
and PHB's M64 windows (BARs).

quoted

quoted

@@ -3096,7 +3103,7 @@ static resource_size_t

pnv_pci_window_alignment(struct pci_bus *bus,

quoted

quoted

quoted

quoted

 	 * 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.

As I understood, 64-bits non-prefetchable BARs cannot live behind
M64 (64-bits prefetchable) windows.

quoted

quoted

quoted

quoted

quoted

quoted

 	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.

Yeah, it seems it partially reverts your changes. The start/end addresses
are usable after resource resizing/assignment is finished. Before that,
we still need to use the flags.

I agree with Ben that we need to look at this more closely to find a proper fix
rather than this hacky partial revert, but for now it's important that we fix
SR-IOV and thus I think this patch should be carried forward.

Yes, this might be enough for 4.8

This patch is a bandaid, but I believe completely fixing the underlying problem
is not achievable given we're at rc7. 

As a side note, I am going to prototype a heavy refactor of the allocation code
that simplifies things from an EEH perspective and allows us to use more generic
PCI code.

quoted

Thanks,
Gavin

quoted

Cheers,
Ben.