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

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.=A0=A0This broke SR-IOV as t=

he

quoted

m64
check in pnv_pci_ioda_fixup_iov_resources() fails.
=20
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 un=
der
a prefetchable region (window), but any BAR can live under non-prefetchab=
le
region (window).

quoted

Revert these cases to the previous behaviour, adding a new helper
function
to do so.=A0=A0This is named pnv_pci_is_m64_flags() to make it clear t=

his

quoted

function is only looking at resource flags and should not be relied
on for
non-SRIOV resources.
=20
Fixes: 5958d19a143e ("Fix incorrect PE reservation attempt on some
64-bit BARs")
Reported-by: Alexey Kardashevskiy <redacted>
Signed-off-by: Russell Currey <redacted>
---
=A0arch/powerpc/platforms/powernv/pci-ioda.c | 11 +++++++++--
=A01 file changed, 9 insertions(+), 2 deletions(-)
=20

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)
=A0		r->start < (phb->ioda.m64_base + phb-

quoted

ioda.m64_size));

=A0}
=A0
+static inline bool pnv_pci_is_m64_flags(unsigned long
resource_flags)
+{
+	unsigned long flags =3D (IORESOURCE_MEM_64 |
IORESOURCE_PREFETCH);
+
+	return (resource_flags & flags) =3D=3D flags;
+}
=20

I don't agree. See below.

quoted

=A0static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, in=

t

quoted

pe_no)
=A0{
=A0	phb->ioda.pe_array[pe_no].phb =3D phb;

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

pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
=A0		res =3D &pdev->resource[i + PCI_IOV_RESOURCES];
=A0		if (!res->flags || res->parent)
=A0			continue;
-		if (!pnv_pci_is_m64(phb, res)) {
+		if (!pnv_pci_is_m64_flags(res->flags)) {
=A0			dev_warn(&pdev->dev, "Don't support SR-IOV
with"
=A0					" non M64 VF BAR%d: %pR.
\n",
=A0				=A0i, 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

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

pnv_pci_window_alignment(struct pci_bus *bus,
=A0	=A0* alignment for any 64-bit resource, PCIe doesn't care and
=A0	=A0* bridges only do 64-bit prefetchable anyway.
=A0	=A0*/
-	if (phb->ioda.m64_segsize && (type & IORESOURCE_MEM_64))
+	if (phb->ioda.m64_segsize && pnv_pci_is_m64_flags(type))
=A0		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

=A0	if (type & IORESOURCE_MEM)
=A0		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.

Thanks,
Gavin

Cheers,
Ben.