[PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[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

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

-- 
2.9.3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] drivers: pci: do not disregard parent resources starting at 0x0

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

Fixes: f44116ae8818 ("PCI: Remove pci_find_parent_resource() use for allocation")
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7904d02ffdb9..53a41b1f7ef7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -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
-- 
2.9.3

[PATCH 2/2] arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/pci.c  | 20 ++++++++++++++++++--
 include/linux/pci-acpi.h |  7 ++++---
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 4f0e3ebfea4b..c88e07e2eb84 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -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);
+	/*
+	 * Invoke the PCI device specific method (_DSM) #5 'Ignore PCI Boot
+	 * Configuration', which tells us whether the firmware wants us to
+	 * preserve the configuration of the PCI resource tree for this root
+	 * bridge.
+	 */
+	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 1,
+				IGNORE_PCI_BOOT_CONFIG_DSM, NULL);
+	if (obj && obj->type == ACPI_TYPE_INTEGER && obj->integer.value == 0) {
+		/* preserve existing resource assignment */
+		pci_bus_claim_resources(bus);
+	} 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);
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 7a4e83a8c89c..308111489b83 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -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) { }
-- 
2.9.3

Re: [PATCH 1/2] drivers: pci: do not disregard parent resources starting at 0x0

[Ard Biesheuvel]

On 11 April 2017 at 17:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02ffdb9..53a41b1f7ef7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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

Actually, I managed to confuse myself a bit here: $SUBJECT is
inaccurate, given that it is not the parent resource being disregarded
if it starts at 0x0 but the containee.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drivers: pci: do not disregard parent resources starting at 0x0

[Lorenzo Pieralisi]

[+Yinghai, Bjorn]

On Tue, Apr 11, 2017 at 05:33:12PM +0100, Ard Biesheuvel wrote:
> 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")

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.

Yinghai, Bjorn ?

Thanks,
Lorenzo

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7904d02ffdb9..53a41b1f7ef7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -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
> -- 
> 2.9.3
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

[Lorenzo Pieralisi]

On Tue, Apr 11, 2017 at 05:33:13PM +0100, Ard Biesheuvel wrote:
> 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 <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/pci.c  | 20 ++++++++++++++++++--
>  include/linux/pci-acpi.h |  7 ++++---
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 4f0e3ebfea4b..c88e07e2eb84 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -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);
> +	/*
> +	 * Invoke the PCI device specific method (_DSM) #5 'Ignore PCI Boot
> +	 * Configuration', which tells us whether the firmware wants us to
> +	 * preserve the configuration of the PCI resource tree for this root
> +	 * bridge.
> +	 */
> +	obj = acpi_evaluate_dsm(ACPI_HANDLE(bus->bridge), pci_acpi_dsm_uuid, 1,
> +				IGNORE_PCI_BOOT_CONFIG_DSM, NULL);
> +	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.

Lorenzo

> +	} 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);
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 7a4e83a8c89c..308111489b83 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -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) { }
> -- 
> 2.9.3
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

[Sinan Kaya]

On 4/12/2017 1:26 PM, Lorenzo Pieralisi wrote:
>> +	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.

IMO, DSMs are always considered optional to enable additional features
in the operating system that otherwise are not required or are not defined
in any of the PCI specs. 

I think we are abusing the DSM here. We want to require the presence of
a DSM but we want to require it to be 0 in order to have a UEFI
compatible behavior. 

I think we need to turn it the other way around by having a UEFI compatible
behavior and do reassign only if this DSM is 1.

I understand that you are worried about regressions. We can try to fix
it however time it takes before merging this.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drivers: pci: do not disregard parent resources starting at 0x0

[Ard Biesheuvel]

On 12 April 2017 at 14:24, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> [+Yinghai, Bjorn]
>
> On Tue, Apr 11, 2017 at 05:33:12PM +0100, Ard Biesheuvel wrote:
>> 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")
>
> 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.
But given that my involvement with PCI is much more recent than that,
it is very well possible that I am missing something here.

-- 
Ard.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drivers: pci: do not disregard parent resources starting at 0x0

[Lorenzo Pieralisi]

On Thu, Apr 13, 2017 at 08:39:12AM +0100, Ard Biesheuvel wrote:
> On 12 April 2017 at 14:24, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > [+Yinghai, Bjorn]
> >
> > On Tue, Apr 11, 2017 at 05:33:12PM +0100, Ard Biesheuvel wrote:
> >> 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")
> >
> > 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.

I agree with you that res->start usage changed with f44116ae8818 but
I am not sure you can call that a regression unless we prove there
was some code relying on the previous behaviour (and it is not just
x86).

Anyway, I am happy to put these two patches (with some tweaks on patch
2) on a branch for testing on ARM64 ACPI platforms to see the best
way forward.

Thanks,
Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] arm64: acpi/pci: invoke _DSM whether to preserve firmware PCI setup

[Bjorn Helgaas]

On Wed, Apr 12, 2017 at 02:03:25PM -0400, Sinan Kaya wrote:
> On 4/12/2017 1:26 PM, Lorenzo Pieralisi wrote:
> >> +	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 read that section as saying "if the _DSM doesn't exist, or if the
_DSM returns 0, the OS must preserve any BAR and bridge window
assignments done by firmware."

I was not aware of any other statement that restricted the OS from
changing assignments done by firmware, and I've always assumed that
the OS completely owns PCI devices after handoff from firmware.  But
maybe my assumption has been wrong.  And I know there are BIOSes that
do assume the OS doesn't change things, e.g., for the HP iLO, which is
used by runtime firmware.

The _DSM is generic, not arm64-specific, so if we add support for it,
I'd like to at least evaluate the _DSM in the generic code somewhere,
e.g., in drivers/pci/pci-acpi.c where we evaluate it for other
function indices.

Maybe we will still need arch-specific code to use the generic
knowledge.  Or maybe we should add x86 code to prevent reassignment
unless this _DSM returns 1, although that might break some of the
"pci=realloc" scenarios.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Bjorn Helgaas]

On Tue, Apr 11, 2017 at 05:33:11PM +0100, Ard Biesheuvel wrote:
> 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(-)

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 17 May 2017 at 22:56, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Tue, Apr 11, 2017 at 05:33:11PM +0100, Ard Biesheuvel wrote:
>> 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(-)
>
> 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.

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Bjorn Helgaas]

On Thu, May 18, 2017 at 11:59:14AM +0100, Ard Biesheuvel wrote:
> On 17 May 2017 at 22:56, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Apr 11, 2017 at 05:33:11PM +0100, Ard Biesheuvel wrote:
> >> 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(-)
> >
> > 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)

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.

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

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.

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

Bjorn

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 18 May 2017 at 15:05, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, May 18, 2017 at 11:59:14AM +0100, Ard Biesheuvel wrote:
>> On 17 May 2017 at 22:56, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Tue, Apr 11, 2017 at 05:33:11PM +0100, Ard Biesheuvel wrote:
>> >> 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(-)
>> >
>> > 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)
>
> 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?
>

Yes. It is basically

struct {
  u64 base;
  u64 size;
  enum pixelformat fmt;
}

and the driver takes ownership of the region and proceeds to use it as
a framebuffer. At runtime, there is not way to cross reference this
with other protocols from the UEFI graphics driver stack (i.e., PCI
i/o), and so moving BARs is not the only problem: on ARM systems,
[base, base + size) could well be a slice of DRAM, in which case we
should be using cacheable accesses, while a framebuffer behind a PCI
BAR requires the memory accesses to hit the PCI window for the side
effects to be noticeable.

Obviously, this is a very x86 centric arrangement, where BARs are
usually preserved, and the coherency issue does not exist.


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

The current code simply claims the BAR from a PCI_FIXUP_CLASS_HEADER()
quirk, at which time the bridge windows are not configured yet, and so
this fails unless there are no bridges to take into account. Note that
this is an improvement over the old situation, where we simply
dereferenced the framebuffer window described by efifb without any
checking at all.

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

I'd much rather take the presence of such tables as a hint that we
should leave the PCI resource allocation alone altogether.

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

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.

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Lorenzo Pieralisi]

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

[...]

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

Thanks !
Lorenzo

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
>
> [...]
>
>> >> 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).
>> >
>>
>> 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.
>

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?

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Lorenzo Pieralisi]

On Thu, May 18, 2017 at 05:51:44PM +0100, Ard Biesheuvel wrote:
> On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
> >
> > [...]
> >
> >> >> 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).
> >> >
> >>
> >> 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.
> >
> 
> 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.

Thanks a lot !
Lorenzo

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 18 May 2017 at 18:46, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, May 18, 2017 at 05:51:44PM +0100, Ard Biesheuvel wrote:
>> On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
>> >
>> > [...]
>> >
>> >> >> 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).
>> >> >
>> >>
>> >> 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.
>> >
>>
>> 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.
>

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.

Thoughts?

-- 
Ard.

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Lorenzo Pieralisi]

On Thu, Jun 01, 2017 at 03:04:09PM +0000, Ard Biesheuvel wrote:
> On 18 May 2017 at 18:46, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Thu, May 18, 2017 at 05:51:44PM +0100, Ard Biesheuvel wrote:
> >> On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >> > On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
> >> >
> >> > [...]
> >> >
> >> >> >> 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).
> >> >> >
> >> >>
> >> >> 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.
> >> >
> >>
> >> 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.
> >
> 
> 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.

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.

Thanks,
Lorenzo

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 1 June 2017 at 16:15, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, Jun 01, 2017 at 03:04:09PM +0000, Ard Biesheuvel wrote:
>> On 18 May 2017 at 18:46, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > On Thu, May 18, 2017 at 05:51:44PM +0100, Ard Biesheuvel wrote:
>> >> On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> >> > On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
>> >> >
>> >> > [...]
>> >> >
>> >> >> >> 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).
>> >> >> >
>> >> >>
>> >> >> 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.
>> >> >
>> >>
>> >> 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.
>> >
>>
>> 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.
>
> 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

> 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

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Lorenzo Pieralisi]

On Thu, Jun 01, 2017 at 04:18:54PM +0000, Ard Biesheuvel wrote:
> On 1 June 2017 at 16:15, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Thu, Jun 01, 2017 at 03:04:09PM +0000, Ard Biesheuvel wrote:
> >> On 18 May 2017 at 18:46, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >> > On Thu, May 18, 2017 at 05:51:44PM +0100, Ard Biesheuvel wrote:
> >> >> On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >> >> > On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
> >> >> >
> >> >> > [...]
> >> >> >
> >> >> >> >> 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).
> >> >> >> >
> >> >> >>
> >> >> >> 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.
> >> >> >
> >> >>
> >> >> 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.
> >> >
> >>
> >> 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.
> >
> > 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
> 
> > 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

I am not sure the call to:

pci_bus_insert_busn_res(child, max+1, 0xff);

is there to do anything useful given that the bus range resource is
claimed later but I need to debug it to prove my point.

Lorenzo

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Lorenzo Pieralisi]

Hi Ard,

On Thu, Jun 01, 2017 at 04:18:54PM +0000, Ard Biesheuvel wrote:
> On 1 June 2017 at 16:15, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Thu, Jun 01, 2017 at 03:04:09PM +0000, Ard Biesheuvel wrote:
> >> On 18 May 2017 at 18:46, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >> > On Thu, May 18, 2017 at 05:51:44PM +0100, Ard Biesheuvel wrote:
> >> >> On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> >> >> > On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
> >> >> >
> >> >> > [...]
> >> >> >
> >> >> >> >> 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).
> >> >> >> >
> >> >> >>
> >> >> >> 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.
> >> >> >
> >> >>
> >> >> 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.
> >> >
> >>
> >> 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.
> >
> > 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
> 
> > 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

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 !

Lorenzo

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 6 June 2017 at 08:59, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> Hi Ard,
>
> On Thu, Jun 01, 2017 at 04:18:54PM +0000, Ard Biesheuvel wrote:
>> On 1 June 2017 at 16:15, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > On Thu, Jun 01, 2017 at 03:04:09PM +0000, Ard Biesheuvel wrote:
>> >> On 18 May 2017 at 18:46, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> >> > On Thu, May 18, 2017 at 05:51:44PM +0100, Ard Biesheuvel wrote:
>> >> >> On 18 May 2017 at 16:47, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> >> >> > On Thu, May 18, 2017 at 04:10:28PM +0100, Ard Biesheuvel wrote:
>> >> >> >
>> >> >> > [...]
>> >> >> >
>> >> >> >> >> 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).
>> >> >> >> >
>> >> >> >>
>> >> >> >> 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.
>> >> >> >
>> >> >>
>> >> >> 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.
>> >> >
>> >>
>> >> 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.
>> >
>> > 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
>>
>> > 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
>
> 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?

Thanks,
Ard.

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Lorenzo Pieralisi]

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

[...]

> > 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?

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.

Thanks !
Lorenzo

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 6 June 2017 at 10:02, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Tue, Jun 06, 2017 at 09:14:26AM +0000, Ard Biesheuvel wrote:
>
> [...]
>
>> > 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?
>
> 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

"""
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index c7e3e6387a49..9a529c6369ac 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -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.

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Lorenzo Pieralisi]

On Wed, Jun 07, 2017 at 01:45:19PM +0000, Ard Biesheuvel wrote:
> On 6 June 2017 at 10:02, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> > On Tue, Jun 06, 2017 at 09:14:26AM +0000, Ard Biesheuvel wrote:
> >
> > [...]
> >
> >> > 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?
> >
> > 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
> 
> """
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index c7e3e6387a49..9a529c6369ac 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -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

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

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

Thanks !
Lorenzo

Re: [PATCH 0/2] arm64: acpi/pci: allow the firmware BAR configuration to be preserved

[Ard Biesheuvel]

On 12 June 2017 at 18:55, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Wed, Jun 07, 2017 at 01:45:19PM +0000, Ard Biesheuvel wrote:
>> On 6 June 2017 at 10:02, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> > On Tue, Jun 06, 2017 at 09:14:26AM +0000, Ard Biesheuvel wrote:
>> >
>> > [...]
>> >
>> >> > 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?
>> >
>> > 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
>>
>> """
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index c7e3e6387a49..9a529c6369ac 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -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
>
> (2) Why ? You have the ACPI root bridge handle at that point. Anyway
> (1) makes (2) irrelevant unfortunately.
>

OK, it wasn't clear to me that the companion device has already been
created at that point. But it doesn't matter, indeed ...

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

OK. I would still like to fix the EFI framebuffer issue, but given
that the current code is no longer unsafe, it is no longer a priority
for me, so I will revisit it when we have a better handle on this.