: x86: use acpi flags for apic mapping

ACPI FADT has entries to describe the apic mapping of a system. Currently, these are ignored in the apic code.

Introduce apic_is_acpi_clustered_box() and read the FADT to determine if a box is clustered or not.

Signed-off-by Prarit Bhargava <[email protected]>

arch/x86/include/asm/apic.h +8 -0

index 1fa03e0..4518683 100644
@@ -252,6 +252,14 @@ static inline int apic_is_clustered_box(void)
 }
 #endif
 
+enum apic_acpi_map_status {
+	APIC_ACPI_BOTH,
+	APIC_ACPI_CLUSTER,
+	APIC_ACPI_PHYSICAL,
+	APIC_ACPI_NONE
+};
+extern enum apic_acpi_map_status apic_is_acpi_clustered_box(void);
+
 extern u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask);
 extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
 

arch/x86/kernel/apic/apic.c +0 -0

index e5a4a1e..6ca346a 100644

arch/x86/kernel/apic/apic.c +35 -0

+	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) {
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL &&
+		    acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER) {
+			/*
+			 * The rest of the code assumes physical flat
+			 * in this case.
+			 */
+			return APIC_ACPI_BOTH;
+		}
+
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER)
+			return APIC_ACPI_CLUSTER;
+
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)
+			return APIC_ACPI_PHYSICAL;
+	}
+
+	return APIC_ACPI_NONE;
+}
+#endif
+
 static void __cpuinit dmi_check_multi(void)
 {
 	if (multi_checked)

@@ -2208,6 +2232,20 @@ static void __cpuinit dmi_check_multi(void)
  */
 __cpuinit int apic_is_clustered_box(void)
 {
+#ifdef CONFIG_ACPI
+	switch (apic_is_acpi_clustered_box()) {
+		case APIC_ACPI_PHYSICAL:
+		case APIC_ACPI_BOTH: /* assume physical flat in this case */
+			return 0;
+			break;
+		case APIC_ACPI_CLUSTER:
+			return 1;
+			break;
+		default:
+			break;
+	}
+#endif
+
 	dmi_check_multi();
 	if (multi)
 		return 1;

arch/x86/kernel/apic/apic_flat_64.c +0 -0

index 09d3b17..80e291a 100644

arch/x86/kernel/apic/apic_flat_64.c +16 -4

-	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
-		printk(KERN_DEBUG "system APIC only can use physical flat");
-		return 1;
+	switch (apic_is_acpi_clustered_box()) {
+		case APIC_ACPI_BOTH:
+			printk(KERN_WARNING FW_BUG "ACPI has set apic mode to "
+			       "both clustered and physical flat.  Please "
+			       "contact your firmware vendor for an update.\n");
+			/*
+			 * In this case assume physical flat as only a very
+			 * limited number of systems use cluster
+			 */
+			printk(KERN_DEBUG "system APIC using physical flat\n");
+			return 1;
+			break;
+		case APIC_ACPI_CLUSTER:
+			printk(KERN_DEBUG "system APIC can only use cluster\n");
+			return 0;
+			break;

Review

Quoted 5 lines

(quoted diff) +3 -0

+#ifdef CONFIG_ACPI
+enum apic_acpi_map_status apic_is_acpi_clustered_box(void)
+{

It's a bit strange that function is "is" prefixed and returns not true or false but enum, perhaps we may name it apic_acpi_dst_model() or something like that?

Quoted 10 lines

arch/x86/kernel/apic/apic.c static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = { · line 2194

Quoted 5 lines

arch/x86/kernel/apic/apic.c +3 -0

+#ifdef CONFIG_ACPI
+enum apic_acpi_map_status apic_is_acpi_clustered_box(void)
+{

It's a bit strange that function is "is" prefixed and returns not true or false but enum, perhaps we may name it apic_acpi_dst_model() or something like that?

Sure, np -- new patch.

P.

arch/x86/include/asm/apic.h +8 -0

index 1fa03e0..6b63f95 100644
@@ -252,6 +252,14 @@ static inline int apic_is_clustered_box(void)
 }
 #endif
 
+enum apic_acpi_map_status {
+	APIC_ACPI_BOTH,
+	APIC_ACPI_CLUSTER,
+	APIC_ACPI_PHYSICAL,
+	APIC_ACPI_NONE
+};
+extern enum apic_acpi_map_status apic_acpi_dst_model(void);
+
 extern u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask);
 extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
 

arch/x86/kernel/apic/apic.c +2 -0

index e5a4a1e..e94a189 100644
@@ -2189,6 +2189,30 @@ static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = {
 	{}
 };
 
+#ifdef CONFIG_ACPI
+enum apic_acpi_map_status apic_acpi_dst_model(void)

arch/x86/kernel/apic/apic.c +26 -0

+
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER)
+			return APIC_ACPI_CLUSTER;
+
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)
+			return APIC_ACPI_PHYSICAL;
+	}
+
+	return APIC_ACPI_NONE;
+}
+#endif
+
 static void __cpuinit dmi_check_multi(void)
 {
 	if (multi_checked)

@@ -2208,6 +2232,20 @@ static void __cpuinit dmi_check_multi(void)
  */
 __cpuinit int apic_is_clustered_box(void)
 {
+#ifdef CONFIG_ACPI
+	switch (apic_acpi_dst_model()) {
+		case APIC_ACPI_PHYSICAL:
+		case APIC_ACPI_BOTH: /* assume physical flat in this case */
+			return 0;
+			break;
+		case APIC_ACPI_CLUSTER:
+			return 1;
+			break;
+		default:
+			break;
+	}
+#endif
+
 	dmi_check_multi();
 	if (multi)
 		return 1;

arch/x86/kernel/apic/apic_flat_64.c +16 -7

index 09d3b17..c2318ac 100644
@@ -231,14 +231,32 @@ static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 #ifdef CONFIG_ACPI
 	/*
-	 * Quirk: some x86_64 machines can only use physical APIC mode
-	 * regardless of how many processors are present (x86_64 ES7000
-	 * is an example).
+	 * Some x86_64 machines can only use clustered or physical APIC
+	 * mode regardless of how many processors are present.
 	 */
-	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
-		printk(KERN_DEBUG "system APIC only can use physical flat");
-		return 1;
+	switch (apic_acpi_dst_model()) {
+		case APIC_ACPI_BOTH:
+			printk(KERN_WARNING FW_BUG "ACPI has set apic mode to "
+			       "both clustered and physical flat.  Please "
+			       "contact your firmware vendor for an update.\n");
+			/*
+			 * In this case assume physical flat as only a very
+			 * limited number of systems use cluster
+			 */
+			printk(KERN_DEBUG "system APIC using physical flat\n");
+			return 1;
+			break;
+		case APIC_ACPI_CLUSTER:
+			printk(KERN_DEBUG "system APIC can only use cluster\n");

Quoted 10 lines · Prarit Bhargava

Quoted 10 lines

arch/x86/kernel/apic/apic.c static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = { · line 2194

Quoted 5 lines

arch/x86/kernel/apic/apic.c +3 -0

+#ifdef CONFIG_ACPI
+enum apic_acpi_map_status apic_is_acpi_clustered_box(void)
+{

It's a bit strange that function is "is" prefixed and returns not true or false but enum, perhaps we may name it apic_acpi_dst_model() or something like that?

Quoted 4 lines

Sure, np -- new patch.

P.

Hi Prarit,

just have reviewed it again and got some questions:

Quoted 27 lines

arch/x86/include/asm/apic.h +8 -0

index 1fa03e0..6b63f95 100644
@@ -252,6 +252,14 @@ static inline int apic_is_clustered_box(void)
 }
 #endif
 
+enum apic_acpi_map_status {
+	APIC_ACPI_BOTH,
+	APIC_ACPI_CLUSTER,
+	APIC_ACPI_PHYSICAL,
+	APIC_ACPI_NONE
+};
+extern enum apic_acpi_map_status apic_acpi_dst_model(void);
+
 extern u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask);
 extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
 

arch/x86/kernel/apic/apic.c +0 -0

index e5a4a1e..e94a189 100644
@@ -2189,6 +2189,30 @@ static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = {
 	{}
 };
 

arch/x86/kernel/apic/apic.c static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = {

Quoted 13 lines

arch/x86/kernel/apic/apic.c +12 -0

+#ifdef CONFIG_ACPI
+enum apic_acpi_map_status apic_acpi_dst_model(void)
+{
+	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) {
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL &&
+		    acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER) {
+			/*
+			 * The rest of the code assumes physical flat
+			 * in this case.
+			 */
+			return APIC_ACPI_BOTH;
+		}
 

Havin both flags set in ACPI FADT make me worry -- I suspect this means acpi is screwed (this is ok, who doubt :) but the problem is HOW should we treat TSC instability in such case? The current code assumes (tsc.c)

that cluster mode has TSC unstable and if we had both bits set which tsc mode we should choose? I suspect we have to assume that TSC unstable then.

Quoted 73 lines

arch/x86/kernel/apic/apic.c +12 -0

+
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER)
+			return APIC_ACPI_CLUSTER;
+
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)
+			return APIC_ACPI_PHYSICAL;
+	}
+
+	return APIC_ACPI_NONE;
+}
+#endif
+
 static void __cpuinit dmi_check_multi(void)
 {

arch/x86/kernel/apic/apic.c +14 -0

 	if (multi_checked)

@@ -2208,6 +2232,20 @@ static void __cpuinit dmi_check_multi(void)
  */
 __cpuinit int apic_is_clustered_box(void)
 {
+#ifdef CONFIG_ACPI
+	switch (apic_acpi_dst_model()) {
+		case APIC_ACPI_PHYSICAL:
+		case APIC_ACPI_BOTH: /* assume physical flat in this case */
+			return 0;
+			break;
+		case APIC_ACPI_CLUSTER:
+			return 1;
+			break;
+		default:
+			break;
+	}
+#endif
+
 	dmi_check_multi();
 	if (multi)
 		return 1;

arch/x86/kernel/apic/apic_flat_64.c +21 -7

index 09d3b17..c2318ac 100644
@@ -231,14 +231,32 @@ static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 #ifdef CONFIG_ACPI
 	/*
-	 * Quirk: some x86_64 machines can only use physical APIC mode
-	 * regardless of how many processors are present (x86_64 ES7000
-	 * is an example).
+	 * Some x86_64 machines can only use clustered or physical APIC
+	 * mode regardless of how many processors are present.
 	 */
-	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
-		(acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL)) {
-		printk(KERN_DEBUG "system APIC only can use physical flat");
-		return 1;
+	switch (apic_acpi_dst_model()) {
+		case APIC_ACPI_BOTH:
+			printk(KERN_WARNING FW_BUG "ACPI has set apic mode to "
+			       "both clustered and physical flat.  Please "
+			       "contact your firmware vendor for an update.\n");
+			/*
+			 * In this case assume physical flat as only a very
+			 * limited number of systems use cluster
+			 */
+			printk(KERN_DEBUG "system APIC using physical flat\n");
+			return 1;
+			break;
+		case APIC_ACPI_CLUSTER:
+			printk(KERN_DEBUG "system APIC can only use cluster\n");
+			return 0;
+			break;
+		case APIC_ACPI_PHYSICAL:
+			printk(KERN_DEBUG "system APIC can only use physical"
+			       " flat\n");

arch/x86/kernel/apic/apic_flat_64.c static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id) · line 261

Quoted 6 lines

arch/x86/kernel/apic/apic_flat_64.c +4 -0

+			return 1;
+			break;
+		default:
+			break;
 	}
 

Not sure, but it seems this may broke IBM and EXA machines which should use physical destination mode, hmm?

arch/x86/kernel/apic/apic_flat_64.c static int physflat_acpi_madt_oem_check(char *oem_id, char *oem_table_id) · line 262

Quoted 4 lines

arch/x86/kernel/apic/apic_flat_64.c +0 -0

 	if (!strncmp(oem_id, "IBM", 3) && !strncmp(oem_table_id, "EXA", 3)) {

Has this patch been tested on real hardware? Asking so since I don't have neither IBM nor EXA machine.

I'm CC'ing experts I know were involved.

-- Cyrill

Quoted 48 lines · Cyrill Gorcunov

Quoted 10 lines · Prarit Bhargava

Quoted 10 lines

arch/x86/kernel/apic/apic.c static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = { · line 2194

Quoted 5 lines

arch/x86/kernel/apic/apic.c +3 -0

+#ifdef CONFIG_ACPI
+enum apic_acpi_map_status apic_is_acpi_clustered_box(void)
+{

It's a bit strange that function is "is" prefixed and returns not true or false but enum, perhaps we may name it apic_acpi_dst_model() or something like that?

Quoted 4 lines

Sure, np -- new patch.

P.

Hi Prarit,

just have reviewed it again and got some questions:

Quoted 27 lines

arch/x86/include/asm/apic.h +8 -0

index 1fa03e0..6b63f95 100644
@@ -252,6 +252,14 @@ static inline int apic_is_clustered_box(void)
 }
 #endif
 
+enum apic_acpi_map_status {
+	APIC_ACPI_BOTH,
+	APIC_ACPI_CLUSTER,
+	APIC_ACPI_PHYSICAL,
+	APIC_ACPI_NONE
+};
+extern enum apic_acpi_map_status apic_acpi_dst_model(void);
+
 extern u8 setup_APIC_eilvt_mce(u8 vector, u8 msg_type, u8 mask);
 extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
 

arch/x86/kernel/apic/apic.c +0 -0

index e5a4a1e..e94a189 100644
@@ -2189,6 +2189,30 @@ static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = {
 	{}
 };
 

Quoted 17 lines

arch/x86/kernel/apic/apic.c static const __cpuinitconst struct dmi_system_id multi_dmi_table[] = {

Quoted 13 lines

arch/x86/kernel/apic/apic.c +12 -0

+#ifdef CONFIG_ACPI
+enum apic_acpi_map_status apic_acpi_dst_model(void)
+{
+	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) {
+		if (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL &&
+		    acpi_gbl_FADT.flags & ACPI_FADT_APIC_CLUSTER) {
+			/*
+			 * The rest of the code assumes physical flat
+			 * in this case.
+			 */
+			return APIC_ACPI_BOTH;
+		}
 

Havin both flags set in ACPI FADT make me worry -- I suspect this means acpi is screwed (this is ok, who doubt :) but the problem is HOW should we treat TSC instability in such case? The current code assumes (tsc.c)

In the case of BOTH the code will assume physical_flat everywhere --
therefore tsc is is stable. Since the number of cluster systems is low
it is unlikely that BOTH & cluster actually occur. I suppose I could
add (yet another) boot parameter to force cluster/flat/phys_flat if one doesn't already exist.... but I think that the likelihood of anyone hitting BOTH & wanting cluster is 0.

Quoted 3 lines

that cluster mode has TSC unstable and if we had both bits set which tsc mode we should choose? I suspect we have to assume that TSC unstable then.

It seems we don't need this patch.

so all system should support phys apic mode, but system with less 8 cpus is supposed to support cluster mode for better performance.

current kernel will try to use cluster mode if nr_cpus is less than 8.

some system have problem like IBM and NCR only support phys apic mode even cpus < 8 ...

Thanks

Yinghai

Quoted 5 lines

It seems we don't need this patch.

so all system should support phys apic mode, but system with less 8 cpus is supposed to support cluster mode for better performance.

Quoted 5 lines

current kernel will try to use cluster mode if nr_cpus is less than 8.

some system have problem like IBM and NCR only support phys apic mode even cpus < 8 ...

What about systems that have set the ACPI values? We just ignore those then?