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]>
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 @@
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,
};
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 @@
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>
+#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 @@
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 */
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.)
On Friday 29 October 2004 05:02 pm, Suresh Siddha wrote:
> 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)
>
> 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-)
> --
>
> 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 @@
> 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,
> };
> 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 @@
> 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>
> +#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 @@
> 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 */
--
James Cleverdon
IBM LTC (xSeries Linux Solutions)
{jamesclv(Unix, preferred), cleverdj(Notes)} at us dot ibm dot comm
On Fri, Oct 29, 2004 at 05:35:32PM -0700, James Cleverdon wrote:
> 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
I suppose not. It seems a bit unnecessary, but OK if it makes
them happy. Maybe Intel has something in mind for 2008.
On Saturday 30 October 2004 05:56 am, Andi Kleen wrote:
> On Fri, Oct 29, 2004 at 05:35:32PM -0700, James Cleverdon wrote:
> > 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
IBM LTC (xSeries Linux Solutions)
{jamesclv(Unix, preferred), cleverdj(Notes)} at us dot ibm dot comm
On Fri, Oct 29, 2004 at 05:02:15PM -0700, Siddha, Suresh B wrote:
> 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