Per spec, the Consumer/Producer bit is defined only for Extended Address Space descriptors and should be ignored for QWord/DWord/Word Address Space descriptors. My understanding is that this is because x86 BIOSes didn't use the bit consistently, so it couldn't be relied upon. The Extended descriptors were added later and are presumably more reliable.
Linux currently looks at Consumer/Producer for all Address Space descriptors, although we don't use the result very much. The x86 and ia64 host bridge driver code doesn't use Consumer/Producer to identify windows; it assumes all descriptors are windows.
We do currently use Consumer/Producer to decide whether to apply _TRA. The change in these patches is to ignore Consumer/Producer for QWord/DWord/Word, so we'll apply _TRA from Consumer descriptors where we didn't before. Per spec, that should be safe because _TRA is required to be zero for Consumer resources.
If we want to describe host bridge register space and ECAM space directly in the PNP0A03 device on ARM64 (and I guess potentially even on x86 & ia64), I think we need changes like this so the arch code can use IORESOURCE_WINDOW to tell which descriptors are windows and which are registers.
This is a subtle area, so please take a look and let me know what you think.
These patches are based on v4.9-rc1. It's getting pretty late in the cycle, but I think we'd really like to get the ARM64 ACPI PCI story sorted out for the v4.10 merge window.
---
Bjorn Helgaas (2): ACPI: Combine acpi_dev_resource_address_space() and acpi_dev_resource_ext_address_space() ACPI: Ignore Consumer/Producer for QWord/DWord/Word Address Space
drivers/acpi/ioapic.c | 3 - drivers/acpi/resource.c | 130 ++++++++++++++++++---------------------- drivers/pnp/pnpacpi/rsparser.c | 3 - include/linux/acpi.h | 2 - 4 files changed, 59 insertions(+), 79 deletions(-)
Previously acpi_dev_resource_address_space() and acpi_dev_resource_ext_address_space() were wrappers that called acpi_decode_space(). We need to distinguish between Word/DWord/QWord address descriptors and Extended address descriptors, which was impossible in acpi_decode_space().
Fold the acpi_dev_resource_address_space() and acpi_dev_resource_ext_address_space() functionality into acpi_decode_space() and call the result acpi_dev_resource_address_space().
No functional change intended.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/acpi/ioapic.c | 3 - drivers/acpi/resource.c | 114 ++++++++++++++++------------------------ drivers/pnp/pnpacpi/rsparser.c | 3 - include/linux/acpi.h | 2 - 4 files changed, 48 insertions(+), 74 deletions(-)
diff --git a/drivers/acpi/ioapic.c b/drivers/acpi/ioapic.c index 6d7ce6e..6d91417 100644 --- a/drivers/acpi/ioapic.c +++ b/drivers/acpi/ioapic.c @@ -50,8 +50,7 @@ static acpi_status setup_res(struct acpi_resource *acpi_res, void *data) return AE_OK;
if (!acpi_dev_resource_memory(acpi_res, res)) { - if (acpi_dev_resource_address_space(acpi_res, &win) || - acpi_dev_resource_ext_address_space(acpi_res, &win)) + if (acpi_dev_resource_address_space(acpi_res, &win)) *res = win.res; } if ((res->flags & IORESOURCE_PREFETCH) || diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 56241eb..2732d39e 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -197,16 +197,54 @@ bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res) } EXPORT_SYMBOL_GPL(acpi_dev_resource_io);
-static bool acpi_decode_space(struct resource_win *win, - struct acpi_resource_address *addr, - struct acpi_address64_attribute *attr) +/** + * acpi_dev_resource_address_space - Extract ACPI address space information. + * @ares: Input ACPI resource object. + * @win: Output generic resource object. + * + * Check if the given ACPI resource object represents an address space resource + * and if that's the case, use the information in it to populate the generic + * resource object pointed to by @win. + * + * Return: + * 1) false with win->res.flags setting to zero: not the expected resource type + * 2) false with IORESOURCE_DISABLED in win->res.flags: valid unassigned + * resource + * 3) true: valid assigned resource + XXX check these return values + */ +bool acpi_dev_resource_address_space(struct acpi_resource *ares, + struct resource_win *win) { - u8 iodec = attr->granularity == 0xfff ? ACPI_DECODE_10 : ACPI_DECODE_16; - bool wp = addr->info.mem.write_protect; - u64 len = attr->address_length; - u64 start, end, offset = 0; + struct acpi_resource_address *addr; + struct acpi_address64_attribute *attr; + struct acpi_resource_extended_address64 *ext_addr; + struct acpi_resource_address64 addr64; + acpi_status status; + u8 iodec; + bool wp; + u64 len, start, end, offset = 0; struct resource *res = &win->res;
+ win->res.flags = 0; + + if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64) { + ext_addr = &ares->data.ext_address64; + addr = (struct acpi_resource_address *)ext_addr; + attr = &ext_addr->address; + } else { + status = acpi_resource_to_address64(ares, &addr64); + if (ACPI_FAILURE(status)) + return false; + + addr = (struct acpi_resource_address *)&addr64; + attr = &addr64.address; + } + + iodec = attr->granularity == 0xfff ? ACPI_DECODE_10 : ACPI_DECODE_16; + wp = addr->info.mem.write_protect; + len = attr->address_length; + /* * Filter out invalid descriptor according to ACPI Spec 5.0, section * 6.4.3.5 Address Space Resource Descriptors. @@ -264,68 +302,9 @@ static bool acpi_decode_space(struct resource_win *win,
return !(res->flags & IORESOURCE_DISABLED); } - -/** - * acpi_dev_resource_address_space - Extract ACPI address space information. - * @ares: Input ACPI resource object. - * @win: Output generic resource object. - * - * Check if the given ACPI resource object represents an address space resource - * and if that's the case, use the information in it to populate the generic - * resource object pointed to by @win. - * - * Return: - * 1) false with win->res.flags setting to zero: not the expected resource type - * 2) false with IORESOURCE_DISABLED in win->res.flags: valid unassigned - * resource - * 3) true: valid assigned resource - */ -bool acpi_dev_resource_address_space(struct acpi_resource *ares, - struct resource_win *win) -{ - struct acpi_resource_address64 addr; - - win->res.flags = 0; - if (ACPI_FAILURE(acpi_resource_to_address64(ares, &addr))) - return false; - - return acpi_decode_space(win, (struct acpi_resource_address *)&addr, - &addr.address); -} EXPORT_SYMBOL_GPL(acpi_dev_resource_address_space);
/** - * acpi_dev_resource_ext_address_space - Extract ACPI address space information. - * @ares: Input ACPI resource object. - * @win: Output generic resource object. - * - * Check if the given ACPI resource object represents an extended address space - * resource and if that's the case, use the information in it to populate the - * generic resource object pointed to by @win. - * - * Return: - * 1) false with win->res.flags setting to zero: not the expected resource type - * 2) false with IORESOURCE_DISABLED in win->res.flags: valid unassigned - * resource - * 3) true: valid assigned resource - */ -bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares, - struct resource_win *win) -{ - struct acpi_resource_extended_address64 *ext_addr; - - win->res.flags = 0; - if (ares->type != ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64) - return false; - - ext_addr = &ares->data.ext_address64; - - return acpi_decode_space(win, (struct acpi_resource_address *)ext_addr, - &ext_addr->address); -} -EXPORT_SYMBOL_GPL(acpi_dev_resource_ext_address_space); - -/** * acpi_dev_irq_flags - Determine IRQ resource flags. * @triggering: Triggering type as provided by ACPI. * @polarity: Interrupt polarity as provided by ACPI. @@ -542,8 +521,7 @@ static acpi_status acpi_dev_process_resource(struct acpi_resource *ares,
if (acpi_dev_resource_memory(ares, res) || acpi_dev_resource_io(ares, res) - || acpi_dev_resource_address_space(ares, &win) - || acpi_dev_resource_ext_address_space(ares, &win)) + || acpi_dev_resource_address_space(ares, &win)) return acpi_dev_new_resource_entry(&win, c);
for (i = 0; acpi_dev_resource_interrupt(ares, i, res); i++) { diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c index 4b717c6..bc61651 100644 --- a/drivers/pnp/pnpacpi/rsparser.c +++ b/drivers/pnp/pnpacpi/rsparser.c @@ -184,8 +184,7 @@ static acpi_status pnpacpi_allocated_resource(struct acpi_resource *res, struct resource *r = &win.res; int i, flags;
- if (acpi_dev_resource_address_space(res, &win) - || acpi_dev_resource_ext_address_space(res, &win)) { + if (acpi_dev_resource_address_space(res, &win)) { pnp_add_resource(dev, &win.res); return AE_OK; } diff --git a/include/linux/acpi.h b/include/linux/acpi.h index ddbeda6..a06a877 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -399,8 +399,6 @@ bool acpi_dev_resource_memory(struct acpi_resource *ares, struct resource *res); bool acpi_dev_resource_io(struct acpi_resource *ares, struct resource *res); bool acpi_dev_resource_address_space(struct acpi_resource *ares, struct resource_win *win); -bool acpi_dev_resource_ext_address_space(struct acpi_resource *ares, - struct resource_win *win); unsigned long acpi_dev_irq_flags(u8 triggering, u8 polarity, u8 shareable); unsigned int acpi_dev_get_irq_type(int triggering, int polarity); bool acpi_dev_resource_interrupt(struct acpi_resource *ares, int index,
Per ACPI spec r6.0, sec 6.4.3.5.1, 2, 3, Bit [0] of General Flags (the Consumer/Producer bit) should be ignored for QWord/DWord/Word Address Space descriptors. The Consumer/Producer bit is defined only for the Extended Address Space descriptor.
Ignore Consumer/Producer except for Extended Address Space descriptors.
Note that for QWord/DWord/Word descriptors, we previously applied the translation offset (_TRA) only when the Consumer/Producer bit was set. This patch changes that: for those descriptors, we ignore Consumer/Producer and always apply the translation offset.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com --- drivers/acpi/resource.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 2732d39e..b45cd8f 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -261,11 +261,16 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, * primary side. Non-bridge devices must list 0 for all Address * Translation offset bits. */ - if (addr->producer_consumer == ACPI_PRODUCER) + if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64) { + if (addr->producer_consumer == ACPI_PRODUCER) + offset = attr->translation_offset; + else if (attr->translation_offset) + pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n", + attr->translation_offset); + } else { offset = attr->translation_offset; - else if (attr->translation_offset) - pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n", - attr->translation_offset); + } + start = attr->minimum + offset; end = attr->maximum + offset;
@@ -294,7 +299,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, return false; }
- if (addr->producer_consumer == ACPI_PRODUCER) + if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 && + addr->producer_consumer == ACPI_PRODUCER) res->flags |= IORESOURCE_WINDOW;
if (addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
On Tue, Nov 29, 2016 at 12:43:34PM -0600, Bjorn Helgaas wrote:
Per ACPI spec r6.0, sec 6.4.3.5.1, 2, 3, Bit [0] of General Flags (the Consumer/Producer bit) should be ignored for QWord/DWord/Word Address Space descriptors. The Consumer/Producer bit is defined only for the Extended Address Space descriptor.
Ignore Consumer/Producer except for Extended Address Space descriptors.
Note that for QWord/DWord/Word descriptors, we previously applied the translation offset (_TRA) only when the Consumer/Producer bit was set.
"..Consumer/Producer bit was clear" ? If that bit was set:
struct acpi_resource_address->producer_consumer == ACPI_CONSUMER
and we are not applying the _TRA offset in that case, right ?
Thanks, Lorenzo
This patch changes that: for those descriptors, we ignore Consumer/Producer and always apply the translation offset.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com
drivers/acpi/resource.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 2732d39e..b45cd8f 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -261,11 +261,16 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, * primary side. Non-bridge devices must list 0 for all Address * Translation offset bits. */
- if (addr->producer_consumer == ACPI_PRODUCER)
- if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64) {
if (addr->producer_consumer == ACPI_PRODUCER)offset = attr->translation_offset;else if (attr->translation_offset)pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",attr->translation_offset);- } else { offset = attr->translation_offset;
- else if (attr->translation_offset)
pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",attr->translation_offset);
- }
- start = attr->minimum + offset; end = attr->maximum + offset;
@@ -294,7 +299,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, return false; }
- if (addr->producer_consumer == ACPI_PRODUCER)
- if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 &&
addr->producer_consumer == ACPI_PRODUCER)if (addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
-- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 30, 2016 at 12:04:25PM +0000, Lorenzo Pieralisi wrote:
On Tue, Nov 29, 2016 at 12:43:34PM -0600, Bjorn Helgaas wrote:
Per ACPI spec r6.0, sec 6.4.3.5.1, 2, 3, Bit [0] of General Flags (the Consumer/Producer bit) should be ignored for QWord/DWord/Word Address Space descriptors. The Consumer/Producer bit is defined only for the Extended Address Space descriptor.
Ignore Consumer/Producer except for Extended Address Space descriptors.
Note that for QWord/DWord/Word descriptors, we previously applied the translation offset (_TRA) only when the Consumer/Producer bit was set.
"..Consumer/Producer bit was clear" ? If that bit was set:
struct acpi_resource_address->producer_consumer == ACPI_CONSUMER
and we are not applying the _TRA offset in that case, right ?
Right, of course. How about this instead?
Note that for QWord/DWord/Word descriptors, we previously applied the translation offset (_TRA) only for Producers, i.e., when the Consumer/ Producer bit was clear. This patch changes that: for those descriptors, we ignore Consumer/Producer and always apply the translation offset.
This patch changes that: for those descriptors, we ignore Consumer/Producer and always apply the translation offset.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com
drivers/acpi/resource.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 2732d39e..b45cd8f 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -261,11 +261,16 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, * primary side. Non-bridge devices must list 0 for all Address * Translation offset bits. */
- if (addr->producer_consumer == ACPI_PRODUCER)
- if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64) {
if (addr->producer_consumer == ACPI_PRODUCER)offset = attr->translation_offset;else if (attr->translation_offset)pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",attr->translation_offset);- } else { offset = attr->translation_offset;
- else if (attr->translation_offset)
pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",attr->translation_offset);
- }
- start = attr->minimum + offset; end = attr->maximum + offset;
@@ -294,7 +299,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, return false; }
- if (addr->producer_consumer == ACPI_PRODUCER)
- if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 &&
addr->producer_consumer == ACPI_PRODUCER)if (addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
-- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Nov 30, 2016 at 09:56:17AM -0600, Bjorn Helgaas wrote:
On Wed, Nov 30, 2016 at 12:04:25PM +0000, Lorenzo Pieralisi wrote:
On Tue, Nov 29, 2016 at 12:43:34PM -0600, Bjorn Helgaas wrote:
Per ACPI spec r6.0, sec 6.4.3.5.1, 2, 3, Bit [0] of General Flags (the Consumer/Producer bit) should be ignored for QWord/DWord/Word Address Space descriptors. The Consumer/Producer bit is defined only for the Extended Address Space descriptor.
Ignore Consumer/Producer except for Extended Address Space descriptors.
Note that for QWord/DWord/Word descriptors, we previously applied the translation offset (_TRA) only when the Consumer/Producer bit was set.
"..Consumer/Producer bit was clear" ? If that bit was set:
struct acpi_resource_address->producer_consumer == ACPI_CONSUMER
and we are not applying the _TRA offset in that case, right ?
Right, of course. How about this instead?
Note that for QWord/DWord/Word descriptors, we previously applied the translation offset (_TRA) only for Producers, i.e., when the Consumer/ Producer bit was clear. This patch changes that: for those descriptors, we ignore Consumer/Producer and always apply the translation offset.
Yes that's a perfect description, thanks a lot !
Lorenzo
This patch changes that: for those descriptors, we ignore Consumer/Producer and always apply the translation offset.
Signed-off-by: Bjorn Helgaas bhelgaas@google.com
drivers/acpi/resource.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/resource.c b/drivers/acpi/resource.c index 2732d39e..b45cd8f 100644 --- a/drivers/acpi/resource.c +++ b/drivers/acpi/resource.c @@ -261,11 +261,16 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, * primary side. Non-bridge devices must list 0 for all Address * Translation offset bits. */
- if (addr->producer_consumer == ACPI_PRODUCER)
- if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64) {
if (addr->producer_consumer == ACPI_PRODUCER)offset = attr->translation_offset;else if (attr->translation_offset)pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",attr->translation_offset);- } else { offset = attr->translation_offset;
- else if (attr->translation_offset)
pr_debug("ACPI: translation_offset(%lld) is invalid for non-bridge device.\n",attr->translation_offset);
- }
- start = attr->minimum + offset; end = attr->maximum + offset;
@@ -294,7 +299,8 @@ bool acpi_dev_resource_address_space(struct acpi_resource *ares, return false; }
- if (addr->producer_consumer == ACPI_PRODUCER)
- if (ares->type == ACPI_RESOURCE_TYPE_EXTENDED_ADDRESS64 &&
addr->producer_consumer == ACPI_PRODUCER)if (addr->info.mem.caching == ACPI_PREFETCHABLE_MEMORY)
-- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi,
On Tue, Nov 29, 2016 at 7:43 PM, Bjorn Helgaas bhelgaas@google.com wrote:
Per spec, the Consumer/Producer bit is defined only for Extended Address Space descriptors and should be ignored for QWord/DWord/Word Address Space descriptors. My understanding is that this is because x86 BIOSes didn't use the bit consistently, so it couldn't be relied upon. The Extended descriptors were added later and are presumably more reliable.
Linux currently looks at Consumer/Producer for all Address Space descriptors, although we don't use the result very much. The x86 and ia64 host bridge driver code doesn't use Consumer/Producer to identify windows; it assumes all descriptors are windows.
We do currently use Consumer/Producer to decide whether to apply _TRA. The change in these patches is to ignore Consumer/Producer for QWord/DWord/Word, so we'll apply _TRA from Consumer descriptors where we didn't before. Per spec, that should be safe because _TRA is required to be zero for Consumer resources.
If we want to describe host bridge register space and ECAM space directly in the PNP0A03 device on ARM64 (and I guess potentially even on x86 & ia64), I think we need changes like this so the arch code can use IORESOURCE_WINDOW to tell which descriptors are windows and which are registers.
It would be good to copy/move the above paragraph to the changelog of patch [2/2].
The reason for that change is quite unclear otherwise.
This is a subtle area, so please take a look and let me know what you think.
These patches are based on v4.9-rc1. It's getting pretty late in the cycle, but I think we'd really like to get the ARM64 ACPI PCI story sorted out for the v4.10 merge window.
So it is better if changes of this sort spend a few weeks in linux-next before they get merged, just in case they trigger some obscure issue and need to be rethought.
I'm not against these changes, but I won't be entirely comfortable with them going straight into the mainline.
Thanks, Rafael
-
Bjorn Helgaas -
Bjorn Helgaas -
Lorenzo Pieralisi -
Rafael J. Wysocki