ACPI: Fix for supporting > 256 processor declaration limit

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).

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

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.

Quoted 24 lines · 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.

Quoted 5 lines

[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.

Quoted 5 lines

[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.

Quoted 7 lines

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?

Quoted 6 lines

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.

Quoted 18 lines · 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.

Quoted 5 lines

[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.

Quoted 24 lines

[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

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

CC Alexey Starikovskiy <[email protected]>

---
 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
===================================================================

drivers/acpi/processor_core.c +1 -0

@@ -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
===================================================================

include/acpi/acpi_drivers.h +1 -0

@@ -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
===================================================================

drivers/acpi/scan.c +1 -2

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 24 lines · Zhao Yakui

Quoted 18 lines · 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.

Quoted 5 lines

[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.

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 25 lines

Quoted 24 lines

[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

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

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.

Quoted 57 lines

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

CC Alexey Starikovskiy <[email protected]>

---
 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
===================================================================

drivers/acpi/processor_core.c +1 -0

@@ -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
===================================================================

include/acpi/acpi_drivers.h +1 -0

@@ -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
===================================================================

drivers/acpi/scan.c +1 -2

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 33 lines · Myron Stowe

Quoted 26 lines · Zhao Yakui

Quoted 18 lines · 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.

Quoted 6 lines

[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.

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 45 lines

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.

Quoted 6 lines

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

Quoted 24 lines

Quoted 24 lines

[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

Quoted 5 lines · Zhao Yakui

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

Quoted 7 lines

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

Quoted 58 lines

Quoted 57 lines

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

CC Alexey Starikovskiy <[email protected]>

---
 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
===================================================================

drivers/acpi/processor_core.c +1 -0

@@ -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
===================================================================

include/acpi/acpi_drivers.h +1 -0

@@ -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
===================================================================

drivers/acpi/scan.c +1 -2

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

CC Alexey Starikovskiy <[email protected]>

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

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================

drivers/acpi/processor_core.c +12 -14

@@ -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)

drivers/acpi/processor_core.c +16 -15

-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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 14 lines · Myron Stowe

Quoted 5 lines · Zhao Yakui

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

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 78 lines

"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

Quoted 58 lines

Quoted 57 lines

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

CC Alexey Starikovskiy <[email protected]>

---
 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
===================================================================

drivers/acpi/processor_core.c +1 -0

@@ -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
===================================================================

include/acpi/acpi_drivers.h +1 -0

@@ -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
===================================================================

drivers/acpi/scan.c +1 -2

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 72 lines

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

CC Alexey Starikovskiy <[email protected]>

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

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================

drivers/acpi/processor_core.c +18 -14

@@ -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:

drivers/acpi/processor_core.c static int map_lapic_id(struct acpi_subt · line 456

Quoted 3 lines

drivers/acpi/processor_core.c +2 -0

+	*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

Quoted 115 lines

drivers/acpi/processor_core.c +16 -15

-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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 72 lines

Quoted 72 lines

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

CC Alexey Starikovskiy <[email protected]>

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

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================

drivers/acpi/processor_core.c +18 -14

@@ -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:

Quoted 8 lines

drivers/acpi/processor_core.c static int map_lapic_id(struct acpi_subt · line 456

Quoted 3 lines

drivers/acpi/processor_core.c +2 -0

+	*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.

Quoted 1 line

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).

Quoted 4 lines

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

Quoted 122 lines

Thanks,
John

Quoted 115 lines

drivers/acpi/processor_core.c +16 -15

-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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Myron Stowe                             HP Open Source & Linux Org

Quoted 86 lines · John Keller

Quoted 72 lines

Quoted 72 lines

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

CC Alexey Starikovskiy <[email protected]>

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

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================

drivers/acpi/processor_core.c +18 -14

@@ -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:

Quoted 8 lines

drivers/acpi/processor_core.c static int map_lapic_id(struct acpi_subt · line 456

Quoted 3 lines

drivers/acpi/processor_core.c +2 -0

+	*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.

Quoted 1 line

I don't know of a problem with this, just making sure I understand this.

Yeah - there were problems ;)

Quoted 7 lines

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

Quoted 158 lines

- 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).

Quoted 4 lines

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

Quoted 122 lines

Thanks,
John

Quoted 115 lines

drivers/acpi/processor_core.c +16 -15

-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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Myron Stowe                             HP Open Source & Linux Org

Quoted 14 lines · Myron Stowe

Quoted 5 lines · Zhao Yakui

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

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 77 lines

"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

Quoted 58 lines

Quoted 57 lines

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

CC Alexey Starikovskiy <[email protected]>

---
 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
===================================================================

drivers/acpi/processor_core.c +1 -0

@@ -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
===================================================================

include/acpi/acpi_drivers.h +1 -0

@@ -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
===================================================================

drivers/acpi/scan.c +1 -2

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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!

Quoted 81 lines

Of course we should add such enhancement.

Quoted 77 lines

"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

Quoted 58 lines

Quoted 57 lines

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

CC Alexey Starikovskiy <[email protected]>

---
 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
===================================================================

drivers/acpi/processor_core.c 2008-10-21 13:11:00.000000000 -0600 renamed from drivers/acpi/processor_core.c 2008-10-16 19:00:37.000000000 -0600 +1 -0

@@ -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
===================================================================

include/acpi/acpi_drivers.h 2008-10-20 13:23:28.000000000 -0600 renamed from include/acpi/acpi_drivers.h 2008-10-16 18:53:26.000000000 -0600 +1 -0

@@ -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
===================================================================

drivers/acpi/scan.c 2008-10-21 13:09:09.000000000 -0600 renamed from drivers/acpi/scan.c 2008-10-16 19:04:58.000000000 -0600 +1 -2

@@ -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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 94 lines

Quoted 86 lines · John Keller

Quoted 72 lines

Quoted 72 lines

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

CC Alexey Starikovskiy <[email protected]>

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

Index: linux-2.6/drivers/acpi/processor_core.c
===================================================================

drivers/acpi/processor_core.c +18 -14

@@ -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:

Quoted 8 lines

drivers/acpi/processor_core.c static int map_lapic_id(struct acpi_subt · line 456

Quoted 3 lines

drivers/acpi/processor_core.c +2 -0

+	*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.

Quoted 1 line

I don't know of a problem with this, just making sure I understand this.

Yeah - there were problems ;)

Quoted 7 lines

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).

Quoted 4 lines

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

Quoted 164 lines

Thanks for the reply,
John

Quoted 158 lines

- 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).

Quoted 4 lines

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

Quoted 122 lines

Thanks,
John

Quoted 115 lines

drivers/acpi/processor_core.c +16 -15

-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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Myron Stowe                             HP Open Source & Linux Org

Signature · 1 line

Myron Stowe                             HP Open Source & Linux Org

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.

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.