[PATCH] 2.4.23-rc1: cpu_sibling_map fix

[PATCH] 2.4.23-rc1: cpu_sibling_map fix

[James Cleverdon]

[-- Attachment #1: Type: text/plain, Size: 2860 bytes --]

On summit-based machines the cpu_sibling_map data has been hosed for some 
time.  I found out why in Intel's IA-32 Software Deveveopers' Manual Vol 2 
under CPUID.  Looks like the value that cpuid returns is the one latched at 
reset, and doesn't reflect any changes made by the BIOS later:

	* Local APIC ID (high byte of EBX)--this number is the 8-bit ID that is
	  assigned to the local APIC on the processor during power up. This field
	  was introduced in the Pentium 4 processor.

Also, the code in init_intel was a bit overdesigned.  Until Intel releases a 
chip with a non-power-of-2 sibling count on it, there's no point in all that 
bit bashing.


diff -pru 2.4.23-rc1/arch/i386/kernel/setup.c rc1/arch/i386/kernel/setup.c
--- 2.4.23-rc1/arch/i386/kernel/setup.c	2003-11-11 18:09:57.000000000 -0800
+++ rc1/arch/i386/kernel/setup.c	2003-11-12 15:18:44.000000000 -0800
@@ -2461,8 +2461,6 @@ static void __init init_intel(struct cpu
 		extern	int phys_proc_id[NR_CPUS];
 		
 		u32 	eax, ebx, ecx, edx;
-		int 	index_lsb, index_msb, tmp;
-		int	initial_apic_id;
 		int 	cpu = smp_processor_id();
 
 		cpuid(1, &eax, &ebx, &ecx, &edx);
@@ -2471,11 +2469,10 @@ static void __init init_intel(struct cpu
 		if (smp_num_siblings == 1) {
 			printk(KERN_INFO  "CPU: Hyper-Threading is disabled\n");
 		} else if (smp_num_siblings > 1 ) {
-			index_lsb = 0;
-			index_msb = 31;
 			/*
 			 * At this point we only support two siblings per
-			 * processor package.
+			 * processor package.  Assume will be power of 2 until
+			 * we learn otherwise.
 			 */
 #define NR_SIBLINGS	2
 			if (smp_num_siblings != NR_SIBLINGS) {
@@ -2483,20 +2480,7 @@ static void __init init_intel(struct cpu
 				smp_num_siblings = 1;
 				return;
 			}
-			tmp = smp_num_siblings;
-			while ((tmp & 1) == 0) {
-				tmp >>=1 ;
-				index_lsb++;
-			}
-			tmp = smp_num_siblings;
-			while ((tmp & 0x80000000 ) == 0) {
-				tmp <<=1 ;
-				index_msb--;
-			}
-			if (index_lsb != index_msb )
-				index_msb++;
-			initial_apic_id = ebx >> 24 & 0xff;
-			phys_proc_id[cpu] = initial_apic_id >> index_msb;
+			phys_proc_id[cpu] = hard_smp_processor_id() & ~(smp_num_siblings - 1);
 
 			printk(KERN_INFO  "CPU: Physical Processor ID: %d\n",
                                phys_proc_id[cpu]);
diff -pru 2.4.23-rc1/include/asm-i386/apicdef.h rc1/include/asm-i386/apicdef.h
--- 2.4.23-rc1/include/asm-i386/apicdef.h	2003-08-25 04:44:43.000000000 -0700
+++ rc1/include/asm-i386/apicdef.h	2003-11-12 16:32:54.000000000 -0800
@@ -11,8 +11,8 @@
 #define		APIC_DEFAULT_PHYS_BASE	0xfee00000
  
 #define		APIC_ID		0x20
-#define			APIC_ID_MASK		(0x0F<<24)
-#define			GET_APIC_ID(x)		(((x)>>24)&0x0F)
+#define			APIC_ID_MASK		(0xFFu<<24)
+#define			GET_APIC_ID(x)		(((unsigned)(x)>>24)&0xFF)
 #define		APIC_LVR	0x30
 #define			APIC_LVR_MASK		0xFF00FF
 #define			GET_APIC_VERSION(x)	((x)&0xFF)

[-- Attachment #2: cpu_sibling_map_2003-11-12_2.4.23-rc1 --]
[-- Type: text/x-diff, Size: 2202 bytes --]

diff -pru 2.4.23-rc1/arch/i386/kernel/setup.c rc1/arch/i386/kernel/setup.c
--- 2.4.23-rc1/arch/i386/kernel/setup.c	2003-11-11 18:09:57.000000000 -0800
+++ rc1/arch/i386/kernel/setup.c	2003-11-12 15:18:44.000000000 -0800
@@ -2461,8 +2461,6 @@ static void __init init_intel(struct cpu
 		extern	int phys_proc_id[NR_CPUS];
 		
 		u32 	eax, ebx, ecx, edx;
-		int 	index_lsb, index_msb, tmp;
-		int	initial_apic_id;
 		int 	cpu = smp_processor_id();
 
 		cpuid(1, &eax, &ebx, &ecx, &edx);
@@ -2471,11 +2469,10 @@ static void __init init_intel(struct cpu
 		if (smp_num_siblings == 1) {
 			printk(KERN_INFO  "CPU: Hyper-Threading is disabled\n");
 		} else if (smp_num_siblings > 1 ) {
-			index_lsb = 0;
-			index_msb = 31;
 			/*
 			 * At this point we only support two siblings per
-			 * processor package.
+			 * processor package.  Assume will be power of 2 until
+			 * we learn otherwise.
 			 */
 #define NR_SIBLINGS	2
 			if (smp_num_siblings != NR_SIBLINGS) {
@@ -2483,20 +2480,7 @@ static void __init init_intel(struct cpu
 				smp_num_siblings = 1;
 				return;
 			}
-			tmp = smp_num_siblings;
-			while ((tmp & 1) == 0) {
-				tmp >>=1 ;
-				index_lsb++;
-			}
-			tmp = smp_num_siblings;
-			while ((tmp & 0x80000000 ) == 0) {
-				tmp <<=1 ;
-				index_msb--;
-			}
-			if (index_lsb != index_msb )
-				index_msb++;
-			initial_apic_id = ebx >> 24 & 0xff;
-			phys_proc_id[cpu] = initial_apic_id >> index_msb;
+			phys_proc_id[cpu] = hard_smp_processor_id() & ~(smp_num_siblings - 1);
 
 			printk(KERN_INFO  "CPU: Physical Processor ID: %d\n",
                                phys_proc_id[cpu]);
diff -pru 2.4.23-rc1/include/asm-i386/apicdef.h rc1/include/asm-i386/apicdef.h
--- 2.4.23-rc1/include/asm-i386/apicdef.h	2003-08-25 04:44:43.000000000 -0700
+++ rc1/include/asm-i386/apicdef.h	2003-11-12 16:32:54.000000000 -0800
@@ -11,8 +11,8 @@
 #define		APIC_DEFAULT_PHYS_BASE	0xfee00000
  
 #define		APIC_ID		0x20
-#define			APIC_ID_MASK		(0x0F<<24)
-#define			GET_APIC_ID(x)		(((x)>>24)&0x0F)
+#define			APIC_ID_MASK		(0xFFu<<24)
+#define			GET_APIC_ID(x)		(((unsigned)(x)>>24)&0xFF)
 #define		APIC_LVR	0x30
 #define			APIC_LVR_MASK		0xFF00FF
 #define			GET_APIC_VERSION(x)	((x)&0xFF)

Re: [PATCH] 2.4.23-rc1: cpu_sibling_map fix

[James Cleverdon]

On Thursday 13 November 2003 11:32 am, Nakajima, Jun wrote:
> Can you please explain why the information by CPUID does not work for
> summit-based machine? The CPUID does not reflect the H/W configuration,
> and the BIOS needs to communicate via local APIC Ids instead?
>
> 	Jun

Easy.  Note this bit from Intel's IA-32 manual:

* Local APIC ID (high byte of EBX)--this number is the 8-bit ID that is
  assigned to the local APIC on the processor during power up.  This
  field was introduced in the Pentium 4 processor.

On almost all systems, the value latched in the CPU when it comes out of reset 
doesn't change over the life of the system.  However, our Summit boxes (and I 
suspect most NUMA boxen) have a BIOS that builds the full system out of 
smaller building blocks, each of which starts as an independent computer.  
The act of doing so means rewriting the local APIC ID register with a 
different value.  (It turns out that the CPU only latches 6 bits of APIC ID, 
but we need 8.)

Thus, we need the patch that reads the APIC ID register instead of just using 
the value from cpuid.  And we probably won't be the only ones either.

> > -----Original Message-----
[ Snip! ]


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

RE: [PATCH] 2.4.23-rc1: cpu_sibling_map fix

[Nakajima, Jun]

Can you please explain why the information by CPUID does not work for
summit-based machine? The CPUID does not reflect the H/W configuration,
and the BIOS needs to communicate via local APIC Ids instead?

	Jun

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of James Cleverdon
> Sent: Wednesday, November 12, 2003 7:57 PM
> To: linux-kernel@vger.kernel.org; Marcelo Tosatti
> Subject: [PATCH] 2.4.23-rc1: cpu_sibling_map fix
> 
> On summit-based machines the cpu_sibling_map data has been hosed for
some
> time.  I found out why in Intel's IA-32 Software Deveveopers' Manual
Vol 2
> under CPUID.  Looks like the value that cpuid returns is the one
latched
> at
> reset, and doesn't reflect any changes made by the BIOS later:
> 
> 	* Local APIC ID (high byte of EBX)--this number is the 8-bit ID
that
> is
> 	  assigned to the local APIC on the processor during power up.
This
> field
> 	  was introduced in the Pentium 4 processor.
> 
> Also, the code in init_intel was a bit overdesigned.  Until Intel
releases
> a
> chip with a non-power-of-2 sibling count on it, there's no point in
all
> that
> bit bashing.
> 
> 
> diff -pru 2.4.23-rc1/arch/i386/kernel/setup.c
rc1/arch/i386/kernel/setup.c
> --- 2.4.23-rc1/arch/i386/kernel/setup.c	2003-11-11
18:09:57.000000000 -
> 0800
> +++ rc1/arch/i386/kernel/setup.c	2003-11-12 15:18:44.000000000
-0800
> @@ -2461,8 +2461,6 @@ static void __init init_intel(struct cpu
>  		extern	int phys_proc_id[NR_CPUS];
> 
>  		u32 	eax, ebx, ecx, edx;
> -		int 	index_lsb, index_msb, tmp;
> -		int	initial_apic_id;
>  		int 	cpu = smp_processor_id();
> 
>  		cpuid(1, &eax, &ebx, &ecx, &edx);
> @@ -2471,11 +2469,10 @@ static void __init init_intel(struct cpu
>  		if (smp_num_siblings == 1) {
>  			printk(KERN_INFO  "CPU: Hyper-Threading is
disabled\n");
>  		} else if (smp_num_siblings > 1 ) {
> -			index_lsb = 0;
> -			index_msb = 31;
>  			/*
>  			 * At this point we only support two siblings
per
> -			 * processor package.
> +			 * processor package.  Assume will be power of 2
until
> +			 * we learn otherwise.
>  			 */
>  #define NR_SIBLINGS	2
>  			if (smp_num_siblings != NR_SIBLINGS) {
> @@ -2483,20 +2480,7 @@ static void __init init_intel(struct cpu
>  				smp_num_siblings = 1;
>  				return;
>  			}
> -			tmp = smp_num_siblings;
> -			while ((tmp & 1) == 0) {
> -				tmp >>=1 ;
> -				index_lsb++;
> -			}
> -			tmp = smp_num_siblings;
> -			while ((tmp & 0x80000000 ) == 0) {
> -				tmp <<=1 ;
> -				index_msb--;
> -			}
> -			if (index_lsb != index_msb )
> -				index_msb++;
> -			initial_apic_id = ebx >> 24 & 0xff;
> -			phys_proc_id[cpu] = initial_apic_id >>
index_msb;
> +			phys_proc_id[cpu] = hard_smp_processor_id() &
> ~(smp_num_siblings - 1);
> 
>  			printk(KERN_INFO  "CPU: Physical Processor ID:
%d\n",
>                                 phys_proc_id[cpu]);
> diff -pru 2.4.23-rc1/include/asm-i386/apicdef.h rc1/include/asm-
> i386/apicdef.h
> --- 2.4.23-rc1/include/asm-i386/apicdef.h	2003-08-25
04:44:43.000000000 -
> 0700
> +++ rc1/include/asm-i386/apicdef.h	2003-11-12 16:32:54.000000000
-0800
> @@ -11,8 +11,8 @@
>  #define		APIC_DEFAULT_PHYS_BASE	0xfee00000
> 
>  #define		APIC_ID		0x20
> -#define			APIC_ID_MASK		(0x0F<<24)
> -#define			GET_APIC_ID(x)		(((x)>>24)&0x0F)
> +#define			APIC_ID_MASK		(0xFFu<<24)
> +#define			GET_APIC_ID(x)
(((unsigned)(x)>>24)&0xFF)
>  #define		APIC_LVR	0x30
>  #define			APIC_LVR_MASK		0xFF00FF
>  #define			GET_APIC_VERSION(x)	((x)&0xFF)