[PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Andreas Herrmann]

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD multi-node
processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
nodes on one socket. And thus there are cores having different
numa-node-id but with equal phys_proc_id. For example on such a system
we now get

[    0.228109] Booting Node   0, Processors  #1
[    0.232337] smpboot cpu 1: start_ip = 83000
[    0.252088]  #2
[    0.253746] smpboot cpu 2: start_ip = 83000
[    0.272086]  #3
[    0.276018] smpboot cpu 3: start_ip = 83000
[    0.296088]  #4
[    0.297745] smpboot cpu 4: start_ip = 83000
[    0.316088]  #5
[    0.320021] smpboot cpu 5: start_ip = 83000
[    0.340113]  Ok.
[    0.342324] Booting Node   1, Processors  #6
[    0.344344] smpboot cpu 6: start_ip = 83000
[    0.016000] NUMA core number 1 differs from configured core number 0
[    0.372110]  #7
[    0.373771] smpboot cpu 7: start_ip = 83000
[    0.016000] NUMA core number 1 differs from configured core number 0
[    0.396104]  #8
[    0.397764] smpboot cpu 8: start_ip = 83000
[    0.016000] NUMA core number 1 differs from configured core number 0
[    0.420109]  #9
[    0.421773] smpboot cpu 9: start_ip = 83000
[    0.016000] NUMA core number 1 differs from configured core number 0
[    0.444113]  #10
[    0.445865] smpboot cpu 10: start_ip = 83000
[    0.016000] NUMA core number 1 differs from configured core number 0
[    0.468111]  #11
[    0.472030] smpboot cpu 11: start_ip = 83000
[    0.016000] NUMA core number 1 differs from configured core number 0

These NUMA core numbering error messages are plain wrong.

The confusing/misleading error message was introduced with commit
64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
override to fix up NUMA core numbering) and should be removed.

Reported-by: Borislav Petkov <borislav.petkov@amd.com>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/kernel/cpu/common.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

BTW, I wonder why the fixup code isn't called from the Intel path.  At
least the mentioned patch suggests that something more generic was
introduced here.

And I am curious why there is this specific condition to decide
whether a call to x86_cpuinit.fixup_cpu_id is required.


Regards,
Andreas


diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d43cad7..2ef7685 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1163,7 +1163,6 @@ static void dbg_restore_debug_regs(void)
  */
 void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
 }
 
 /*
-- 
1.7.8.4

Re: [PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Borislav Petkov]

On Mon, Feb 20, 2012 at 06:17:05PM +0100, Andreas Herrmann wrote:
> 
> It's only called from amd.c:srat_detect_node(). The introduced
> condition for calling the fixup code is true for all AMD multi-node
> processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
> nodes on one socket. And thus there are cores having different
> numa-node-id but with equal phys_proc_id. For example on such a system
> we now get
> 
> [    0.228109] Booting Node   0, Processors  #1
> [    0.232337] smpboot cpu 1: start_ip = 83000
> [    0.252088]  #2
> [    0.253746] smpboot cpu 2: start_ip = 83000
> [    0.272086]  #3
> [    0.276018] smpboot cpu 3: start_ip = 83000
> [    0.296088]  #4
> [    0.297745] smpboot cpu 4: start_ip = 83000
> [    0.316088]  #5
> [    0.320021] smpboot cpu 5: start_ip = 83000
> [    0.340113]  Ok.
> [    0.342324] Booting Node   1, Processors  #6
> [    0.344344] smpboot cpu 6: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.372110]  #7
> [    0.373771] smpboot cpu 7: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.396104]  #8
> [    0.397764] smpboot cpu 8: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.420109]  #9
> [    0.421773] smpboot cpu 9: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.444113]  #10
> [    0.445865] smpboot cpu 10: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.468111]  #11
> [    0.472030] smpboot cpu 11: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> 
> These NUMA core numbering error messages are plain wrong.
> 
> The confusing/misleading error message was introduced with commit
> 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
> override to fix up NUMA core numbering) and should be removed.
> 
> Reported-by: Borislav Petkov <borislav.petkov@amd.com>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  arch/x86/kernel/cpu/common.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> BTW, I wonder why the fixup code isn't called from the Intel path.  At
> least the mentioned patch suggests that something more generic was
> introduced here.

Right, and I would remove the check in amd.c:srat_detect_node() instead
of removing the printk statement in the default implementation.

IOW, we need more info on why the check was added only to the AMD path?
Daniel?

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Daniel J Blueman]

On 21/02/2012 10:27, Borislav Petkov wrote:
> On Mon, Feb 20, 2012 at 06:17:05PM +0100, Andreas Herrmann wrote:
>
> It's only called from amd.c:srat_detect_node(). The introduced
> condition for calling the fixup code is true for all AMD multi-node
> processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
> nodes on one socket. And thus there are cores having different
> numa-node-id but with equal phys_proc_id. For example on such a system
> we now get
>
> [    0.228109] Booting Node   0, Processors  #1
> [    0.232337] smpboot cpu 1: start_ip = 83000
> [    0.252088]  #2
> [    0.253746] smpboot cpu 2: start_ip = 83000
> [    0.272086]  #3
> [    0.276018] smpboot cpu 3: start_ip = 83000
> [    0.296088]  #4
> [    0.297745] smpboot cpu 4: start_ip = 83000
> [    0.316088]  #5
> [    0.320021] smpboot cpu 5: start_ip = 83000
> [    0.340113]  Ok.
> [    0.342324] Booting Node   1, Processors  #6
> [    0.344344] smpboot cpu 6: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.372110]  #7
> [    0.373771] smpboot cpu 7: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.396104]  #8
> [    0.397764] smpboot cpu 8: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.420109]  #9
> [    0.421773] smpboot cpu 9: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.444113]  #10
> [    0.445865] smpboot cpu 10: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
> [    0.468111]  #11
> [    0.472030] smpboot cpu 11: start_ip = 83000
> [    0.016000] NUMA core number 1 differs from configured core number 0
>
> These NUMA core numbering error messages are plain wrong.
>
> The confusing/misleading error message was introduced with commit
> 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
> override to fix up NUMA core numbering) and should be removed.
>
> Reported-by: Borislav Petkov<borislav.petkov@amd.com>
> Signed-off-by: Andreas Herrmann<andreas.herrmann3@amd.com>
> ---
>   arch/x86/kernel/cpu/common.c |    1 -
>   1 files changed, 0 insertions(+), 1 deletions(-)
>
> BTW, I wonder why the fixup code isn't called from the Intel path.  At
> least the mentioned patch suggests that something more generic was
> introduced here.
> Right, and I would remove the check in amd.c:srat_detect_node() instead
> of removing the printk statement in the default implementation.
>
> IOW, we need more info on why the check was added only to the AMD path?
> Daniel?

The check and fixup wasn't needed in the Intel path thus far, so wasn't 
added.

We could specialise the 'if (c->phys_proc_id != node)' test to check for 
x86_cpuinit.fixup_cpu_id being NULL and drop the default override, if 
that is preferred?

Thanks,
   Daniel

-- 
Daniel J Blueman
Principal Software Engineer, Numascale Asia

Re: [PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Borislav Petkov]

On Tue, Feb 21, 2012 at 11:05:21AM +0000, Daniel J Blueman wrote:
> The check and fixup wasn't needed in the Intel path thus far, so
> wasn't added.
> 
> We could specialise the 'if (c->phys_proc_id != node)' test to check
> for x86_cpuinit.fixup_cpu_id being NULL and drop the default
> override, if that is preferred?

Before that, why do you need that check in the AMD path at all? Please
give a more detailed explanation as to why is it needed on the AMD path
at all.

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Daniel J Blueman]

On 21/02/2012 11:20, Borislav Petkov wrote:
> On Tue, Feb 21, 2012 at 11:05:21AM +0000, Daniel J Blueman wrote:
>> The check and fixup wasn't needed in the Intel path thus far, so
>> wasn't added.
>>
>> We could specialise the 'if (c->phys_proc_id != node)' test to check
>> for x86_cpuinit.fixup_cpu_id being NULL and drop the default
>> override, if that is preferred?
> Before that, why do you need that check in the AMD path at all? Please
> give a more detailed explanation as to why is it needed on the AMD path
> at all.

Since Numascale's NumaConnect bridges multiple separate HyperTransport 
fabrics across multiple servers together, the HT IDs written in the 
hardware thus don't match the information in the SRAT table constructed 
in the bootloader, thus we need to set this to the logical value [1, 
'fixup_cpu_id'].

[1] https://lkml.org/lkml/2011/12/5/292

-- 
Daniel J Blueman
Principal Software Engineer, Numascale Asia

Re: [PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Andreas Herrmann]

On Tue, Feb 21, 2012 at 11:05:21AM +0000, Daniel J Blueman wrote:
> On 21/02/2012 10:27, Borislav Petkov wrote:
> > On Mon, Feb 20, 2012 at 06:17:05PM +0100, Andreas Herrmann wrote:

[snip]

> > BTW, I wonder why the fixup code isn't called from the Intel path.  At
> > least the mentioned patch suggests that something more generic was
> > introduced here.
> > Right, and I would remove the check in amd.c:srat_detect_node() instead
> > of removing the printk statement in the default implementation.
> >
> > IOW, we need more info on why the check was added only to the AMD path?
> > Daniel?
> 
> The check and fixup wasn't needed in the Intel path thus far, so wasn't 
> added.
> 
> We could specialise the 'if (c->phys_proc_id != node)' test to check for 
> x86_cpuinit.fixup_cpu_id being NULL and drop the default override, if 
> that is preferred?

It seems that all the stuff in x86_init.[ch] is using default/noop
functions instead of NULL pointer checks. So we shouldn't deviate from
this for x86_cpuinit.fixup_cpu_id.

I think attached patch is more suitable to avoid the wrong warning
message.

Please review.


Thanks,

Andreas

--
x86: Remove wrong error message in x86_default_fixup_cpu_id

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD multi-node
processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
nodes on one socket. Thus there are cores having different
numa-node-id but with equal phys_proc_id.

There is no point to print error messages in such a situation.

The confusing/misleading error message was introduced with commit
64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
override to fix up NUMA core numbering).

Change the default fixup function (remove the error message), move the
Numascale-specific condition for calling the fixup into the
fixup-function itself and slightly adapt the comment.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/include/asm/x86_init.h      |    2 +-
 arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
 arch/x86/kernel/cpu/amd.c            |    8 ++++----
 arch/x86/kernel/cpu/common.c         |    9 ---------
 arch/x86/kernel/x86_init.c           |    1 +
 5 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 517d476..1bcacef 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,6 +189,6 @@ extern struct x86_msi_ops x86_msi;
 
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
-extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
+extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n);
 
 #endif
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 09d3d8c..ade0182 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -201,8 +201,11 @@ static void __init map_csrs(void)
 
 static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	c->phys_proc_id = node;
-	per_cpu(cpu_llc_id, smp_processor_id()) = node;
+
+	if (c->phys_proc_id != node) {
+		c->phys_proc_id = node;
+		per_cpu(cpu_llc_id, smp_processor_id()) = node;
+	}
 }
 
 static int __init numachip_system_init(void)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index f4773f4..52b7287 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -352,11 +352,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
 		node = per_cpu(cpu_llc_id, cpu);
 
 	/*
-	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
-	 * so invoke platform-specific handler
+	 * On multi-fabric platform (e.g. Numascale NumaChip) a
+	 * platform-specific handler needs to be called to fixup some
+	 * IDs of the CPU.
 	 */
-	if (c->phys_proc_id != node)
-		x86_cpuinit.fixup_cpu_id(c, node);
+	x86_cpuinit.fixup_cpu_id(c, node);
 
 	if (!node_online(node)) {
 		/*
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d43cad7..37da7a6 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1158,15 +1158,6 @@ static void dbg_restore_debug_regs(void)
 #endif /* ! CONFIG_KGDB */
 
 /*
- * Prints an error where the NUMA and configured core-number mismatch and the
- * platform didn't override this to fix it up
- */
-void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
-{
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
-}
-
-/*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
  * and IDT. We reload them nevertheless, this function acts as a
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 947a06c..67cf78a 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
 	},
 };
 
+void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n) { }
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
 	.fixup_cpu_id			= x86_default_fixup_cpu_id,
-- 
1.7.8.4

Re: [PATCH] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Daniel J Blueman]

On 22/02/2012 13:47, Andreas Herrmann wrote:
> On Tue, Feb 21, 2012 at 11:05:21AM +0000, Daniel J Blueman wrote:
>> On 21/02/2012 10:27, Borislav Petkov wrote:
>>> On Mon, Feb 20, 2012 at 06:17:05PM +0100, Andreas Herrmann wrote:
> [snip]
>
>>> BTW, I wonder why the fixup code isn't called from the Intel path.  At
>>> least the mentioned patch suggests that something more generic was
>>> introduced here.
>>> Right, and I would remove the check in amd.c:srat_detect_node() instead
>>> of removing the printk statement in the default implementation.
>>>
>>> IOW, we need more info on why the check was added only to the AMD path?
>>> Daniel?
>> The check and fixup wasn't needed in the Intel path thus far, so wasn't
>> added.
>>
>> We could specialise the 'if (c->phys_proc_id != node)' test to check for
>> x86_cpuinit.fixup_cpu_id being NULL and drop the default override, if
>> that is preferred?
> It seems that all the stuff in x86_init.[ch] is using default/noop
> functions instead of NULL pointer checks. So we shouldn't deviate from
> this for x86_cpuinit.fixup_cpu_id.
>
> I think attached patch is more suitable to avoid the wrong warning
> message.
>
> Please review.

Yes, this looks reasonable and tests out successfully on systems with 
and without NumaConnect.

Signed-off-by: Daniel J Blueman <daniel@numascale-asia.com>

Thanks,
   Daniel

>
>
> Thanks,
>
> Andreas
>
> --
> x86: Remove wrong error message in x86_default_fixup_cpu_id
>
> It's only called from amd.c:srat_detect_node(). The introduced
> condition for calling the fixup code is true for all AMD multi-node
> processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
> nodes on one socket. Thus there are cores having different
> numa-node-id but with equal phys_proc_id.
>
> There is no point to print error messages in such a situation.
>
> The confusing/misleading error message was introduced with commit
> 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
> override to fix up NUMA core numbering).
>
> Change the default fixup function (remove the error message), move the
> Numascale-specific condition for calling the fixup into the
> fixup-function itself and slightly adapt the comment.
>
> Signed-off-by: Andreas Herrmann<andreas.herrmann3@amd.com>
> ---
>   arch/x86/include/asm/x86_init.h      |    2 +-
>   arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
>   arch/x86/kernel/cpu/amd.c            |    8 ++++----
>   arch/x86/kernel/cpu/common.c         |    9 ---------
>   arch/x86/kernel/x86_init.c           |    1 +
>   5 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index 517d476..1bcacef 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -189,6 +189,6 @@ extern struct x86_msi_ops x86_msi;
>
>   extern void x86_init_noop(void);
>   extern void x86_init_uint_noop(unsigned int unused);
> -extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
> +extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n);
>
>   #endif
> diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
> index 09d3d8c..ade0182 100644
> --- a/arch/x86/kernel/apic/apic_numachip.c
> +++ b/arch/x86/kernel/apic/apic_numachip.c
> @@ -201,8 +201,11 @@ static void __init map_csrs(void)
>
>   static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
>   {
> -	c->phys_proc_id = node;
> -	per_cpu(cpu_llc_id, smp_processor_id()) = node;
> +
> +	if (c->phys_proc_id != node) {
> +		c->phys_proc_id = node;
> +		per_cpu(cpu_llc_id, smp_processor_id()) = node;
> +	}
>   }
>
>   static int __init numachip_system_init(void)
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index f4773f4..52b7287 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -352,11 +352,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
>   		node = per_cpu(cpu_llc_id, cpu);
>
>   	/*
> -	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
> -	 * so invoke platform-specific handler
> +	 * On multi-fabric platform (e.g. Numascale NumaChip) a
> +	 * platform-specific handler needs to be called to fixup some
> +	 * IDs of the CPU.
>   	 */
> -	if (c->phys_proc_id != node)
> -		x86_cpuinit.fixup_cpu_id(c, node);
> +	x86_cpuinit.fixup_cpu_id(c, node);
>
>   	if (!node_online(node)) {
>   		/*
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index d43cad7..37da7a6 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1158,15 +1158,6 @@ static void dbg_restore_debug_regs(void)
>   #endif /* ! CONFIG_KGDB */
>
>   /*
> - * Prints an error where the NUMA and configured core-number mismatch and the
> - * platform didn't override this to fix it up
> - */
> -void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
> -{
> -	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
> -}
> -
> -/*
>    * cpu_init() initializes state that is per-CPU. Some data is already
>    * initialized (naturally) in the bootstrap process, such as the GDT
>    * and IDT. We reload them nevertheless, this function acts as a
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 947a06c..67cf78a 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
>   	},
>   };
>
> +void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n) { }
>   struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
>   	.setup_percpu_clockev		= setup_secondary_APIC_clock,
>   	.fixup_cpu_id			= x86_default_fixup_cpu_id,
-- 
Daniel J Blueman
Principal Software Engineer, Numascale Asia

[PATCH resend] x86: Remove wrong error message in x86_default_fixup_cpu_id

[Andreas Herrmann]

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD multi-node
processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
nodes on one socket. Thus there are cores having different
numa-node-id but with equal phys_proc_id.

There is no point to print error messages in such a situation.

The confusing/misleading error message was introduced with commit
64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
override to fix up NUMA core numbering).

Change the default fixup function (remove the error message), move the
Numascale-specific condition for calling the fixup into the
fixup-function itself and slightly adapt the comment.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>
---
 arch/x86/include/asm/x86_init.h      |    2 +-
 arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
 arch/x86/kernel/cpu/amd.c            |    8 ++++----
 arch/x86/kernel/cpu/common.c         |    9 ---------
 arch/x86/kernel/x86_init.c           |    1 +
 5 files changed, 11 insertions(+), 16 deletions(-)


Please apply.


Thanks,

Andreas


diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 517d476..1bcacef 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,6 +189,6 @@ extern struct x86_msi_ops x86_msi;
 
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
-extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
+extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n);
 
 #endif
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 09d3d8c..ade0182 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -201,8 +201,11 @@ static void __init map_csrs(void)
 
 static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	c->phys_proc_id = node;
-	per_cpu(cpu_llc_id, smp_processor_id()) = node;
+
+	if (c->phys_proc_id != node) {
+		c->phys_proc_id = node;
+		per_cpu(cpu_llc_id, smp_processor_id()) = node;
+	}
 }
 
 static int __init numachip_system_init(void)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index f4773f4..52b7287 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -352,11 +352,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
 		node = per_cpu(cpu_llc_id, cpu);
 
 	/*
-	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
-	 * so invoke platform-specific handler
+	 * On multi-fabric platform (e.g. Numascale NumaChip) a
+	 * platform-specific handler needs to be called to fixup some
+	 * IDs of the CPU.
 	 */
-	if (c->phys_proc_id != node)
-		x86_cpuinit.fixup_cpu_id(c, node);
+	x86_cpuinit.fixup_cpu_id(c, node);
 
 	if (!node_online(node)) {
 		/*
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d43cad7..37da7a6 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1158,15 +1158,6 @@ static void dbg_restore_debug_regs(void)
 #endif /* ! CONFIG_KGDB */
 
 /*
- * Prints an error where the NUMA and configured core-number mismatch and the
- * platform didn't override this to fix it up
- */
-void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
-{
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
-}
-
-/*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
  * and IDT. We reload them nevertheless, this function acts as a
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 947a06c..67cf78a 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
 	},
 };
 
+void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n) { }
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
 	.fixup_cpu_id			= x86_default_fixup_cpu_id,
-- 
1.7.8.4

[tip:x86/platform] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[tip-bot for Andreas Herrmann]

Commit-ID:  d7dedbe3694bbf97b4e3409e1fa52d97a3b9f028
Gitweb:     http://git.kernel.org/tip/d7dedbe3694bbf97b4e3409e1fa52d97a3b9f028
Author:     Andreas Herrmann <andreas.herrmann3@amd.com>
AuthorDate: Fri, 24 Feb 2012 16:31:27 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 27 Feb 2012 10:35:47 +0100

x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD
multi-node processors, e.g. Magny-Cours and Interlagos. There we
have 2 NUMA nodes on one socket. Thus there are cores having
different numa-node-id but with equal phys_proc_id.

There is no point to print error messages in such a situation.

The confusing/misleading error message was introduced with
commit 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add
x86_init platform override to fix up NUMA core numbering).

Change the default fixup function (remove the error message),
move the Numascale-specific condition for calling the fixup into
the fixup-function itself and slightly adapt the comment.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>
Cc: Borislav Petkov <bp@amd64.org>
Cc: Borislav Petkov <borislav.petkov@amd.com>
Cc: Steffen Persvold <sp@numascale.com>
Link: http://lkml.kernel.org/r/20120224153127.GD28921@alberich.amd.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/x86_init.h      |    2 +-
 arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
 arch/x86/kernel/cpu/amd.c            |    8 ++++----
 arch/x86/kernel/cpu/common.c         |    9 ---------
 arch/x86/kernel/x86_init.c           |    1 +
 5 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 517d476..1bcacef 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,6 +189,6 @@ extern struct x86_msi_ops x86_msi;
 
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
-extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
+extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n);
 
 #endif
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 09d3d8c..ade0182 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -201,8 +201,11 @@ static void __init map_csrs(void)
 
 static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	c->phys_proc_id = node;
-	per_cpu(cpu_llc_id, smp_processor_id()) = node;
+
+	if (c->phys_proc_id != node) {
+		c->phys_proc_id = node;
+		per_cpu(cpu_llc_id, smp_processor_id()) = node;
+	}
 }
 
 static int __init numachip_system_init(void)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index f4773f4..52b7287 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -352,11 +352,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
 		node = per_cpu(cpu_llc_id, cpu);
 
 	/*
-	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
-	 * so invoke platform-specific handler
+	 * On multi-fabric platform (e.g. Numascale NumaChip) a
+	 * platform-specific handler needs to be called to fixup some
+	 * IDs of the CPU.
 	 */
-	if (c->phys_proc_id != node)
-		x86_cpuinit.fixup_cpu_id(c, node);
+	x86_cpuinit.fixup_cpu_id(c, node);
 
 	if (!node_online(node)) {
 		/*
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index d43cad7..37da7a6 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1158,15 +1158,6 @@ static void dbg_restore_debug_regs(void)
 #endif /* ! CONFIG_KGDB */
 
 /*
- * Prints an error where the NUMA and configured core-number mismatch and the
- * platform didn't override this to fix it up
- */
-void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
-{
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
-}
-
-/*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
  * and IDT. We reload them nevertheless, this function acts as a
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 947a06c..67cf78a 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
 	},
 };
 
+void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n) { }
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
 	.fixup_cpu_id			= x86_default_fixup_cpu_id,

Re: [tip:x86/platform] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[Borislav Petkov]

On Mon, Feb 27, 2012 at 04:07:33AM -0800, tip-bot for Andreas Herrmann wrote:
> Commit-ID:  d7dedbe3694bbf97b4e3409e1fa52d97a3b9f028
> Gitweb:     http://git.kernel.org/tip/d7dedbe3694bbf97b4e3409e1fa52d97a3b9f028
> Author:     Andreas Herrmann <andreas.herrmann3@amd.com>
> AuthorDate: Fri, 24 Feb 2012 16:31:27 +0100
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Mon, 27 Feb 2012 10:35:47 +0100
> 
> x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()
> 
> It's only called from amd.c:srat_detect_node(). The introduced
> condition for calling the fixup code is true for all AMD
> multi-node processors, e.g. Magny-Cours and Interlagos. There we
> have 2 NUMA nodes on one socket. Thus there are cores having
> different numa-node-id but with equal phys_proc_id.
> 
> There is no point to print error messages in such a situation.
> 
> The confusing/misleading error message was introduced with
> commit 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add
> x86_init platform override to fix up NUMA core numbering).
> 
> Change the default fixup function (remove the error message),
> move the Numascale-specific condition for calling the fixup into
> the fixup-function itself and slightly adapt the comment.
> 
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>
> Cc: Borislav Petkov <bp@amd64.org>
> Cc: Borislav Petkov <borislav.petkov@amd.com>
> Cc: Steffen Persvold <sp@numascale.com>
> Link: http://lkml.kernel.org/r/20120224153127.GD28921@alberich.amd.com
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

Maybe this one should be queued for x86/urgent instead and should go to
Linus soonish since it cures an issue which came in during the 3.3 merge
window with 64be4c1c2428e148de6081af235e2418e6a66dda?

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[tip:x86/urgent] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[tip-bot for Andreas Herrmann]

Commit-ID:  20b25f03fbd17b4e32b786024a5f9c6306795411
Gitweb:     http://git.kernel.org/tip/20b25f03fbd17b4e32b786024a5f9c6306795411
Author:     Andreas Herrmann <andreas.herrmann3@amd.com>
AuthorDate: Fri, 24 Feb 2012 16:31:27 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 28 Feb 2012 17:30:00 +0100

x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD
multi-node processors, e.g. Magny-Cours and Interlagos. There we
have 2 NUMA nodes on one socket. Thus there are cores having
different numa-node-id but with equal phys_proc_id.

There is no point to print error messages in such a situation.

The confusing/misleading error message was introduced with
commit 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add
x86_init platform override to fix up NUMA core numbering).

Change the default fixup function (remove the error message),
move the Numascale-specific condition for calling the fixup into
the fixup-function itself and slightly adapt the comment.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
Reviewed-by: Daniel J Blueman <daniel@numascale-asia.com>
Acked-by: Borislav Petkov <bp@amd64.org>
Cc: Borislav Petkov <borislav.petkov@amd.com>
Cc: Steffen Persvold <sp@numascale.com>
Link: http://lkml.kernel.org/r/20120224153127.GD28921@alberich.amd.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/x86_init.h      |    2 +-
 arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
 arch/x86/kernel/cpu/amd.c            |    8 ++++----
 arch/x86/kernel/cpu/common.c         |    9 ---------
 arch/x86/kernel/x86_init.c           |    1 +
 5 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 517d476..1bcacef 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,6 +189,6 @@ extern struct x86_msi_ops x86_msi;
 
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
-extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
+extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n);
 
 #endif
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 09d3d8c..ade0182 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -201,8 +201,11 @@ static void __init map_csrs(void)
 
 static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	c->phys_proc_id = node;
-	per_cpu(cpu_llc_id, smp_processor_id()) = node;
+
+	if (c->phys_proc_id != node) {
+		c->phys_proc_id = node;
+		per_cpu(cpu_llc_id, smp_processor_id()) = node;
+	}
 }
 
 static int __init numachip_system_init(void)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index f4773f4..52b7287 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -352,11 +352,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
 		node = per_cpu(cpu_llc_id, cpu);
 
 	/*
-	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
-	 * so invoke platform-specific handler
+	 * On multi-fabric platform (e.g. Numascale NumaChip) a
+	 * platform-specific handler needs to be called to fixup some
+	 * IDs of the CPU.
 	 */
-	if (c->phys_proc_id != node)
-		x86_cpuinit.fixup_cpu_id(c, node);
+	x86_cpuinit.fixup_cpu_id(c, node);
 
 	if (!node_online(node)) {
 		/*
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c0f7d68..1a810e4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1163,15 +1163,6 @@ static void dbg_restore_debug_regs(void)
 #endif /* ! CONFIG_KGDB */
 
 /*
- * Prints an error where the NUMA and configured core-number mismatch and the
- * platform didn't override this to fix it up
- */
-void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
-{
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
-}
-
-/*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
  * and IDT. We reload them nevertheless, this function acts as a
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 947a06c..67cf78a 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
 	},
 };
 
+void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n) { }
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
 	.fixup_cpu_id			= x86_default_fixup_cpu_id,

Re: [tip:x86/urgent] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[Ingo Molnar]

* tip-bot for Andreas Herrmann <andreas.herrmann3@amd.com> wrote:

> +++ b/arch/x86/kernel/cpu/common.c
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 947a06c..67cf78a 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
>  	},
>  };
>  
> +void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n) { }
>  struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {

Hm, I missed this first time around:

 - why is this function global? It's not used by anything else.

 - why is it squeezed before a structure without any vertical 
   separation?

 - why does it have the body as { }, as if it were an inline 
   function?

Really, this should either be a short static function, with a 
proper body, or we should accept a NULL pointer there and check 
for it before calling it - there's a single usage site right 
now.

The latter looks like the cleanest solution to me.

Thanks,

	Ingo

Re: [tip:x86/urgent] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[Andreas Herrmann]

On Fri, Mar 02, 2012 at 12:04:40PM +0100, Ingo Molnar wrote:
> 
> * tip-bot for Andreas Herrmann <andreas.herrmann3@amd.com> wrote:
> 
> > +++ b/arch/x86/kernel/cpu/common.c
> > diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> > index 947a06c..67cf78a 100644
> > --- a/arch/x86/kernel/x86_init.c
> > +++ b/arch/x86/kernel/x86_init.c
> > @@ -90,6 +90,7 @@ struct x86_init_ops x86_init __initdata = {
> >  	},
> >  };
> >  
> > +void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int n) { }
> >  struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
> 
> Hm, I missed this first time around:
> 
>  - why is this function global? It's not used by anything else.

No reason. Should be static.

>  - why is it squeezed before a structure without any vertical 
>    separation?

Should be corrected too.

>  - why does it have the body as { }, as if it were an inline 
>    function?

To make it fit into one line, similar to other default init functions,
e.g.  default_nmi_init().

> Really, this should either be a short static function, with a 
> proper body, or we should accept a NULL pointer there and check 
> for it before calling it - there's a single usage site right 
> now.

> The latter looks like the cleanest solution to me.

Yes, the NULL pointer check is probably the better alternative.
How about below patch version?


Regards,
Andreas
--
x86: Remove wrong error message in x86_default_fixup_cpu_id

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD multi-node
processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
nodes on one socket. Thus there are cores having different
numa-node-id but with equal phys_proc_id.

There is no point to print error messages in such a situation.

The confusing/misleading error message was introduced with commit
64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
override to fix up NUMA core numbering).

Remove the default fixup function (especially the error message) and
replace it by a NULL pointer check, move the Numascale-specific
condition for calling the fixup into the fixup-function itself and
slightly adapt the comment.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/include/asm/x86_init.h      |    1 -
 arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
 arch/x86/kernel/cpu/amd.c            |    7 ++++---
 arch/x86/kernel/cpu/common.c         |    9 ---------
 arch/x86/kernel/x86_init.c           |    1 -
 5 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 517d476..a609c39 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,6 +189,5 @@ extern struct x86_msi_ops x86_msi;
 
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
-extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
 
 #endif
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 09d3d8c..ade0182 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -201,8 +201,11 @@ static void __init map_csrs(void)
 
 static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	c->phys_proc_id = node;
-	per_cpu(cpu_llc_id, smp_processor_id()) = node;
+
+	if (c->phys_proc_id != node) {
+		c->phys_proc_id = node;
+		per_cpu(cpu_llc_id, smp_processor_id()) = node;
+	}
 }
 
 static int __init numachip_system_init(void)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 0a44b90..c8fd678 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -353,10 +353,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
 		node = per_cpu(cpu_llc_id, cpu);
 
 	/*
-	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
-	 * so invoke platform-specific handler
+	 * On multi-fabric platform (e.g. Numascale NumaChip) a
+	 * platform-specific handler needs to be called to fixup some
+	 * IDs of the CPU.
 	 */
-	if (c->phys_proc_id != node)
+	if (x86_cpuinit.fixup_cpu_id)
 		x86_cpuinit.fixup_cpu_id(c, node);
 
 	if (!node_online(node)) {
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index ade9c79..abd0af5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1162,15 +1162,6 @@ static void dbg_restore_debug_regs(void)
 #endif /* ! CONFIG_KGDB */
 
 /*
- * Prints an error where the NUMA and configured core-number mismatch and the
- * platform didn't override this to fix it up
- */
-void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
-{
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
-}
-
-/*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
  * and IDT. We reload them nevertheless, this function acts as a
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 947a06c..83b05ad 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -92,7 +92,6 @@ struct x86_init_ops x86_init __initdata = {
 
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
-	.fixup_cpu_id			= x86_default_fixup_cpu_id,
 };
 
 static void default_nmi_init(void) { };
-- 
1.7.8.4

[PATCH resend] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[Andreas Herrmann]

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD multi-node
processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
nodes on one socket. Thus there are cores having different
numa-node-id but with equal phys_proc_id.

There is no point to print error messages in such a situation.

The confusing/misleading error message was introduced with commit
64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
override to fix up NUMA core numbering).

Remove the default fixup function (especially the error message) and
replace it by a NULL pointer check, move the Numascale-specific
condition for calling the fixup into the fixup-function itself and
slightly adapt the comment.

Cc: <stable@kernel.org>
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 arch/x86/include/asm/x86_init.h      |    1 -
 arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
 arch/x86/kernel/cpu/amd.c            |    7 ++++---
 arch/x86/kernel/cpu/common.c         |    9 ---------
 arch/x86/kernel/x86_init.c           |    1 -
 5 files changed, 9 insertions(+), 16 deletions(-)

This patch didn't make it into v3.3.
But the misleading error message introduced with numachip support was
merged.

Please apply this patch.


Thanks,

Andreas

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index baaca8d..764b66a 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -195,6 +195,5 @@ extern struct x86_msi_ops x86_msi;
 
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
-extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
 
 #endif
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 899803e..23e7542 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -207,8 +207,11 @@ static void __init map_csrs(void)
 
 static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	c->phys_proc_id = node;
-	per_cpu(cpu_llc_id, smp_processor_id()) = node;
+
+	if (c->phys_proc_id != node) {
+		c->phys_proc_id = node;
+		per_cpu(cpu_llc_id, smp_processor_id()) = node;
+	}
 }
 
 static int __init numachip_system_init(void)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 0a44b90..c8fd678 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -353,10 +353,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
 		node = per_cpu(cpu_llc_id, cpu);
 
 	/*
-	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
-	 * so invoke platform-specific handler
+	 * On multi-fabric platform (e.g. Numascale NumaChip) a
+	 * platform-specific handler needs to be called to fixup some
+	 * IDs of the CPU.
 	 */
-	if (c->phys_proc_id != node)
+	if (x86_cpuinit.fixup_cpu_id)
 		x86_cpuinit.fixup_cpu_id(c, node);
 
 	if (!node_online(node)) {
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 67e2583..cf79302 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1163,15 +1163,6 @@ static void dbg_restore_debug_regs(void)
 #endif /* ! CONFIG_KGDB */
 
 /*
- * Prints an error where the NUMA and configured core-number mismatch and the
- * platform didn't override this to fix it up
- */
-void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
-{
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
-}
-
-/*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
  * and IDT. We reload them nevertheless, this function acts as a
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index e9f265f..9cf71d0 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -93,7 +93,6 @@ struct x86_init_ops x86_init __initdata = {
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.early_percpu_clock_init	= x86_init_noop,
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
-	.fixup_cpu_id			= x86_default_fixup_cpu_id,
 };
 
 static void default_nmi_init(void) { };
-- 
1.7.8.5

Re: [PATCH resend] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[Borislav Petkov]

On Mon, Apr 02, 2012 at 06:06:48PM +0200, Andreas Herrmann wrote:
> 
> It's only called from amd.c:srat_detect_node(). The introduced
> condition for calling the fixup code is true for all AMD multi-node
> processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
> nodes on one socket. Thus there are cores having different
> numa-node-id but with equal phys_proc_id.
> 
> There is no point to print error messages in such a situation.
> 
> The confusing/misleading error message was introduced with commit
> 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
> override to fix up NUMA core numbering).
> 
> Remove the default fixup function (especially the error message) and
> replace it by a NULL pointer check, move the Numascale-specific
> condition for calling the fixup into the fixup-function itself and
> slightly adapt the comment.
> 
> Cc: <stable@kernel.org>
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  arch/x86/include/asm/x86_init.h      |    1 -
>  arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
>  arch/x86/kernel/cpu/amd.c            |    7 ++++---
>  arch/x86/kernel/cpu/common.c         |    9 ---------
>  arch/x86/kernel/x86_init.c           |    1 -
>  5 files changed, 9 insertions(+), 16 deletions(-)
> 
> This patch didn't make it into v3.3.
> But the misleading error message introduced with numachip support was
> merged.

Yes, please apply this one, I still get the following on my box with
3.4-rc1+:

[    0.382396] Booting Node   0, Processors  #1 #2 #3 #4 #5 Ok.
[    0.454471] Booting Node   1, Processors  #6
[    0.469600] NUMA core number 1 differs from configured core number 0
[    0.478361]  #7
[    0.490949] NUMA core number 1 differs from configured core number 0
[    0.499695]  #8
[    0.512577] NUMA core number 1 differs from configured core number 0
[    0.521331]  #9
[    0.533921] NUMA core number 1 differs from configured core number 0
[    0.542679]  #10
[    0.555340] NUMA core number 1 differs from configured core number 0
[    0.564088]  #11
[    0.576730] NUMA core number 1 differs from configured core number 0
[    0.585497]  Ok.
[    0.587698] Booting Node   3, Processors  #12
[    0.602916] NUMA core number 3 differs from configured core number 1
[    0.709432]  #13
[    0.722106] NUMA core number 3 differs from configured core number 1
[    0.730856]  #14
[    0.743823] NUMA core number 3 differs from configured core number 1
[    0.752577]  #15
[    0.765253] NUMA core number 3 differs from configured core number 1
[    0.774002]  #16
[    0.786684] NUMA core number 3 differs from configured core number 1
[    0.795436]  #17
[    0.808113] NUMA core number 3 differs from configured core number 1
[    0.816878]  Ok.
[    0.819377] Booting Node   2, Processors  #18
[    0.834597] NUMA core number 2 differs from configured core number 1
[    0.843348]  #19
[    0.856025] NUMA core number 2 differs from configured core number 1
[    0.864777]  #20
[    0.877750] NUMA core number 2 differs from configured core number 1
[    0.886505]  #21
[    0.899180] NUMA core number 2 differs from configured core number 1
[    0.907939]  #22
[    0.920583] NUMA core number 2 differs from configured core number 1
[    0.929335]  #23 Ok.
[    0.942785] NUMA core number 2 differs from configured core number 1
[    0.951262] Brought up 24 CPUs

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH resend] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[Borislav Petkov]

On Wed, Apr 04, 2012 at 02:38:09PM +0200, Borislav Petkov wrote:
> On Mon, Apr 02, 2012 at 06:06:48PM +0200, Andreas Herrmann wrote:
> > 
> > It's only called from amd.c:srat_detect_node(). The introduced
> > condition for calling the fixup code is true for all AMD multi-node
> > processors, e.g. Magny-Cours and Interlagos. There we have 2 NUMA
> > nodes on one socket. Thus there are cores having different
> > numa-node-id but with equal phys_proc_id.
> > 
> > There is no point to print error messages in such a situation.
> > 
> > The confusing/misleading error message was introduced with commit
> > 64be4c1c2428e148de6081af235e2418e6a66dda (x86: Add x86_init platform
> > override to fix up NUMA core numbering).
> > 
> > Remove the default fixup function (especially the error message) and
> > replace it by a NULL pointer check, move the Numascale-specific
> > condition for calling the fixup into the fixup-function itself and
> > slightly adapt the comment.
> > 
> > Cc: <stable@kernel.org>
> > Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> > ---
> >  arch/x86/include/asm/x86_init.h      |    1 -
> >  arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
> >  arch/x86/kernel/cpu/amd.c            |    7 ++++---
> >  arch/x86/kernel/cpu/common.c         |    9 ---------
> >  arch/x86/kernel/x86_init.c           |    1 -
> >  5 files changed, 9 insertions(+), 16 deletions(-)
> > 
> > This patch didn't make it into v3.3.
> > But the misleading error message introduced with numachip support was
> > merged.
> 
> Yes, please apply this one, I still get the following on my box with
> 3.4-rc1+:
> 
> [    0.382396] Booting Node   0, Processors  #1 #2 #3 #4 #5 Ok.
> [    0.454471] Booting Node   1, Processors  #6
> [    0.469600] NUMA core number 1 differs from configured core number 0
> [    0.478361]  #7
> [    0.490949] NUMA core number 1 differs from configured core number 0
> [    0.499695]  #8
> [    0.512577] NUMA core number 1 differs from configured core number 0
> [    0.521331]  #9
> [    0.533921] NUMA core number 1 differs from configured core number 0
> [    0.542679]  #10
> [    0.555340] NUMA core number 1 differs from configured core number 0
> [    0.564088]  #11
> [    0.576730] NUMA core number 1 differs from configured core number 0

Still happens on -rc3. Ingo, can we please merge the above fix before 3.4
is out?

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

[tip:x86/urgent] x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

[tip-bot for Andreas Herrmann]

Commit-ID:  68894632afb2729a1d8785c877840953894c7283
Gitweb:     http://git.kernel.org/tip/68894632afb2729a1d8785c877840953894c7283
Author:     Andreas Herrmann <andreas.herrmann3@amd.com>
AuthorDate: Mon, 2 Apr 2012 18:06:48 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 16 Apr 2012 20:43:43 +0200

x86/platform: Remove incorrect error message in x86_default_fixup_cpu_id()

It's only called from amd.c:srat_detect_node(). The introduced
condition for calling the fixup code is true for all AMD
multi-node processors, e.g. Magny-Cours and Interlagos. There we
have 2 NUMA nodes on one socket. Thus there are cores having
different numa-node-id but with equal phys_proc_id.

There is no point to print error messages in such a situation.

The confusing/misleading error message was introduced with
commit 64be4c1c2428e148de6081af235e2418e6a66dda ("x86: Add
x86_init platform override to fix up NUMA core numbering").

Remove the default fixup function (especially the error message)
and replace it by a NULL pointer check, move the
Numascale-specific condition for calling the fixup into the
fixup-function itself and slightly adapt the comment.

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
Acked-by: Borislav Petkov <borislav.petkov@amd.com>
Cc: <stable@kernel.org>
Cc: <sp@numascale.com>
Cc: <bp@amd64.org>
Cc: <daniel@numascale-asia.com>
Link: http://lkml.kernel.org/r/20120402160648.GR27684@alberich.amd.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/x86_init.h      |    1 -
 arch/x86/kernel/apic/apic_numachip.c |    7 +++++--
 arch/x86/kernel/cpu/amd.c            |    7 ++++---
 arch/x86/kernel/cpu/common.c         |    9 ---------
 arch/x86/kernel/x86_init.c           |    1 -
 5 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index baaca8d..764b66a 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -195,6 +195,5 @@ extern struct x86_msi_ops x86_msi;
 
 extern void x86_init_noop(void);
 extern void x86_init_uint_noop(unsigned int unused);
-extern void x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node);
 
 #endif
diff --git a/arch/x86/kernel/apic/apic_numachip.c b/arch/x86/kernel/apic/apic_numachip.c
index 899803e..23e7542 100644
--- a/arch/x86/kernel/apic/apic_numachip.c
+++ b/arch/x86/kernel/apic/apic_numachip.c
@@ -207,8 +207,11 @@ static void __init map_csrs(void)
 
 static void fixup_cpu_id(struct cpuinfo_x86 *c, int node)
 {
-	c->phys_proc_id = node;
-	per_cpu(cpu_llc_id, smp_processor_id()) = node;
+
+	if (c->phys_proc_id != node) {
+		c->phys_proc_id = node;
+		per_cpu(cpu_llc_id, smp_processor_id()) = node;
+	}
 }
 
 static int __init numachip_system_init(void)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 1248f9c..1c67ca1 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -353,10 +353,11 @@ static void __cpuinit srat_detect_node(struct cpuinfo_x86 *c)
 		node = per_cpu(cpu_llc_id, cpu);
 
 	/*
-	 * If core numbers are inconsistent, it's likely a multi-fabric platform,
-	 * so invoke platform-specific handler
+	 * On multi-fabric platform (e.g. Numascale NumaChip) a
+	 * platform-specific handler needs to be called to fixup some
+	 * IDs of the CPU.
 	 */
-	if (c->phys_proc_id != node)
+	if (x86_cpuinit.fixup_cpu_id)
 		x86_cpuinit.fixup_cpu_id(c, node);
 
 	if (!node_online(node)) {
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 67e2583..cf79302 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1163,15 +1163,6 @@ static void dbg_restore_debug_regs(void)
 #endif /* ! CONFIG_KGDB */
 
 /*
- * Prints an error where the NUMA and configured core-number mismatch and the
- * platform didn't override this to fix it up
- */
-void __cpuinit x86_default_fixup_cpu_id(struct cpuinfo_x86 *c, int node)
-{
-	pr_err("NUMA core number %d differs from configured core number %d\n", node, c->phys_proc_id);
-}
-
-/*
  * cpu_init() initializes state that is per-CPU. Some data is already
  * initialized (naturally) in the bootstrap process, such as the GDT
  * and IDT. We reload them nevertheless, this function acts as a
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index e9f265f..9cf71d0 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -93,7 +93,6 @@ struct x86_init_ops x86_init __initdata = {
 struct x86_cpuinit_ops x86_cpuinit __cpuinitdata = {
 	.early_percpu_clock_init	= x86_init_noop,
 	.setup_percpu_clockev		= setup_secondary_APIC_clock,
-	.fixup_cpu_id			= x86_default_fixup_cpu_id,
 };
 
 static void default_nmi_init(void) { };