[PATCH] x86: always store APIC ID of CPU

[PATCH] x86: always store APIC ID of CPU

[Jan Beulich]

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

So far for non-Intel CPUs struct cpuinfo_x86's apicid field didn't get
set, despite MCE code consuming it.

Do some formatting/ordering adjustment to the touched code at once.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -152,6 +152,17 @@ static void __cpuinit get_cpu_vendor(str
 	this_cpu = &default_cpu;
 }
 
+/*
+ * 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
+{
+	return hard_smp_processor_id() >> index_msb;
+}
 
 /* Do minimum CPU detection early.
    Fields really needed: vendor, cpuid_level, family, model, mask, cache alignment.
@@ -216,6 +227,7 @@ static void __cpuinit generic_identify(s
 	if (c->x86 >= 0x6)
 		c->x86_model += ((tfms >> 16) & 0xF) << 4;
 	c->x86_mask = tfms & 15;
+	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
 	if ( cpu_has(c, X86_FEATURE_CLFLSH) )
 		c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8;
 
@@ -339,17 +351,6 @@ void __cpuinit identify_cpu(struct cpuin
 	}
 }
 
-/* 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
-{
-	return hard_smp_processor_id() >> index_msb;
-}
-
 /* leaf 0xb SMT level */
 #define SMT_LEVEL       0
 
@@ -429,14 +430,12 @@ void __cpuinit detect_ht(struct cpuinfo_
 	u32 	eax, ebx, ecx, edx;
 	int 	index_msb, core_bits;
 
-	cpuid(1, &eax, &ebx, &ecx, &edx);
-
-	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
-
-	if (!cpu_has(c, X86_FEATURE_HT) || cpu_has(c, X86_FEATURE_CMP_LEGACY)
-        || cpu_has(c, X86_FEATURE_XTOPOLOGY))
+	if (!cpu_has(c, X86_FEATURE_HT) ||
+	    cpu_has(c, X86_FEATURE_CMP_LEGACY) ||
+	    cpu_has(c, X86_FEATURE_XTOPOLOGY))
 		return;
 
+	cpuid(1, &eax, &ebx, &ecx, &edx);
 	c->x86_num_siblings = (ebx & 0xff0000) >> 16;
 
 	if (c->x86_num_siblings == 1) {




[-- Attachment #2: x86-AMD-set-APICID.patch --]
[-- Type: text/plain, Size: 2396 bytes --]

x86: always store APIC ID of CPU

So far for non-Intel CPUs struct cpuinfo_x86's apicid field didn't get
set, despite MCE code consuming it.

Do some formatting/ordering adjustment to the touched code at once.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -152,6 +152,17 @@ static void __cpuinit get_cpu_vendor(str
 	this_cpu = &default_cpu;
 }
 
+/*
+ * 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
+{
+	return hard_smp_processor_id() >> index_msb;
+}
 
 /* Do minimum CPU detection early.
    Fields really needed: vendor, cpuid_level, family, model, mask, cache alignment.
@@ -216,6 +227,7 @@ static void __cpuinit generic_identify(s
 	if (c->x86 >= 0x6)
 		c->x86_model += ((tfms >> 16) & 0xF) << 4;
 	c->x86_mask = tfms & 15;
+	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
 	if ( cpu_has(c, X86_FEATURE_CLFLSH) )
 		c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8;
 
@@ -339,17 +351,6 @@ void __cpuinit identify_cpu(struct cpuin
 	}
 }
 
-/* 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
-{
-	return hard_smp_processor_id() >> index_msb;
-}
-
 /* leaf 0xb SMT level */
 #define SMT_LEVEL       0
 
@@ -429,14 +430,12 @@ void __cpuinit detect_ht(struct cpuinfo_
 	u32 	eax, ebx, ecx, edx;
 	int 	index_msb, core_bits;
 
-	cpuid(1, &eax, &ebx, &ecx, &edx);
-
-	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
-
-	if (!cpu_has(c, X86_FEATURE_HT) || cpu_has(c, X86_FEATURE_CMP_LEGACY)
-        || cpu_has(c, X86_FEATURE_XTOPOLOGY))
+	if (!cpu_has(c, X86_FEATURE_HT) ||
+	    cpu_has(c, X86_FEATURE_CMP_LEGACY) ||
+	    cpu_has(c, X86_FEATURE_XTOPOLOGY))
 		return;
 
+	cpuid(1, &eax, &ebx, &ecx, &edx);
 	c->x86_num_siblings = (ebx & 0xff0000) >> 16;
 
 	if (c->x86_num_siblings == 1) {

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: always store APIC ID of CPU

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2753 bytes --]

On 26/06/14 11:11, Jan Beulich wrote:
> So far for non-Intel CPUs struct cpuinfo_x86's apicid field didn't get
> set, despite MCE code consuming it.
>
> Do some formatting/ordering adjustment to the touched code at once.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -152,6 +152,17 @@ static void __cpuinit get_cpu_vendor(str
>  	this_cpu = &default_cpu;
>  }
>  
> +/*
> + * 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)

What is the point of the cpuid_apic parameter?  It is unused.

index_msb should probably be unsigned as it is used to shift with.

~Andrew

> +{
> +	return hard_smp_processor_id() >> index_msb;
> +}
>  
>  /* Do minimum CPU detection early.
>     Fields really needed: vendor, cpuid_level, family, model, mask, cache alignment.
> @@ -216,6 +227,7 @@ static void __cpuinit generic_identify(s
>  	if (c->x86 >= 0x6)
>  		c->x86_model += ((tfms >> 16) & 0xF) << 4;
>  	c->x86_mask = tfms & 15;
> +	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
>  	if ( cpu_has(c, X86_FEATURE_CLFLSH) )
>  		c->x86_clflush_size = ((ebx >> 8) & 0xff) * 8;
>  
> @@ -339,17 +351,6 @@ void __cpuinit identify_cpu(struct cpuin
>  	}
>  }
>  
> -/* 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
> -{
> -	return hard_smp_processor_id() >> index_msb;
> -}
> -
>  /* leaf 0xb SMT level */
>  #define SMT_LEVEL       0
>  
> @@ -429,14 +430,12 @@ void __cpuinit detect_ht(struct cpuinfo_
>  	u32 	eax, ebx, ecx, edx;
>  	int 	index_msb, core_bits;
>  
> -	cpuid(1, &eax, &ebx, &ecx, &edx);
> -
> -	c->apicid = phys_pkg_id((ebx >> 24) & 0xFF, 0);
> -
> -	if (!cpu_has(c, X86_FEATURE_HT) || cpu_has(c, X86_FEATURE_CMP_LEGACY)
> -        || cpu_has(c, X86_FEATURE_XTOPOLOGY))
> +	if (!cpu_has(c, X86_FEATURE_HT) ||
> +	    cpu_has(c, X86_FEATURE_CMP_LEGACY) ||
> +	    cpu_has(c, X86_FEATURE_XTOPOLOGY))
>  		return;
>  
> +	cpuid(1, &eax, &ebx, &ecx, &edx);
>  	c->x86_num_siblings = (ebx & 0xff0000) >> 16;
>  
>  	if (c->x86_num_siblings == 1) {
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3623 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86: always store APIC ID of CPU

[Jan Beulich]

>>> On 26.06.14 at 13:02, <andrew.cooper3@citrix.com> wrote:
> On 26/06/14 11:11, Jan Beulich wrote:
>> +/*
>> + * 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
> 
> What is the point of the cpuid_apic parameter?  It is unused.
> 
> index_msb should probably be unsigned as it is used to shift with.

For both of these - yes, I agree, but no, I don't want to change
this here (I'm just moving the function up after all). The history of
this is a bit difficult to interpret: Before in change in 2006 some
variants of this function used both inputs, but some didn't. I
suppose Keir didn't want to risk breaking clustered APIC systems,
albeit as far as I looked only Summit systems were actually ignoring
the passed in APIC ID, and we're not supporting these anymore.

As to shift counts, I don't think it really matters whether they're
signed or unsigned, as anything outside the range [0,bitwidth-1]
produce undefined operations anyway.

Jan

Re: [PATCH] x86: always store APIC ID of CPU

[Andrew Cooper]

On 26/06/14 12:30, Jan Beulich wrote:
>>>> On 26.06.14 at 13:02, <andrew.cooper3@citrix.com> wrote:
>> On 26/06/14 11:11, Jan Beulich wrote:
>>> +/*
>>> + * 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 inline u32 phys_pkg_id(u32 cpuid_apic, int index_msb)
>> What is the point of the cpuid_apic parameter?  It is unused.
>>
>> index_msb should probably be unsigned as it is used to shift with.
> For both of these - yes, I agree, but no, I don't want to change
> this here (I'm just moving the function up after all). The history of
> this is a bit difficult to interpret: Before in change in 2006 some
> variants of this function used both inputs, but some didn't. I
> suppose Keir didn't want to risk breaking clustered APIC systems,
> albeit as far as I looked only Summit systems were actually ignoring
> the passed in APIC ID, and we're not supporting these anymore.
>
> As to shift counts, I don't think it really matters whether they're
> signed or unsigned, as anything outside the range [0,bitwidth-1]
> produce undefined operations anyway.
>
> Jan
>

Fair enough.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>