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

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

[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)
  {
	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(-)

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;
-- 
2.21.0

Re: [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string

[Sudeep Holla]

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

Re: [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string

[Al Stone]

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.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string

[Sudeep Holla]

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

Re: [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string

[Zhang Rui]

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

Re: [RFC PATCH] ACPI / processors: allow a processor device _UID to be a string

[Al Stone]

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.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------