arm64: acpi/pci: allow the firmware BAR configuration to be preserved

Commit f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation") updated the logic that iterates over all bus resources and compares them to a given resource, in order to decide whether one is the parent of the latter.

This change inadvertently causes pci_find_parent_resource() to disregard resources starting at address 0x0, resulting in an error such as the one below on ARM systems whose I/O window starts at 0x0.

pci_bus 0000:00: root bus resource [mem 0x10000000-0x3efeffff window] pci_bus 0000:00: root bus resource [io 0x0000-0xffff window]
pci_bus 0000:00: root bus resource [mem 0x8000000000-0xffffffffff window] pci_bus 0000:00: root bus resource [bus 00-0f]
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:02.0: PCI bridge to [bus 02]
pci 0000:00:03.0: PCI bridge to [bus 03]
pci 0000:00:03.0: can't claim BAR 13 [io 0x0000-0x0fff]: no compatible bridge window pci 0000:03:01.0: can't claim BAR 0 [io 0x0000-0x001f]: no compatible bridge window

While this never happens on x86, it is perfectly legal in general for a PCI MMIO or IO window to start at address 0x0, and it was supported in the code before commit f44116ae8818.

So let's drop the test for res->start != 0; resource_contains() already checks whether [start, end) completely covers the resource, and so it should be redundant.

Fixes f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation")

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/pci.c +0 -0

index 7904d02ffdb9..53a41b1f7ef7 100644

Commit f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation") updated the logic that iterates over all bus resources and compares them to a given resource, in order to decide whether one is the parent of the latter.

This change inadvertently causes pci_find_parent_resource() to disregard resources starting at address 0x0, resulting in an error such as the one below on ARM systems whose I/O window starts at 0x0.

pci_bus 0000:00: root bus resource [mem 0x10000000-0x3efeffff window] pci_bus 0000:00: root bus resource [io 0x0000-0xffff window]
pci_bus 0000:00: root bus resource [mem 0x8000000000-0xffffffffff window] pci_bus 0000:00: root bus resource [bus 00-0f]
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:02.0: PCI bridge to [bus 02]
pci 0000:00:03.0: PCI bridge to [bus 03]
pci 0000:00:03.0: can't claim BAR 13 [io 0x0000-0x0fff]: no compatible bridge window pci 0000:03:01.0: can't claim BAR 0 [io 0x0000-0x001f]: no compatible bridge window

While this never happens on x86, it is perfectly legal in general for a PCI MMIO or IO window to start at address 0x0, and it was supported in the code before commit f44116ae8818.

So let's drop the test for res->start != 0; resource_contains() already checks whether [start, end) completely covers the resource, and so it should be redundant.

Fixes f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation")

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pci/pci.c +1 -1

index 7904d02ffdb9..53a41b1f7ef7 100644
@@ -454,7 +454,7 @@ struct resource *pci_find_parent_resource(const struct pci_dev *dev,
 	pci_bus_for_each_resource(bus, r, i) {
 		if (!r)
 			continue;
-		if (res->start && resource_contains(r, res)) {
+		if (resource_contains(r, res)) {
 
 			/*
 			 * If the window is prefetchable but the BAR is

Signature · 1 line

2.9.3

[+Yinghai, Bjorn]

Quoted 27 lines · Ard Biesheuvel

Commit f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation") updated the logic that iterates over all bus resources and compares them to a given resource, in order to decide whether one is the parent of the latter.

This change inadvertently causes pci_find_parent_resource() to disregard resources starting at address 0x0, resulting in an error such as the one below on ARM systems whose I/O window starts at 0x0.

pci_bus 0000:00: root bus resource [mem 0x10000000-0x3efeffff window] pci_bus 0000:00: root bus resource [io 0x0000-0xffff window]
pci_bus 0000:00: root bus resource [mem 0x8000000000-0xffffffffff window] pci_bus 0000:00: root bus resource [bus 00-0f]
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:02.0: PCI bridge to [bus 02]
pci 0000:00:03.0: PCI bridge to [bus 03]
pci 0000:00:03.0: can't claim BAR 13 [io 0x0000-0x0fff]: no compatible bridge window pci 0000:03:01.0: can't claim BAR 0 [io 0x0000-0x001f]: no compatible bridge window

While this never happens on x86, it is perfectly legal in general for a PCI MMIO or IO window to start at address 0x0, and it was supported in the code before commit f44116ae8818.

So let's drop the test for res->start != 0; resource_contains() already checks whether [start, end) completely covers the resource, and so it should be redundant.

Quoted 1 line

Fixes f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation")

I know this code fixes IO claiming on ARM/ARM64 (well, it fixes nothing because we never claim resources on ARM/ARM64 apart from kvmtool and generic host bridge), my _big_ worry is that it can cause endless regressions on other arches, in any case I would be really really careful about adding a Fixes: tag to it.

Quoted 30 lines · Lorenzo Pieralisi

[+Yinghai, Bjorn]

Quoted 27 lines · Ard Biesheuvel

Commit f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation") updated the logic that iterates over all bus resources and compares them to a given resource, in order to decide whether one is the parent of the latter.

This change inadvertently causes pci_find_parent_resource() to disregard resources starting at address 0x0, resulting in an error such as the one below on ARM systems whose I/O window starts at 0x0.

pci_bus 0000:00: root bus resource [mem 0x10000000-0x3efeffff window] pci_bus 0000:00: root bus resource [io 0x0000-0xffff window]
pci_bus 0000:00: root bus resource [mem 0x8000000000-0xffffffffff window] pci_bus 0000:00: root bus resource [bus 00-0f]
pci 0000:00:01.0: PCI bridge to [bus 01]
pci 0000:00:02.0: PCI bridge to [bus 02]
pci 0000:00:03.0: PCI bridge to [bus 03]
pci 0000:00:03.0: can't claim BAR 13 [io 0x0000-0x0fff]: no compatible bridge window pci 0000:03:01.0: can't claim BAR 0 [io 0x0000-0x001f]: no compatible bridge window

While this never happens on x86, it is perfectly legal in general for a PCI MMIO or IO window to start at address 0x0, and it was supported in the code before commit f44116ae8818.

So let's drop the test for res->start != 0; resource_contains() already checks whether [start, end) completely covers the resource, and so it should be redundant.

Quoted 8 lines

Quoted 1 line

Fixes f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation")

I know this code fixes IO claiming on ARM/ARM64 (well, it fixes nothing because we never claim resources on ARM/ARM64 apart from kvmtool and generic host bridge), my _big_ worry is that it can cause endless regressions on other arches, in any case I would be really really careful about adding a Fixes: tag to it.

The patch is only 3 years old, and is obviously a regression given that the change in behavior described here occurs as a side effect.

On arm64 ACPI systems, we unconditionally reconfigure the entire PCI hierarchy at boot. This is a departure from what is customary on ACPI systems, and may break assumptions in some places (e.g., EFIFB), that the kernel will leave BARs of enabled PCI devices where they are.

Given that PCI already specifies a device specific ACPI method (_DSM) for PCI root bridge nodes that tells us whether the firmware thinks the configuration should be left alone, let's sidestep the entire policy debate about whether the PCI configuration should be preserved or not, and put it under the control of the firmware instead.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/pci.c  | 20 ++++++++++++++++++--
 include/linux/pci-acpi.h |  7 ++++---
 2 files changed, 22 insertions(+), 5 deletions(-)

arch/arm64/kernel/pci.c +2 -2

index 4f0e3ebfea4b..c88e07e2eb84 100644
@@ -185,6 +185,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	struct acpi_pci_generic_root_info *ri;
 	struct pci_bus *bus, *child;
 	struct acpi_pci_root_ops *root_ops;
+	union acpi_object *obj;
 
 	ri = kzalloc_node(sizeof(*ri), GFP_KERNEL, node);
 	if (!ri)

@@ -208,8 +209,23 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
 	if (!bus)
 		return NULL;
 
-	pci_bus_size_bridges(bus);
-	pci_bus_assign_resources(bus);
+	/*

arch/arm64/kernel/pci.c +6 -0

+	} else {
+		/* reconfigure the resource tree from scratch */
+		pci_bus_size_bridges(bus);
+		pci_bus_assign_resources(bus);
+	}
+	ACPI_FREE(obj);
 
 	list_for_each_entry(child, &bus->children, node)
 		pcie_bus_configure_settings(child);

include/linux/pci-acpi.h +4 -3

index 7a4e83a8c89c..308111489b83 100644
@@ -106,9 +106,10 @@ static inline void acpiphp_check_host_bridge(struct acpi_device *adev) { }
 #endif
 
 extern const u8 pci_acpi_dsm_uuid[];
-#define DEVICE_LABEL_DSM	0x07
-#define RESET_DELAY_DSM		0x08
-#define FUNCTION_DELAY_DSM	0x09
+#define IGNORE_PCI_BOOT_CONFIG_DSM	0x05
+#define DEVICE_LABEL_DSM		0x07
+#define RESET_DELAY_DSM			0x08
+#define FUNCTION_DELAY_DSM		0x09
 
 #else	/* CONFIG_ACPI */
 static inline void acpi_pci_add_bus(struct pci_bus *bus) { }

Signature · 1 line

2.9.3

arch/arm64/kernel/pci.c struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) · line 222

Quoted 3 lines

arch/arm64/kernel/pci.c +3 -0

+	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) {
+		/* preserve existing resource assignment */
+		pci_bus_claim_resources(bus);

Ok. By the PCI FW specs, we should also assign unassigned resources here (ie resources that can't be claimed). Furthermore, by the PCI FW spec, if !obj this is the path we should be taking (PCI firmware specification Rev 3.1, 4.6.5, Description: 0:)

"...This situation is the same as the legacy situation where this _DSM is not provided."

which makes me think that the PCI FW specification expects FW set-up to be claimed on boot, it is my interpretation.

I wonder how many UEFI developers know this _DSM even exists. If we want to enforce it at least we should mandate its usage at SBSA level, it is a major change and I want to understand the reasons behind it, so far, as I said, I may see why this was needed on x86 but on ARM64 I really don't.

Quoted 8 lines · Lorenzo Pieralisi

arch/arm64/kernel/pci.c struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) · line 222

Quoted 3 lines

arch/arm64/kernel/pci.c +3 -0

+	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) {
+		/* preserve existing resource assignment */
+		pci_bus_claim_resources(bus);

Ok. By the PCI FW specs, we should also assign unassigned resources here (ie resources that can't be claimed). Furthermore, by the PCI FW spec, if !obj this is the path we should be taking (PCI firmware specification Rev 3.1, 4.6.5, Description: 0:)

Quoted 5 lines

"...This situation is the same as the legacy situation where this _DSM is not provided."

which makes me think that the PCI FW specification expects FW set-up to be claimed on boot, it is my interpretation.

This is a followup to the discussion regarding whether ACPI/arm64 systems should preserve the PCI configuration performed by the firmware, or always reconfigure it from scratch.

This series proposes to put it under the control of the firmware, by invoking the _DSM method, and preserving the firmware configuration only when it returns 0.

Patch #1 is a preparatory bugfix that solves the issue that I/O bridge windows starting at 0x0 are dismissed when looking for parent resources.

Patch #2 adds the logic to invoke the _DSM and claim the existing configuration rather than reallocate it from scratch if it returns '0'

Ard Biesheuvel (2):
drivers: pci: do not disregard parent resources starting at 0x0
arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

 arch/arm64/kernel/pci.c  | 20 ++++++++++++++++++--
 drivers/pci/pci.c        |  2 +-
 include/linux/pci-acpi.h |  7 ++++---
 3 files changed, 23 insertions(+), 6 deletions(-)

Quoted 18 lines · Ard Biesheuvel

This is a followup to the discussion regarding whether ACPI/arm64 systems should preserve the PCI configuration performed by the firmware, or always reconfigure it from scratch.

This series proposes to put it under the control of the firmware, by invoking the _DSM method, and preserving the firmware configuration only when it returns 0.

Patch #1 is a preparatory bugfix that solves the issue that I/O bridge windows starting at 0x0 are dismissed when looking for parent resources.

Patch #2 adds the logic to invoke the _DSM and claim the existing configuration rather than reallocate it from scratch if it returns '0'

Ard Biesheuvel (2):
drivers: pci: do not disregard parent resources starting at 0x0
arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

Quoted 4 lines

 arch/arm64/kernel/pci.c  | 20 ++++++++++++++++++--
 drivers/pci/pci.c        |  2 +-
 include/linux/pci-acpi.h |  7 ++++---
 3 files changed, 23 insertions(+), 6 deletions(-)

I applied the first to pci/resource for v4.13. I didn't do anything with the second because I don't think we have consensus on what the right thing to do is yet.

Quoted 19 lines · Bjorn Helgaas

Quoted 18 lines · Ard Biesheuvel

This is a followup to the discussion regarding whether ACPI/arm64 systems should preserve the PCI configuration performed by the firmware, or always reconfigure it from scratch.

This series proposes to put it under the control of the firmware, by invoking the _DSM method, and preserving the firmware configuration only when it returns 0.

Patch #1 is a preparatory bugfix that solves the issue that I/O bridge windows starting at 0x0 are dismissed when looking for parent resources.

Patch #2 adds the logic to invoke the _DSM and claim the existing configuration rather than reallocate it from scratch if it returns '0'

Ard Biesheuvel (2):
drivers: pci: do not disregard parent resources starting at 0x0
arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

Quoted 8 lines

Quoted 4 lines

 arch/arm64/kernel/pci.c  | 20 ++++++++++++++++++--
 drivers/pci/pci.c        |  2 +-
 include/linux/pci-acpi.h |  7 ++++---
 3 files changed, 23 insertions(+), 6 deletions(-)

I applied the first to pci/resource for v4.13. I didn't do anything with the second because I don't think we have consensus on what the right thing to do is yet.

Thanks Bjorn.

Re PCI reconfiguration: currently, the EFI framebuffer is broken on all systems where the associated BAR lives on a device that is not on bus 0, which is of course everything other than QEMU (which cannot use the EFI framebuffer for other reasons when running under KVM)

So the first question is whether anyone cares. I do, but not deeply:
the EFI framebuffer, while useful in cases where there is no real GFC driver, is a bit of a hack anyway.

Then we have the question whether we can work around it in another
way: I already proposed an alternative patch which simply records the associated BAR before PCI is reconfigured, and refuses to enable EFIFB if the BAR has moves. This is a workaround rather than a real fix, but in many cases, the kernel and UEFI appear to arrive at similar conclusions regarding where to put the framebuffer BAR, probably because of the simple reason that its size (ergo alignment) will cause it to be allocated first.

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 29 lines · Ard Biesheuvel

Quoted 19 lines · Bjorn Helgaas

Quoted 18 lines · Ard Biesheuvel

This is a followup to the discussion regarding whether ACPI/arm64 systems should preserve the PCI configuration performed by the firmware, or always reconfigure it from scratch.

This series proposes to put it under the control of the firmware, by invoking the _DSM method, and preserving the firmware configuration only when it returns 0.

Patch #1 is a preparatory bugfix that solves the issue that I/O bridge windows starting at 0x0 are dismissed when looking for parent resources.

Patch #2 adds the logic to invoke the _DSM and claim the existing configuration rather than reallocate it from scratch if it returns '0'

Ard Biesheuvel (2):
drivers: pci: do not disregard parent resources starting at 0x0
arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

Quoted 8 lines

Quoted 4 lines

 arch/arm64/kernel/pci.c  | 20 ++++++++++++++++++--
 drivers/pci/pci.c        |  2 +-
 include/linux/pci-acpi.h |  7 ++++---
 3 files changed, 23 insertions(+), 6 deletions(-)

I applied the first to pci/resource for v4.13. I didn't do anything with the second because I don't think we have consensus on what the right thing to do is yet.

Quoted 6 lines

Thanks Bjorn.

Re PCI reconfiguration: currently, the EFI framebuffer is broken on all systems where the associated BAR lives on a device that is not on bus 0, which is of course everything other than QEMU (which cannot use the EFI framebuffer for other reasons when running under KVM)

I don't know much about the EFI framebuffer. It doesn't look like efifb.c does any runtime calls to EFI. Is that code basically just a way to discover the address & size of the frame buffer from EFI information?

I don't see the connection to bus 0. As long as we figure out the buffer address and configure bridges so the buffer is routed to the device, the location in the bus hierarchy shouldn't matter.

Quoted 4 lines

So the first question is whether anyone cares. I do, but not deeply:
the EFI framebuffer, while useful in cases where there is no real GFC driver, is a bit of a hack anyway.

Quoted 8 lines

Then we have the question whether we can work around it in another
way: I already proposed an alternative patch which simply records the associated BAR before PCI is reconfigured, and refuses to enable EFIFB if the BAR has moves. This is a workaround rather than a real fix, but in many cases, the kernel and UEFI appear to arrive at similar conclusions regarding where to put the framebuffer BAR, probably because of the simple reason that its size (ergo alignment) will cause it to be allocated first.

This is reminiscent of an issue with IOMMU descriptions: IIRC, DMAR and related tables are based on the current bus numbering scheme, and if we renumber buses, we have to remember the "boot-time to current" mapping to interpret the tables (I'm not sure we actually do this today). Maybe we need some sort of similar mapping for BARs.

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Quoted 39 lines · Lorenzo Pieralisi

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Quoted 3 lines

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Right. AFAICT this is the part that is typically handled by pcibios_resource_survey() et al, whose default __weak implementations are empty functions. Shall I override those for arm64 to host this logic?

Quoted 40 lines · Ard Biesheuvel

Quoted 39 lines · Lorenzo Pieralisi

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Quoted 8 lines

Quoted 3 lines

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Right. AFAICT this is the part that is typically handled by pcibios_resource_survey() et al, whose default __weak implementations are empty functions. Shall I override those for arm64 to host this logic?

I think it makes sense yes unless Bjorn spots something wrong with that but you should also call it in ARM64 pci_acpi_scan_root() since it is not called by PCI core on non-hot-added bridges, I reckon you figured that out already though.

Quoted 50 lines · Lorenzo Pieralisi

Quoted 40 lines · Ard Biesheuvel

Quoted 39 lines · Lorenzo Pieralisi

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Quoted 8 lines

Quoted 3 lines

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Right. AFAICT this is the part that is typically handled by pcibios_resource_survey() et al, whose default __weak implementations are empty functions. Shall I override those for arm64 to host this logic?

Quoted 5 lines

I think it makes sense yes unless Bjorn spots something wrong with that but you should also call it in ARM64 pci_acpi_scan_root() since it is not called by PCI core on non-hot-added bridges, I reckon you figured that out already though.

Another data point for this discussion: currently, when booting arm64 via DT, we set PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS (unless PCI_PROBE_ONLY is requested), which forces not only resource allocations but also the bus numbering to be reconfigured from scratch.

On arm64/ACPI, we never set those flags, which will cause pci_scan_bridge() to preserve the secondary and subordinate bridge numbers if they are non-zero. This actually prevents log messages like

pci_bus 0000:02: busn_res: can not insert [bus 02-ff] under [bus 00-7f] (conflicts with (null) [bus 00-7f])

which I see on AMD Seattle as well as QEMU when booting via DT (and I suspect on any DT PCI root whose bus range != {0x0 0xff>). However, it also means that we already have different behavior between ACPI and DT boot on arm64, which makes it ambiguous what the behavior should be if _DSM indicates that the configuration should not be preserved. IOW, 'reconfigure everything' currently means different things between DT and ACPI boot.

Quoted 70 lines · Ard Biesheuvel

Quoted 50 lines · Lorenzo Pieralisi

Quoted 40 lines · Ard Biesheuvel

Quoted 39 lines · Lorenzo Pieralisi

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Quoted 8 lines

Quoted 3 lines

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Right. AFAICT this is the part that is typically handled by pcibios_resource_survey() et al, whose default __weak implementations are empty functions. Shall I override those for arm64 to host this logic?

Quoted 5 lines

I think it makes sense yes unless Bjorn spots something wrong with that but you should also call it in ARM64 pci_acpi_scan_root() since it is not called by PCI core on non-hot-added bridges, I reckon you figured that out already though.

Another data point for this discussion: currently, when booting arm64 via DT, we set PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS (unless PCI_PROBE_ONLY is requested), which forces not only resource allocations but also the bus numbering to be reconfigured from scratch.

On arm64/ACPI, we never set those flags, which will cause pci_scan_bridge() to preserve the secondary and subordinate bridge numbers if they are non-zero. This actually prevents log messages like

pci_bus 0000:02: busn_res: can not insert [bus 02-ff] under [bus 00-7f] (conflicts with (null) [bus 00-7f])

Quoted 7 lines

which I see on AMD Seattle as well as QEMU when booting via DT (and I suspect on any DT PCI root whose bus range != {0x0 0xff>). However, it also means that we already have different behavior between ACPI and DT boot on arm64, which makes it ambiguous what the behavior should be if _DSM indicates that the configuration should not be preserved. IOW, 'reconfigure everything' currently means different things between DT and ACPI boot.

IMO they should mean the same thing which implies setting those flags (ie PCI_REASSIGN_ALL_RSRC has no effect though on ARM64) on ARM64 ACPI systems as a starting point and then changes in this thread can be applied on top.

Having said that, I am not sure the message you get above is really effective (not sure I undestand the net effect of that resource conflict) so I should look into this.

Quoted 79 lines · Lorenzo Pieralisi

Quoted 70 lines · Ard Biesheuvel

Quoted 50 lines · Lorenzo Pieralisi

Quoted 40 lines · Ard Biesheuvel

Quoted 39 lines · Lorenzo Pieralisi

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Quoted 8 lines

Quoted 3 lines

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Right. AFAICT this is the part that is typically handled by pcibios_resource_survey() et al, whose default __weak implementations are empty functions. Shall I override those for arm64 to host this logic?

Quoted 5 lines

I think it makes sense yes unless Bjorn spots something wrong with that but you should also call it in ARM64 pci_acpi_scan_root() since it is not called by PCI core on non-hot-added bridges, I reckon you figured that out already though.

Another data point for this discussion: currently, when booting arm64 via DT, we set PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS (unless PCI_PROBE_ONLY is requested), which forces not only resource allocations but also the bus numbering to be reconfigured from scratch.

On arm64/ACPI, we never set those flags, which will cause pci_scan_bridge() to preserve the secondary and subordinate bridge numbers if they are non-zero. This actually prevents log messages like

pci_bus 0000:02: busn_res: can not insert [bus 02-ff] under [bus 00-7f] (conflicts with (null) [bus 00-7f])

Quoted 7 lines

which I see on AMD Seattle as well as QEMU when booting via DT (and I suspect on any DT PCI root whose bus range != {0x0 0xff>). However, it also means that we already have different behavior between ACPI and DT boot on arm64, which makes it ambiguous what the behavior should be if _DSM indicates that the configuration should not be preserved. IOW, 'reconfigure everything' currently means different things between DT and ACPI boot.

Quoted 5 lines

IMO they should mean the same thing which implies setting those flags (ie PCI_REASSIGN_ALL_RSRC has no effect though on ARM64) on ARM64 ACPI systems as a starting point and then changes in this thread can be applied on top.

OK

Quoted 4 lines

Having said that, I am not sure the message you get above is really effective (not sure I undestand the net effect of that resource conflict) so I should look into this.

It appears to be behavior that is known to be incorrect but is preserved for historical reasons.

Please refer to
12d8706963f0 Revert "PCI: Make sure bus number resources stay within their parents bounds"
1820ffdccb9b PCI: Make sure bus number resources stay within their parents bounds

Quoted 79 lines · Lorenzo Pieralisi

Quoted 70 lines · Ard Biesheuvel

Quoted 50 lines · Lorenzo Pieralisi

Quoted 40 lines · Ard Biesheuvel

Quoted 39 lines · Lorenzo Pieralisi

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Quoted 8 lines

Quoted 3 lines

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Right. AFAICT this is the part that is typically handled by pcibios_resource_survey() et al, whose default __weak implementations are empty functions. Shall I override those for arm64 to host this logic?

Quoted 5 lines

I think it makes sense yes unless Bjorn spots something wrong with that but you should also call it in ARM64 pci_acpi_scan_root() since it is not called by PCI core on non-hot-added bridges, I reckon you figured that out already though.

Another data point for this discussion: currently, when booting arm64 via DT, we set PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS (unless PCI_PROBE_ONLY is requested), which forces not only resource allocations but also the bus numbering to be reconfigured from scratch.

On arm64/ACPI, we never set those flags, which will cause pci_scan_bridge() to preserve the secondary and subordinate bridge numbers if they are non-zero. This actually prevents log messages like

pci_bus 0000:02: busn_res: can not insert [bus 02-ff] under [bus 00-7f] (conflicts with (null) [bus 00-7f])

Quoted 7 lines

which I see on AMD Seattle as well as QEMU when booting via DT (and I suspect on any DT PCI root whose bus range != {0x0 0xff>). However, it also means that we already have different behavior between ACPI and DT boot on arm64, which makes it ambiguous what the behavior should be if _DSM indicates that the configuration should not be preserved. IOW, 'reconfigure everything' currently means different things between DT and ACPI boot.

Quoted 5 lines

IMO they should mean the same thing which implies setting those flags (ie PCI_REASSIGN_ALL_RSRC has no effect though on ARM64) on ARM64 ACPI systems as a starting point and then changes in this thread can be applied on top.

OK

Quoted 4 lines

Having said that, I am not sure the message you get above is really effective (not sure I undestand the net effect of that resource conflict) so I should look into this.

It appears to be behavior that is known to be incorrect but is preserved for historical reasons.

Please refer to
12d8706963f0 Revert "PCI: Make sure bus number resources stay within their parents bounds"
1820ffdccb9b PCI: Make sure bus number resources stay within their parents bounds

Hi Ard,

Quoted 93 lines · Ard Biesheuvel

Quoted 79 lines · Lorenzo Pieralisi

Quoted 70 lines · Ard Biesheuvel

Quoted 50 lines · Lorenzo Pieralisi

Quoted 40 lines · Ard Biesheuvel

Quoted 39 lines · Lorenzo Pieralisi

On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:

[...]

Quoted 13 lines

Quoted 6 lines

Quoted 5 lines

Re _DSM: I think it makes sense to honour it, because it puts the allocation under the control of the firmware, which completely removes the burden of having to reason about a policy in the kernel. That leaves the question which will be the default, but that is of minor importance IMO.

Quoted 6 lines

I agree; we should try to follow the spec unless we have a good reason not to, which argues for honoring the _DSM, so I think it's worth a try. Booting with "pci=realloc" could override the _DSM and taint the kernel (because we don't know the effect of reassigning something the firmware told us not to touch).

Quoted 3 lines

I'd like to hear Lorenzo's view on this first, but I can certainly respin my _DSM patch to take pci=realloc into account, and move the handling to generic code as well.

I agree with both of you on _DSM implementation and interpretation.

Now, if we use it correctly (ie by the FW standard) on ARM64 systems we are going to trigger regressions, that's certain (ie we can then boot with pci=realloc - still, we are breaking systems), that's the reason why for patch(2) I'd like to create a branch and send a CFT for ARM64 ACPI testing before queuing it (either I can set-up a testing branch or we ask Bjorn to do it - as you guys prefer - as long as we have a branch for people to test patch(2) on ARM64 ACPI systems).

You still need to assign resources that could not be claimed though so patch(2) still needs updating:

PCI FW spec 3.1 - 4.6.5

"...However, the operating system is free to configure the devices in this hierarchy that have not been configured by the firmware."

Quoted 8 lines

Quoted 3 lines

Which in kernel-speak it means that you have to assign resources that could not be claimed.

Right. AFAICT this is the part that is typically handled by pcibios_resource_survey() et al, whose default __weak implementations are empty functions. Shall I override those for arm64 to host this logic?

Quoted 5 lines

I think it makes sense yes unless Bjorn spots something wrong with that but you should also call it in ARM64 pci_acpi_scan_root() since it is not called by PCI core on non-hot-added bridges, I reckon you figured that out already though.

Another data point for this discussion: currently, when booting arm64 via DT, we set PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS (unless PCI_PROBE_ONLY is requested), which forces not only resource allocations but also the bus numbering to be reconfigured from scratch.

On arm64/ACPI, we never set those flags, which will cause pci_scan_bridge() to preserve the secondary and subordinate bridge numbers if they are non-zero. This actually prevents log messages like

pci_bus 0000:02: busn_res: can not insert [bus 02-ff] under [bus 00-7f] (conflicts with (null) [bus 00-7f])

Quoted 7 lines

which I see on AMD Seattle as well as QEMU when booting via DT (and I suspect on any DT PCI root whose bus range != {0x0 0xff>). However, it also means that we already have different behavior between ACPI and DT boot on arm64, which makes it ambiguous what the behavior should be if _DSM indicates that the configuration should not be preserved. IOW, 'reconfigure everything' currently means different things between DT and ACPI boot.

Quoted 5 lines

IMO they should mean the same thing which implies setting those flags (ie PCI_REASSIGN_ALL_RSRC has no effect though on ARM64) on ARM64 ACPI systems as a starting point and then changes in this thread can be applied on top.

OK

Quoted 4 lines

Having said that, I am not sure the message you get above is really effective (not sure I undestand the net effect of that resource conflict) so I should look into this.

Quoted 8 lines

It appears to be behavior that is known to be incorrect but is preserved for historical reasons.

Please refer to
12d8706963f0 Revert "PCI: Make sure bus number resources stay within their parents bounds"
1820ffdccb9b PCI: Make sure bus number resources stay within their parents bounds

Do you want me to create a branch out of these patches (inclusive of another patch to fix this bus reallocation policy discrepancy) for ARM64 folks to test ? Let me know, thanks !

Quoted 4 lines

Do you want me to create a branch out of these patches (inclusive of another patch to fix this bus reallocation policy discrepancy) for ARM64 folks to test ? Let me know, thanks !

Hi Lorenzo,

Bjorn has already picked up #1, which is now in -next. I will get back to this topic today or tomorrow, so let me respin (including the bus range fix) first, ok?

On Tue, Jun 06, 2017 at 09:14:26AM +0000, Ard Biesheuvel wrote:

[...]

Quoted 5 lines

Quoted 4 lines

Do you want me to create a branch out of these patches (inclusive of another patch to fix this bus reallocation policy discrepancy) for ARM64 folks to test ? Let me know, thanks !

Quoted 5 lines

Hi Lorenzo,

Bjorn has already picked up #1, which is now in -next. I will get back to this topic today or tomorrow, so let me respin (including the bus range fix) first, ok?

Of course, I just want to help you make progress on this, I think we need help for testing them on ARM64 systems to see how to proceed.

Quoted 9 lines · Lorenzo Pieralisi

On Tue, Jun 06, 2017 at 09:14:26AM +0000, Ard Biesheuvel wrote:

[...]

Quoted 5 lines

Quoted 4 lines

Do you want me to create a branch out of these patches (inclusive of another patch to fix this bus reallocation policy discrepancy) for ARM64 folks to test ? Let me know, thanks !

Quoted 9 lines

Quoted 5 lines

Hi Lorenzo,

Bjorn has already picked up #1, which is now in -next. I will get back to this topic today or tomorrow, so let me respin (including the bus range fix) first, ok?

Of course, I just want to help you make progress on this, I think we need help for testing them on ARM64 systems to see how to proceed.

OK, so I am hitting another issue. While it is quite simple to do this

"""

arch/arm64/kernel/pci.c +3 -0

index c7e3e6387a49..9a529c6369ac 100644
@@ -203,6 +203,9 @@
                return NULL;
        }
 
+       /* ignore bus ranges assigned by the firmware */
+       pci_add_flags(PCI_REASSIGN_ALL_BUS);
+
        root_ops->release_info = pci_acpi_generic_release_info;
        root_ops->prepare_resources = pci_acpi_root_prepare_resources;
        root_ops->pci_ops = &ri->cfg->ops->pci_ops;

"""

and get the ACPI code to behave in the same way as the DT code, including the resulting quirks, i.e.

pci_bus 0000:01: busn_res: can not insert [bus 01-ff] under [bus 00-0f] (conflicts with (null) [bus 00-0f])

there are two problems when we want to refine this to take _DSM #5 into account.

1) PCI_REASSIGN_ALL_BUS is a global flag, while _DSM is per PCI root
2) setting the flag conditionally on whether _DSM #5 allows it is difficult to achieve, given that the flag needs to be set before acpi_pci_root_create(), at which point we cannot access the _DSM yet

So I can send out the above as a separate patch, but right now, I am not entirely sure how to proceed with conditionally preserving the firmware configuration.

Quoted 47 lines · Ard Biesheuvel

Quoted 9 lines · Lorenzo Pieralisi

On Tue, Jun 06, 2017 at 09:14:26AM +0000, Ard Biesheuvel wrote:

[...]

Quoted 5 lines

Quoted 4 lines

Do you want me to create a branch out of these patches (inclusive of another patch to fix this bus reallocation policy discrepancy) for ARM64 folks to test ? Let me know, thanks !

Quoted 9 lines

Quoted 5 lines

Hi Lorenzo,

Bjorn has already picked up #1, which is now in -next. I will get back to this topic today or tomorrow, so let me respin (including the bus range fix) first, ok?

Of course, I just want to help you make progress on this, I think we need help for testing them on ARM64 systems to see how to proceed.

OK, so I am hitting another issue. While it is quite simple to do this

"""

arch/arm64/kernel/pci.c +3 -0

index c7e3e6387a49..9a529c6369ac 100644
@@ -203,6 +203,9 @@
                return NULL;
        }
 
+       /* ignore bus ranges assigned by the firmware */
+       pci_add_flags(PCI_REASSIGN_ALL_BUS);
+
        root_ops->release_info = pci_acpi_generic_release_info;
        root_ops->prepare_resources = pci_acpi_root_prepare_resources;
        root_ops->pci_ops = &ri->cfg->ops->pci_ops;

"""

and get the ACPI code to behave in the same way as the DT code, including the resulting quirks, i.e.

pci_bus 0000:01: busn_res: can not insert [bus 01-ff] under [bus 00-0f] (conflicts with (null) [bus 00-0f])

there are two problems when we want to refine this to take _DSM #5 into account.

Quoted 4 lines

1) PCI_REASSIGN_ALL_BUS is a global flag, while _DSM is per PCI root
2) setting the flag conditionally on whether _DSM #5 allows it is difficult to achieve, given that the flag needs to be set before acpi_pci_root_create(), at which point we cannot access the _DSM yet

(2) Why ? You have the ACPI root bridge handle at that point. Anyway
(1) makes (2) irrelevant unfortunately.

Quoted 3 lines

So I can send out the above as a separate patch, but right now, I am not entirely sure how to proceed with conditionally preserving the firmware configuration.

Yes, this definitely requires some focus because that's becoming a bit unwieldy to manage consistently, I will look into this.