* Re: [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs
2019-06-05 3:38 [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs Alexey Kardashevskiy
@ 2019-06-05 4:47 ` Sam Bobroff
2019-06-05 5:24 ` Oliver
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Sam Bobroff @ 2019-06-05 4:47 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Shawn Anastasio, Michael Roth, Oliver O'Halloran,
linuxppc-dev, David Gibson
[-- Attachment #1: Type: text/plain, Size: 3337 bytes --]
On Wed, Jun 05, 2019 at 01:38:14PM +1000, Alexey Kardashevskiy wrote:
> When the firmware does PCI BAR resource allocation, it passes the assigned
> addresses and flags (prefetch/64bit/...) via the "reg" property of
> a PCI device device tree node so the kernel does not need to do
> resource allocation.
>
> The flags are stored in resource::flags - the lower byte stores
> PCI_BASE_ADDRESS_SPACE/etc bits and the other bytes are IORESOURCE_IO/etc.
> Some flags from PCI_BASE_ADDRESS_xxx and IORESOURCE_xxx are duplicated,
> such as PCI_BASE_ADDRESS_MEM_PREFETCH/PCI_BASE_ADDRESS_MEM_TYPE_64/etc.
> When parsing the "reg" property, we copy the prefetch flag but we skip
> on PCI_BASE_ADDRESS_MEM_TYPE_64 which leaves the flags out of sync.
>
> The missing IORESOURCE_MEM_64 flag comes into play under 2 conditions:
> 1. we remove PCI_PROBE_ONLY for pseries (by hacking pSeries_setup_arch()
> or by passing "/chosen/linux,pci-probe-only");
> 2. we request resource alignment (by passing pci=resource_alignment=
> via the kernel cmd line to request PAGE_SIZE alignment or defining
> ppc_md.pcibios_default_alignment which returns anything but 0). Note that
> the alignment requests are ignored if PCI_PROBE_ONLY is enabled.
>
> With 1) and 2), the generic PCI code in the kernel unconditionally
> decides to:
> - reassign the BARs in pci_specified_resource_alignment() (works fine)
> - write new BARs to the device - this fails for 64bit BARs as the generic
> code looks at IORESOURCE_MEM_64 (not set) and writes only lower 32bits
> of the BAR and leaves the upper 32bit unmodified which breaks BAR mapping
> in the hypervisor.
>
> This fixes the issue by copying the flag. This is useful if we want to
> enforce certain BAR alignment per platform as handling subpage sized BARs
> is proven to cause problems with hotplug (SLOF already aligns BARs to 64k).
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> This code is there for ages (from 200x) hence no "Fixes:".
>
> Ideally I want to enforce /chosen/linux,pci-probe-only in QEMU as
> at the moment:
> - pci=resource_alignment= alone does not do anything;
> - /chosen/linux,pci-probe-only alone does not cause the kernel to
> reassign resources;
> - pci=resource_alignment= with /chosen/linux,pci-probe-only is broken
> anyway.
Looks good to me. I gave it a quick test for regressions, with a host
and QEMU guest (with some passed-through devices) both using the patch
and it seemed fine.
Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
> ---
> arch/powerpc/kernel/pci_of_scan.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index 24191ea2d9a7..64ad92016b63 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -45,6 +45,8 @@ unsigned int pci_parse_of_flags(u32 addr0, int bridge)
> if (addr0 & 0x02000000) {
> flags = IORESOURCE_MEM | PCI_BASE_ADDRESS_SPACE_MEMORY;
> flags |= (addr0 >> 22) & PCI_BASE_ADDRESS_MEM_TYPE_64;
> + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> + flags |= IORESOURCE_MEM_64;
> flags |= (addr0 >> 28) & PCI_BASE_ADDRESS_MEM_TYPE_1M;
> if (addr0 & 0x40000000)
> flags |= IORESOURCE_PREFETCH
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs
2019-06-05 3:38 [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs Alexey Kardashevskiy
2019-06-05 4:47 ` Sam Bobroff
@ 2019-06-05 5:24 ` Oliver
2019-06-05 8:12 ` Shawn Anastasio
2019-06-30 8:37 ` Michael Ellerman
3 siblings, 0 replies; 5+ messages in thread
From: Oliver @ 2019-06-05 5:24 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Shawn Anastasio, Sam Bobroff, Michael Roth, linuxppc-dev,
David Gibson
On Wed, Jun 5, 2019 at 1:38 PM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
> When the firmware does PCI BAR resource allocation, it passes the assigned
> addresses and flags (prefetch/64bit/...) via the "reg" property of
> a PCI device device tree node so the kernel does not need to do
> resource allocation.
>
> The flags are stored in resource::flags - the lower byte stores
> PCI_BASE_ADDRESS_SPACE/etc bits and the other bytes are IORESOURCE_IO/etc.
> Some flags from PCI_BASE_ADDRESS_xxx and IORESOURCE_xxx are duplicated,
> such as PCI_BASE_ADDRESS_MEM_PREFETCH/PCI_BASE_ADDRESS_MEM_TYPE_64/etc.
> When parsing the "reg" property, we copy the prefetch flag but we skip
> on PCI_BASE_ADDRESS_MEM_TYPE_64 which leaves the flags out of sync.
>
> The missing IORESOURCE_MEM_64 flag comes into play under 2 conditions:
> 1. we remove PCI_PROBE_ONLY for pseries (by hacking pSeries_setup_arch()
> or by passing "/chosen/linux,pci-probe-only");
> 2. we request resource alignment (by passing pci=resource_alignment=
> via the kernel cmd line to request PAGE_SIZE alignment or defining
> ppc_md.pcibios_default_alignment which returns anything but 0). Note that
> the alignment requests are ignored if PCI_PROBE_ONLY is enabled.
>
> With 1) and 2), the generic PCI code in the kernel unconditionally
> decides to:
> - reassign the BARs in pci_specified_resource_alignment() (works fine)
> - write new BARs to the device - this fails for 64bit BARs as the generic
> code looks at IORESOURCE_MEM_64 (not set) and writes only lower 32bits
> of the BAR and leaves the upper 32bit unmodified which breaks BAR mapping
> in the hypervisor.
>
> This fixes the issue by copying the flag. This is useful if we want to
> enforce certain BAR alignment per platform as handling subpage sized BARs
> is proven to cause problems with hotplug (SLOF already aligns BARs to 64k).
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> This code is there for ages (from 200x) hence no "Fixes:".
>
> Ideally I want to enforce /chosen/linux,pci-probe-only in QEMU as
> at the moment:
> - pci=resource_alignment= alone does not do anything;
> - /chosen/linux,pci-probe-only alone does not cause the kernel to
> reassign resources;
> - pci=resource_alignment= with /chosen/linux,pci-probe-only is broken
> anyway.
>
> ---
> arch/powerpc/kernel/pci_of_scan.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index 24191ea2d9a7..64ad92016b63 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -45,6 +45,8 @@ unsigned int pci_parse_of_flags(u32 addr0, int bridge)
> if (addr0 & 0x02000000) {
> flags = IORESOURCE_MEM | PCI_BASE_ADDRESS_SPACE_MEMORY;
> flags |= (addr0 >> 22) & PCI_BASE_ADDRESS_MEM_TYPE_64;
> + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> + flags |= IORESOURCE_MEM_64;
> flags |= (addr0 >> 28) & PCI_BASE_ADDRESS_MEM_TYPE_1M;
> if (addr0 & 0x40000000)
> flags |= IORESOURCE_PREFETCH
> --
> 2.17.1
Seems like an oversight that PROBE_ONLY has been papering over for years.
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs
2019-06-05 3:38 [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs Alexey Kardashevskiy
2019-06-05 4:47 ` Sam Bobroff
2019-06-05 5:24 ` Oliver
@ 2019-06-05 8:12 ` Shawn Anastasio
2019-06-30 8:37 ` Michael Ellerman
3 siblings, 0 replies; 5+ messages in thread
From: Shawn Anastasio @ 2019-06-05 8:12 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Sam Bobroff, Michael Roth, Oliver O'Halloran, David Gibson
On 6/4/19 10:38 PM, Alexey Kardashevskiy wrote:
> When the firmware does PCI BAR resource allocation, it passes the assigned
> addresses and flags (prefetch/64bit/...) via the "reg" property of
> a PCI device device tree node so the kernel does not need to do
> resource allocation.
>
> The flags are stored in resource::flags - the lower byte stores
> PCI_BASE_ADDRESS_SPACE/etc bits and the other bytes are IORESOURCE_IO/etc.
> Some flags from PCI_BASE_ADDRESS_xxx and IORESOURCE_xxx are duplicated,
> such as PCI_BASE_ADDRESS_MEM_PREFETCH/PCI_BASE_ADDRESS_MEM_TYPE_64/etc.
> When parsing the "reg" property, we copy the prefetch flag but we skip
> on PCI_BASE_ADDRESS_MEM_TYPE_64 which leaves the flags out of sync.
>
> The missing IORESOURCE_MEM_64 flag comes into play under 2 conditions:
> 1. we remove PCI_PROBE_ONLY for pseries (by hacking pSeries_setup_arch()
> or by passing "/chosen/linux,pci-probe-only");
> 2. we request resource alignment (by passing pci=resource_alignment=
> via the kernel cmd line to request PAGE_SIZE alignment or defining
> ppc_md.pcibios_default_alignment which returns anything but 0). Note that
> the alignment requests are ignored if PCI_PROBE_ONLY is enabled.
>
> With 1) and 2), the generic PCI code in the kernel unconditionally
> decides to:
> - reassign the BARs in pci_specified_resource_alignment() (works fine)
> - write new BARs to the device - this fails for 64bit BARs as the generic
> code looks at IORESOURCE_MEM_64 (not set) and writes only lower 32bits
> of the BAR and leaves the upper 32bit unmodified which breaks BAR mapping
> in the hypervisor.
>
> This fixes the issue by copying the flag. This is useful if we want to
> enforce certain BAR alignment per platform as handling subpage sized BARs
> is proven to cause problems with hotplug (SLOF already aligns BARs to 64k).
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>
> This code is there for ages (from 200x) hence no "Fixes:".
>
> Ideally I want to enforce /chosen/linux,pci-probe-only in QEMU as
> at the moment:
> - pci=resource_alignment= alone does not do anything;
> - /chosen/linux,pci-probe-only alone does not cause the kernel to
> reassign resources;
> - pci=resource_alignment= with /chosen/linux,pci-probe-only is broken
> anyway.
>
> ---
> arch/powerpc/kernel/pci_of_scan.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index 24191ea2d9a7..64ad92016b63 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -45,6 +45,8 @@ unsigned int pci_parse_of_flags(u32 addr0, int bridge)
> if (addr0 & 0x02000000) {
> flags = IORESOURCE_MEM | PCI_BASE_ADDRESS_SPACE_MEMORY;
> flags |= (addr0 >> 22) & PCI_BASE_ADDRESS_MEM_TYPE_64;
> + if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
> + flags |= IORESOURCE_MEM_64;
> flags |= (addr0 >> 28) & PCI_BASE_ADDRESS_MEM_TYPE_1M;
> if (addr0 & 0x40000000)
> flags |= IORESOURCE_PREFETCH
>
I have confirmed that this fixes the case with PCI_PROBE_ONLY
disabled and a ppc_md.pcibios_default_alignment implementation that
returns PAGE_SIZE.
Reviewed-by: Shawn Anastasio <shawn@anastas.io>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs
2019-06-05 3:38 [PATCH kernel] powerpc/pci/of: Fix OF flags parsing for 64bit BARs Alexey Kardashevskiy
` (2 preceding siblings ...)
2019-06-05 8:12 ` Shawn Anastasio
@ 2019-06-30 8:37 ` Michael Ellerman
3 siblings, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2019-06-30 8:37 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev
Cc: Shawn Anastasio, Alexey Kardashevskiy, Michael Roth, Sam Bobroff,
Oliver O'Halloran, David Gibson
On Wed, 2019-06-05 at 03:38:14 UTC, Alexey Kardashevskiy wrote:
> When the firmware does PCI BAR resource allocation, it passes the assigned
> addresses and flags (prefetch/64bit/...) via the "reg" property of
> a PCI device device tree node so the kernel does not need to do
> resource allocation.
>
> The flags are stored in resource::flags - the lower byte stores
> PCI_BASE_ADDRESS_SPACE/etc bits and the other bytes are IORESOURCE_IO/etc.
> Some flags from PCI_BASE_ADDRESS_xxx and IORESOURCE_xxx are duplicated,
> such as PCI_BASE_ADDRESS_MEM_PREFETCH/PCI_BASE_ADDRESS_MEM_TYPE_64/etc.
> When parsing the "reg" property, we copy the prefetch flag but we skip
> on PCI_BASE_ADDRESS_MEM_TYPE_64 which leaves the flags out of sync.
>
> The missing IORESOURCE_MEM_64 flag comes into play under 2 conditions:
> 1. we remove PCI_PROBE_ONLY for pseries (by hacking pSeries_setup_arch()
> or by passing "/chosen/linux,pci-probe-only");
> 2. we request resource alignment (by passing pci=resource_alignment=
> via the kernel cmd line to request PAGE_SIZE alignment or defining
> ppc_md.pcibios_default_alignment which returns anything but 0). Note that
> the alignment requests are ignored if PCI_PROBE_ONLY is enabled.
>
> With 1) and 2), the generic PCI code in the kernel unconditionally
> decides to:
> - reassign the BARs in pci_specified_resource_alignment() (works fine)
> - write new BARs to the device - this fails for 64bit BARs as the generic
> code looks at IORESOURCE_MEM_64 (not set) and writes only lower 32bits
> of the BAR and leaves the upper 32bit unmodified which breaks BAR mapping
> in the hypervisor.
>
> This fixes the issue by copying the flag. This is useful if we want to
> enforce certain BAR alignment per platform as handling subpage sized BARs
> is proven to cause problems with hotplug (SLOF already aligns BARs to 64k).
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Reviewed-by: Sam Bobroff <sbobroff@linux.ibm.com>
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
> Reviewed-by: Shawn Anastasio <shawn@anastas.io>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/df5be5be8735ef2ae80d5ae1f2453cd81a035c4b
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread