x86-64: fix sibling map again!

PATCH In review Suresh Siddha 3 participants 5 messages Oct 30, 2004 – Oct 31, 2004

Share & tools

Suresh Siddha

author

2004-10-30 00:12 UTC #0 lore raw

PATCH 1/1 x86-64: fix sibling map again!

Recent x86-64 sibling map fix for clustered mode by James
(http://linux.bkbits.net:8080/linux-2.6/cset@414b34a6jkiHQ5AnhA269av76y3ZAw?nav=index.html)
is not the recommended way of fixing it.

That patch assumes BIOS for non-clustered systems accept the HW assigned value. Why make this assumption when we can fix it in a better fashion(which is also used by x86 kernel's today)

Basically use HW assigned apic_id's(returned by cpuid) for non clustered systems and for clustered use BIOS provided apic_id's. Appended patch does this.

Note: Similar issue was earlier disussed in context of x86 approx an year back and James then backed out his changes.
http://www.ussg.iu.edu/hypermail/linux/kernel/0312.2/0167.html

Signed-off-by Suresh Siddha <[email protected]>

arch/x86_64/kernel/genapic_cluster.c +11 -0

diff -Nru linux-2.6.10-rc1/arch/x86_64/kernel/genapic_cluster.c linux-ht/arch/x86_64/kernel/genapic_cluster.c
@@ -111,6 +111,16 @@
 		return BAD_APICID;
 }
 
+/* cpuid returns the value latched in the HW at reset, not the APIC ID
+ * register's value.  For any box whose BIOS changes APIC IDs, like
+ * clustered APIC systems, we must use hard_smp_processor_id.
+ *
+ * See Intel's IA-32 SW Dev's Manual Vol2 under CPUID.
+ */
+static unsigned int phys_pkg_id(int index_msb)
+{
+	return hard_smp_processor_id() >> index_msb;
+}
 
 struct genapic apic_cluster = {
 	.name = "clustered",

@@ -124,4 +134,5 @@
 	.send_IPI_allbutself = cluster_send_IPI_allbutself,
 	.send_IPI_mask = cluster_send_IPI_mask,
 	.cpu_mask_to_apicid = cluster_cpu_mask_to_apicid,
+	.phys_pkg_id = phys_pkg_id,
 };

arch/x86_64/kernel/genapic_flat.c +8 -0

diff -Nru linux-2.6.10-rc1/arch/x86_64/kernel/genapic_flat.c linux-ht/arch/x86_64/kernel/genapic_flat.c
@@ -103,6 +103,13 @@
 	return cpus_addr(cpumask)[0] & APIC_ALL_CPUS;
 }
 
+static unsigned int phys_pkg_id(int index_msb)
+{
+	u32 ebx;
+
+	ebx = cpuid_ebx(1);
+	return ((ebx >> 24) & 0xFF) >> index_msb;
+}
 
 struct genapic apic_flat =  {
 	.name = "flat",

@@ -116,4 +123,5 @@
 	.send_IPI_allbutself = flat_send_IPI_allbutself,
 	.send_IPI_mask = flat_send_IPI_mask,
 	.cpu_mask_to_apicid = flat_cpu_mask_to_apicid,
+	.phys_pkg_id = phys_pkg_id,
 };

arch/x86_64/kernel/setup.c +2 -3

diff -Nru linux-2.6.10-rc1/arch/x86_64/kernel/setup.c linux-ht/arch/x86_64/kernel/setup.c
@@ -56,6 +56,7 @@
 #include <asm/smp.h>
 #include <asm/proto.h>
 #include <asm/setup.h>
+#include <asm/mach_apic.h>
 
 /*
  * Machine setup..

@@ -710,7 +711,6 @@
 #ifdef CONFIG_SMP
 	u32 	eax, ebx, ecx, edx;
 	int 	index_lsb, index_msb, tmp;
-	int	initial_apic_id;
 	int 	cpu = smp_processor_id();
 	
 	if (!cpu_has(c, X86_FEATURE_HT))

@@ -745,8 +745,7 @@
 		}
 		if (index_lsb != index_msb )
 			index_msb++;
-		initial_apic_id = hard_smp_processor_id();
-		phys_proc_id[cpu] = initial_apic_id >> index_msb;
+		phys_proc_id[cpu] = phys_pkg_id(index_msb);
 		
 		printk(KERN_INFO  "CPU: Physical Processor ID: %d\n",
 		       phys_proc_id[cpu]);

include/asm-x86_64/genapic.h +1 -0

diff -Nru linux-2.6.10-rc1/include/asm-x86_64/genapic.h linux-ht/include/asm-x86_64/genapic.h
@@ -26,6 +26,7 @@
 	void (*send_IPI_all)(int vector);
 	/* */
 	unsigned int (*cpu_mask_to_apicid)(cpumask_t cpumask);
+	unsigned int (*phys_pkg_id)(int index_msb);
 };

include/asm-x86_64/mach_apic.h +1 -0

diff -Nru linux-2.6.10-rc1/include/asm-x86_64/mach_apic.h linux-ht/include/asm-x86_64/mach_apic.h
@@ -24,5 +24,6 @@
 #define send_IPI_allbutself (genapic->send_IPI_allbutself)
 #define send_IPI_all (genapic->send_IPI_all)
 #define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)
+#define phys_pkg_id	(genapic->phys_pkg_id)
 
 #endif /* __ASM_MACH_APIC_H */

James Cleverdon

2004-10-30 00:42 UTC #1 lore raw

Hey Suresh,

Can you tell me why Intel considers cpuid to be The One True Way(TM) to get sibling info? Especially on x86-64, which doesn't have the same level of APIC weirdness that i386 does.

(I won't even mention the fact that someone messed up on the MSR the BIOS can use to set bits 7:5 in the cpuid ID value. It should allow the BIOS to set bits 7:3.)

Quoted 8 lines · Suresh Siddha

Recent x86-64 sibling map fix for clustered mode by James
(http://linux.bkbits.net:8080/linux-2.6/cset@414b34a6jkiHQ5AnhA269av7
6y3ZAw?nav=index.html) is not the recommended way of fixing it.
That patch assumes BIOS for non-clustered systems accept the HW assigned value. Why make this assumption when we can fix it in a better fashion(which is also used by x86 kernel's today)

Quoted 7 lines

Basically use HW assigned apic_id's(returned by cpuid) for non clustered systems and for clustered use BIOS provided apic_id's. Appended patch does this.
Note: Similar issue was earlier disussed in context of x86 approx an year back and James then backed out his changes.
http://www.ussg.iu.edu/hypermail/linux/kernel/0312.2/0167.html

Not quite. I just stopped arguing and tried to reach _some_
sort of compromise. 8-)

Quoted 114 lines

--
Signed-off-by Suresh Siddha <[email protected]>
diff -Nru linux-2.6.10-rc1/arch/x86_64/kernel/genapic_cluster.c linux-ht/arch/x86_64/kernel/genapic_cluster.c ---
linux-2.6.10-rc1/arch/x86_64/kernel/genapic_cluster.c 2004-10-08
20:32:25.000000000 -0700 +++
linux-ht/arch/x86_64/kernel/genapic_cluster.c 2004-10-09
18:55:32.091017328 -0700 @@ -111,6 +111,16 @@
arch/x86_64/kernel/genapic_cluster.c +1 -0
 		return BAD_APICID;
 }
 
+/* cpuid returns the value latched in the HW at reset, not the APIC
ID + * register's value. For any box whose BIOS changes APIC IDs, like + * clustered APIC systems, we must use hard_smp_processor_id. + *
+ * See Intel's IA-32 SW Dev's Manual Vol2 under CPUID.
+ */
arch/x86_64/kernel/genapic_cluster.c +5 -0
+static unsigned int phys_pkg_id(int index_msb)
+{
+	return hard_smp_processor_id() >> index_msb;
+}
 
 struct genapic apic_cluster = {
 	.name = "clustered",
@@ -124,4 +134,5 @@
 	.send_IPI_allbutself = cluster_send_IPI_allbutself,
 	.send_IPI_mask = cluster_send_IPI_mask,
 	.cpu_mask_to_apicid = cluster_cpu_mask_to_apicid,
+	.phys_pkg_id = phys_pkg_id,
 };
diff -Nru linux-2.6.10-rc1/arch/x86_64/kernel/genapic_flat.c linux-ht/arch/x86_64/kernel/genapic_flat.c ---
linux-2.6.10-rc1/arch/x86_64/kernel/genapic_flat.c 2004-10-08
20:32:25.000000000 -0700 +++
linux-ht/arch/x86_64/kernel/genapic_flat.c 2004-10-09
18:55:25.908957144 -0700 @@ -103,6 +103,13 @@
arch/x86_64/kernel/genapic_flat.c +8 -0
 	return cpus_addr(cpumask)[0] & APIC_ALL_CPUS;
 }
 
+static unsigned int phys_pkg_id(int index_msb)
+{
+	u32 ebx;
+
+	ebx = cpuid_ebx(1);
+	return ((ebx >> 24) & 0xFF) >> index_msb;
+}
 
 struct genapic apic_flat =  {
 	.name = "flat",
@@ -116,4 +123,5 @@
 	.send_IPI_allbutself = flat_send_IPI_allbutself,
 	.send_IPI_mask = flat_send_IPI_mask,
 	.cpu_mask_to_apicid = flat_cpu_mask_to_apicid,
+	.phys_pkg_id = phys_pkg_id,
 };
diff -Nru linux-2.6.10-rc1/arch/x86_64/kernel/setup.c linux-ht/arch/x86_64/kernel/setup.c ---
linux-2.6.10-rc1/arch/x86_64/kernel/setup.c 2004-10-22
14:38:16.000000000 -0700 +++
linux-ht/arch/x86_64/kernel/setup.c 2004-10-09 15:24:02.000000000
-0700 @@ -56,6 +56,7 @@
#include <asm/smp.h>
#include <asm/proto.h>
#include <asm/setup.h>
arch/x86_64/kernel/setup.c +2 -3
+#include <asm/mach_apic.h>
 
 /*
  * Machine setup..
@@ -710,7 +711,6 @@
 #ifdef CONFIG_SMP
 	u32 	eax, ebx, ecx, edx;
 	int 	index_lsb, index_msb, tmp;
-	int	initial_apic_id;
 	int 	cpu = smp_processor_id();
 
 	if (!cpu_has(c, X86_FEATURE_HT))
@@ -745,8 +745,7 @@
 		}
 		if (index_lsb != index_msb )
 			index_msb++;
-		initial_apic_id = hard_smp_processor_id();
-		phys_proc_id[cpu] = initial_apic_id >> index_msb;
+		phys_proc_id[cpu] = phys_pkg_id(index_msb);
 
 		printk(KERN_INFO  "CPU: Physical Processor ID: %d\n",
 		       phys_proc_id[cpu]);
diff -Nru linux-2.6.10-rc1/include/asm-x86_64/genapic.h linux-ht/include/asm-x86_64/genapic.h ---
linux-2.6.10-rc1/include/asm-x86_64/genapic.h 2004-10-08
20:32:26.000000000 -0700 +++
linux-ht/include/asm-x86_64/genapic.h 2004-10-09 15:23:45.000000000
-0700 @@ -26,6 +26,7 @@
include/asm-x86_64/genapic.h +1 -0
 	void (*send_IPI_all)(int vector);
 	/* */
 	unsigned int (*cpu_mask_to_apicid)(cpumask_t cpumask);
+	unsigned int (*phys_pkg_id)(int index_msb);
 };
diff -Nru linux-2.6.10-rc1/include/asm-x86_64/mach_apic.h linux-ht/include/asm-x86_64/mach_apic.h ---
linux-2.6.10-rc1/include/asm-x86_64/mach_apic.h 2004-10-08
20:32:26.000000000 -0700 +++
linux-ht/include/asm-x86_64/mach_apic.h 2004-10-08 20:42:09.000000000
-0700 @@ -24,5 +24,6 @@
#define send_IPI_allbutself (genapic->send_IPI_allbutself) #define send_IPI_all (genapic->send_IPI_all) #define cpu_mask_to_apicid (genapic->cpu_mask_to_apicid)
+#define phys_pkg_id (genapic->phys_pkg_id)
#endif /* __ASM_MACH_APIC_H */

Signature · 3 lines

James Cleverdon
IBM LTC (xSeries Linux Solutions)
{jamesclv(Unix, preferred), cleverdj(Notes)} at us dot ibm dot comm

Andi Kleen

2004-10-30 13:00 UTC #2 lore raw

Quoted 9 lines · James Cleverdon

Hey Suresh,
Can you tell me why Intel considers cpuid to be The One True Way(TM) to get sibling info? Especially on x86-64, which doesn't have the same level of APIC weirdness that i386 does.
(I won't even mention the fact that someone messed up on the MSR the BIOS can use to set bits 7:5 in the cpuid ID value. It should allow the BIOS to set bits 7:3.)

I have no great opinion on either ways, but I suppose it is best to do the same thing as i386 and apply Suresh's patch.

James, any strong objections?

-Andi

James Cleverdon

2004-10-30 23:24 UTC #3 lore raw

I suppose not. It seems a bit unnecessary, but OK if it makes them happy. Maybe Intel has something in mind for 2008.

Quoted 17 lines · Andi Kleen

Quoted 9 lines · James Cleverdon
Hey Suresh,
Can you tell me why Intel considers cpuid to be The One True Way(TM) to get sibling info? Especially on x86-64, which doesn't have the same level of APIC weirdness that i386 does.
(I won't even mention the fact that someone messed up on the MSR the BIOS can use to set bits 7:5 in the cpuid ID value. It should allow the BIOS to set bits 7:3.)
I have no great opinion on either ways, but I suppose it is best to do the same thing as i386 and apply Suresh's patch.
James, any strong objections?
-Andi

Signature · 3 lines

James Cleverdon
IBM LTC (xSeries Linux Solutions)
{jamesclv(Unix, preferred), cleverdj(Notes)} at us dot ibm dot comm

Andi Kleen

2004-10-31 01:33 UTC #4 lore raw

In reply to Suresh Siddha

Quoted 3 lines · Suresh B

That patch assumes BIOS for non-clustered systems accept the HW assigned value. Why make this assumption when we can fix it in a better fashion(which is also used by x86 kernel's today)

Thanks Suresh. I applied it to my tree.

-Andi