From: Russell Currey <hidden> Date: 2016-09-14 06:37:29
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.
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(-)
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.
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>
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
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(-)
pnv_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
@@ -3096,7 +3103,7 @@ static resource_size_t
pnv_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.
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
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
From: Russell Currey <hidden> Date: 2016-09-19 06:37:51
On Wed, 2016-09-14 at 21:30 +1000, Gavin Shan wrote:
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).
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")
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(-)
pnv_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.
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,
* 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
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.
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.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2016-09-19 10:45:46
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>
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.
From: Michael Ellerman <hidden> Date: 2016-09-25 03:33:27
On Wed, 2016-14-09 at 06:37:17 UTC, 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.
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>
Tested-by: Alexey Kardashevskiy <redacted>