| Message ID | 20190610200734.1182-1-ahs3@redhat.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] ACPI / processors: allow a processor device _UID to be a string | expand |
On Mon, Jun 10, 2019 at 02:07:34PM -0600, Al Stone wrote: > In the ACPI specification, section 6.1.12, a _UID may be either an > integer or a string object. Up until now, when defining processor > Device()s in ACPI (_HID ACPI0007), only integers were allowed even > though this ignored the specification. As a practical matter, it > was not an issue. > > Recently, some DSDTs have shown up that look like this: > > Device (XX00) > { > Name (_HID, "ACPI0007" /* Processor Device */) > Name (_UID, "XYZZY-XX00") > ..... > } > > which is perfectly legal. However, the kernel will report instead: > I am not sure how this can be perfectly legal from specification perspective. It's legal with respect to AML namespace but then the other condition of this matching with entries in static tables like MADT is not possible where there are declared to be simple 4 byte integer/word. Same is true for even ACPI0010, the processor container objects which need to match entries in PPTT, ACPI Processor UID(in MADT): The OS associates this GICC(applies even for APIC and family) Structure with a processor device object in the namespace when the _UID child object of the processor device evaluates to a numeric value that matches the numeric value in this field. So for me that indicates it can't be string unless you have some ways to match those _UID entries to ACPI Processor ID in MADT and PPTT. Let me know if I am missing to consider something here. -- Regards, Sudeep
On 6/11/19 6:53 AM, Sudeep Holla wrote: > On Mon, Jun 10, 2019 at 02:07:34PM -0600, Al Stone wrote: >> In the ACPI specification, section 6.1.12, a _UID may be either an >> integer or a string object. Up until now, when defining processor >> Device()s in ACPI (_HID ACPI0007), only integers were allowed even >> though this ignored the specification. As a practical matter, it >> was not an issue. >> >> Recently, some DSDTs have shown up that look like this: >> >> Device (XX00) >> { >> Name (_HID, "ACPI0007" /* Processor Device */) >> Name (_UID, "XYZZY-XX00") >> ..... >> } >> >> which is perfectly legal. However, the kernel will report instead: >> > > I am not sure how this can be perfectly legal from specification > perspective. It's legal with respect to AML namespace but then the > other condition of this matching with entries in static tables like > MADT is not possible where there are declared to be simple 4 byte > integer/word. Same is true for even ACPI0010, the processor container > objects which need to match entries in PPTT, > > ACPI Processor UID(in MADT): The OS associates this GICC(applies even > for APIC and family) Structure with a processor device object in > the namespace when the _UID child object of the processor device > evaluates to a numeric value that matches the numeric value in this > field. > > So for me that indicates it can't be string unless you have some ways to > match those _UID entries to ACPI Processor ID in MADT and PPTT. > > Let me know if I am missing to consider something here. > > -- > Regards, > Sudeep > Harumph. I think what we have here is a big mess in the spec, but that is exactly why this is an RFC. The MADT can have any of ~16 different subtables, as you note. Of those, only these require a numeric _UID: -- Type 0x0: Processor Local APIC -- Type 0x4: Local APIC NMI [0] -- Type 0x7: Processor Local SAPIC [1] -- Type 0x9: Processor Local x2APIC -- Type 0xa: Local x2APIC NMI [0] -- Type 0xb: GICC Note [0]: a value of !0x0 is also allowed, indicating all processors [1]: this has two fields that could be interpreted as an ID when used together It does not appear that you could build a usable system without any of these subtables -- but perhaps someone knows of incantations that could -- which is why I thought a string _UID might be viable. If we consider the PPTT too, then yeah, _UID must be an integer for some devices. Thanks for the feedback; it forced me to double-check my thinking about the MADT. The root cause of the issue is not the kernel in this case, but a lack of clarity in the spec -- or at least implied requirements that probably need to be explicit. I'll send in a spec change.
On Tue, Jun 11, 2019 at 10:03:15AM -0600, Al Stone wrote: > On 6/11/19 6:53 AM, Sudeep Holla wrote: > > On Mon, Jun 10, 2019 at 02:07:34PM -0600, Al Stone wrote: > >> In the ACPI specification, section 6.1.12, a _UID may be either an > >> integer or a string object. Up until now, when defining processor > >> Device()s in ACPI (_HID ACPI0007), only integers were allowed even > >> though this ignored the specification. As a practical matter, it > >> was not an issue. > >> > >> Recently, some DSDTs have shown up that look like this: > >> > >> Device (XX00) > >> { > >> Name (_HID, "ACPI0007" /* Processor Device */) > >> Name (_UID, "XYZZY-XX00") > >> ..... > >> } > >> > >> which is perfectly legal. However, the kernel will report instead: > >> > > > > I am not sure how this can be perfectly legal from specification > > perspective. It's legal with respect to AML namespace but then the > > other condition of this matching with entries in static tables like > > MADT is not possible where there are declared to be simple 4 byte > > integer/word. Same is true for even ACPI0010, the processor container > > objects which need to match entries in PPTT, > > > > ACPI Processor UID(in MADT): The OS associates this GICC(applies even > > for APIC and family) Structure with a processor device object in > > the namespace when the _UID child object of the processor device > > evaluates to a numeric value that matches the numeric value in this > > field. > > > > So for me that indicates it can't be string unless you have some ways to > > match those _UID entries to ACPI Processor ID in MADT and PPTT. > > > > Let me know if I am missing to consider something here. > > > > -- > > Regards, > > Sudeep > > > > Harumph. I think what we have here is a big mess in the spec, but > that is exactly why this is an RFC. > > The MADT can have any of ~16 different subtables, as you note. Of > those, only these require a numeric _UID: > > -- Type 0x0: Processor Local APIC > -- Type 0x4: Local APIC NMI [0] > -- Type 0x7: Processor Local SAPIC [1] > -- Type 0x9: Processor Local x2APIC > -- Type 0xa: Local x2APIC NMI [0] > -- Type 0xb: GICC > > Note [0]: a value of !0x0 is also allowed, indicating all processors > [1]: this has two fields that could be interpreted as an ID when > used together > > It does not appear that you could build a usable system without any > of these subtables -- but perhaps someone knows of incantations that > could -- which is why I thought a string _UID might be viable. > I hope no one is shipping such device yet or am I wrong ? We can ask them to fix as Linux simply can't boot on such system or even if it boots, it may have issues with acpi_processor drivers. > If we consider the PPTT too, then yeah, _UID must be an integer for > some devices. > > Thanks for the feedback; it forced me to double-check my thinking about > the MADT. The root cause of the issue is not the kernel in this case, > but a lack of clarity in the spec -- or at least implied requirements > that probably need to be explicit. I'll send in a spec change. > Completely agreed. Even little more clarification on this is helpful. Thanks for volunteering :) to take up spec change, much appreciated. -- Regards, Sudeep
On 二, 2019-06-11 at 17:11 +0100, Sudeep Holla wrote: > On Tue, Jun 11, 2019 at 10:03:15AM -0600, Al Stone wrote: > > > > On 6/11/19 6:53 AM, Sudeep Holla wrote: > > > > > > On Mon, Jun 10, 2019 at 02:07:34PM -0600, Al Stone wrote: > > > > > > > > In the ACPI specification, section 6.1.12, a _UID may be either > > > > an > > > > integer or a string object. Up until now, when defining > > > > processor > > > > Device()s in ACPI (_HID ACPI0007), only integers were allowed > > > > even > > > > though this ignored the specification. As a practical matter, > > > > it > > > > was not an issue. > > > > > > > > Recently, some DSDTs have shown up that look like this: > > > > > > > > Device (XX00) > > > > { > > > > Name (_HID, "ACPI0007" /* Processor Device */) > > > > Name (_UID, "XYZZY-XX00") > > > > ..... > > > > } > > > > > > > > which is perfectly legal. However, the kernel will report > > > > instead: > > > > > > > I am not sure how this can be perfectly legal from specification > > > perspective. It's legal with respect to AML namespace but then > > > the > > > other condition of this matching with entries in static tables > > > like > > > MADT is not possible where there are declared to be simple 4 byte > > > integer/word. Same is true for even ACPI0010, the processor > > > container > > > objects which need to match entries in PPTT, > > > > > > ACPI Processor UID(in MADT): The OS associates this GICC(applies > > > even > > > for APIC and family) Structure with a processor device object in > > > the namespace when the _UID child object of the processor device > > > evaluates to a numeric value that matches the numeric value in > > > this > > > field. > > > > > > So for me that indicates it can't be string unless you have some > > > ways to > > > match those _UID entries to ACPI Processor ID in MADT and PPTT. > > > > > > Let me know if I am missing to consider something here. > > > > > > -- > > > Regards, > > > Sudeep > > > > > Harumph. I think what we have here is a big mess in the spec, but > > that is exactly why this is an RFC. > > > > The MADT can have any of ~16 different subtables, as you note. Of > > those, only these require a numeric _UID: > > > > -- Type 0x0: Processor Local APIC > > -- Type 0x4: Local APIC NMI [0] > > -- Type 0x7: Processor Local SAPIC [1] > > -- Type 0x9: Processor Local x2APIC > > -- Type 0xa: Local x2APIC NMI [0] > > -- Type 0xb: GICC > > > > Note [0]: a value of !0x0 is also allowed, indicating all > > processors > > [1]: this has two fields that could be interpreted as an ID > > when > > used together > > > > It does not appear that you could build a usable system without any > > of these subtables -- but perhaps someone knows of incantations > > that > > could -- which is why I thought a string _UID might be viable. > > > I hope no one is shipping such device yet or am I wrong ? > We can ask them to fix as Linux simply can't boot on such system or > even if it boots, it may have issues with acpi_processor drivers. > > > > > If we consider the PPTT too, then yeah, _UID must be an integer for > > some devices. > > > > Thanks for the feedback; it forced me to double-check my thinking > > about > > the MADT. The root cause of the issue is not the kernel in this > > case, > > but a lack of clarity in the spec -- or at least implied > > requirements > > that probably need to be explicit. I'll send in a spec change. > > > Completely agreed. Even little more clarification on this is helpful. > Thanks for volunteering :) to take up spec change, much appreciated. > hmmm, we've run into the same problem, and I think the problem is that 1. this is a BIOS bug, because we do need numeric _UID when using "Device" object, because we need to use it as a reference to find the processor APIC ID in MADT. Thus a BIOS fix is indeed needed in this case. 2. Although ACPI spec has made "Processor" object deprecated, it does not provide a clear ASL example about how to use "Device" object, plus the clarification of this is in the MADT section instead of the "Declare Processors" section, which could be easy overlooked, thus I totally agree with you that we need some spec change here. Thanks! -rui > -- > Regards, > Sudeep
On 6/11/19 10:11 AM, Sudeep Holla wrote: > On Tue, Jun 11, 2019 at 10:03:15AM -0600, Al Stone wrote: >> On 6/11/19 6:53 AM, Sudeep Holla wrote: >>> On Mon, Jun 10, 2019 at 02:07:34PM -0600, Al Stone wrote: >>>> In the ACPI specification, section 6.1.12, a _UID may be either an >>>> integer or a string object. Up until now, when defining processor >>>> Device()s in ACPI (_HID ACPI0007), only integers were allowed even >>>> though this ignored the specification. As a practical matter, it >>>> was not an issue. >>>> >>>> Recently, some DSDTs have shown up that look like this: >>>> >>>> Device (XX00) >>>> { >>>> Name (_HID, "ACPI0007" /* Processor Device */) >>>> Name (_UID, "XYZZY-XX00") >>>> ..... >>>> } >>>> >>>> which is perfectly legal. However, the kernel will report instead: >>>> >>> >>> I am not sure how this can be perfectly legal from specification >>> perspective. It's legal with respect to AML namespace but then the >>> other condition of this matching with entries in static tables like >>> MADT is not possible where there are declared to be simple 4 byte >>> integer/word. Same is true for even ACPI0010, the processor container >>> objects which need to match entries in PPTT, >>> >>> ACPI Processor UID(in MADT): The OS associates this GICC(applies even >>> for APIC and family) Structure with a processor device object in >>> the namespace when the _UID child object of the processor device >>> evaluates to a numeric value that matches the numeric value in this >>> field. >>> >>> So for me that indicates it can't be string unless you have some ways to >>> match those _UID entries to ACPI Processor ID in MADT and PPTT. >>> >>> Let me know if I am missing to consider something here. >>> >>> -- >>> Regards, >>> Sudeep >>> >> >> Harumph. I think what we have here is a big mess in the spec, but >> that is exactly why this is an RFC. >> >> The MADT can have any of ~16 different subtables, as you note. Of >> those, only these require a numeric _UID: >> >> -- Type 0x0: Processor Local APIC >> -- Type 0x4: Local APIC NMI [0] >> -- Type 0x7: Processor Local SAPIC [1] >> -- Type 0x9: Processor Local x2APIC >> -- Type 0xa: Local x2APIC NMI [0] >> -- Type 0xb: GICC >> >> Note [0]: a value of !0x0 is also allowed, indicating all processors >> [1]: this has two fields that could be interpreted as an ID when >> used together >> >> It does not appear that you could build a usable system without any >> of these subtables -- but perhaps someone knows of incantations that >> could -- which is why I thought a string _UID might be viable. >> > > I hope no one is shipping such device yet or am I wrong ? > We can ask them to fix as Linux simply can't boot on such system or > even if it boots, it may have issues with acpi_processor drivers. I don't think it's shipping, but even if it is, I'm going to have to insist they fix their tables, just as a practical matter. I need to ask if it boots that other OS, too. >> If we consider the PPTT too, then yeah, _UID must be an integer for >> some devices. >> >> Thanks for the feedback; it forced me to double-check my thinking about >> the MADT. The root cause of the issue is not the kernel in this case, >> but a lack of clarity in the spec -- or at least implied requirements >> that probably need to be explicit. I'll send in a spec change. >> > > Completely agreed. Even little more clarification on this is helpful. > Thanks for volunteering :) to take up spec change, much appreciated. No problem, and glad to do it.
diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 449d86d39965..f40163e0edf9 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -14,6 +14,7 @@ */ #include <linux/acpi.h> +#include <linux/atomic.h> #include <linux/device.h> #include <linux/kernel.h> #include <linux/module.h> @@ -32,6 +33,8 @@ ACPI_MODULE_NAME("processor"); DEFINE_PER_CPU(struct acpi_processor *, processors); EXPORT_PER_CPU_SYMBOL(processors); +static atomic_t last_used_string_cpuid; + /* -------------------------------------------------------------------------- Errata Handling -------------------------------------------------------------------------- */ @@ -229,6 +232,49 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr) } #endif /* CONFIG_ACPI_HOTPLUG_CPU */ +static acpi_status acpi_processor_evaluate_uid(struct acpi_device *device) +{ + acpi_status status = AE_OK; + struct acpi_processor *pr; + union acpi_object object = { 0 }; + struct acpi_buffer buffer = { sizeof(union acpi_object), &object }; + + if (!device) + return -ENODEV; + + /* + * Declared with "Device" statement; match _UID. + * + * By definition in the ACPI spec, _UID may return either + * an integer or a string. + */ + pr = acpi_driver_data(device); + if (!pr) + return -ENODEV; + + status = acpi_evaluate_object(pr->handle, METHOD_NAME__UID, + NULL, &buffer); + if (ACPI_FAILURE(status)) { + ACPI_FREE(buffer.pointer); + return status; + } + + if (object.type == ACPI_TYPE_INTEGER) { + pr->acpi_id = object.integer.value; + pr->acpi_name = NULL; + ACPI_FREE(buffer.pointer); /* no longer needed */ + } else if (object.type == ACPI_TYPE_STRING) { + /* no need to be clever, just pick an unused number */ + pr->acpi_id = atomic_inc_return(&last_used_string_cpuid); + pr->acpi_name = object.string.pointer; /* still needed */ + } else { + ACPI_FREE(buffer.pointer); + return AE_BAD_DATA; + } + + return status; +} + static int acpi_processor_get_info(struct acpi_device *device) { union acpi_object object = { 0 }; @@ -265,12 +311,8 @@ static int acpi_processor_get_info(struct acpi_device *device) pr->acpi_id = object.processor.proc_id; } else { - /* - * Declared with "Device" statement; match _UID. - * Note that we don't handle string _UIDs yet. - */ - status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID, - NULL, &value); + /* Declared with "Device" statement; match _UID. */ + status = acpi_processor_evaluate_uid(device); if (ACPI_FAILURE(status)) { dev_err(&device->dev, "Failed to evaluate processor _UID (0x%x)\n", @@ -278,7 +320,6 @@ static int acpi_processor_get_info(struct acpi_device *device) return -ENODEV; } device_declaration = 1; - pr->acpi_id = value; } if (acpi_duplicate_processor_id(pr->acpi_id)) { @@ -435,6 +476,7 @@ static int acpi_processor_add(struct acpi_device *device, device->driver_data = NULL; per_cpu(processors, pr->id) = NULL; err_free_pr: + kfree(pr->acpi_name); kfree(pr); return result; } @@ -484,6 +526,7 @@ static void acpi_processor_remove(struct acpi_device *device) out: free_cpumask_var(pr->throttling.shared_cpu_map); + kfree(pr->acpi_name); kfree(pr); } #endif /* CONFIG_ACPI_HOTPLUG_CPU */ diff --git a/include/acpi/processor.h b/include/acpi/processor.h index 1194a4c78d55..115e9eee830b 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -216,6 +216,7 @@ struct acpi_processor_flags { struct acpi_processor { acpi_handle handle; u32 acpi_id; + char *acpi_name; /* only used if _UID is a string, otherwise null */ phys_cpuid_t phys_id; /* CPU hardware ID such as APIC ID for x86 */ u32 id; /* CPU logical ID allocated by OS */ u32 pblk;
In the ACPI specification, section 6.1.12, a _UID may be either an integer or a string object. Up until now, when defining processor Device()s in ACPI (_HID ACPI0007), only integers were allowed even though this ignored the specification. As a practical matter, it was not an issue. Recently, some DSDTs have shown up that look like this: Device (XX00) { Name (_HID, "ACPI0007" /* Processor Device */) Name (_UID, "XYZZY-XX00") ..... } which is perfectly legal. However, the kernel will report instead: ... [ 0.706970] ACPI: \_SB_.XXXX.XX00: Invalid processor object ... [ 0.776998] acpi ACPI0007:xx: Failed to evaluate processor _UID (0xc) ... The second message comes from acpi_processor_get_info() not being able to use the string value for _UID that was provided in the DSDT; the 0xc in the message is actually the buffer size of the returned value from trying to evaluate _UID. We correct this by first adding a field called acpi_name to struct acpi_processor. Then we add the function acpi_processor_evaluate_uid() to get the returned _UID object and, if it is an integer, behave exactly as acpi_processor_get_info() did before, but if it is a string, assign it an arbitrary integer value (required by all the users of this struct) and capture the actual string used. This results in the functions acpi_processor_add() and acpi_processor_remove() changing also so that we do not inadvertently forget to free the space used for the new acpi_name field. Why RFC and not a straight patch? I have some concerns that need broader knowledge than I have at my disposal: 1) Is there a need to expose the CPU name captured from the ACPI namespace in sysfs (or elsewhere)? I could not think of any good reason but I probably just missed something. 2) Is making last_used_string_cpuid atomic required? I think it is, but only because I think acpi_processor_add() could get called on multiple CPUs at roughly the same time. If that never happens, an int would be just fine. 3) Is it necessary to check that a _UID string is a duplicate of the _UID string of some other CPU Device() (_HID ACPI0007)? The code will ensure unique integer IDs, and the firmware writer should not have provided duplicate string _UIDs, but things happen. I would need to add this code to this patch. 4) Testing: I only have one very fragile machine to test this on (the firmware is really, really unstable right now); more testing would be greatly appreciated. Signed-off-by: Al Stone <ahs3@redhat.com> --- drivers/acpi/acpi_processor.c | 57 ++++++++++++++++++++++++++++++----- include/acpi/processor.h | 1 + 2 files changed, 51 insertions(+), 7 deletions(-)