Re: [PATCH] powernv/pci: Fix m64 checks for SR-IOV and window alignment

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

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:
>> Commit 5958d19a143e checks for prefetchable m64 BARs by comparing the
>> addresses instead of using resource flags.=A0=A0This broke SR-IOV as t=
he
>> 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).

>
>> 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
>> 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 <aik@ozlabs.ru>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>> ---
>> =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-
>> >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.
>
>> =A0static struct pnv_ioda_pe *pnv_ioda_init_pe(struct pnv_phb *phb, in=
t
>> 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).

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

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