[PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Myron Stowe]

Len, Alexey:

The following three item patch series fixes an issue with the introduction
of > 256 processor declaration support: "Allow processor to be declared with
the Device() instead of Processor()" (git SHA 11bf04c4).

The root issue is in the lsapic mapping logic of drivers/acpi/processor_core.c.
Currently, the logic tries both types of matches irregardless of declaration
type and relies on one failing.  According to the spec - lsapic mapping is
dependent on how the processor object was declared: CPUs declared using the
"Processor" statement should use the Local SAPIC's 'processor_id', and CPUs
declared using the "Device" statement should use the 'uid'.

Reference: Section 8.4 Declaring Processors; Section 5.2.11.13 Local SAPIC
Structure.  "Advanced Configuration and Power Interface Specification",
Revision 3.0b, October 2006.


[PATCH 1/3] disambiguates the processor declaration type that is currently
conflated so that subsequent logic can behave based explicitly on the
declaration's type.  I expect the disambiguation this patch introduces will
also be advantageous when extending the > 256 processor support for x86 via
x2APIC.

[PATCH 2/3] addresses the root issue as stated above.  With respect to this
patch I'm ambivalent about the 'printk' introduced in "map_lsapic_id()" -
perhaps it should be removed?

[PATCH 3/3] is non-functional white space/spelling fixes in the related code. 


While the specific fix is ia64 focused the underlying code affected is common
to both x86 and ia64.  I have tested on the following platform/namespace
combinations:
  ia64 with "Processor" type namespace processor declarations,
  ia64 with "Device" type namespace processor declarations,
  x86 with "Processor" type namespace processor declarations.

Note that this patch series does *not* handle "Device" type processor
declarations that contain a string type _UID object under the processor
device's scope (I am currently not aware of any platforms that have such to
test against).

All comments welcome.

Myron

-- 
Myron Stowe                             HP Open Source & Linux Org

[PATCH 1/3] ACPI: Disambiguate processor declaration type

[Myron Stowe]

Declaring processors in ACPI namespace can be done using either a "Processor"
definition or a "Device" definition (see section 8.4 - Declaring Processors;
"Advanced Configuration and Power Interface Specification", Revision 3.0b).
Currently the two processor declaration types are conflated.

This patch disambiguates the processor declaration's definition type enabling
subsequent code to behave uniquely based explicitly on the declaration's type.

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
CC: Alexey Starikovskiy <aystarik@gmail.com>

---
 drivers/acpi/processor_core.c |    1 +
 drivers/acpi/scan.c           |    2 +-
 include/acpi/acpi_drivers.h   |    1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================
--- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-16 19:00:37.000000000 -0600
+++ linux-2.6/drivers/acpi/processor_core.c	2008-10-21 13:11:00.000000000 -0600
@@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
 
 
 static const struct acpi_device_id processor_device_ids[] = {
+	{ACPI_PROCESSOR_OBJECT_HID, 0},
 	{ACPI_PROCESSOR_HID, 0},
 	{"", 0},
 };
Index: linux-2.6/include/acpi/acpi_drivers.h
===================================================================
--- linux-2.6.orig/include/acpi/acpi_drivers.h	2008-10-16 18:53:26.000000000 -0600
+++ linux-2.6/include/acpi/acpi_drivers.h	2008-10-20 13:23:28.000000000 -0600
@@ -41,6 +41,7 @@
  */
 
 #define ACPI_POWER_HID			"LNXPOWER"
+#define ACPI_PROCESSOR_OBJECT_HID	"ACPI_CPU"
 #define ACPI_PROCESSOR_HID		"ACPI0007"
 #define ACPI_SYSTEM_HID			"LNXSYSTM"
 #define ACPI_THERMAL_HID		"LNXTHERM"
Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c	2008-10-16 19:04:58.000000000 -0600
+++ linux-2.6/drivers/acpi/scan.c	2008-10-21 13:09:09.000000000 -0600
@@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
 		hid = ACPI_POWER_HID;
 		break;
 	case ACPI_BUS_TYPE_PROCESSOR:
-		hid = ACPI_PROCESSOR_HID;
+		hid = ACPI_PROCESSOR_OBJECT_HID;
 		break;
 	case ACPI_BUS_TYPE_SYSTEM:
 		hid = ACPI_SYSTEM_HID;

[PATCH 2/3] ACPI: Behave uniquely based on processor declaration definition type

[Myron Stowe]

Associating a Local SAPIC with a processor object is dependent upon the
processor object's definition type.  CPUs declared as "Processor" should use
the Local SAPIC's 'processor_id', and CPUs declared as "Device" should use the
'uid' (see section 5.2.11.13 - Local SAPIC Structure; "Advanced Configuration
and Power Interface Specification", Revision 3.0b).  

This patch changes the lsapic mapping logic to rely on the distinction of
how the processor object was declared - the mapping can't just try both types
of matches irregardless of declaration type and rely on one failing as is
currently being done.

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
CC: Alexey Starikovskiy <aystarik@gmail.com>

---
 drivers/acpi/processor_core.c |   64 +++++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================
--- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-21 13:12:13.000000000 -0600
+++ linux-2.6/drivers/acpi/processor_core.c	2008-10-22 12:12:11.000000000 -0600
@@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(stru
 /* Use the acpiid in MADT to map cpus in case of SMP */
 
 #ifndef CONFIG_SMP
-static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
+static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
 #else
 
 static struct acpi_table_madt *madt;
@@ -429,27 +429,33 @@ static int map_lapic_id(struct acpi_subt
 }
 
 static int map_lsapic_id(struct acpi_subtable_header *entry,
-		  u32 acpi_id, int *apic_id)
+		int device_declaration, u32 acpi_id, int *apic_id)
 {
 	struct acpi_madt_local_sapic *lsapic =
 		(struct acpi_madt_local_sapic *)entry;
+	u32 tmp = (lsapic->id << 8) | lsapic->eid;
+
 	/* Only check enabled APICs*/
-	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
-		/* First check against id */
-		if (lsapic->processor_id == acpi_id) {
-			*apic_id = (lsapic->id << 8) | lsapic->eid;
-			return 1;
-		/* Check against optional uid */
-		} else if (entry->length >= 16 &&
-			lsapic->uid == acpi_id) {
-			*apic_id = lsapic->uid;
-			return 1;
-		}
-	}
+	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
+		return 0;
+
+	/* Device statement declaration type */
+	if (device_declaration) {
+		if (entry->length < 16)
+			printk(KERN_ERR PREFIX "Invalid Local SAPIC structure corresponding to Device statement processor with SAPIC ID %#x\n", tmp);
+		else if (lsapic->uid == acpi_id)
+			goto found;
+	/* Processor statement declaration type */
+	} else if (lsapic->processor_id == acpi_id)
+		goto found;
+
 	return 0;
+found:
+	*apic_id = tmp;
+	return 1;
 }
 
-static int map_madt_entry(u32 acpi_id)
+static int map_madt_entry(int type, u32 acpi_id)
 {
 	unsigned long madt_end, entry;
 	int apic_id = -1;
@@ -470,7 +476,7 @@ static int map_madt_entry(u32 acpi_id)
 			if (map_lapic_id(header, acpi_id, &apic_id))
 				break;
 		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
-			if (map_lsapic_id(header, acpi_id, &apic_id))
+			if (map_lsapic_id(header, type, acpi_id, &apic_id))
 				break;
 		}
 		entry += header->length;
@@ -478,7 +484,7 @@ static int map_madt_entry(u32 acpi_id)
 	return apic_id;
 }
 
-static int map_mat_entry(acpi_handle handle, u32 acpi_id)
+static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
 {
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *obj;
@@ -501,7 +507,7 @@ static int map_mat_entry(acpi_handle han
 	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
 		map_lapic_id(header, acpi_id, &apic_id);
 	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
-		map_lsapic_id(header, acpi_id, &apic_id);
+		map_lsapic_id(header, type, acpi_id, &apic_id);
 	}
 
 exit:
@@ -510,14 +516,14 @@ exit:
 	return apic_id;
 }
 
-static int get_cpu_id(acpi_handle handle, u32 acpi_id)
+static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
 {
 	int i;
 	int apic_id = -1;
 
-	apic_id = map_mat_entry(handle, acpi_id);
+	apic_id = map_mat_entry(handle, type, acpi_id);
 	if (apic_id == -1)
-		apic_id = map_madt_entry(acpi_id);
+		apic_id = map_madt_entry(type, acpi_id);
 	if (apic_id == -1)
 		return apic_id;
 
@@ -533,15 +539,16 @@ static int get_cpu_id(acpi_handle handle
                                  Driver Interface
    -------------------------------------------------------------------------- */
 
-static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
+static int acpi_processor_get_info(struct acpi_device *device)
 {
 	acpi_status status = 0;
 	union acpi_object object = { 0 };
 	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
-	int cpu_index;
+	struct acpi_processor *pr;
+	int cpu_index, device_declaration = 0;
 	static int cpu0_initialized;
 
-
+	pr = acpi_driver_data(device);
 	if (!pr)
 		return -EINVAL;
 
@@ -562,8 +569,8 @@ static int acpi_processor_get_info(struc
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "No bus mastering arbitration control\n"));
 
-	/* Check if it is a Device with HID and UID */
-	if (has_uid) {
+	/* Check if it is a Device statement declaration with HID and UID */
+	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
 		unsigned long value;
 		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
 						NULL, &value);
@@ -571,6 +578,7 @@ static int acpi_processor_get_info(struc
 			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
 			return -ENODEV;
 		}
+		device_declaration = 1;
 		pr->acpi_id = value;
 	} else {
 		/*
@@ -590,7 +598,7 @@ static int acpi_processor_get_info(struc
 		*/
 		pr->acpi_id = object.processor.proc_id;
 	}
-	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
+	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
 
 	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
 	if (!cpu0_initialized && (cpu_index == -1) &&
@@ -662,7 +670,7 @@ static int __cpuinit acpi_processor_star
 
 	pr = acpi_driver_data(device);
 
-	result = acpi_processor_get_info(pr, device->flags.unique_id);
+	result = acpi_processor_get_info(device);
 	if (result) {
 		/* Processor is physically not present */
 		return 0;

[PATCH 3/3] ACPI: 80 column adherence and spelling fix (no functional change)

[Myron Stowe]

Signed-off-by: Myron Stowe <myron.stowe@hp.com>

---
 drivers/acpi/processor_core.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================
--- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-22 12:12:11.000000000 -0600
+++ linux-2.6/drivers/acpi/processor_core.c	2008-10-22 12:42:22.000000000 -0600
@@ -582,9 +582,9 @@ static int acpi_processor_get_info(struc
 		pr->acpi_id = value;
 	} else {
 		/*
-		* Evalute the processor object.  Note that it is common on SMP to
-		* have the first (boot) processor with a valid PBLK address while
-		* all others have a NULL address.
+		* Evaluate the Processor object.  Note that it is common on SMP
+		* to have the first (boot) processor with a valid PBLK address
+		* while * all others have a NULL address.
 		*/
 		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
 		if (ACPI_FAILURE(status)) {
@@ -594,7 +594,8 @@ static int acpi_processor_get_info(struc
 
 		/*
 		* TBD: Synch processor ID (via LAPIC/LSAPIC structures) on SMP.
-		*      >>> 'acpi_get_processor_id(acpi_id, &id)' in arch/xxx/acpi.c
+		*      >>> 'acpi_get_processor_id(acpi_id, &id)' in
+		*      arch/xxx/acpi.c
 		*/
 		pr->acpi_id = object.processor.proc_id;
 	}

Re: [PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Alexey Starikovskiy]

Hi Myron,

It is great to see that this functionality found some care after
2+ years :)
Please check your patches to conform to patch submission guidelines,
namely 80 char string limit... :) scripts/checkpatch.pl is quite good for
such purpose.

Myron Stowe wrote:
> Len, Alexey:
>
> The following three item patch series fixes an issue with the introduction
> of > 256 processor declaration support: "Allow processor to be declared with
> the Device() instead of Processor()" (git SHA 11bf04c4).
>
> The root issue is in the lsapic mapping logic of drivers/acpi/processor_core.c.
> Currently, the logic tries both types of matches irregardless of declaration
> type and relies on one failing.  According to the spec - lsapic mapping is
> dependent on how the processor object was declared: CPUs declared using the
> "Processor" statement should use the Local SAPIC's 'processor_id', and CPUs
> declared using the "Device" statement should use the 'uid'.
>
> Reference: Section 8.4 Declaring Processors; Section 5.2.11.13 Local SAPIC
> Structure.  "Advanced Configuration and Power Interface Specification",
> Revision 3.0b, October 2006.
>
>
> [PATCH 1/3] disambiguates the processor declaration type that is currently
> conflated so that subsequent logic can behave based explicitly on the
> declaration's type.  I expect the disambiguation this patch introduces will
> also be advantageous when extending the > 256 processor support for x86 via
> x2APIC.
>
> [PATCH 2/3] addresses the root issue as stated above.  With respect to this
> patch I'm ambivalent about the 'printk' introduced in "map_lsapic_id()" -
> perhaps it should be removed?
>
>   
Errors causing change from default behaviour should be visible, so
printk is ok, please only make it conform to 80 char string length limit.
> [PATCH 3/3] is non-functional white space/spelling fixes in the related code. 
>
>
> While the specific fix is ia64 focused the underlying code affected is common
> to both x86 and ia64.
That was intentional to have it common, as the less frequently used
case will rotten without care.
>   I have tested on the following platform/namespace
> combinations:
>   ia64 with "Processor" type namespace processor declarations,
>   ia64 with "Device" type namespace processor declarations,
>   x86 with "Processor" type namespace processor declarations.
>
>   
I had no machine with second variant, so it probably didn't work there, 
right?
> Note that this patch series does *not* handle "Device" type processor
> declarations that contain a string type _UID object under the processor
> device's scope (I am currently not aware of any platforms that have such to
> test against).
>
>   
Original didn't do it either -- it is too difficult and does not give 
any advantage
to HW vendors over numeral ID.

Regards,
Alex.

Re: [PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Zhao Yakui]

On Wed, 2008-10-22 at 13:10 -0600, Myron Stowe wrote:
> Len, Alexey:
> 
> The following three item patch series fixes an issue with the introduction
> of > 256 processor declaration support: "Allow processor to be declared with
> the Device() instead of Processor()" (git SHA 11bf04c4).
> 
> The root issue is in the lsapic mapping logic of drivers/acpi/processor_core.c.
> Currently, the logic tries both types of matches irregardless of declaration
> type and relies on one failing.  According to the spec - lsapic mapping is
> dependent on how the processor object was declared: CPUs declared using the
> "Processor" statement should use the Local SAPIC's 'processor_id', and CPUs
> declared using the "Device" statement should use the 'uid'.
> 
> Reference: Section 8.4 Declaring Processors; Section 5.2.11.13 Local SAPIC
> Structure.  "Advanced Configuration and Power Interface Specification",
> Revision 3.0b, October 2006.
> 
> 
> [PATCH 1/3] disambiguates the processor declaration type that is currently
> conflated so that subsequent logic can behave based explicitly on the
> declaration's type.  I expect the disambiguation this patch introduces will
> also be advantageous when extending the > 256 processor support for x86 via
> x2APIC.
In the patch the new HID("ACPI_CPU") for processor is introduced. Is it
required?
If there exists the _UID object under the scope of Processor, it won't
work well. 

According to the spec the returned value of the _UID object for
processor/device can be numeric value or a string. If it is a numeric
value, we should match it with the ACPI_UID field of slapic table to get
the APIC id. If it is string, we should match it with the ACPI_UID
string field of slapic table get the APIC ID.
> 
> [PATCH 2/3] addresses the root issue as stated above.  With respect to this
> patch I'm ambivalent about the 'printk' introduced in "map_lsapic_id()" -
> perhaps it should be removed?
> 
> [PATCH 3/3] is non-functional white space/spelling fixes in the related code. 
> 
> 
> While the specific fix is ia64 focused the underlying code affected is common
> to both x86 and ia64.  I have tested on the following platform/namespace
> combinations:
>   ia64 with "Processor" type namespace processor declarations,
>   ia64 with "Device" type namespace processor declarations,
>   x86 with "Processor" type namespace processor declarations.
> 
> Note that this patch series does *not* handle "Device" type processor
> declarations that contain a string type _UID object under the processor
> device's scope (I am currently not aware of any platforms that have such to
> test against).
> 
> All comments welcome.
> 
> Myron
> 

Re: [PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Myron Stowe]

On Thu, 2008-10-23 at 09:34 +0400, Alexey Starikovskiy wrote:
> Hi Myron,
> 
> It is great to see that this functionality found some care after
> 2+ years :)
> Please check your patches to conform to patch submission guidelines,
> namely 80 char string limit... :) scripts/checkpatch.pl is quite good for
> such purpose.
Yes - I did run the patches through scripts/checkpatch.pl.  I had
received feedback from a colleague to ignore the 80 column limit for
printk strings so that they are more 'grep'able.

You're feedback is making me rethink this advice - 'grep'ing for such
occurrences in the kernel is a known issue so most of us have learned to
'grep' for a smaller, sub-string, snippets.  This, along with just
general coding style guidelines, now convinces me that I should conform
to the 80 column limit as it is a good rule (which is why I couldn't
resist creating [PATCH3/3] as part of the series  ;)  ).
> 
> Myron Stowe wrote:
> > Len, Alexey:
> >
> > The following three item patch series fixes an issue with the introduction
> > of > 256 processor declaration support: "Allow processor to be declared with
> > the Device() instead of Processor()" (git SHA 11bf04c4).
> >
> > The root issue is in the lsapic mapping logic of drivers/acpi/processor_core.c.
> > Currently, the logic tries both types of matches irregardless of declaration
> > type and relies on one failing.  According to the spec - lsapic mapping is
> > dependent on how the processor object was declared: CPUs declared using the
> > "Processor" statement should use the Local SAPIC's 'processor_id', and CPUs
> > declared using the "Device" statement should use the 'uid'.
> >
> > Reference: Section 8.4 Declaring Processors; Section 5.2.11.13 Local SAPIC
> > Structure.  "Advanced Configuration and Power Interface Specification",
> > Revision 3.0b, October 2006.
> >
> >
> > [PATCH 1/3] disambiguates the processor declaration type that is currently
> > conflated so that subsequent logic can behave based explicitly on the
> > declaration's type.  I expect the disambiguation this patch introduces will
> > also be advantageous when extending the > 256 processor support for x86 via
> > x2APIC.
> >
> > [PATCH 2/3] addresses the root issue as stated above.  With respect to this
> > patch I'm ambivalent about the 'printk' introduced in "map_lsapic_id()" -
> > perhaps it should be removed?
> >
> >   
> Errors causing change from default behaviour should be visible, so
> printk is ok, please only make it conform to 80 char string length limit.
> > [PATCH 3/3] is non-functional white space/spelling fixes in the related code. 
> >
> >
> > While the specific fix is ia64 focused the underlying code affected is common
> > to both x86 and ia64.
> That was intentional to have it common, as the less frequently used
> case will rotten without care.
Agreed - it should be common (was just making to comment as to why I
also tested x86 and to let others know this patch indirectly
touches/affects x86).
> >   I have tested on the following platform/namespace
> > combinations:
> >   ia64 with "Processor" type namespace processor declarations,
> >   ia64 with "Device" type namespace processor declarations,
> >   x86 with "Processor" type namespace processor declarations.
> >
> >   
> I had no machine with second variant, so it probably didn't work there, 
> right?
Yes - we now have a platform that uses "Device" declarations for the
first time and the kernel 'panick'ed thus this patch.  You had all the
proper infrastructure in place - just a pretty simple omission which,
without something to test against would be easy to not see.
> > Note that this patch series does *not* handle "Device" type processor
> > declarations that contain a string type _UID object under the processor
> > device's scope (I am currently not aware of any platforms that have such to
> > test against).
> >
> >   
> Original didn't do it either -- it is too difficult and does not give 
> any advantage
> to HW vendors over numeral ID.
Yeah.  I was going to tackle this and it turned out to get pretty messy
fast.  Fortunately the error path printk in acpi_processor_get_info()
when a "Device" declaration with a _UID that is *not* an integer is
encountered will trigger and indicate exactly where the issue is at some
future date when a vendor utilizes this combination.

Thanks for the feedback!
Myron
> 
> Regards,
> Alex.
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Myron Stowe]

On Thu, 2008-10-23 at 17:32 +0800, Zhao Yakui wrote:
> On Wed, 2008-10-22 at 13:10 -0600, Myron Stowe wrote:
> > Len, Alexey:
> > 
> > The following three item patch series fixes an issue with the introduction
> > of > 256 processor declaration support: "Allow processor to be declared with
> > the Device() instead of Processor()" (git SHA 11bf04c4).
> > 
> > The root issue is in the lsapic mapping logic of drivers/acpi/processor_core.c.
> > Currently, the logic tries both types of matches irregardless of declaration
> > type and relies on one failing.  According to the spec - lsapic mapping is
> > dependent on how the processor object was declared: CPUs declared using the
> > "Processor" statement should use the Local SAPIC's 'processor_id', and CPUs
> > declared using the "Device" statement should use the 'uid'.
> > 
> > Reference: Section 8.4 Declaring Processors; Section 5.2.11.13 Local SAPIC
> > Structure.  "Advanced Configuration and Power Interface Specification",
> > Revision 3.0b, October 2006.
> > 
> > 
> > [PATCH 1/3] disambiguates the processor declaration type that is currently
> > conflated so that subsequent logic can behave based explicitly on the
> > declaration's type.  I expect the disambiguation this patch introduces will
> > also be advantageous when extending the > 256 processor support for x86 via
> > x2APIC.
> In the patch the new HID("ACPI_CPU") for processor is introduced. Is it
> required?
> If there exists the _UID object under the scope of Processor, it won't
> work well.
I'm not sure I understand your concern.  If you are concerned about the
case of a "Processor" type declaration that has a _UID object within
it's scope then this patch will handle such correctly (by the way - some
of our past platforms do have this combination).  This is basically what
[PATCH 1/3] addresses - setting the underlying infrastructure so that
subsequent mapping related logic - [PATCH2/3] - can explicitly
disambiguate between "Processor" declarations and "Device" declarations
and use the correct component corresponding to the declaration type
used.

If I misunderstood your question, or I am understanding your question
but you don't see how [PATCH 1/3] solves your concern then please ask
again.
> 
> According to the spec the returned value of the _UID object for
> processor/device can be numeric value or a string. If it is a numeric
> value, we should match it with the ACPI_UID field of slapic table to get
> the APIC id. If it is string, we should match it with the ACPI_UID
> string field of slapic table get the APIC ID.
Correct - thus my "warning" so to speak in the last paragraph of the
original posting.  I was going to tackle this and it turned out to get
pretty messy fast.  Fortunately the error path printk in
acpi_processor_get_info() when a "Device" declaration with a _UID that
is *not* an integer is encountered will trigger and indicate exactly
where the issue is at some future date when a vendor utilizes this
combination.

Thanks for your reply!
Myron
> > 
> > [PATCH 2/3] addresses the root issue as stated above.  With respect to this
> > patch I'm ambivalent about the 'printk' introduced in "map_lsapic_id()" -
> > perhaps it should be removed?
> > 
> > [PATCH 3/3] is non-functional white space/spelling fixes in the related code. 
> > 
> > 
> > While the specific fix is ia64 focused the underlying code affected is common
> > to both x86 and ia64.  I have tested on the following platform/namespace
> > combinations:
> >   ia64 with "Processor" type namespace processor declarations,
> >   ia64 with "Device" type namespace processor declarations,
> >   x86 with "Processor" type namespace processor declarations.
> > 
> > Note that this patch series does *not* handle "Device" type processor
> > declarations that contain a string type _UID object under the processor
> > device's scope (I am currently not aware of any platforms that have such to
> > test against).
> > 
> > All comments welcome.
> > 
> > Myron
> > 
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH 1/3] ACPI: Disambiguate processor declaration type

[Zhao Yakui]

On Wed, 2008-10-22 at 13:12 -0600, Myron Stowe wrote:
> Declaring processors in ACPI namespace can be done using either a "Processor"
> definition or a "Device" definition (see section 8.4 - Declaring Processors;
> "Advanced Configuration and Power Interface Specification", Revision 3.0b).
> Currently the two processor declaration types are conflated.
In most cases there is no _UID object under the scope of processor
namespace. So the current source can work well without adding the new
HID("ACPI_CPU") for processor definition.

If there exists the _UID object under the scope of processor namespace,
IMO the ProcessorID returned by _UID will have higher priority. In such
case the patch can't work well.

> 
> This patch disambiguates the processor declaration's definition type enabling
> subsequent code to behave uniquely based explicitly on the declaration's type.
> 
> Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> CC: Alexey Starikovskiy <aystarik@gmail.com>
> 
> ---
>  drivers/acpi/processor_core.c |    1 +
>  drivers/acpi/scan.c           |    2 +-
>  include/acpi/acpi_drivers.h   |    1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/acpi/processor_core.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-16 19:00:37.000000000 -0600
> +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-21 13:11:00.000000000 -0600
> @@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
>  
> 
>  static const struct acpi_device_id processor_device_ids[] = {
> +	{ACPI_PROCESSOR_OBJECT_HID, 0},
>  	{ACPI_PROCESSOR_HID, 0},
>  	{"", 0},
>  };
> Index: linux-2.6/include/acpi/acpi_drivers.h
> ===================================================================
> --- linux-2.6.orig/include/acpi/acpi_drivers.h	2008-10-16 18:53:26.000000000 -0600
> +++ linux-2.6/include/acpi/acpi_drivers.h	2008-10-20 13:23:28.000000000 -0600
> @@ -41,6 +41,7 @@
>   */
>  
>  #define ACPI_POWER_HID			"LNXPOWER"
> +#define ACPI_PROCESSOR_OBJECT_HID	"ACPI_CPU"
>  #define ACPI_PROCESSOR_HID		"ACPI0007"
>  #define ACPI_SYSTEM_HID			"LNXSYSTM"
>  #define ACPI_THERMAL_HID		"LNXTHERM"
> Index: linux-2.6/drivers/acpi/scan.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/scan.c	2008-10-16 19:04:58.000000000 -0600
> +++ linux-2.6/drivers/acpi/scan.c	2008-10-21 13:09:09.000000000 -0600
> @@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
>  		hid = ACPI_POWER_HID;
>  		break;
>  	case ACPI_BUS_TYPE_PROCESSOR:
> -		hid = ACPI_PROCESSOR_HID;
> +		hid = ACPI_PROCESSOR_OBJECT_HID;
>  		break;
>  	case ACPI_BUS_TYPE_SYSTEM:
>  		hid = ACPI_SYSTEM_HID;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Zhao Yakui]

On Thu, 2008-10-23 at 10:11 -0600, Myron Stowe wrote:
> On Thu, 2008-10-23 at 17:32 +0800, Zhao Yakui wrote:
> > On Wed, 2008-10-22 at 13:10 -0600, Myron Stowe wrote:
> > > Len, Alexey:
> > > 
> > > The following three item patch series fixes an issue with the introduction
> > > of > 256 processor declaration support: "Allow processor to be declared with
> > > the Device() instead of Processor()" (git SHA 11bf04c4).
> > > 
> > > The root issue is in the lsapic mapping logic of drivers/acpi/processor_core.c.
> > > Currently, the logic tries both types of matches irregardless of declaration
> > > type and relies on one failing.  According to the spec - lsapic mapping is
> > > dependent on how the processor object was declared: CPUs declared using the
> > > "Processor" statement should use the Local SAPIC's 'processor_id', and CPUs
> > > declared using the "Device" statement should use the 'uid'.
> > > 
> > > Reference: Section 8.4 Declaring Processors; Section 5.2.11.13 Local SAPIC
> > > Structure.  "Advanced Configuration and Power Interface Specification",
> > > Revision 3.0b, October 2006.
> > > 
> > > 
> > > [PATCH 1/3] disambiguates the processor declaration type that is currently
> > > conflated so that subsequent logic can behave based explicitly on the
> > > declaration's type.  I expect the disambiguation this patch introduces will
> > > also be advantageous when extending the > 256 processor support for x86 via
> > > x2APIC.
> > In the patch the new HID("ACPI_CPU") for processor is introduced. Is it
> > required?
> > If there exists the _UID object under the scope of Processor, it won't
> > work well.
> I'm not sure I understand your concern.  If you are concerned about the
> case of a "Processor" type declaration that has a _UID object within
> it's scope then this patch will handle such correctly (by the way - some
> of our past platforms do have this combination).  This is basically what
> [PATCH 1/3] addresses - setting the underlying infrastructure so that
> subsequent mapping related logic - [PATCH2/3] - can explicitly
> disambiguate between "Processor" declarations and "Device" declarations
> and use the correct component corresponding to the declaration type
> used.
In most cases there is no _UID object under the scope of Processor
namespace. And in such cases it is unnecessary to add the new
HID("ACPI_CPU") for the processor. 

But if there exists the _UID object under the scope of processor
namespace , IMO this patch is not appropriate. The ACPI_ID returned by
_UID object should have higher priority than the ACPI_ID defined in the
processor definition.  In such case OS should match the ACPI_ID obtained
from the _UID with slapic table to get the correct processor ID.

Based on the above analysis IMO it is unnecessary to add the new
HID("ACPI_CPU") for the processor.
Thanks.

> 
> If I misunderstood your question, or I am understanding your question
> but you don't see how [PATCH 1/3] solves your concern then please ask
> again.
> > 
> > According to the spec the returned value of the _UID object for
> > processor/device can be numeric value or a string. If it is a numeric
> > value, we should match it with the ACPI_UID field of slapic table to get
> > the APIC id. If it is string, we should match it with the ACPI_UID
> > string field of slapic table get the APIC ID.
> Correct - thus my "warning" so to speak in the last paragraph of the
> original posting.  I was going to tackle this and it turned out to get
> pretty messy fast.  Fortunately the error path printk in
> acpi_processor_get_info() when a "Device" declaration with a _UID that
> is *not* an integer is encountered will trigger and indicate exactly
> where the issue is at some future date when a vendor utilizes this
> combination.
> 
> Thanks for your reply!
> Myron
> > > 
> > > [PATCH 2/3] addresses the root issue as stated above.  With respect to this
> > > patch I'm ambivalent about the 'printk' introduced in "map_lsapic_id()" -
> > > perhaps it should be removed?
> > > 
> > > [PATCH 3/3] is non-functional white space/spelling fixes in the related code. 
> > > 
> > > 
> > > While the specific fix is ia64 focused the underlying code affected is common
> > > to both x86 and ia64.  I have tested on the following platform/namespace
> > > combinations:
> > >   ia64 with "Processor" type namespace processor declarations,
> > >   ia64 with "Device" type namespace processor declarations,
> > >   x86 with "Processor" type namespace processor declarations.
> > > 
> > > Note that this patch series does *not* handle "Device" type processor
> > > declarations that contain a string type _UID object under the processor
> > > device's scope (I am currently not aware of any platforms that have such to
> > > test against).
> > > 
> > > All comments welcome.
> > > 
> > > Myron
> > > 
> > 

Re: [PATCH 1/3] ACPI: Disambiguate processor declaration type

[Myron Stowe]

On Fri, 2008-10-24 at 09:16 +0800, Zhao Yakui wrote:
> On Wed, 2008-10-22 at 13:12 -0600, Myron Stowe wrote:
> > Declaring processors in ACPI namespace can be done using either a "Processor"
> > definition or a "Device" definition (see section 8.4 - Declaring Processors;
> > "Advanced Configuration and Power Interface Specification", Revision 3.0b).
> > Currently the two processor declaration types are conflated.
> In most cases there is no _UID object under the scope of processor
> namespace. So the current source can work well without adding the new
> HID("ACPI_CPU") for processor definition.
> 
> If there exists the _UID object under the scope of processor namespace,
> IMO the ProcessorID returned by _UID will have higher priority. In such
> case the patch can't work well.

According to section 5.2.11.13 "Local SAPIC Structure" - Local SAPIC to
processor object association uses the 'ProcessorID' for CPUs declared
with "Processor" statements and Local SAPIC to processor object
association for CPUs declared with "Device" statements use the '_UID'.
There is no "higher priority" - the association is fixed and must take
into account the type of CPU declaration - either "Processor" or
"Device" - to use the appropriate field - either 'ProcessorID' or '_UID'
- for the match.

In example, the combinations of CPU declaration type used in combination
with whether or not the CPU declaration contains a _UID child object are
  "Processor" without a _UID child object (which our systems have)
  "Processor" with a _UID child object (which our systems have)
  "Device" without a _UID child object
  "Device" with a _UID child object (which our systems now have)
In the "Processor" declarations the match to the Local SAPIC is based on
the 'ProcessorID' value regardless of whether or not there is a _UID
child object.  For "Device" declarations, the match to the Local SAPIC
is based on the '_UID' of the child object - so the third case above
("Device" without a _UID child object) would be illegal.


This patch separates the type of CPU declaration that was encountered in
the namespace (the current code conflated them into a single #define).
The separation enables the mapping logic, that is done later, know
explicitly which CPU declaration type was used so that it can use the
proper field - 'ProcessorID' or '_UID' for the association.

If you do not agree with this interpretation of the spec then please let
me know where you believe I am wrong.

Myron
> 
> > 
> > This patch disambiguates the processor declaration's definition type enabling
> > subsequent code to behave uniquely based explicitly on the declaration's type.
> > 
> > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > 
> > ---
> >  drivers/acpi/processor_core.c |    1 +
> >  drivers/acpi/scan.c           |    2 +-
> >  include/acpi/acpi_drivers.h   |    1 +
> >  3 files changed, 3 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/acpi/processor_core.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-16 19:00:37.000000000 -0600
> > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-21 13:11:00.000000000 -0600
> > @@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
> >  
> > 
> >  static const struct acpi_device_id processor_device_ids[] = {
> > +	{ACPI_PROCESSOR_OBJECT_HID, 0},
> >  	{ACPI_PROCESSOR_HID, 0},
> >  	{"", 0},
> >  };
> > Index: linux-2.6/include/acpi/acpi_drivers.h
> > ===================================================================
> > --- linux-2.6.orig/include/acpi/acpi_drivers.h	2008-10-16 18:53:26.000000000 -0600
> > +++ linux-2.6/include/acpi/acpi_drivers.h	2008-10-20 13:23:28.000000000 -0600
> > @@ -41,6 +41,7 @@
> >   */
> >  
> >  #define ACPI_POWER_HID			"LNXPOWER"
> > +#define ACPI_PROCESSOR_OBJECT_HID	"ACPI_CPU"
> >  #define ACPI_PROCESSOR_HID		"ACPI0007"
> >  #define ACPI_SYSTEM_HID			"LNXSYSTM"
> >  #define ACPI_THERMAL_HID		"LNXTHERM"
> > Index: linux-2.6/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/scan.c	2008-10-16 19:04:58.000000000 -0600
> > +++ linux-2.6/drivers/acpi/scan.c	2008-10-21 13:09:09.000000000 -0600
> > @@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
> >  		hid = ACPI_POWER_HID;
> >  		break;
> >  	case ACPI_BUS_TYPE_PROCESSOR:
> > -		hid = ACPI_PROCESSOR_HID;
> > +		hid = ACPI_PROCESSOR_OBJECT_HID;
> >  		break;
> >  	case ACPI_BUS_TYPE_SYSTEM:
> >  		hid = ACPI_SYSTEM_HID;
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH 0/3] ACPI: Fix for supporting > 256 processor declaration limit

[Alexey Starikovskiy]

Myron,

I think you could ignore this person... I've spent more than 2 weeks 
explaining
him my changes to EC, but he insisted on quality of his "analysis", 
until was proven
to be completely wrong by test run on his own hardware.
If you have any questions about why he is so vocal, ask Len -- he is the 
manager of
Yakui...

Regards,
Alex.


Zhao Yakui wrote:
> On Thu, 2008-10-23 at 10:11 -0600, Myron Stowe wrote:
>   
>> On Thu, 2008-10-23 at 17:32 +0800, Zhao Yakui wrote:
>>     
>>> On Wed, 2008-10-22 at 13:10 -0600, Myron Stowe wrote:
>>>       
>>>> Len, Alexey:
>>>>
>>>> The following three item patch series fixes an issue with the introduction
>>>> of > 256 processor declaration support: "Allow processor to be declared with
>>>> the Device() instead of Processor()" (git SHA 11bf04c4).
>>>>
>>>> The root issue is in the lsapic mapping logic of drivers/acpi/processor_core.c.
>>>> Currently, the logic tries both types of matches irregardless of declaration
>>>> type and relies on one failing.  According to the spec - lsapic mapping is
>>>> dependent on how the processor object was declared: CPUs declared using the
>>>> "Processor" statement should use the Local SAPIC's 'processor_id', and CPUs
>>>> declared using the "Device" statement should use the 'uid'.
>>>>
>>>> Reference: Section 8.4 Declaring Processors; Section 5.2.11.13 Local SAPIC
>>>> Structure.  "Advanced Configuration and Power Interface Specification",
>>>> Revision 3.0b, October 2006.
>>>>
>>>>
>>>> [PATCH 1/3] disambiguates the processor declaration type that is currently
>>>> conflated so that subsequent logic can behave based explicitly on the
>>>> declaration's type.  I expect the disambiguation this patch introduces will
>>>> also be advantageous when extending the > 256 processor support for x86 via
>>>> x2APIC.
>>>>         
>>> In the patch the new HID("ACPI_CPU") for processor is introduced. Is it
>>> required?
>>> If there exists the _UID object under the scope of Processor, it won't
>>> work well.
>>>       
>> I'm not sure I understand your concern.  If you are concerned about the
>> case of a "Processor" type declaration that has a _UID object within
>> it's scope then this patch will handle such correctly (by the way - some
>> of our past platforms do have this combination).  This is basically what
>> [PATCH 1/3] addresses - setting the underlying infrastructure so that
>> subsequent mapping related logic - [PATCH2/3] - can explicitly
>> disambiguate between "Processor" declarations and "Device" declarations
>> and use the correct component corresponding to the declaration type
>> used.
>>     
> In most cases there is no _UID object under the scope of Processor
> namespace. And in such cases it is unnecessary to add the new
> HID("ACPI_CPU") for the processor. 
>
> But if there exists the _UID object under the scope of processor
> namespace , IMO this patch is not appropriate. The ACPI_ID returned by
> _UID object should have higher priority than the ACPI_ID defined in the
> processor definition.  In such case OS should match the ACPI_ID obtained
> from the _UID with slapic table to get the correct processor ID.
>
> Based on the above analysis IMO it is unnecessary to add the new
> HID("ACPI_CPU") for the processor.
> Thanks.
>
>   
>> If I misunderstood your question, or I am understanding your question
>> but you don't see how [PATCH 1/3] solves your concern then please ask
>> again.
>>     
>>> According to the spec the returned value of the _UID object for
>>> processor/device can be numeric value or a string. If it is a numeric
>>> value, we should match it with the ACPI_UID field of slapic table to get
>>> the APIC id. If it is string, we should match it with the ACPI_UID
>>> string field of slapic table get the APIC ID.
>>>       
>> Correct - thus my "warning" so to speak in the last paragraph of the
>> original posting.  I was going to tackle this and it turned out to get
>> pretty messy fast.  Fortunately the error path printk in
>> acpi_processor_get_info() when a "Device" declaration with a _UID that
>> is *not* an integer is encountered will trigger and indicate exactly
>> where the issue is at some future date when a vendor utilizes this
>> combination.
>>
>> Thanks for your reply!
>> Myron
>>     
>>>> [PATCH 2/3] addresses the root issue as stated above.  With respect to this
>>>> patch I'm ambivalent about the 'printk' introduced in "map_lsapic_id()" -
>>>> perhaps it should be removed?
>>>>
>>>> [PATCH 3/3] is non-functional white space/spelling fixes in the related code. 
>>>>
>>>>
>>>> While the specific fix is ia64 focused the underlying code affected is common
>>>> to both x86 and ia64.  I have tested on the following platform/namespace
>>>> combinations:
>>>>   ia64 with "Processor" type namespace processor declarations,
>>>>   ia64 with "Device" type namespace processor declarations,
>>>>   x86 with "Processor" type namespace processor declarations.
>>>>
>>>> Note that this patch series does *not* handle "Device" type processor
>>>> declarations that contain a string type _UID object under the processor
>>>> device's scope (I am currently not aware of any platforms that have such to
>>>> test against).
>>>>
>>>> All comments welcome.
>>>>
>>>> Myron
>>>>
>>>>         
>
>   

Re: [PATCH 1/3] ACPI: Disambiguate processor declaration type

[Zhao Yakui]

On Thu, 2008-10-23 at 21:07 -0600, Myron Stowe wrote:
> On Fri, 2008-10-24 at 09:16 +0800, Zhao Yakui wrote:
> > On Wed, 2008-10-22 at 13:12 -0600, Myron Stowe wrote:
> > > Declaring processors in ACPI namespace can be done using either a "Processor"
> > > definition or a "Device" definition (see section 8.4 - Declaring Processors;
> > > "Advanced Configuration and Power Interface Specification", Revision 3.0b).
> > > Currently the two processor declaration types are conflated.
> > In most cases there is no _UID object under the scope of processor
> > namespace. So the current source can work well without adding the new
> > HID("ACPI_CPU") for processor definition.
> > 
> > If there exists the _UID object under the scope of processor namespace,
> > IMO the ProcessorID returned by _UID will have higher priority. In such
> > case the patch can't work well.
> 
> According to section 5.2.11.13 "Local SAPIC Structure" - Local SAPIC to
> processor object association uses the 'ProcessorID' for CPUs declared
> with "Processor" statements and Local SAPIC to processor object
> association for CPUs declared with "Device" statements use the '_UID'.
> There is no "higher priority" - the association is fixed and must take
> into account the type of CPU declaration - either "Processor" or
> "Device" - to use the appropriate field - either 'ProcessorID' or '_UID'
> - for the match.
Agree with what you said. For the processor definition using Processor
macro, we can get the ACPI ID by the processor block or _UID object (if
it exists) and match it with the ACPI processor ID filed of LAPIC table
or sapic table. 
If there is no _UID object under the scope of processor namespace, the
current source code still can work well. In such case it is unnecessary
to add a new HID. Of course it is harmless if you add it.
> In example, the combinations of CPU declaration type used in combination
> with whether or not the CPU declaration contains a _UID child object are
>   "Processor" without a _UID child object (which our systems have)
>   "Processor" with a _UID child object (which our systems have)
It is very lucky that there exists such a system. Will you please
confirm whether the ACPI ID returned by _UID object is consistent with
the ACPI ID declared in the processor block?
If they are the same value, any one can be used. 
If they are different, maybe we should confirm which one is correct.

The remaining issue is that _UID can return the string according to ACPI
spec. If the _UID string is returned, it should be matched with the ACPI
UID_string field of slapic table to get the APIC ID of processor. 

If you can confirm that the integer is returned by the _UID object under
the scope of processor using device definition on your system, I think
that your patch is OK. 

Of course we should add such enhancement.

>   "Device" without a _UID child object
>   "Device" with a _UID child object (which our systems now have)
> In the "Processor" declarations the match to the Local SAPIC is based on
> the 'ProcessorID' value regardless of whether or not there is a _UID
> child object.  For "Device" declarations, the match to the Local SAPIC
> is based on the '_UID' of the child object - so the third case above
> ("Device" without a _UID child object) would be illegal.
> 
> 
> This patch separates the type of CPU declaration that was encountered in
> the namespace (the current code conflated them into a single #define).
> The separation enables the mapping logic, that is done later, know
> explicitly which CPU declaration type was used so that it can use the
> proper field - 'ProcessorID' or '_UID' for the association.
> 
> If you do not agree with this interpretation of the spec then please let
> me know where you believe I am wrong.
> 
> Myron
> > 
> > > 
> > > This patch disambiguates the processor declaration's definition type enabling
> > > subsequent code to behave uniquely based explicitly on the declaration's type.
> > > 
> > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > > 
> > > ---
> > >  drivers/acpi/processor_core.c |    1 +
> > >  drivers/acpi/scan.c           |    2 +-
> > >  include/acpi/acpi_drivers.h   |    1 +
> > >  3 files changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-16 19:00:37.000000000 -0600
> > > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-21 13:11:00.000000000 -0600
> > > @@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
> > >  
> > > 
> > >  static const struct acpi_device_id processor_device_ids[] = {
> > > +	{ACPI_PROCESSOR_OBJECT_HID, 0},
> > >  	{ACPI_PROCESSOR_HID, 0},
> > >  	{"", 0},
> > >  };
> > > Index: linux-2.6/include/acpi/acpi_drivers.h
> > > ===================================================================
> > > --- linux-2.6.orig/include/acpi/acpi_drivers.h	2008-10-16 18:53:26.000000000 -0600
> > > +++ linux-2.6/include/acpi/acpi_drivers.h	2008-10-20 13:23:28.000000000 -0600
> > > @@ -41,6 +41,7 @@
> > >   */
> > >  
> > >  #define ACPI_POWER_HID			"LNXPOWER"
> > > +#define ACPI_PROCESSOR_OBJECT_HID	"ACPI_CPU"
> > >  #define ACPI_PROCESSOR_HID		"ACPI0007"
> > >  #define ACPI_SYSTEM_HID			"LNXSYSTM"
> > >  #define ACPI_THERMAL_HID		"LNXTHERM"
> > > Index: linux-2.6/drivers/acpi/scan.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/scan.c	2008-10-16 19:04:58.000000000 -0600
> > > +++ linux-2.6/drivers/acpi/scan.c	2008-10-21 13:09:09.000000000 -0600
> > > @@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
> > >  		hid = ACPI_POWER_HID;
> > >  		break;
> > >  	case ACPI_BUS_TYPE_PROCESSOR:
> > > -		hid = ACPI_PROCESSOR_HID;
> > > +		hid = ACPI_PROCESSOR_OBJECT_HID;
> > >  		break;
> > >  	case ACPI_BUS_TYPE_SYSTEM:
> > >  		hid = ACPI_SYSTEM_HID;
> > > 
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 

Re: [PATCH 2/3] ACPI: Behave uniquely based on processor declaration

[John Keller]

> 
> 
> Associating a Local SAPIC with a processor object is dependent upon the
> processor object's definition type.  CPUs declared as "Processor" should use
> the Local SAPIC's 'processor_id', and CPUs declared as "Device" should use the
> 'uid' (see section 5.2.11.13 - Local SAPIC Structure; "Advanced Configuration
> and Power Interface Specification", Revision 3.0b).  
> 
> This patch changes the lsapic mapping logic to rely on the distinction of
> how the processor object was declared - the mapping can't just try both types
> of matches irregardless of declaration type and rely on one failing as is
> currently being done.
> 
> Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> CC: Alexey Starikovskiy <aystarik@gmail.com>
> 
> ---
>  drivers/acpi/processor_core.c |   64 +++++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 28 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/processor_core.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-21 13:12:13.000000000 -0600
> +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-22 12:12:11.000000000 -0600
> @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(stru
>  /* Use the acpiid in MADT to map cpus in case of SMP */
>  
>  #ifndef CONFIG_SMP
> -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
>  #else
>  
>  static struct acpi_table_madt *madt;
> @@ -429,27 +429,33 @@ static int map_lapic_id(struct acpi_subt
>  }
>  
>  static int map_lsapic_id(struct acpi_subtable_header *entry,
> -		  u32 acpi_id, int *apic_id)
> +		int device_declaration, u32 acpi_id, int *apic_id)
>  {
>  	struct acpi_madt_local_sapic *lsapic =
>  		(struct acpi_madt_local_sapic *)entry;
> +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> +
>  	/* Only check enabled APICs*/
> -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> -		/* First check against id */
> -		if (lsapic->processor_id == acpi_id) {
> -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> -			return 1;
> -		/* Check against optional uid */
> -		} else if (entry->length >= 16 &&
> -			lsapic->uid == acpi_id) {
> -			*apic_id = lsapic->uid;
> -			return 1;
> -		}
> -	}
> +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> +		return 0;
> +
> +	/* Device statement declaration type */
> +	if (device_declaration) {
> +		if (entry->length < 16)
> +			printk(KERN_ERR PREFIX "Invalid Local SAPIC structure corresponding to Device statement processor with SAPIC ID %#x\n", tmp);
> +		else if (lsapic->uid == acpi_id)
> +			goto found;
> +	/* Processor statement declaration type */
> +	} else if (lsapic->processor_id == acpi_id)
> +		goto found;
> +
>  	return 0;
> +found:
> +	*apic_id = tmp;
> +	return 1;
>  }

If I read this right, lsapic->uid and acpi_id (Processor Device _UID)
no longer need to equal (lsapic->id << 8) | lsapic->eid), as now 'tmp'
will be the value returned.
Is this correct?
I don't know of a problem with this, just making sure I understand this.

Glad to see this change, as I had concerns of map_lsapic_id() first
trying to match against lsapic->processor_id, and possibly ignoring
lsapic->uid.

Thanks,
John



>  
> -static int map_madt_entry(u32 acpi_id)
> +static int map_madt_entry(int type, u32 acpi_id)
>  {
>  	unsigned long madt_end, entry;
>  	int apic_id = -1;
> @@ -470,7 +476,7 @@ static int map_madt_entry(u32 acpi_id)
>  			if (map_lapic_id(header, acpi_id, &apic_id))
>  				break;
>  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> -			if (map_lsapic_id(header, acpi_id, &apic_id))
> +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
>  				break;
>  		}
>  		entry += header->length;
> @@ -478,7 +484,7 @@ static int map_madt_entry(u32 acpi_id)
>  	return apic_id;
>  }
>  
> -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
>  {
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  	union acpi_object *obj;
> @@ -501,7 +507,7 @@ static int map_mat_entry(acpi_handle han
>  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
>  		map_lapic_id(header, acpi_id, &apic_id);
>  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> -		map_lsapic_id(header, acpi_id, &apic_id);
> +		map_lsapic_id(header, type, acpi_id, &apic_id);
>  	}
>  
>  exit:
> @@ -510,14 +516,14 @@ exit:
>  	return apic_id;
>  }
>  
> -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
>  {
>  	int i;
>  	int apic_id = -1;
>  
> -	apic_id = map_mat_entry(handle, acpi_id);
> +	apic_id = map_mat_entry(handle, type, acpi_id);
>  	if (apic_id == -1)
> -		apic_id = map_madt_entry(acpi_id);
> +		apic_id = map_madt_entry(type, acpi_id);
>  	if (apic_id == -1)
>  		return apic_id;
>  
> @@ -533,15 +539,16 @@ static int get_cpu_id(acpi_handle handle
>                                   Driver Interface
>     -------------------------------------------------------------------------- */
>  
> -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> +static int acpi_processor_get_info(struct acpi_device *device)
>  {
>  	acpi_status status = 0;
>  	union acpi_object object = { 0 };
>  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> -	int cpu_index;
> +	struct acpi_processor *pr;
> +	int cpu_index, device_declaration = 0;
>  	static int cpu0_initialized;
>  
> -
> +	pr = acpi_driver_data(device);
>  	if (!pr)
>  		return -EINVAL;
>  
> @@ -562,8 +569,8 @@ static int acpi_processor_get_info(struc
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  				  "No bus mastering arbitration control\n"));
>  
> -	/* Check if it is a Device with HID and UID */
> -	if (has_uid) {
> +	/* Check if it is a Device statement declaration with HID and UID */
> +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
>  		unsigned long value;
>  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
>  						NULL, &value);
> @@ -571,6 +578,7 @@ static int acpi_processor_get_info(struc
>  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
>  			return -ENODEV;
>  		}
> +		device_declaration = 1;
>  		pr->acpi_id = value;
>  	} else {
>  		/*
> @@ -590,7 +598,7 @@ static int acpi_processor_get_info(struc
>  		*/
>  		pr->acpi_id = object.processor.proc_id;
>  	}
> -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
>  
>  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
>  	if (!cpu0_initialized && (cpu_index == -1) &&
> @@ -662,7 +670,7 @@ static int __cpuinit acpi_processor_star
>  
>  	pr = acpi_driver_data(device);
>  
> -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> +	result = acpi_processor_get_info(device);
>  	if (result) {
>  		/* Processor is physically not present */
>  		return 0;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/3] ACPI: Disambiguate processor declaration type

[Myron Stowe]

On Fri, 2008-10-24 at 13:36 +0800, Zhao Yakui wrote:
> On Thu, 2008-10-23 at 21:07 -0600, Myron Stowe wrote:
> > On Fri, 2008-10-24 at 09:16 +0800, Zhao Yakui wrote:
> > > On Wed, 2008-10-22 at 13:12 -0600, Myron Stowe wrote:
> > > > Declaring processors in ACPI namespace can be done using either a "Processor"
> > > > definition or a "Device" definition (see section 8.4 - Declaring Processors;
> > > > "Advanced Configuration and Power Interface Specification", Revision 3.0b).
> > > > Currently the two processor declaration types are conflated.
> > > In most cases there is no _UID object under the scope of processor
> > > namespace. So the current source can work well without adding the new
> > > HID("ACPI_CPU") for processor definition.
> > > 
> > > If there exists the _UID object under the scope of processor namespace,
> > > IMO the ProcessorID returned by _UID will have higher priority. In such
> > > case the patch can't work well.
> > 
> > According to section 5.2.11.13 "Local SAPIC Structure" - Local SAPIC to
> > processor object association uses the 'ProcessorID' for CPUs declared
> > with "Processor" statements and Local SAPIC to processor object
> > association for CPUs declared with "Device" statements use the '_UID'.
> > There is no "higher priority" - the association is fixed and must take
> > into account the type of CPU declaration - either "Processor" or
> > "Device" - to use the appropriate field - either 'ProcessorID' or '_UID'
> > - for the match.
> Agree with what you said. For the processor definition using Processor
> macro, we can get the ACPI ID by the processor block or _UID object (if
> it exists) and match it with the ACPI processor ID filed of LAPIC table
> or sapic table.
No, I do not believe this is correct.  For a processor defined in ACPI
namespace using the "Processor" statement type, it's 'ProcessorID' field
must be used to map to the Local SAPIC table entry regardless of whether
or not a '_UID' child object also appears.  The "Processor" statement's
'ProcessorID' field is used to match against the Local SAPIC table
entries 'ACPIProcessorID' field.  A '_UID' child object has no bearing
with respect to the mapping for "Processor" statement types.
>  
> If there is no _UID object under the scope of processor namespace, the
> current source code still can work well. In such case it is unnecessary
> to add a new HID. Of course it is harmless if you add it.
> > In example, the combinations of CPU declaration type used in combination
> > with whether or not the CPU declaration contains a _UID child object are
> >   "Processor" without a _UID child object (which our systems have)
> >   "Processor" with a _UID child object (which our systems have)
> It is very lucky that there exists such a system. Will you please
> confirm whether the ACPI ID returned by _UID object is consistent with
> the ACPI ID declared in the processor block?
Here is an small portion of the namespace of one our existing platforms
with this combination (by the way, this system is a HP SuperDome and has
been in the field for 7+ years now):

                    Processor (P002, 0x02, 0x00000000, 0x00)
                    {
                        Method (SSTA, 0, NotSerialized)
                        {
                            Return (0x0000000F)
                        }
        
                        Name (_SUN, 0x01)
                        Name (_STR, Unicode ("Location:00-FF-FF-00-FF-01-FF-11"))
                        Name (_UID, "0083f9b132089e43_HP_01_00_00")
                        Zero
                        Name (_PXM, 0x00)
                        Method (_MAT, 0, NotSerialized)
                        {
                            Return (Buffer (0x0C)
                            {
                                0x07, 0x0C, 0x02, 0x04, 0x00, 0x00, 0x00, 0x00,
                                0x01, 0x00, 0x00, 0x00
                            })
        		...

This is a CPU defined using the "Processor" declaration statement.  This
CPU's 'ProcessorID' filed is '0x02' (the second argument).  Note also
the _MAT object that is basically just a copy of the MADT table's Local
SAPIC entry that corresponds to this CPU.  Decoding it yields (remember
little endian):
        Type: 0x07
        Length: 0x0C
        ACPI Processor ID: 0x02
        Local SAPIC ID: 0x04
        Local SAPIC EID: 0x00
        Reserved: 0x000000
        Flags: 0x00000001
        
> If they are the same value, any one can be used.
No, this is not true - even if they did happen to be the same value.
The "Processor" statement's 'ProcessorID' field (0x02) *must* be used to
match the Local SAPIC's 'ACPIProcessorID' field (0x02).  The '_UID'
child object has *no* bearing what so ever in these regards for
"Processor" statement's.
> If they are different, maybe we should confirm which one is correct.
Exactly - the spec is already pretty explicit about this.
> 
> The remaining issue is that _UID can return the string according to ACPI
> spec. If the _UID string is returned, it should be matched with the ACPI
> UID_string field of slapic table to get the APIC ID of processor. 
Yes, but *only* in the case of a processor defined using the "Device"
statement in namespace is the '_UID' field used to map to the Local
SAPIC entry (again, see the example above as it is this type of case).

This is basically the opposite case.  For a processor defined in ACPI
namespace using the "Device" statement type it's '_UID' field must be
used to map to the Local SAPIC table entry - the Local SAPIC's
'ACPIProcessorID' field has no bearing in this case.  The "Device"
statement's '_UID' field is used to match against the Local SAPIC table
entries '_UID' field with the complication that the '_UID' field can be
either an integer or a string.

As I indicated earlier, I chose not to cover this case as it became
complicated and there are currently no platforms that use this
combination yet.  I'm sure some will come at which time we will have to
address such a case.  As I also indicated earlier - the code will
currently 'printk' a message indicating that this exact scenario is has
occurred.
> 
> If you can confirm that the integer is returned by the _UID object under
> the scope of processor using device definition on your system, I think
> that your patch is OK. 
Yes - my testing of various platforms that cover three of the four
possible cases (the fourth case - a "Device" statement defined processor
without a '_UID' object - I believe is invalid) has shown that:
      * The code, as it currently is, panics on platforms that are now
        beginning to use the "Device" statement for declaring
        processors,
      * This patch series addresses this case and shows no regressions
        on existing platforms that use the other two cases.
> 
> Of course we should add such enhancement.
I hope this information now helps you understand the need for this
patch.
> 
> >   "Device" without a _UID child object
> >   "Device" with a _UID child object (which our systems now have)
> > In the "Processor" declarations the match to the Local SAPIC is based on
> > the 'ProcessorID' value regardless of whether or not there is a _UID
> > child object.  For "Device" declarations, the match to the Local SAPIC
> > is based on the '_UID' of the child object - so the third case above
> > ("Device" without a _UID child object) would be illegal.
> > 
> > 
> > This patch separates the type of CPU declaration that was encountered in
> > the namespace (the current code conflated them into a single #define).
> > The separation enables the mapping logic, that is done later, know
> > explicitly which CPU declaration type was used so that it can use the
> > proper field - 'ProcessorID' or '_UID' for the association.
> > 
> > If you do not agree with this interpretation of the spec then please let
> > me know where you believe I am wrong.
> > 
> > Myron
> > > 
> > > > 
> > > > This patch disambiguates the processor declaration's definition type enabling
> > > > subsequent code to behave uniquely based explicitly on the declaration's type.
> > > > 
> > > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > > > 
> > > > ---
> > > >  drivers/acpi/processor_core.c |    1 +
> > > >  drivers/acpi/scan.c           |    2 +-
> > > >  include/acpi/acpi_drivers.h   |    1 +
> > > >  3 files changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-16 19:00:37.000000000 -0600
> > > > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-21 13:11:00.000000000 -0600
> > > > @@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
> > > >  
> > > > 
> > > >  static const struct acpi_device_id processor_device_ids[] = {
> > > > +	{ACPI_PROCESSOR_OBJECT_HID, 0},
> > > >  	{ACPI_PROCESSOR_HID, 0},
> > > >  	{"", 0},
> > > >  };
> > > > Index: linux-2.6/include/acpi/acpi_drivers.h
> > > > ===================================================================
> > > > --- linux-2.6.orig/include/acpi/acpi_drivers.h	2008-10-16 18:53:26.000000000 -0600
> > > > +++ linux-2.6/include/acpi/acpi_drivers.h	2008-10-20 13:23:28.000000000 -0600
> > > > @@ -41,6 +41,7 @@
> > > >   */
> > > >  
> > > >  #define ACPI_POWER_HID			"LNXPOWER"
> > > > +#define ACPI_PROCESSOR_OBJECT_HID	"ACPI_CPU"
> > > >  #define ACPI_PROCESSOR_HID		"ACPI0007"
> > > >  #define ACPI_SYSTEM_HID			"LNXSYSTM"
> > > >  #define ACPI_THERMAL_HID		"LNXTHERM"
> > > > Index: linux-2.6/drivers/acpi/scan.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/scan.c	2008-10-16 19:04:58.000000000 -0600
> > > > +++ linux-2.6/drivers/acpi/scan.c	2008-10-21 13:09:09.000000000 -0600
> > > > @@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
> > > >  		hid = ACPI_POWER_HID;
> > > >  		break;
> > > >  	case ACPI_BUS_TYPE_PROCESSOR:
> > > > -		hid = ACPI_PROCESSOR_HID;
> > > > +		hid = ACPI_PROCESSOR_OBJECT_HID;
> > > >  		break;
> > > >  	case ACPI_BUS_TYPE_SYSTEM:
> > > >  		hid = ACPI_SYSTEM_HID;
> > > > 
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH 2/3] ACPI: Behave uniquely based on processor declaration

[Myron Stowe]

On Fri, 2008-10-24 at 10:56 -0500, John Keller wrote:
> > 
> > 
> > Associating a Local SAPIC with a processor object is dependent upon the
> > processor object's definition type.  CPUs declared as "Processor" should use
> > the Local SAPIC's 'processor_id', and CPUs declared as "Device" should use the
> > 'uid' (see section 5.2.11.13 - Local SAPIC Structure; "Advanced Configuration
> > and Power Interface Specification", Revision 3.0b).  
> > 
> > This patch changes the lsapic mapping logic to rely on the distinction of
> > how the processor object was declared - the mapping can't just try both types
> > of matches irregardless of declaration type and rely on one failing as is
> > currently being done.
> > 
> > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > 
> > ---
> >  drivers/acpi/processor_core.c |   64 +++++++++++++++++++++++-------------------
> >  1 file changed, 36 insertions(+), 28 deletions(-)
> > 
> > Index: linux-2.6/drivers/acpi/processor_core.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-21 13:12:13.000000000 -0600
> > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-22 12:12:11.000000000 -0600
> > @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(stru
> >  /* Use the acpiid in MADT to map cpus in case of SMP */
> >  
> >  #ifndef CONFIG_SMP
> > -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
> >  #else
> >  
> >  static struct acpi_table_madt *madt;
> > @@ -429,27 +429,33 @@ static int map_lapic_id(struct acpi_subt
> >  }
> >  
> >  static int map_lsapic_id(struct acpi_subtable_header *entry,
> > -		  u32 acpi_id, int *apic_id)
> > +		int device_declaration, u32 acpi_id, int *apic_id)
> >  {
> >  	struct acpi_madt_local_sapic *lsapic =
> >  		(struct acpi_madt_local_sapic *)entry;
> > +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> > +
> >  	/* Only check enabled APICs*/
> > -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> > -		/* First check against id */
> > -		if (lsapic->processor_id == acpi_id) {
> > -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> > -			return 1;
> > -		/* Check against optional uid */
> > -		} else if (entry->length >= 16 &&
> > -			lsapic->uid == acpi_id) {
> > -			*apic_id = lsapic->uid;
> > -			return 1;
> > -		}
> > -	}
> > +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> > +		return 0;
> > +
> > +	/* Device statement declaration type */
> > +	if (device_declaration) {
> > +		if (entry->length < 16)
> > +			printk(KERN_ERR PREFIX "Invalid Local SAPIC structure corresponding to Device statement processor with SAPIC ID %#x\n", tmp);
> > +		else if (lsapic->uid == acpi_id)
> > +			goto found;
> > +	/* Processor statement declaration type */
> > +	} else if (lsapic->processor_id == acpi_id)
> > +		goto found;
> > +
> >  	return 0;
> > +found:
> > +	*apic_id = tmp;
> > +	return 1;
> >  }
> 
> If I read this right, lsapic->uid and acpi_id (Processor Device _UID)
> no longer need to equal (lsapic->id << 8) | lsapic->eid), as now 'tmp'
> will be the value returned.
> Is this correct?
Yes.
> I don't know of a problem with this, just making sure I understand this.
Yeah - there were problems  ;)

What I think you might be getting at is that there are two possible
explanations for why this logic has been working till now:
      * There have been platforms using the "Device" declaration but the
        firmware must have went through great pains to ensure that all
        these fields involved *artificially* matched (which is what I
        believe you are alluding to and I believe is a misunderstanding
        of the spec if that was the case).
  - or -
      * There have been no platforms using the "Device" declaration and
        so map_lsapic_id()'s 'if (lsapic->processor_id == acpi_id)' was
        always succeeding and the 'else if ...' path was never taken.  I
        tend to believe this has been the case (but that is pure
        assumption which means little).
> 
> Glad to see this change, as I had concerns of map_lsapic_id() first
> trying to match against lsapic->processor_id, and possibly ignoring
> lsapic->uid.
Yes - the logic as it currently is, is not specific/strict enough.  What
I seem to be failing to be able to express well enough earlier in the
thread is basically this:

        CPUs declared in namespace using a "Processor" statement must
        use the "Processor" statement's 'ProcessorID' field to match to
        the corresponding Local SAPIC entry (matching the Local SAPIC
        entries 'ACPIProcessorID' field).  Whether or not the
        "Processor" statement has a '_UID' child object has no bearing
        with respect to the mapping.
        
        Conversely, CPUs declared in namespace using a "Device"
        statement must use the "Device" statement's '_UID' child object
        field to match to the corresponding Local SAPIC entry (matching
        the Local SAPIC entries '_UID' field).
        
At least this is how we are interpreting section 5.2.11.13 "Local SAPIC
Structure" of the specification (specifically see the Description for
both ACPI Processor ID and ACPI Processor UID Value).

Thanks for your query!
Myron
> 
> Thanks,
> John
> 
> 
> 
> >  
> > -static int map_madt_entry(u32 acpi_id)
> > +static int map_madt_entry(int type, u32 acpi_id)
> >  {
> >  	unsigned long madt_end, entry;
> >  	int apic_id = -1;
> > @@ -470,7 +476,7 @@ static int map_madt_entry(u32 acpi_id)
> >  			if (map_lapic_id(header, acpi_id, &apic_id))
> >  				break;
> >  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > -			if (map_lsapic_id(header, acpi_id, &apic_id))
> > +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
> >  				break;
> >  		}
> >  		entry += header->length;
> > @@ -478,7 +484,7 @@ static int map_madt_entry(u32 acpi_id)
> >  	return apic_id;
> >  }
> >  
> > -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
> >  {
> >  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> >  	union acpi_object *obj;
> > @@ -501,7 +507,7 @@ static int map_mat_entry(acpi_handle han
> >  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
> >  		map_lapic_id(header, acpi_id, &apic_id);
> >  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > -		map_lsapic_id(header, acpi_id, &apic_id);
> > +		map_lsapic_id(header, type, acpi_id, &apic_id);
> >  	}
> >  
> >  exit:
> > @@ -510,14 +516,14 @@ exit:
> >  	return apic_id;
> >  }
> >  
> > -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
> >  {
> >  	int i;
> >  	int apic_id = -1;
> >  
> > -	apic_id = map_mat_entry(handle, acpi_id);
> > +	apic_id = map_mat_entry(handle, type, acpi_id);
> >  	if (apic_id == -1)
> > -		apic_id = map_madt_entry(acpi_id);
> > +		apic_id = map_madt_entry(type, acpi_id);
> >  	if (apic_id == -1)
> >  		return apic_id;
> >  
> > @@ -533,15 +539,16 @@ static int get_cpu_id(acpi_handle handle
> >                                   Driver Interface
> >     -------------------------------------------------------------------------- */
> >  
> > -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > +static int acpi_processor_get_info(struct acpi_device *device)
> >  {
> >  	acpi_status status = 0;
> >  	union acpi_object object = { 0 };
> >  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> > -	int cpu_index;
> > +	struct acpi_processor *pr;
> > +	int cpu_index, device_declaration = 0;
> >  	static int cpu0_initialized;
> >  
> > -
> > +	pr = acpi_driver_data(device);
> >  	if (!pr)
> >  		return -EINVAL;
> >  
> > @@ -562,8 +569,8 @@ static int acpi_processor_get_info(struc
> >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >  				  "No bus mastering arbitration control\n"));
> >  
> > -	/* Check if it is a Device with HID and UID */
> > -	if (has_uid) {
> > +	/* Check if it is a Device statement declaration with HID and UID */
> > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> >  		unsigned long value;
> >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> >  						NULL, &value);
> > @@ -571,6 +578,7 @@ static int acpi_processor_get_info(struc
> >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> >  			return -ENODEV;
> >  		}
> > +		device_declaration = 1;
> >  		pr->acpi_id = value;
> >  	} else {
> >  		/*
> > @@ -590,7 +598,7 @@ static int acpi_processor_get_info(struc
> >  		*/
> >  		pr->acpi_id = object.processor.proc_id;
> >  	}
> > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> >  
> >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > @@ -662,7 +670,7 @@ static int __cpuinit acpi_processor_star
> >  
> >  	pr = acpi_driver_data(device);
> >  
> > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > +	result = acpi_processor_get_info(device);
> >  	if (result) {
> >  		/* Processor is physically not present */
> >  		return 0;
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH 2/3] ACPI: Behave uniquely based on processor

[John Keller]

> 
> On Fri, 2008-10-24 at 10:56 -0500, John Keller wrote:
> > > 
> > > 
> > > Associating a Local SAPIC with a processor object is dependent upon the
> > > processor object's definition type.  CPUs declared as "Processor" should use
> > > the Local SAPIC's 'processor_id', and CPUs declared as "Device" should use the
> > > 'uid' (see section 5.2.11.13 - Local SAPIC Structure; "Advanced Configuration
> > > and Power Interface Specification", Revision 3.0b).  
> > > 
> > > This patch changes the lsapic mapping logic to rely on the distinction of
> > > how the processor object was declared - the mapping can't just try both types
> > > of matches irregardless of declaration type and rely on one failing as is
> > > currently being done.
> > > 
> > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > > 
> > > ---
> > >  drivers/acpi/processor_core.c |   64 +++++++++++++++++++++++-------------------
> > >  1 file changed, 36 insertions(+), 28 deletions(-)
> > > 
> > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-21 13:12:13.000000000 -0600
> > > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-22 12:12:11.000000000 -0600
> > > @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(stru
> > >  /* Use the acpiid in MADT to map cpus in case of SMP */
> > >  
> > >  #ifndef CONFIG_SMP
> > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
> > >  #else
> > >  
> > >  static struct acpi_table_madt *madt;
> > > @@ -429,27 +429,33 @@ static int map_lapic_id(struct acpi_subt
> > >  }
> > >  
> > >  static int map_lsapic_id(struct acpi_subtable_header *entry,
> > > -		  u32 acpi_id, int *apic_id)
> > > +		int device_declaration, u32 acpi_id, int *apic_id)
> > >  {
> > >  	struct acpi_madt_local_sapic *lsapic =
> > >  		(struct acpi_madt_local_sapic *)entry;
> > > +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> > > +
> > >  	/* Only check enabled APICs*/
> > > -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> > > -		/* First check against id */
> > > -		if (lsapic->processor_id == acpi_id) {
> > > -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> > > -			return 1;
> > > -		/* Check against optional uid */
> > > -		} else if (entry->length >= 16 &&
> > > -			lsapic->uid == acpi_id) {
> > > -			*apic_id = lsapic->uid;
> > > -			return 1;
> > > -		}
> > > -	}
> > > +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> > > +		return 0;
> > > +
> > > +	/* Device statement declaration type */
> > > +	if (device_declaration) {
> > > +		if (entry->length < 16)
> > > +			printk(KERN_ERR PREFIX "Invalid Local SAPIC structure corresponding to Device statement processor with SAPIC ID %#x\n", tmp);
> > > +		else if (lsapic->uid == acpi_id)
> > > +			goto found;
> > > +	/* Processor statement declaration type */
> > > +	} else if (lsapic->processor_id == acpi_id)
> > > +		goto found;
> > > +
> > >  	return 0;
> > > +found:
> > > +	*apic_id = tmp;
> > > +	return 1;
> > >  }
> > 
> > If I read this right, lsapic->uid and acpi_id (Processor Device _UID)
> > no longer need to equal (lsapic->id << 8) | lsapic->eid), as now 'tmp'
> > will be the value returned.
> > Is this correct?
> Yes.
> > I don't know of a problem with this, just making sure I understand this.
> Yeah - there were problems  ;)
> 
> What I think you might be getting at is that there are two possible
> explanations for why this logic has been working till now:
>       * There have been platforms using the "Device" declaration but the
>         firmware must have went through great pains to ensure that all
>         these fields involved *artificially* matched (which is what I
>         believe you are alluding to and I believe is a misunderstanding
>         of the spec if that was the case).

Yes, this is what motivated my questions.
As part of testing firmware support for an upcoming platform, where
"Device" declarations will be used, I quickly learned that currently
all these fields need to match.

Thanks for the reply,
John


>   - or -
>       * There have been no platforms using the "Device" declaration and
>         so map_lsapic_id()'s 'if (lsapic->processor_id == acpi_id)' was
>         always succeeding and the 'else if ...' path was never taken. I
>         tend to believe this has been the case (but that is pure
>         assumption which means little).
> > 
> > Glad to see this change, as I had concerns of map_lsapic_id() first
> > trying to match against lsapic->processor_id, and possibly ignoring
> > lsapic->uid.
> Yes - the logic as it currently is, is not specific/strict enough.  What
> I seem to be failing to be able to express well enough earlier in the
> thread is basically this:
> 
>         CPUs declared in namespace using a "Processor" statement must
>         use the "Processor" statement's 'ProcessorID' field to match to
>         the corresponding Local SAPIC entry (matching the Local SAPIC
>         entries 'ACPIProcessorID' field).  Whether or not the
>         "Processor" statement has a '_UID' child object has no bearing
>         with respect to the mapping.
>         
>         Conversely, CPUs declared in namespace using a "Device"
>         statement must use the "Device" statement's '_UID' child object
>         field to match to the corresponding Local SAPIC entry (matching
>         the Local SAPIC entries '_UID' field).
>         
> At least this is how we are interpreting section 5.2.11.13 "Local SAPIC
> Structure" of the specification (specifically see the Description for
> both ACPI Processor ID and ACPI Processor UID Value).
> 
> Thanks for your query!
> Myron
> > 
> > Thanks,
> > John
> > 
> > 
> > 
> > >  
> > > -static int map_madt_entry(u32 acpi_id)
> > > +static int map_madt_entry(int type, u32 acpi_id)
> > >  {
> > >  	unsigned long madt_end, entry;
> > >  	int apic_id = -1;
> > > @@ -470,7 +476,7 @@ static int map_madt_entry(u32 acpi_id)
> > >  			if (map_lapic_id(header, acpi_id, &apic_id))
> > >  				break;
> > >  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > -			if (map_lsapic_id(header, acpi_id, &apic_id))
> > > +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
> > >  				break;
> > >  		}
> > >  		entry += header->length;
> > > @@ -478,7 +484,7 @@ static int map_madt_entry(u32 acpi_id)
> > >  	return apic_id;
> > >  }
> > >  
> > > -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > > +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
> > >  {
> > >  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > >  	union acpi_object *obj;
> > > @@ -501,7 +507,7 @@ static int map_mat_entry(acpi_handle han
> > >  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
> > >  		map_lapic_id(header, acpi_id, &apic_id);
> > >  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > -		map_lsapic_id(header, acpi_id, &apic_id);
> > > +		map_lsapic_id(header, type, acpi_id, &apic_id);
> > >  	}
> > >  
> > >  exit:
> > > @@ -510,14 +516,14 @@ exit:
> > >  	return apic_id;
> > >  }
> > >  
> > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
> > >  {
> > >  	int i;
> > >  	int apic_id = -1;
> > >  
> > > -	apic_id = map_mat_entry(handle, acpi_id);
> > > +	apic_id = map_mat_entry(handle, type, acpi_id);
> > >  	if (apic_id == -1)
> > > -		apic_id = map_madt_entry(acpi_id);
> > > +		apic_id = map_madt_entry(type, acpi_id);
> > >  	if (apic_id == -1)
> > >  		return apic_id;
> > >  
> > > @@ -533,15 +539,16 @@ static int get_cpu_id(acpi_handle handle
> > >                                   Driver Interface
> > >     -------------------------------------------------------------------------- */
> > >  
> > > -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > +static int acpi_processor_get_info(struct acpi_device *device)
> > >  {
> > >  	acpi_status status = 0;
> > >  	union acpi_object object = { 0 };
> > >  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> > > -	int cpu_index;
> > > +	struct acpi_processor *pr;
> > > +	int cpu_index, device_declaration = 0;
> > >  	static int cpu0_initialized;
> > >  
> > > -
> > > +	pr = acpi_driver_data(device);
> > >  	if (!pr)
> > >  		return -EINVAL;
> > >  
> > > @@ -562,8 +569,8 @@ static int acpi_processor_get_info(struc
> > >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > >  				  "No bus mastering arbitration control\n"));
> > >  
> > > -	/* Check if it is a Device with HID and UID */
> > > -	if (has_uid) {
> > > +	/* Check if it is a Device statement declaration with HID and UID */
> > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > >  		unsigned long value;
> > >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> > >  						NULL, &value);
> > > @@ -571,6 +578,7 @@ static int acpi_processor_get_info(struc
> > >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> > >  			return -ENODEV;
> > >  		}
> > > +		device_declaration = 1;
> > >  		pr->acpi_id = value;
> > >  	} else {
> > >  		/*
> > > @@ -590,7 +598,7 @@ static int acpi_processor_get_info(struc
> > >  		*/
> > >  		pr->acpi_id = object.processor.proc_id;
> > >  	}
> > > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> > >  
> > >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> > >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > > @@ -662,7 +670,7 @@ static int __cpuinit acpi_processor_star
> > >  
> > >  	pr = acpi_driver_data(device);
> > >  
> > > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > > +	result = acpi_processor_get_info(device);
> > >  	if (result) {
> > >  		/* Processor is physically not present */
> > >  		return 0;
> > > 
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > 
> -- 
> Myron Stowe                             HP Open Source & Linux Org
> 
> 

Re: [PATCH 2/3] ACPI: Behave uniquely based on processor

[Myron Stowe]

On Fri, 2008-10-24 at 13:42 -0500, John Keller wrote:
> > 
> > On Fri, 2008-10-24 at 10:56 -0500, John Keller wrote:
> > > > 
> > > > 
> > > > Associating a Local SAPIC with a processor object is dependent upon the
> > > > processor object's definition type.  CPUs declared as "Processor" should use
> > > > the Local SAPIC's 'processor_id', and CPUs declared as "Device" should use the
> > > > 'uid' (see section 5.2.11.13 - Local SAPIC Structure; "Advanced Configuration
> > > > and Power Interface Specification", Revision 3.0b).  
> > > > 
> > > > This patch changes the lsapic mapping logic to rely on the distinction of
> > > > how the processor object was declared - the mapping can't just try both types
> > > > of matches irregardless of declaration type and rely on one failing as is
> > > > currently being done.
> > > > 
> > > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > > > 
> > > > ---
> > > >  drivers/acpi/processor_core.c |   64 +++++++++++++++++++++++-------------------
> > > >  1 file changed, 36 insertions(+), 28 deletions(-)
> > > > 
> > > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-21 13:12:13.000000000 -0600
> > > > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-22 12:12:11.000000000 -0600
> > > > @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(stru
> > > >  /* Use the acpiid in MADT to map cpus in case of SMP */
> > > >  
> > > >  #ifndef CONFIG_SMP
> > > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> > > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
> > > >  #else
> > > >  
> > > >  static struct acpi_table_madt *madt;
> > > > @@ -429,27 +429,33 @@ static int map_lapic_id(struct acpi_subt
> > > >  }
> > > >  
> > > >  static int map_lsapic_id(struct acpi_subtable_header *entry,
> > > > -		  u32 acpi_id, int *apic_id)
> > > > +		int device_declaration, u32 acpi_id, int *apic_id)
> > > >  {
> > > >  	struct acpi_madt_local_sapic *lsapic =
> > > >  		(struct acpi_madt_local_sapic *)entry;
> > > > +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> > > > +
> > > >  	/* Only check enabled APICs*/
> > > > -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> > > > -		/* First check against id */
> > > > -		if (lsapic->processor_id == acpi_id) {
> > > > -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> > > > -			return 1;
> > > > -		/* Check against optional uid */
> > > > -		} else if (entry->length >= 16 &&
> > > > -			lsapic->uid == acpi_id) {
> > > > -			*apic_id = lsapic->uid;
> > > > -			return 1;
> > > > -		}
> > > > -	}
> > > > +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> > > > +		return 0;
> > > > +
> > > > +	/* Device statement declaration type */
> > > > +	if (device_declaration) {
> > > > +		if (entry->length < 16)
> > > > +			printk(KERN_ERR PREFIX "Invalid Local SAPIC structure corresponding to Device statement processor with SAPIC ID %#x\n", tmp);
> > > > +		else if (lsapic->uid == acpi_id)
> > > > +			goto found;
> > > > +	/* Processor statement declaration type */
> > > > +	} else if (lsapic->processor_id == acpi_id)
> > > > +		goto found;
> > > > +
> > > >  	return 0;
> > > > +found:
> > > > +	*apic_id = tmp;
> > > > +	return 1;
> > > >  }
> > > 
> > > If I read this right, lsapic->uid and acpi_id (Processor Device _UID)
> > > no longer need to equal (lsapic->id << 8) | lsapic->eid), as now 'tmp'
> > > will be the value returned.
> > > Is this correct?
> > Yes.
> > > I don't know of a problem with this, just making sure I understand this.
> > Yeah - there were problems  ;)
> > 
> > What I think you might be getting at is that there are two possible
> > explanations for why this logic has been working till now:
> >       * There have been platforms using the "Device" declaration but the
> >         firmware must have went through great pains to ensure that all
> >         these fields involved *artificially* matched (which is what I
> >         believe you are alluding to and I believe is a misunderstanding
> >         of the spec if that was the case).
> 
> Yes, this is what motivated my questions.
> As part of testing firmware support for an upcoming platform, where
> "Device" declarations will be used, I quickly learned that currently
> all these fields need to match.
Great!  I too was bringing up an upcoming platform that is our first
platform to use "Device" declarations and got bit because of the current
logic.

So, based on the ACPI specification, my patch, and replies: do you agree
that for "Device" statement declarations the platform should have the
"Device" statement's '_UID' child object value match the corresponding
MADT (or_MAT) table Local SAPIC entry's "ACPI Processor UID Value" to
end up with the correct ID:EID?  The fact that the platform's firmware
(ACPI namespace) needed to also match the Local SAPIC entry's "ACPI
Processor ID" was really not necessary (this was the broken part of the
existing implementation) was really unnecessary and is really a
misunderstanding or misinterpretation of the specification?

Let me know how this area goes for you with my patches - hopefully they
help solve your issues!

Thanks for your information and please keep corresponding until we get
this solved to all of our satisfaction,

Myron
> 
> Thanks for the reply,
> John
> 
> 
> >   - or -
> >       * There have been no platforms using the "Device" declaration and
> >         so map_lsapic_id()'s 'if (lsapic->processor_id == acpi_id)' was
> >         always succeeding and the 'else if ...' path was never taken. I
> >         tend to believe this has been the case (but that is pure
> >         assumption which means little).
> > > 
> > > Glad to see this change, as I had concerns of map_lsapic_id() first
> > > trying to match against lsapic->processor_id, and possibly ignoring
> > > lsapic->uid.
> > Yes - the logic as it currently is, is not specific/strict enough.  What
> > I seem to be failing to be able to express well enough earlier in the
> > thread is basically this:
> > 
> >         CPUs declared in namespace using a "Processor" statement must
> >         use the "Processor" statement's 'ProcessorID' field to match to
> >         the corresponding Local SAPIC entry (matching the Local SAPIC
> >         entries 'ACPIProcessorID' field).  Whether or not the
> >         "Processor" statement has a '_UID' child object has no bearing
> >         with respect to the mapping.
> >         
> >         Conversely, CPUs declared in namespace using a "Device"
> >         statement must use the "Device" statement's '_UID' child object
> >         field to match to the corresponding Local SAPIC entry (matching
> >         the Local SAPIC entries '_UID' field).
> >         
> > At least this is how we are interpreting section 5.2.11.13 "Local SAPIC
> > Structure" of the specification (specifically see the Description for
> > both ACPI Processor ID and ACPI Processor UID Value).
> > 
> > Thanks for your query!
> > Myron
> > > 
> > > Thanks,
> > > John
> > > 
> > > 
> > > 
> > > >  
> > > > -static int map_madt_entry(u32 acpi_id)
> > > > +static int map_madt_entry(int type, u32 acpi_id)
> > > >  {
> > > >  	unsigned long madt_end, entry;
> > > >  	int apic_id = -1;
> > > > @@ -470,7 +476,7 @@ static int map_madt_entry(u32 acpi_id)
> > > >  			if (map_lapic_id(header, acpi_id, &apic_id))
> > > >  				break;
> > > >  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > > -			if (map_lsapic_id(header, acpi_id, &apic_id))
> > > > +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
> > > >  				break;
> > > >  		}
> > > >  		entry += header->length;
> > > > @@ -478,7 +484,7 @@ static int map_madt_entry(u32 acpi_id)
> > > >  	return apic_id;
> > > >  }
> > > >  
> > > > -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > > > +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
> > > >  {
> > > >  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > > >  	union acpi_object *obj;
> > > > @@ -501,7 +507,7 @@ static int map_mat_entry(acpi_handle han
> > > >  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
> > > >  		map_lapic_id(header, acpi_id, &apic_id);
> > > >  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > > -		map_lsapic_id(header, acpi_id, &apic_id);
> > > > +		map_lsapic_id(header, type, acpi_id, &apic_id);
> > > >  	}
> > > >  
> > > >  exit:
> > > > @@ -510,14 +516,14 @@ exit:
> > > >  	return apic_id;
> > > >  }
> > > >  
> > > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
> > > >  {
> > > >  	int i;
> > > >  	int apic_id = -1;
> > > >  
> > > > -	apic_id = map_mat_entry(handle, acpi_id);
> > > > +	apic_id = map_mat_entry(handle, type, acpi_id);
> > > >  	if (apic_id == -1)
> > > > -		apic_id = map_madt_entry(acpi_id);
> > > > +		apic_id = map_madt_entry(type, acpi_id);
> > > >  	if (apic_id == -1)
> > > >  		return apic_id;
> > > >  
> > > > @@ -533,15 +539,16 @@ static int get_cpu_id(acpi_handle handle
> > > >                                   Driver Interface
> > > >     -------------------------------------------------------------------------- */
> > > >  
> > > > -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > > +static int acpi_processor_get_info(struct acpi_device *device)
> > > >  {
> > > >  	acpi_status status = 0;
> > > >  	union acpi_object object = { 0 };
> > > >  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> > > > -	int cpu_index;
> > > > +	struct acpi_processor *pr;
> > > > +	int cpu_index, device_declaration = 0;
> > > >  	static int cpu0_initialized;
> > > >  
> > > > -
> > > > +	pr = acpi_driver_data(device);
> > > >  	if (!pr)
> > > >  		return -EINVAL;
> > > >  
> > > > @@ -562,8 +569,8 @@ static int acpi_processor_get_info(struc
> > > >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > > >  				  "No bus mastering arbitration control\n"));
> > > >  
> > > > -	/* Check if it is a Device with HID and UID */
> > > > -	if (has_uid) {
> > > > +	/* Check if it is a Device statement declaration with HID and UID */
> > > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > > >  		unsigned long value;
> > > >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> > > >  						NULL, &value);
> > > > @@ -571,6 +578,7 @@ static int acpi_processor_get_info(struc
> > > >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> > > >  			return -ENODEV;
> > > >  		}
> > > > +		device_declaration = 1;
> > > >  		pr->acpi_id = value;
> > > >  	} else {
> > > >  		/*
> > > > @@ -590,7 +598,7 @@ static int acpi_processor_get_info(struc
> > > >  		*/
> > > >  		pr->acpi_id = object.processor.proc_id;
> > > >  	}
> > > > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > > > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> > > >  
> > > >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> > > >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > > > @@ -662,7 +670,7 @@ static int __cpuinit acpi_processor_star
> > > >  
> > > >  	pr = acpi_driver_data(device);
> > > >  
> > > > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > > > +	result = acpi_processor_get_info(device);
> > > >  	if (result) {
> > > >  		/* Processor is physically not present */
> > > >  		return 0;
> > > > 
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > 
> > > 
> > -- 
> > Myron Stowe                             HP Open Source & Linux Org
> > 
> > 
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH 1/3] ACPI: Disambiguate processor declaration type

[Myron Stowe]

On Fri, 2008-10-24 at 13:36 +0800, Zhao Yakui wrote:
> On Thu, 2008-10-23 at 21:07 -0600, Myron Stowe wrote:
> > On Fri, 2008-10-24 at 09:16 +0800, Zhao Yakui wrote:
> > > On Wed, 2008-10-22 at 13:12 -0600, Myron Stowe wrote:
> > > > Declaring processors in ACPI namespace can be done using either a "Processor"
> > > > definition or a "Device" definition (see section 8.4 - Declaring Processors;
> > > > "Advanced Configuration and Power Interface Specification", Revision 3.0b).
> > > > Currently the two processor declaration types are conflated.
> > > In most cases there is no _UID object under the scope of processor
> > > namespace. So the current source can work well without adding the new
> > > HID("ACPI_CPU") for processor definition.
> > > 
> > > If there exists the _UID object under the scope of processor namespace,
> > > IMO the ProcessorID returned by _UID will have higher priority. In such
> > > case the patch can't work well.
> > 
> > According to section 5.2.11.13 "Local SAPIC Structure" - Local SAPIC to
> > processor object association uses the 'ProcessorID' for CPUs declared
> > with "Processor" statements and Local SAPIC to processor object
> > association for CPUs declared with "Device" statements use the '_UID'.
> > There is no "higher priority" - the association is fixed and must take
> > into account the type of CPU declaration - either "Processor" or
> > "Device" - to use the appropriate field - either 'ProcessorID' or '_UID'
> > - for the match.
> Agree with what you said. For the processor definition using Processor
> macro, we can get the ACPI ID by the processor block or _UID object (if
> it exists) and match it with the ACPI processor ID filed of LAPIC table
> or sapic table. 
> If there is no _UID object under the scope of processor namespace, the
> current source code still can work well. In such case it is unnecessary
> to add a new HID. Of course it is harmless if you add it.
> > In example, the combinations of CPU declaration type used in combination
> > with whether or not the CPU declaration contains a _UID child object are
> >   "Processor" without a _UID child object (which our systems have)
> >   "Processor" with a _UID child object (which our systems have)
> It is very lucky that there exists such a system. Will you please
> confirm whether the ACPI ID returned by _UID object is consistent with
> the ACPI ID declared in the processor block?
> If they are the same value, any one can be used. 
> If they are different, maybe we should confirm which one is correct.
> 
> The remaining issue is that _UID can return the string according to ACPI
> spec. If the _UID string is returned, it should be matched with the ACPI
> UID_string field of slapic table to get the APIC ID of processor. 
> 
> If you can confirm that the integer is returned by the _UID object under
> the scope of processor using device definition on your system, I think
> that your patch is OK. 
Yakui:

After I encountered the issue that this patch series addresses I wrote
up a synopsis in my notes.  I find that trying to write something down
really helps uncover areas I think I am familiar with but may not really
be.  Plus, I then have the notes for future reference.

Anyway, I really didn't plan on sending this out but here is my write up
from encountering this issue initially.  Please take the time to read
this and follow the code.  If you follow the examples carefully I
believe that you will then come to the same conclusion that I did and
thus then understand the need for this patch series.

Thanks for your questions as I'm sure others have had the exact same
ones.

Myron


SYNOPSIS (This is IA64 specific)

During early boot two tables are created which map Logical CPUs to the
CPUs Physical {S}APIC ID.  These tables are 'smp_boot_data' and
'ia64_cpu_to_sapic[]'.

  arch/ia64/include/asm/smp.h::
  extern struct smp_boot_data {
        int cpu_count;
        int cpu_phys_id[NR_CPUS];
  } smp_boot_data __initdata;

  arch/ia64/kernel/smpboot.c::
  /* which logical CPU number maps to which CPU (physical APIC ID) */
  volatile int ia64_cpu_to_sapicid[NR_CPUS];
  EXPORT_SYMBOL(ia64_cpu_to_sapicid);

A CPUs Physical {S}APIC ID, often referred to as the CPUs Physical ID,
is obtained from the CPUs corresponding LOCAL APIC entry in the
platform's MADT by concatinating the Logical ID and EID -
"lid.f.id << 8 | lid.f.eid;".

During initialization arch/ia64/kernel/acpi.c::acpi_boot_init() is
invoked which parses the MADT via acpi_parse_lsapic().
acpi_parse_lsapic() sets up 'available_cpus', 'total_cpus', and
'smp_boot_data' which contains the 'cpu_phys_id[]' table.  Later, still
within the acpi_boot_init() routine, smp_build_cpu_map() is invoked.
smp_build_cpu_map() initializes the 'arch_cpu_to_apicid[]' (which for
IA64 is 'ia64_cpu_to_sapicid[]) table using information from
'smp_boot_data'.

        acpi_boot_init()
        {
                acpi_parse_lsapic();
                ...
                smp_build_cpu_map();
        }

In the above initialization, 'boot_cpu_id' is handled specially -
  arch/ia64/kernel/smpboot.c::smp_build_cpu_map()
  int boot_cpu_id = hard_smp_processor_id();

  arch/ia64/include/asm/smp.h::
  static inline unsigned int ia64_get_lid (void)
  {
        union {
                struct {
                        unsigned long reserved : 16;
                        unsigned long eid : 8;
                        unsigned long id : 8;
                        unsigned long ignored : 32;
                } f;
                unsigned long bits;
        } lid;

        lid.bits = ia64_getreg(_IA64_REG_CR_LID);
        return lid.f.id << 8 | lid.f.eid;
  }

  #define hard_smp_processor_id()         ia64_get_lid()

The result of this setup running on our new system yields:
  smp_build_cpu_map
    arch_cpu_to_apicid[00] 0x0000
    arch_cpu_to_apicid[01] 0x0100
    arch_cpu_to_apicid[02] 0x0200
    arch_cpu_to_apicid[03] 0x0300
    arch_cpu_to_apicid[04] 0x1000
    arch_cpu_to_apicid[05] 0x1100
    arch_cpu_to_apicid[06] 0x1200
    arch_cpu_to_apicid[07] 0x1300
    arch_cpu_to_apicid[08] 0x0002
    arch_cpu_to_apicid[09] 0x0102
    arch_cpu_to_apicid[10] 0x0202
    arch_cpu_to_apicid[11] 0x0302
    arch_cpu_to_apicid[12] 0x1002
    arch_cpu_to_apicid[13] 0x1102
    arch_cpu_to_apicid[14] 0x1202
    arch_cpu_to_apicid[15] 0x1302
  16 CPUs available, 16 CPUs total


Later in the kernel's boot processing 'acpi_processor_init()' is
invoked.  Via object oriented obfuscation both 'acpi_processor_add()'
and 'acpi_processor_start()' get invoked.  As part of
'acpi_processor_start()' processing 'acpi_processor_get_info()' is
called to get processor specific information (all of which is in
drivers/acpi/processor_core.c). 'acpi_processor_get_info()' is called
with two parameters; a pointer to an 'acpi_processor' struct and a flag
indicating whether or not the processor object has a _UID.  The latter
_UID flag was set earlier in the boot process - presumably when the ACPI
namespace was parsed.

The processor's _UID flag indicates which mechanism was used in the
namespace for declaring the processor: the ASL Processor statement or
ASL Device statement.  If the Device statement was used then the
processor's 'acpi_id' is set to it's _UID value: 'pr->acpi_id = value;'.
For Processor statement declarations the processor's 'acpi_id' is set to
the ProcessorID value: 'pr->acpi_id = object.processor.proc_id;'.  With
the processor's acpi_id ('pr->acpi_id') value obtained, 'get_cpu_id()'
is called to get and set the corresponding Logical CPU ID
('cpu_index'/'pr->apic_id').

'get_cpu_id()' first maps the processor's 'acpi_id' to the corresponding
'apic_id' using either the processor's _MAT ACPI entry if one exists or
the platforms MADT ACPI table.  Either way, the entry/table is parsed
for LOCAL_{S}APIC entries.  The parsing - 'map_lapic_id()' or
'map_lsapic_id()' - first checks the 'lapic_flags' field to see if the
processor is ENABLED (non-enabled processors are ignored).  Next the
'processor_id' member is checked against the 'acpi_id' for a match (this
checking takes into consideration _UID values for Local SAPIC entries:
        /* First check against id */
        if (lsapic->processor_id == acpi_id) {
                *apic_id = (lsapic->id << 8) | lsapic->eid;
                return 1;
        /* Check against optional uid */
        } else if (entry->length >= 16 && lsapic->uid == acpi_id) {
                *apic_id = lsapic->uid;
                return 1;
        }
Note that precedence was given to matching ASL Processor statement based
declarations over ASL Device statement declarations (this precedence is
the opposite of what was used in 'acpi_processor_get_info()').

If a 'processor_id' member match to the 'acpi_id' occurs, it is assigned
to 'apic_id' and used to obtain the CPU's Logical ID.  This mapping
utilizes the 'ia64_cpu_to_sapicid[]' table created earler in the
kernel's boot processing.  Each possible CPU is checked to see if it's
Physical ID matches the 'apic_id'.
        for_each_possible_cpu(i) {
                if (cpu_physical_id(i) == apic_id)
                        return i;
        }
'cpu_physical_id' is an alias for the ia64_cpu_to_sapic[]' table -
  #define cpu_physical_id(i) ia64_cpu_to_sapicid[i]):

So, there are at least three issues here:
      * Both Processor statement and Device statement processor
        declarations get conflated to 'ACPI0007'.  Code then later tries
        to make the distinction bases solely on whether or not the
        processor declaration's scope has a _UID object (i.e. Processor
        statement declarations can have a _UID within the processor's
        scope!).  This logic is not strict enough - it is *not*
        comparing the correct components which is dependent on the
        processor's declaration type.
      * When mapping the 'acpi_id' the precidence is reversed from
        before (i.e. initially precidence is given to _UID, subsequently
        in the mapping it is not.  Actually this is pretty much a mute
        point in that this type of logic is not strict enough as covered
        above).
      * In the mapping - map_lsapic_id() - the fall-through path
        incorrectly returns the Local SAPIC _UID value and *not*
        '(lsapic->id << 8) | lsapic->eid' and so the subsequent mapping
        at the end of 'get_cpu_id' against the table set up at early
        boot will *not* match (or worse - a false positive can occur).
Basically what is occuring is the table 'ia64_cpu_to_sapicid[]' was
correctly calculated using ID:EID and, in the case of Device statement
declarations, the processor's _UID value is being used trying to
match/map to the ID:EID value.


As a concrete example, using our new system's FW, the MADT - when
decoded - is basically:

  LogicalID               PhysicalID (ID:EID)   _UID
  ----------------------------------------------------------
  arch_cpu_to_apicid[00]  0x0000                0x00
  arch_cpu_to_apicid[01]  0x0100                0x01
  arch_cpu_to_apicid[02]  0x0200                0x02
  arch_cpu_to_apicid[03]  0x0300                0x03
  arch_cpu_to_apicid[04]  0x1000                0x04
  arch_cpu_to_apicid[05]  0x1100                0x05
  arch_cpu_to_apicid[06]  0x1200                0x06
  arch_cpu_to_apicid[07]  0x1300                0x07
  arch_cpu_to_apicid[08]  0x0002                0x10
  arch_cpu_to_apicid[09]  0x0102                0x11
  arch_cpu_to_apicid[0A]  0x0202                0x12
  arch_cpu_to_apicid[0B]  0x0302                0x13
  arch_cpu_to_apicid[0C]  0x1002                0x14
  arch_cpu_to_apicid[0D]  0x1102                0x15
  arch_cpu_to_apicid[0E]  0x1202                0x16
  arch_cpu_to_apicid[0F]  0x1302                0x17

During early boot processing both the 'smp_boot_data' and
'ia64_cpu_to_sapic[]' table are initialized.  Both tables end up with a
mapping of Logical Processor ID to Physical {S}APIC ID as shown above
(at this point ignore the _UID column).  In example, Logical CPU 0x0B's
entry contains the processor's correspoding Local APIC ID of 0x0302.

Next, 'acpi_processor_get_info()' is called with it's second argument
indicating the processor object contains a _UID method (the implication
being that the ACPI namespace processor declaration used the AML Device
statement [is this necessarly true - i.e. can a processor statement have
a _UID method within it's scope?]).  'acpi_processor_get_info()' gives
precedence to the _UID and so calls 'get_cpu_id()' with the processor's
_UID as the 'acpi_id'.  Continuing the example, 'get_cpu_id()' is called
on behalf of Logical processor 0xB with a _UID value of 0x13.
        Device (P00B)
        {
                Name (_HID, "ACPI0007")
                Name (_UID, 0x00000013)
                ...
                Method (_MAT, 0, NotSerialized)
                {
                    Return (Buffer (0x10)
                    {
                        0x07, 0x10, 0x00, 0x03, 0x02, 0x00, 0x00, 0x00,
                        0x01, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00
                    })
                }

'get_cpu_id()' attempts to map the 'acpi_id' of 0x13 to the
corresponding _MAT entries PhysicalID - 0x0302 - which does not match.
Next an attempt to map the 'acpi_id' to the _MAT entries _UID - 0x13 -
which does match so 'apic_id' is set to 0x13.  The final step of mapping
the 'apic_id' to a Logical CPU ID is attempted by parsing all the
possible CPUS via the 'arch_cpu_to_apicid[]' table entries is made but
no 'arch_cpu_to_apicid[]' entry value is 0x13!

Note that a "false positive" can be obtained - follow the same steps
using the values corresponding to Logical CPU 0x02 whose _UID value ix
0x2.  The code will produce an incorrect Logical CPU value of 0x08!

> 
> Of course we should add such enhancement.
> 
> >   "Device" without a _UID child object
> >   "Device" with a _UID child object (which our systems now have)
> > In the "Processor" declarations the match to the Local SAPIC is based on
> > the 'ProcessorID' value regardless of whether or not there is a _UID
> > child object.  For "Device" declarations, the match to the Local SAPIC
> > is based on the '_UID' of the child object - so the third case above
> > ("Device" without a _UID child object) would be illegal.
> > 
> > 
> > This patch separates the type of CPU declaration that was encountered in
> > the namespace (the current code conflated them into a single #define).
> > The separation enables the mapping logic, that is done later, know
> > explicitly which CPU declaration type was used so that it can use the
> > proper field - 'ProcessorID' or '_UID' for the association.
> > 
> > If you do not agree with this interpretation of the spec then please let
> > me know where you believe I am wrong.
> > 
> > Myron
> > > 
> > > > 
> > > > This patch disambiguates the processor declaration's definition type enabling
> > > > subsequent code to behave uniquely based explicitly on the declaration's type.
> > > > 
> > > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > > > 
> > > > ---
> > > >  drivers/acpi/processor_core.c |    1 +
> > > >  drivers/acpi/scan.c           |    2 +-
> > > >  include/acpi/acpi_drivers.h   |    1 +
> > > >  3 files changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-16 19:00:37.000000000 -0600
> > > > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-21 13:11:00.000000000 -0600
> > > > @@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
> > > >  
> > > > 
> > > >  static const struct acpi_device_id processor_device_ids[] = {
> > > > +	{ACPI_PROCESSOR_OBJECT_HID, 0},
> > > >  	{ACPI_PROCESSOR_HID, 0},
> > > >  	{"", 0},
> > > >  };
> > > > Index: linux-2.6/include/acpi/acpi_drivers.h
> > > > ===================================================================
> > > > --- linux-2.6.orig/include/acpi/acpi_drivers.h	2008-10-16 18:53:26.000000000 -0600
> > > > +++ linux-2.6/include/acpi/acpi_drivers.h	2008-10-20 13:23:28.000000000 -0600
> > > > @@ -41,6 +41,7 @@
> > > >   */
> > > >  
> > > >  #define ACPI_POWER_HID			"LNXPOWER"
> > > > +#define ACPI_PROCESSOR_OBJECT_HID	"ACPI_CPU"
> > > >  #define ACPI_PROCESSOR_HID		"ACPI0007"
> > > >  #define ACPI_SYSTEM_HID			"LNXSYSTM"
> > > >  #define ACPI_THERMAL_HID		"LNXTHERM"
> > > > Index: linux-2.6/drivers/acpi/scan.c
> > > > ===================================================================
> > > > --- linux-2.6.orig/drivers/acpi/scan.c	2008-10-16 19:04:58.000000000 -0600
> > > > +++ linux-2.6/drivers/acpi/scan.c	2008-10-21 13:09:09.000000000 -0600
> > > > @@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
> > > >  		hid = ACPI_POWER_HID;
> > > >  		break;
> > > >  	case ACPI_BUS_TYPE_PROCESSOR:
> > > > -		hid = ACPI_PROCESSOR_HID;
> > > > +		hid = ACPI_PROCESSOR_OBJECT_HID;
> > > >  		break;
> > > >  	case ACPI_BUS_TYPE_SYSTEM:
> > > >  		hid = ACPI_SYSTEM_HID;
> > > > 
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> 
-- 
Myron Stowe                             HP Open Source & Linux Org

Re: [PATCH 1/3] ACPI: Disambiguate processor declaration type

[Zhao Yakui]

On Fri, 2008-10-24 at 14:23 -0700, Myron Stowe wrote:
thanks for the detailed info and explanation.
Understand what you said and agree with your analysis.
There are some types of processor definitions.
   a. the processor is declared by using processor block in ACPI
namespace without _UID object.
      The ACPI ID is obtained from the processor block definition. 
      For example: processor(CPU0, 0x02, 0x810, 0x06). And the ACPI ID
is 0x02.
      Then the ACPI_id is matched with the ACPI processor ID field of
Local APIC table or SLAPIC table to get the processor APIC ID. (If the
SLAPIC table is used, the APIC id is obtained by the SAPIC_ID &
SAPIC_EID field).
     
      This case can be handled by your patch. This is handled by adding
the new HID("ACPI_CPU") for processor.
   b. the processor is declared by using processor block in ACPI
namespace with the _UID object. (For example: On your own system).
      It seems that the string is returned by the _UID object. In such
case we should continue to use the ACPI ID obtained by the ACPI
processor block. Of course the ACPI id is matched with the ACPI
processor ID field of LAPIC table or SLAPIC table.
      And what you have done is right. 
      
      Of course please add some comments that the ACPI ID is always
obtained from the processor block if the processor object is declared by
processor.
      
  c. the processor is declared by device with the HID
object("ACPI0007"). And the _UID object also exists.
     According to the spec the returned value of _UID can be an integer
or string.
     If the returned value is an integer, it is matched with the ACPI
processor UID field of SAPIC table to get the processor ID. And what you
have done is correct. And I agree with what you have done.
    
     If the returned value is a string, it should be matched with the
ACPI processor UID string field of SAPIC table to get the processor ID.
Now this case is not handled by your patch. Of course maybe there
doesn't exist such a system. So we can ask the user to send the ACPIdump
and then add the corresponding support when a string is returned by the
_UID object. It will be great if we can add the support about this.

  d. the processor is declared by the device with the HID
object("ACPI0007"). But there is no the _UID object. 
     This case is incorrect. And we can't get the ACPI processor ID.
     And this case can be handed by your patch. But IMO the debug info
is not enough.

     How about add the following debug info in your patch?
+       if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
+               union acpi_object *element;
+               struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER,
NULL };
                unsigned long long value;
-               status = acpi_evaluate_integer(pr->handle,
METHOD_NAME__UID,
-                                               NULL, &value);
+               if (!has_uid) {
+                       printk(KERN_ERR PREFIX "No _UID object for
ACPI0007"
+                               " device\n");
+                       return -ENODEV;
+               }
+               status = acpi_evaluate_object(pr->handle,
METHOD_NAME__UID,
+                                               NULL, &buffer);
                if (ACPI_FAILURE(status)) {
-                       printk(KERN_ERR PREFIX "Evaluating processor
_UID\n");
+                       printk(KERN_ERR PREFIX "Evaluating in _UID
object\n");
                        return -ENODEV;
                }
+               element = buffer->pointer;
// If we can match this string with the ACPI Processor UID string of
SAPIC table, it will be great.
+               if (element->type == ACPI_TYPE_STRING ||
+                       elememt->type == ACPI_TYPE_BUFFER) {
+                       printk(KERN_DEBUG PREFIX "a string is returned
by the"
+                               " _UID object of ACPI0007\n");
+                       kfree(buffer.pointer);
+                       return -ENODEV;
+               }
+               value =(unsigned long long)(element->integer.value);
+               device_declaration = 1;
                pr->acpi_id = value;

     
     
      
      

   
 

> Yakui:
> 
> After I encountered the issue that this patch series addresses I wrote
> up a synopsis in my notes.  I find that trying to write something down
> really helps uncover areas I think I am familiar with but may not really
> be.  Plus, I then have the notes for future reference.
> 
> Anyway, I really didn't plan on sending this out but here is my write up
> from encountering this issue initially.  Please take the time to read
> this and follow the code.  If you follow the examples carefully I
> believe that you will then come to the same conclusion that I did and
> thus then understand the need for this patch series.
> 
> Thanks for your questions as I'm sure others have had the exact same
> ones.
> 
> Myron
> 
> 
> SYNOPSIS (This is IA64 specific)
> 
> During early boot two tables are created which map Logical CPUs to the
> CPUs Physical {S}APIC ID.  These tables are 'smp_boot_data' and
> 'ia64_cpu_to_sapic[]'.
> 
>   arch/ia64/include/asm/smp.h::
>   extern struct smp_boot_data {
>         int cpu_count;
>         int cpu_phys_id[NR_CPUS];
>   } smp_boot_data __initdata;
> 
>   arch/ia64/kernel/smpboot.c::
>   /* which logical CPU number maps to which CPU (physical APIC ID) */
>   volatile int ia64_cpu_to_sapicid[NR_CPUS];
>   EXPORT_SYMBOL(ia64_cpu_to_sapicid);
> 
> A CPUs Physical {S}APIC ID, often referred to as the CPUs Physical ID,
> is obtained from the CPUs corresponding LOCAL APIC entry in the
> platform's MADT by concatinating the Logical ID and EID -
> "lid.f.id << 8 | lid.f.eid;".
> 
> During initialization arch/ia64/kernel/acpi.c::acpi_boot_init() is
> invoked which parses the MADT via acpi_parse_lsapic().
> acpi_parse_lsapic() sets up 'available_cpus', 'total_cpus', and
> 'smp_boot_data' which contains the 'cpu_phys_id[]' table.  Later, still
> within the acpi_boot_init() routine, smp_build_cpu_map() is invoked.
> smp_build_cpu_map() initializes the 'arch_cpu_to_apicid[]' (which for
> IA64 is 'ia64_cpu_to_sapicid[]) table using information from
> 'smp_boot_data'.
> 
>         acpi_boot_init()
>         {
>                 acpi_parse_lsapic();
>                 ...
>                 smp_build_cpu_map();
>         }
> 
> In the above initialization, 'boot_cpu_id' is handled specially -
>   arch/ia64/kernel/smpboot.c::smp_build_cpu_map()
>   int boot_cpu_id = hard_smp_processor_id();
> 
>   arch/ia64/include/asm/smp.h::
>   static inline unsigned int ia64_get_lid (void)
>   {
>         union {
>                 struct {
>                         unsigned long reserved : 16;
>                         unsigned long eid : 8;
>                         unsigned long id : 8;
>                         unsigned long ignored : 32;
>                 } f;
>                 unsigned long bits;
>         } lid;
> 
>         lid.bits = ia64_getreg(_IA64_REG_CR_LID);
>         return lid.f.id << 8 | lid.f.eid;
>   }
> 
>   #define hard_smp_processor_id()         ia64_get_lid()
> 
> The result of this setup running on our new system yields:
>   smp_build_cpu_map
>     arch_cpu_to_apicid[00] 0x0000
>     arch_cpu_to_apicid[01] 0x0100
>     arch_cpu_to_apicid[02] 0x0200
>     arch_cpu_to_apicid[03] 0x0300
>     arch_cpu_to_apicid[04] 0x1000
>     arch_cpu_to_apicid[05] 0x1100
>     arch_cpu_to_apicid[06] 0x1200
>     arch_cpu_to_apicid[07] 0x1300
>     arch_cpu_to_apicid[08] 0x0002
>     arch_cpu_to_apicid[09] 0x0102
>     arch_cpu_to_apicid[10] 0x0202
>     arch_cpu_to_apicid[11] 0x0302
>     arch_cpu_to_apicid[12] 0x1002
>     arch_cpu_to_apicid[13] 0x1102
>     arch_cpu_to_apicid[14] 0x1202
>     arch_cpu_to_apicid[15] 0x1302
>   16 CPUs available, 16 CPUs total
> 
> 
> Later in the kernel's boot processing 'acpi_processor_init()' is
> invoked.  Via object oriented obfuscation both 'acpi_processor_add()'
> and 'acpi_processor_start()' get invoked.  As part of
> 'acpi_processor_start()' processing 'acpi_processor_get_info()' is
> called to get processor specific information (all of which is in
> drivers/acpi/processor_core.c). 'acpi_processor_get_info()' is called
> with two parameters; a pointer to an 'acpi_processor' struct and a flag
> indicating whether or not the processor object has a _UID.  The latter
> _UID flag was set earlier in the boot process - presumably when the ACPI
> namespace was parsed.
> 
> The processor's _UID flag indicates which mechanism was used in the
> namespace for declaring the processor: the ASL Processor statement or
> ASL Device statement.  If the Device statement was used then the
> processor's 'acpi_id' is set to it's _UID value: 'pr->acpi_id = value;'.
> For Processor statement declarations the processor's 'acpi_id' is set to
> the ProcessorID value: 'pr->acpi_id = object.processor.proc_id;'.  With
> the processor's acpi_id ('pr->acpi_id') value obtained, 'get_cpu_id()'
> is called to get and set the corresponding Logical CPU ID
> ('cpu_index'/'pr->apic_id').
> 
> 'get_cpu_id()' first maps the processor's 'acpi_id' to the corresponding
> 'apic_id' using either the processor's _MAT ACPI entry if one exists or
> the platforms MADT ACPI table.  Either way, the entry/table is parsed
> for LOCAL_{S}APIC entries.  The parsing - 'map_lapic_id()' or
> 'map_lsapic_id()' - first checks the 'lapic_flags' field to see if the
> processor is ENABLED (non-enabled processors are ignored).  Next the
> 'processor_id' member is checked against the 'acpi_id' for a match (this
> checking takes into consideration _UID values for Local SAPIC entries:
>         /* First check against id */
>         if (lsapic->processor_id == acpi_id) {
>                 *apic_id = (lsapic->id << 8) | lsapic->eid;
>                 return 1;
>         /* Check against optional uid */
>         } else if (entry->length >= 16 && lsapic->uid == acpi_id) {
>                 *apic_id = lsapic->uid;
>                 return 1;
>         }
> Note that precedence was given to matching ASL Processor statement based
> declarations over ASL Device statement declarations (this precedence is
> the opposite of what was used in 'acpi_processor_get_info()').
> 
> If a 'processor_id' member match to the 'acpi_id' occurs, it is assigned
> to 'apic_id' and used to obtain the CPU's Logical ID.  This mapping
> utilizes the 'ia64_cpu_to_sapicid[]' table created earler in the
> kernel's boot processing.  Each possible CPU is checked to see if it's
> Physical ID matches the 'apic_id'.
>         for_each_possible_cpu(i) {
>                 if (cpu_physical_id(i) == apic_id)
>                         return i;
>         }
> 'cpu_physical_id' is an alias for the ia64_cpu_to_sapic[]' table -
>   #define cpu_physical_id(i) ia64_cpu_to_sapicid[i]):
> 
> So, there are at least three issues here:
>       * Both Processor statement and Device statement processor
>         declarations get conflated to 'ACPI0007'.  Code then later tries
>         to make the distinction bases solely on whether or not the
>         processor declaration's scope has a _UID object (i.e. Processor
>         statement declarations can have a _UID within the processor's
>         scope!).  This logic is not strict enough - it is *not*
>         comparing the correct components which is dependent on the
>         processor's declaration type.
>       * When mapping the 'acpi_id' the precidence is reversed from
>         before (i.e. initially precidence is given to _UID, subsequently
>         in the mapping it is not.  Actually this is pretty much a mute
>         point in that this type of logic is not strict enough as covered
>         above).
>       * In the mapping - map_lsapic_id() - the fall-through path
>         incorrectly returns the Local SAPIC _UID value and *not*
>         '(lsapic->id << 8) | lsapic->eid' and so the subsequent mapping
>         at the end of 'get_cpu_id' against the table set up at early
>         boot will *not* match (or worse - a false positive can occur).
> Basically what is occuring is the table 'ia64_cpu_to_sapicid[]' was
> correctly calculated using ID:EID and, in the case of Device statement
> declarations, the processor's _UID value is being used trying to
> match/map to the ID:EID value.
> 
> 
> As a concrete example, using our new system's FW, the MADT - when
> decoded - is basically:
> 
>   LogicalID               PhysicalID (ID:EID)   _UID
>   ----------------------------------------------------------
>   arch_cpu_to_apicid[00]  0x0000                0x00
>   arch_cpu_to_apicid[01]  0x0100                0x01
>   arch_cpu_to_apicid[02]  0x0200                0x02
>   arch_cpu_to_apicid[03]  0x0300                0x03
>   arch_cpu_to_apicid[04]  0x1000                0x04
>   arch_cpu_to_apicid[05]  0x1100                0x05
>   arch_cpu_to_apicid[06]  0x1200                0x06
>   arch_cpu_to_apicid[07]  0x1300                0x07
>   arch_cpu_to_apicid[08]  0x0002                0x10
>   arch_cpu_to_apicid[09]  0x0102                0x11
>   arch_cpu_to_apicid[0A]  0x0202                0x12
>   arch_cpu_to_apicid[0B]  0x0302                0x13
>   arch_cpu_to_apicid[0C]  0x1002                0x14
>   arch_cpu_to_apicid[0D]  0x1102                0x15
>   arch_cpu_to_apicid[0E]  0x1202                0x16
>   arch_cpu_to_apicid[0F]  0x1302                0x17
> 
> During early boot processing both the 'smp_boot_data' and
> 'ia64_cpu_to_sapic[]' table are initialized.  Both tables end up with a
> mapping of Logical Processor ID to Physical {S}APIC ID as shown above
> (at this point ignore the _UID column).  In example, Logical CPU 0x0B's
> entry contains the processor's correspoding Local APIC ID of 0x0302.
> 
> Next, 'acpi_processor_get_info()' is called with it's second argument
> indicating the processor object contains a _UID method (the implication
> being that the ACPI namespace processor declaration used the AML Device
> statement [is this necessarly true - i.e. can a processor statement have
> a _UID method within it's scope?]).  'acpi_processor_get_info()' gives
> precedence to the _UID and so calls 'get_cpu_id()' with the processor's
> _UID as the 'acpi_id'.  Continuing the example, 'get_cpu_id()' is called
> on behalf of Logical processor 0xB with a _UID value of 0x13.
>         Device (P00B)
>         {
>                 Name (_HID, "ACPI0007")
>                 Name (_UID, 0x00000013)
>                 ...
>                 Method (_MAT, 0, NotSerialized)
>                 {
>                     Return (Buffer (0x10)
>                     {
>                         0x07, 0x10, 0x00, 0x03, 0x02, 0x00, 0x00, 0x00,
>                         0x01, 0x00, 0x00, 0x00, 0x13, 0x00, 0x00, 0x00
>                     })
>                 }
> 
> 'get_cpu_id()' attempts to map the 'acpi_id' of 0x13 to the
> corresponding _MAT entries PhysicalID - 0x0302 - which does not match.
> Next an attempt to map the 'acpi_id' to the _MAT entries _UID - 0x13 -
> which does match so 'apic_id' is set to 0x13.  The final step of mapping
> the 'apic_id' to a Logical CPU ID is attempted by parsing all the
> possible CPUS via the 'arch_cpu_to_apicid[]' table entries is made but
> no 'arch_cpu_to_apicid[]' entry value is 0x13!
> 
> Note that a "false positive" can be obtained - follow the same steps
> using the values corresponding to Logical CPU 0x02 whose _UID value ix
> 0x2.  The code will produce an incorrect Logical CPU value of 0x08!
> 
> >
> > Of course we should add such enhancement.
> >
> > >   "Device" without a _UID child object
> > >   "Device" with a _UID child object (which our systems now have)
> > > In the "Processor" declarations the match to the Local SAPIC is based on
> > > the 'ProcessorID' value regardless of whether or not there is a _UID
> > > child object.  For "Device" declarations, the match to the Local SAPIC
> > > is based on the '_UID' of the child object - so the third case above
> > > ("Device" without a _UID child object) would be illegal.
> > >
> > >
> > > This patch separates the type of CPU declaration that was encountered in
> > > the namespace (the current code conflated them into a single #define).
> > > The separation enables the mapping logic, that is done later, know
> > > explicitly which CPU declaration type was used so that it can use the
> > > proper field - 'ProcessorID' or '_UID' for the association.
> > >
> > > If you do not agree with this interpretation of the spec then please let
> > > me know where you believe I am wrong.
> > >
> > > Myron
> > > >
> > > > >
> > > > > This patch disambiguates the processor declaration's definition type enabling
> > > > > subsequent code to behave uniquely based explicitly on the declaration's type.
> > > > >
> > > > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > > > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > > > >
> > > > > ---
> > > > >  drivers/acpi/processor_core.c |    1 +
> > > > >  drivers/acpi/scan.c           |    2 +-
> > > > >  include/acpi/acpi_drivers.h   |    1 +
> > > > >  3 files changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/acpi/processor_core.c        2008-10-16 19:00:37.000000000 -0600
> > > > > +++ linux-2.6/drivers/acpi/processor_core.c     2008-10-21 13:11:00.000000000 -0600
> > > > > @@ -89,6 +89,7 @@ static int acpi_processor_handle_eject(s
> > > > >
> > > > >
> > > > >  static const struct acpi_device_id processor_device_ids[] = {
> > > > > +       {ACPI_PROCESSOR_OBJECT_HID, 0},
> > > > >         {ACPI_PROCESSOR_HID, 0},
> > > > >         {"", 0},
> > > > >  };
> > > > > Index: linux-2.6/include/acpi/acpi_drivers.h
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/include/acpi/acpi_drivers.h  2008-10-16 18:53:26.000000000 -0600
> > > > > +++ linux-2.6/include/acpi/acpi_drivers.h       2008-10-20 13:23:28.000000000 -0600
> > > > > @@ -41,6 +41,7 @@
> > > > >   */
> > > > >
> > > > >  #define ACPI_POWER_HID                 "LNXPOWER"
> > > > > +#define ACPI_PROCESSOR_OBJECT_HID      "ACPI_CPU"
> > > > >  #define ACPI_PROCESSOR_HID             "ACPI0007"
> > > > >  #define ACPI_SYSTEM_HID                        "LNXSYSTM"
> > > > >  #define ACPI_THERMAL_HID               "LNXTHERM"
> > > > > Index: linux-2.6/drivers/acpi/scan.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/acpi/scan.c  2008-10-16 19:04:58.000000000 -0600
> > > > > +++ linux-2.6/drivers/acpi/scan.c       2008-10-21 13:09:09.000000000 -0600
> > > > > @@ -1025,7 +1025,7 @@ static void acpi_device_set_id(struct ac
> > > > >                 hid = ACPI_POWER_HID;
> > > > >                 break;
> > > > >         case ACPI_BUS_TYPE_PROCESSOR:
> > > > > -               hid = ACPI_PROCESSOR_HID;
> > > > > +               hid = ACPI_PROCESSOR_OBJECT_HID;
> > > > >                 break;
> > > > >         case ACPI_BUS_TYPE_SYSTEM:
> > > > >                 hid = ACPI_SYSTEM_HID;
> > > > >
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > >
> >
> --
> Myron Stowe                             HP Open Source & Linux Org
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] ACPI: Behave uniquely based on processor

[John Keller]

> 
> On Fri, 2008-10-24 at 13:42 -0500, John Keller wrote:
> > > 
> > > On Fri, 2008-10-24 at 10:56 -0500, John Keller wrote:
> > > > > 
> > > > > 
> > > > > Associating a Local SAPIC with a processor object is dependent upon the
> > > > > processor object's definition type.  CPUs declared as "Processor" should use
> > > > > the Local SAPIC's 'processor_id', and CPUs declared as "Device" should use the
> > > > > 'uid' (see section 5.2.11.13 - Local SAPIC Structure; "Advanced Configuration
> > > > > and Power Interface Specification", Revision 3.0b).  
> > > > > 
> > > > > This patch changes the lsapic mapping logic to rely on the distinction of
> > > > > how the processor object was declared - the mapping can't just try both types
> > > > > of matches irregardless of declaration type and rely on one failing as is
> > > > > currently being done.
> > > > > 
> > > > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > > > CC: Alexey Starikovskiy <aystarik@gmail.com>
> > > > > 
> > > > > ---
> > > > >  drivers/acpi/processor_core.c |   64 +++++++++++++++++++++++-------------------
> > > > >  1 file changed, 36 insertions(+), 28 deletions(-)
> > > > > 
> > > > > Index: linux-2.6/drivers/acpi/processor_core.c
> > > > > ===================================================================
> > > > > --- linux-2.6.orig/drivers/acpi/processor_core.c	2008-10-21 13:12:13.000000000 -0600
> > > > > +++ linux-2.6/drivers/acpi/processor_core.c	2008-10-22 12:12:11.000000000 -0600
> > > > > @@ -410,7 +410,7 @@ static int acpi_processor_remove_fs(stru
> > > > >  /* Use the acpiid in MADT to map cpus in case of SMP */
> > > > >  
> > > > >  #ifndef CONFIG_SMP
> > > > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id) {return -1;}
> > > > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
> > > > >  #else
> > > > >  
> > > > >  static struct acpi_table_madt *madt;
> > > > > @@ -429,27 +429,33 @@ static int map_lapic_id(struct acpi_subt
> > > > >  }
> > > > >  
> > > > >  static int map_lsapic_id(struct acpi_subtable_header *entry,
> > > > > -		  u32 acpi_id, int *apic_id)
> > > > > +		int device_declaration, u32 acpi_id, int *apic_id)
> > > > >  {
> > > > >  	struct acpi_madt_local_sapic *lsapic =
> > > > >  		(struct acpi_madt_local_sapic *)entry;
> > > > > +	u32 tmp = (lsapic->id << 8) | lsapic->eid;
> > > > > +
> > > > >  	/* Only check enabled APICs*/
> > > > > -	if (lsapic->lapic_flags & ACPI_MADT_ENABLED) {
> > > > > -		/* First check against id */
> > > > > -		if (lsapic->processor_id == acpi_id) {
> > > > > -			*apic_id = (lsapic->id << 8) | lsapic->eid;
> > > > > -			return 1;
> > > > > -		/* Check against optional uid */
> > > > > -		} else if (entry->length >= 16 &&
> > > > > -			lsapic->uid == acpi_id) {
> > > > > -			*apic_id = lsapic->uid;
> > > > > -			return 1;
> > > > > -		}
> > > > > -	}
> > > > > +	if (!(lsapic->lapic_flags & ACPI_MADT_ENABLED))
> > > > > +		return 0;
> > > > > +
> > > > > +	/* Device statement declaration type */
> > > > > +	if (device_declaration) {
> > > > > +		if (entry->length < 16)
> > > > > +			printk(KERN_ERR PREFIX "Invalid Local SAPIC structure corresponding to Device statement processor with SAPIC ID %#x\n", tmp);
> > > > > +		else if (lsapic->uid == acpi_id)
> > > > > +			goto found;
> > > > > +	/* Processor statement declaration type */
> > > > > +	} else if (lsapic->processor_id == acpi_id)
> > > > > +		goto found;
> > > > > +
> > > > >  	return 0;
> > > > > +found:
> > > > > +	*apic_id = tmp;
> > > > > +	return 1;
> > > > >  }
> > > > 
> > > > If I read this right, lsapic->uid and acpi_id (Processor Device _UID)
> > > > no longer need to equal (lsapic->id << 8) | lsapic->eid), as now 'tmp'
> > > > will be the value returned.
> > > > Is this correct?
> > > Yes.
> > > > I don't know of a problem with this, just making sure I understand this.
> > > Yeah - there were problems  ;)
> > > 
> > > What I think you might be getting at is that there are two possible
> > > explanations for why this logic has been working till now:
> > >       * There have been platforms using the "Device" declaration but the
> > >         firmware must have went through great pains to ensure that all
> > >         these fields involved *artificially* matched (which is what I
> > >         believe you are alluding to and I believe is a misunderstanding
> > >         of the spec if that was the case).
> > 
> > Yes, this is what motivated my questions.
> > As part of testing firmware support for an upcoming platform, where
> > "Device" declarations will be used, I quickly learned that currently
> > all these fields need to match.
> Great!  I too was bringing up an upcoming platform that is our first
> platform to use "Device" declarations and got bit because of the current
> logic.
> 
> So, based on the ACPI specification, my patch, and replies: do you agree
> that for "Device" statement declarations the platform should have the
> "Device" statement's '_UID' child object value match the corresponding
> MADT (or_MAT) table Local SAPIC entry's "ACPI Processor UID Value" to
> end up with the correct ID:EID? 

Agreed.


> The fact that the platform's firmware
> (ACPI namespace) needed to also match the Local SAPIC entry's "ACPI
> Processor ID" was really not necessary (this was the broken part of the
> existing implementation) was really unnecessary and is really a
> misunderstanding or misinterpretation of the specification?

I suspect I agree with you on this too.
The two issues for me were:
	- The Device UID and Local SAPIC entry's UID values needed
	  to also match the Local SAPIC entry's
	  ((LocalSAPIC ID << 8) | LocalSAPIC EID).

	- The map_lsapic_id() code was first trying to match the
	  acpi_id with the Local SAPIC entry's "ACPI Processor UID Value"
	  even if the acpi_id was obtained from the "Device"
	  statement's '_UID'.

> 
> Let me know how this area goes for you with my patches - hopefully they
> help solve your issues!

OK, though I'm still in the very early phases of all this.


> 
> Thanks for your information and please keep corresponding until we get
> this solved to all of our satisfaction,
> 
> Myron
> > 
> > Thanks for the reply,
> > John
> > 
> > 
> > >   - or -
> > >       * There have been no platforms using the "Device" declaration and
> > >         so map_lsapic_id()'s 'if (lsapic->processor_id == acpi_id)' was
> > >         always succeeding and the 'else if ...' path was never taken. I
> > >         tend to believe this has been the case (but that is pure
> > >         assumption which means little).
> > > > 
> > > > Glad to see this change, as I had concerns of map_lsapic_id() first
> > > > trying to match against lsapic->processor_id, and possibly ignoring
> > > > lsapic->uid.
> > > Yes - the logic as it currently is, is not specific/strict enough.  What
> > > I seem to be failing to be able to express well enough earlier in the
> > > thread is basically this:
> > > 
> > >         CPUs declared in namespace using a "Processor" statement must
> > >         use the "Processor" statement's 'ProcessorID' field to match to
> > >         the corresponding Local SAPIC entry (matching the Local SAPIC
> > >         entries 'ACPIProcessorID' field).  Whether or not the
> > >         "Processor" statement has a '_UID' child object has no bearing
> > >         with respect to the mapping.
> > >         
> > >         Conversely, CPUs declared in namespace using a "Device"
> > >         statement must use the "Device" statement's '_UID' child object
> > >         field to match to the corresponding Local SAPIC entry (matching
> > >         the Local SAPIC entries '_UID' field).
> > >         
> > > At least this is how we are interpreting section 5.2.11.13 "Local SAPIC
> > > Structure" of the specification (specifically see the Description for
> > > both ACPI Processor ID and ACPI Processor UID Value).
> > > 
> > > Thanks for your query!
> > > Myron
> > > > 
> > > > Thanks,
> > > > John
> > > > 
> > > > 
> > > > 
> > > > >  
> > > > > -static int map_madt_entry(u32 acpi_id)
> > > > > +static int map_madt_entry(int type, u32 acpi_id)
> > > > >  {
> > > > >  	unsigned long madt_end, entry;
> > > > >  	int apic_id = -1;
> > > > > @@ -470,7 +476,7 @@ static int map_madt_entry(u32 acpi_id)
> > > > >  			if (map_lapic_id(header, acpi_id, &apic_id))
> > > > >  				break;
> > > > >  		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > > > -			if (map_lsapic_id(header, acpi_id, &apic_id))
> > > > > +			if (map_lsapic_id(header, type, acpi_id, &apic_id))
> > > > >  				break;
> > > > >  		}
> > > > >  		entry += header->length;
> > > > > @@ -478,7 +484,7 @@ static int map_madt_entry(u32 acpi_id)
> > > > >  	return apic_id;
> > > > >  }
> > > > >  
> > > > > -static int map_mat_entry(acpi_handle handle, u32 acpi_id)
> > > > > +static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
> > > > >  {
> > > > >  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > > > >  	union acpi_object *obj;
> > > > > @@ -501,7 +507,7 @@ static int map_mat_entry(acpi_handle han
> > > > >  	if (header->type == ACPI_MADT_TYPE_LOCAL_APIC) {
> > > > >  		map_lapic_id(header, acpi_id, &apic_id);
> > > > >  	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
> > > > > -		map_lsapic_id(header, acpi_id, &apic_id);
> > > > > +		map_lsapic_id(header, type, acpi_id, &apic_id);
> > > > >  	}
> > > > >  
> > > > >  exit:
> > > > > @@ -510,14 +516,14 @@ exit:
> > > > >  	return apic_id;
> > > > >  }
> > > > >  
> > > > > -static int get_cpu_id(acpi_handle handle, u32 acpi_id)
> > > > > +static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id)
> > > > >  {
> > > > >  	int i;
> > > > >  	int apic_id = -1;
> > > > >  
> > > > > -	apic_id = map_mat_entry(handle, acpi_id);
> > > > > +	apic_id = map_mat_entry(handle, type, acpi_id);
> > > > >  	if (apic_id == -1)
> > > > > -		apic_id = map_madt_entry(acpi_id);
> > > > > +		apic_id = map_madt_entry(type, acpi_id);
> > > > >  	if (apic_id == -1)
> > > > >  		return apic_id;
> > > > >  
> > > > > @@ -533,15 +539,16 @@ static int get_cpu_id(acpi_handle handle
> > > > >                                   Driver Interface
> > > > >     -------------------------------------------------------------------------- */
> > > > >  
> > > > > -static int acpi_processor_get_info(struct acpi_processor *pr, unsigned has_uid)
> > > > > +static int acpi_processor_get_info(struct acpi_device *device)
> > > > >  {
> > > > >  	acpi_status status = 0;
> > > > >  	union acpi_object object = { 0 };
> > > > >  	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
> > > > > -	int cpu_index;
> > > > > +	struct acpi_processor *pr;
> > > > > +	int cpu_index, device_declaration = 0;
> > > > >  	static int cpu0_initialized;
> > > > >  
> > > > > -
> > > > > +	pr = acpi_driver_data(device);
> > > > >  	if (!pr)
> > > > >  		return -EINVAL;
> > > > >  
> > > > > @@ -562,8 +569,8 @@ static int acpi_processor_get_info(struc
> > > > >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> > > > >  				  "No bus mastering arbitration control\n"));
> > > > >  
> > > > > -	/* Check if it is a Device with HID and UID */
> > > > > -	if (has_uid) {
> > > > > +	/* Check if it is a Device statement declaration with HID and UID */
> > > > > +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
> > > > >  		unsigned long value;
> > > > >  		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> > > > >  						NULL, &value);
> > > > > @@ -571,6 +578,7 @@ static int acpi_processor_get_info(struc
> > > > >  			printk(KERN_ERR PREFIX "Evaluating processor _UID\n");
> > > > >  			return -ENODEV;
> > > > >  		}
> > > > > +		device_declaration = 1;
> > > > >  		pr->acpi_id = value;
> > > > >  	} else {
> > > > >  		/*
> > > > > @@ -590,7 +598,7 @@ static int acpi_processor_get_info(struc
> > > > >  		*/
> > > > >  		pr->acpi_id = object.processor.proc_id;
> > > > >  	}
> > > > > -	cpu_index = get_cpu_id(pr->handle, pr->acpi_id);
> > > > > +	cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id);
> > > > >  
> > > > >  	/* Handle UP system running SMP kernel, with no LAPIC in MADT */
> > > > >  	if (!cpu0_initialized && (cpu_index == -1) &&
> > > > > @@ -662,7 +670,7 @@ static int __cpuinit acpi_processor_star
> > > > >  
> > > > >  	pr = acpi_driver_data(device);
> > > > >  
> > > > > -	result = acpi_processor_get_info(pr, device->flags.unique_id);
> > > > > +	result = acpi_processor_get_info(device);
> > > > >  	if (result) {
> > > > >  		/* Processor is physically not present */
> > > > >  		return 0;
> > > > > 
> > > > > 
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > 
> > > > 
> > > -- 
> > > Myron Stowe                             HP Open Source & Linux Org
> > > 
> > > 
> > 
> -- 
> Myron Stowe                             HP Open Source & Linux Org
> 
> 

Re: [PATCH 1/3] ACPI: Disambiguate processor declaration type

[Bjorn Helgaas]

On Monday 27 October 2008 01:42:37 am Zhao Yakui wrote:
>       Of course please add some comments that the ACPI ID is always
> obtained from the processor block if the processor object is declared by
> processor.

The code's pretty straightforward already, but maybe comments like
this would address your concern:

    if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_HID)) {
+       /*
+        * Declared with "Device" statement; match _UID.
+        * Note that we don't handle string _UID yet.
+        */
        acpi_evaluate_integer(pr->handle, METHOD_NAME__UID, NULL, &value);
        pr->acpi_id = value;
    } else {
+       /* Declared with "Processor" statement; match ProcessorID */
        acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
        pr->acpi_id = object.processor.proc_id;
    }

>      If the [_UID] is a string, it should be matched with the
> ACPI processor UID string field of SAPIC table to get the processor ID.
> Now this case is not handled by your patch. Of course maybe there
> doesn't exist such a system. So we can ask the user to send the ACPIdump
> and then add the corresponding support when a string is returned by the
> _UID object. It will be great if we can add the support about this.

That would be nice, but since we don't have a machine to test it with,
Myron would be adding untested code to the kernel, and I don't think
there's much value in that.

Bjorn