ACPI / processors: allow a processor device _UID to be a string

RFC PATCH In review Al Stone 3 participants 6 messages Jun 10, 2019 – Jun 11, 2019

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Al Stone

author

2019-06-10 20:07 UTC #0 lore raw

RFC PATCH 1/1 ACPI / processors: allow a processor device _UID to be a string

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 <[email protected]>

---
 drivers/acpi/acpi_processor.c | 57 ++++++++++++++++++++++++++++++-----
 include/acpi/processor.h      |  1 +
 2 files changed, 51 insertions(+), 7 deletions(-)

drivers/acpi/acpi_processor.c +50 -7

index 449d86d39965..f40163e0edf9 100644
@@ -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 */

include/acpi/processor.h +1 -0

index 1194a4c78d55..115e9eee830b 100644
@@ -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;

Signature · 1 line

2.21.0

Sudeep Holla

2019-06-11 12:53 UTC #1 lore raw

Quoted 8 lines · Al Stone

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:

Quoted 9 lines

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

Al Stone

author

2019-06-11 16:03 UTC #2 lore raw

Quoted 32 lines · Sudeep Holla

Quoted 8 lines · Al Stone
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:
Quoted 9 lines
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.

Quoted 9 lines

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.

Signature · 8 lines

ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------

Sudeep Holla

2019-06-11 16:11 UTC #3 lore raw

Quoted 56 lines · Al Stone

Quoted 32 lines · Sudeep Holla
Quoted 8 lines · Al Stone
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:
Quoted 9 lines
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.
Quoted 9 lines
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

Quoted 8 lines

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.

Quoted 8 lines

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

Zhang Rui

2019-06-11 16:26 UTC #4 lore raw

Quoted 96 lines · Sudeep Holla

Quoted 74 lines · Al Stone
Quoted 47 lines · Sudeep Holla
Quoted 14 lines · Al Stone
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)
Quoted 9 lines
  {
	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.
Quoted 6 lines
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
Quoted 5 lines
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.
Quoted 4 lines
If we consider the PPTT too, then yeah, _UID must be an integer for some devices.
Quoted 8 lines
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.

Quoted 3 lines

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

Quoted 3 lines

--
Regards,
Sudeep

Al Stone

author

2019-06-11 16:27 UTC #5 lore raw

In reply to Sudeep Holla

Quoted 61 lines · Sudeep Holla

Quoted 60 lines · Al Stone
Quoted 32 lines · Sudeep Holla
Quoted 8 lines · Al Stone
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:
Quoted 9 lines
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.
Quoted 9 lines
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

Quoted 8 lines

Quoted 4 lines
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.

Quoted 11 lines

Quoted 8 lines
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.

Signature · 8 lines

ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
[email protected]
-----------------------------------