[PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Jack Miller]

After a call to firmware SwitchBSP(), Linux can be booted with a thread
that isn't the first in the system. That thread automatically becomes
CPU 0.

Currently get_smca_bank_info() queries CPU 0's MCA types, but if CPU 0
!= hardware thread 0, it will get an incomplete list of MCA types in
smca_banks.

This causes get_name() to return NULL when initing hardware thread 0's
additional types, and then the following error when creating the bank
kobj in threshold_create_bank():

[    1.171552] kobject: can not set name properly!
[    1.171569] kobject_create_and_add: kobject_add error: -12

This error path isn't correctly handled. threshold_init_device() fails,
but later if a thread is offlined, threshold_remove_bank() causes a BUG:

[   67.491772] BUG: unable to handle kernel NULL pointer dereference at           (null)
[   67.491781] IP: mce_threshold_remove_device.part.7+0x82/0x2c0

because per_cpu(threshold_banks, cpu) is unexpectedly NULL.

This patch fixes get_smca_bank_info() to query hardware thread 0, not
necessarily CPU 0, to get a full set of MCA types.

I'm uncertain that reading the APIC ID is correct here, and this will
fail if there is AMD hardware where hardware thread 0's APIC ID != 0,
but the other topology/CPUID based functions don't seem to easily
differentiate CPU 0 and thread 0 or possibly aren't inited at this
point. Suggestions for a better mechanism welcome.

Signed-off-by: Jack Miller <jack@codezen.org>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 6e4a047e4b68..9d74adcf34d2 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -170,8 +170,8 @@ static void get_smca_bank_info(unsigned int bank)
 	struct smca_hwid *s_hwid;
 	u32 high, instance_id;
 
-	/* Collect bank_info using CPU 0 for now. */
-	if (cpu)
+	/* Collect bank_info using hardware thread 0 for now. */
+	if (apic->get_apic_id(apic->read(APIC_ID)) != 0)
 		return;
 
 	if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_IPID(bank), &instance_id, &high)) {
-- 
2.13.2

Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Borislav Petkov]

On Tue, Jun 27, 2017 at 07:06:30PM -0500, Jack Miller wrote:
> After a call to firmware SwitchBSP(),

What is that and who does that?

> Linux can be booted with a thread
> that isn't the first in the system. That thread automatically becomes
> CPU 0.

Btw, you should be seeing other explosions too as a lot of code assumes
CPU 0 is the BSP.

...

> Signed-off-by: Jack Miller <jack@codezen.org>
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index 6e4a047e4b68..9d74adcf34d2 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -170,8 +170,8 @@ static void get_smca_bank_info(unsigned int bank)
>  	struct smca_hwid *s_hwid;
>  	u32 high, instance_id;
>  
> -	/* Collect bank_info using CPU 0 for now. */
> -	if (cpu)
> +	/* Collect bank_info using hardware thread 0 for now. */
> +	if (apic->get_apic_id(apic->read(APIC_ID)) != 0)

Does

	if (cpu != boot_cpu_data.cpu_index)
		return;

work?


-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Jack Miller]

On Wed, Jun 28, 2017 at 4:22 AM, Borislav Petkov <bp@suse.de> wrote:
> On Tue, Jun 27, 2017 at 07:06:30PM -0500, Jack Miller wrote:
>> After a call to firmware SwitchBSP(),
>
> What is that and who does that?

SwitchBSP() is part of the UEFI MPServices Protocol which I believe is
an extension but it is supported by all of the firmwares I've tested
on.

In this case, I'm using a bootloader to SwitchBSP() so that hardware
thread 0 (and thus core 0) can be offlined on AMD hardware
(cpu0_hotplug unsupported). This is currently working by passing
'nomce' to the kernel, but obviously I'd prefer not to disable it.

>
>> Linux can be booted with a thread
>> that isn't the first in the system. That thread automatically becomes
>> CPU 0.
>
> Btw, you should be seeing other explosions too as a lot of code assumes
> CPU 0 is the BSP.

Actually, with 'nomce' or this patch applied the system seems to chug
along merrily, no further errors in dmesg, no further BUGs. Linux
still gets all of the topology correct (i.e. CPU 0's
core/thread/siblings are correctly identified) so really, aside from
userspace programs doing naive stuff with CPU affinity (like expecting
even,odd CPUs to be SMT pairs), I think the overall result here is
that most threads are interchangeable... except when probing certain
features like these MCA types.

>
> ...
>
>> Signed-off-by: Jack Miller <jack@codezen.org>
>> ---
>>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
>> index 6e4a047e4b68..9d74adcf34d2 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
>> @@ -170,8 +170,8 @@ static void get_smca_bank_info(unsigned int bank)
>>       struct smca_hwid *s_hwid;
>>       u32 high, instance_id;
>>
>> -     /* Collect bank_info using CPU 0 for now. */
>> -     if (cpu)
>> +     /* Collect bank_info using hardware thread 0 for now. */
>> +     if (apic->get_apic_id(apic->read(APIC_ID)) != 0)
>
> Does
>
>         if (cpu != boot_cpu_data.cpu_index)
>                 return;
>
> work?

Unfortunately, it doesn't. That value is explicitly set to 0. Most
mechanisms operate around CPU #, which isn't very helpful if the BSP
was changed under the covers.

Alternatively, we could possibly sidestep the APIC ID uncertainty by
patching get_smca_bank_info() to fallback on reading the bank
hwid_mcatype from other online CPUs (it's already using
rdmsr_safe_on_cpu) if its own hwid_mcatype isn't valid/recognized, but
that's a more invasive patch.

Jack

RE: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Ghannam, Yazen]

> -----Original Message-----
> From: themoken@gmail.com [mailto:themoken@gmail.com] On Behalf Of
> Jack Miller
> Sent: Wednesday, June 28, 2017 1:44 PM
> To: Borislav Petkov <bp@suse.de>
> Cc: Jack Miller <jack@codezen.org>; linux-kernel@vger.kernel.org;
> tglx@linutronix.de; Ghannam, Yazen <Yazen.Ghannam@amd.com>;
> x86@kernel.org
> Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 !=
> thread 0
> 
> On Wed, Jun 28, 2017 at 4:22 AM, Borislav Petkov <bp@suse.de> wrote:
> > On Tue, Jun 27, 2017 at 07:06:30PM -0500, Jack Miller wrote:
> >> After a call to firmware SwitchBSP(),
> >
> > What is that and who does that?
> 
> SwitchBSP() is part of the UEFI MPServices Protocol which I believe is an
> extension but it is supported by all of the firmwares I've tested on.
> 
> In this case, I'm using a bootloader to SwitchBSP() so that hardware thread 0
> (and thus core 0) can be offlined on AMD hardware (cpu0_hotplug
> unsupported). This is currently working by passing 'nomce' to the kernel, but
> obviously I'd prefer not to disable it.
> 

Which core are you using as the BSP with SwitchBSP()?

> >
> >> Linux can be booted with a thread
> >> that isn't the first in the system. That thread automatically becomes
> >> CPU 0.
> >
> > Btw, you should be seeing other explosions too as a lot of code
> > assumes CPU 0 is the BSP.
> 
> Actually, with 'nomce' or this patch applied the system seems to chug along
> merrily, no further errors in dmesg, no further BUGs. Linux still gets all of the
> topology correct (i.e. CPU 0's core/thread/siblings are correctly identified) so
> really, aside from userspace programs doing naive stuff with CPU affinity (like
> expecting even,odd CPUs to be SMT pairs), I think the overall result here is
> that most threads are interchangeable... except when probing certain
> features like these MCA types.
> 

Do you see 23 banks named in the new BSP's /sys/devices/system/machinecheck/
folder? You should see non-core banks like l3_cache, umc, etc.

Thanks,
Yazen

Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Borislav Petkov]

On Wed, Jun 28, 2017 at 12:44:17PM -0500, Jack Miller wrote:
> SwitchBSP() is part of the UEFI MPServices Protocol which I believe is
> an extension but it is supported by all of the firmwares I've tested
> on.

Damn, that ubiquitous firmware. One day the kernel will be just a
userspace process to the fw.

> In this case, I'm using a bootloader to SwitchBSP() so that hardware
> thread 0 (and thus core 0) can be offlined on AMD hardware
> (cpu0_hotplug unsupported).

Why unsupported?

I remember doing some quick experiments with booting with "cpu0_hotplug"
and being able to offline the BSP. It was a long time ago though.

> This is currently working by passing 'nomce' to the kernel, but
> obviously I'd prefer not to disable it.

Right, nomce is not an optimal setting.

> Actually, with 'nomce' or this patch applied the system seems to chug
> along merrily, no further errors in dmesg, no further BUGs. Linux
> still gets all of the topology correct (i.e. CPU 0's
> core/thread/siblings are correctly identified) so really, aside from
> userspace programs doing naive stuff with CPU affinity (like expecting
> even,odd CPUs to be SMT pairs), I think the overall result here is
> that most threads are interchangeable... except when probing certain
> features like these MCA types.

May I ask what your goal is? Or is it sekrit stuff? physical hotplug
maybe?

> Unfortunately, it doesn't. That value is explicitly set to 0.

Yeah, I see it in smp_store_boot_cpu_info().

So if we had to be really correct, that code there should set the
*actual* CPU index of the BSP and not simply write a 0. It's that

	BSP index == 0

assumption I've been talking about.

> Most mechanisms operate around CPU #, which isn't very helpful if the
> BSP was changed under the covers.
>
> Alternatively, we could possibly sidestep the APIC ID uncertainty by
> patching get_smca_bank_info() to fallback on reading the bank
> hwid_mcatype from other online CPUs (it's already using
> rdmsr_safe_on_cpu) if its own hwid_mcatype isn't valid/recognized, but
> that's a more invasive patch.

Yeah, I think there is some distinction whether you read the MSRs on the
BSP and on the other threads. Yazen did that in

  5896820e0aa3 ("x86/mce/AMD, EDAC/mce_amd: Define and use tables for known SMCA IP types")

Yazen, why CPU 0? Can we get rid of that check there?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

RE: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@suse.de]
> Sent: Wednesday, June 28, 2017 2:17 PM
> To: Jack Miller <jack@codezen.org>; Ghannam, Yazen
> <Yazen.Ghannam@amd.com>
> Cc: linux-kernel@vger.kernel.org; tglx@linutronix.de; x86@kernel.org
> Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 !=
> thread 0
> 
> On Wed, Jun 28, 2017 at 12:44:17PM -0500, Jack Miller wrote:
> > SwitchBSP() is part of the UEFI MPServices Protocol which I believe is
> > an extension but it is supported by all of the firmwares I've tested
> > on.
> 
> Damn, that ubiquitous firmware. One day the kernel will be just a userspace
> process to the fw.
> 
> > In this case, I'm using a bootloader to SwitchBSP() so that hardware
> > thread 0 (and thus core 0) can be offlined on AMD hardware
> > (cpu0_hotplug unsupported).
> 
> Why unsupported?
> 
> I remember doing some quick experiments with booting with "cpu0_hotplug"
> and being able to offline the BSP. It was a long time ago though.
> 
> > This is currently working by passing 'nomce' to the kernel, but
> > obviously I'd prefer not to disable it.
> 
> Right, nomce is not an optimal setting.
> 
> > Actually, with 'nomce' or this patch applied the system seems to chug
> > along merrily, no further errors in dmesg, no further BUGs. Linux
> > still gets all of the topology correct (i.e. CPU 0's
> > core/thread/siblings are correctly identified) so really, aside from
> > userspace programs doing naive stuff with CPU affinity (like expecting
> > even,odd CPUs to be SMT pairs), I think the overall result here is
> > that most threads are interchangeable... except when probing certain
> > features like these MCA types.
> 
> May I ask what your goal is? Or is it sekrit stuff? physical hotplug maybe?
> 
> > Unfortunately, it doesn't. That value is explicitly set to 0.
> 
> Yeah, I see it in smp_store_boot_cpu_info().
> 
> So if we had to be really correct, that code there should set the
> *actual* CPU index of the BSP and not simply write a 0. It's that
> 
> 	BSP index == 0
> 
> assumption I've been talking about.
> 
> > Most mechanisms operate around CPU #, which isn't very helpful if the
> > BSP was changed under the covers.
> >
> > Alternatively, we could possibly sidestep the APIC ID uncertainty by
> > patching get_smca_bank_info() to fallback on reading the bank
> > hwid_mcatype from other online CPUs (it's already using
> > rdmsr_safe_on_cpu) if its own hwid_mcatype isn't valid/recognized, but
> > that's a more invasive patch.
> 
> Yeah, I think there is some distinction whether you read the MSRs on the BSP
> and on the other threads. Yazen did that in
> 
>   5896820e0aa3 ("x86/mce/AMD, EDAC/mce_amd: Define and use tables for
> known SMCA IP types")
> 
> Yazen, why CPU 0? Can we get rid of that check there?
> 

The non-core MCA banks are only visible to a "master thread" on each Die. The
master thread is the first one on the Die. Since we have the same banks on each
Die we only need to read them once, and I assumed that CPU0 would always be
there.

One thing that I'm concerned about here is if the BSP is changed from CPU0, will
the non-core banks on Die0 be configured? I don't think they will be if they're
not visible to the new BSP. Unless FW does something to make the new BSP the
master thread.

We can get rid of the check, but the SMCA banks array is shared by the entire
system. So we'll need a per_cpu sysfs_id for each bank or we'll end up with
"unfriendly" names. I'll try to find a fix for this.

Please see here:
0b737a9c2af8 x86/ras/amd: Make sysfs names of banks more user-friendly

Thanks,
Yazen

Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Jack Miller]

On Wed, Jun 28, 2017 at 1:00 PM, Ghannam, Yazen <Yazen.Ghannam@amd.com> wrote:
>> -----Original Message-----
>> From: themoken@gmail.com [mailto:themoken@gmail.com] On Behalf Of
>> Jack Miller
>> Sent: Wednesday, June 28, 2017 1:44 PM
>> To: Borislav Petkov <bp@suse.de>
>> Cc: Jack Miller <jack@codezen.org>; linux-kernel@vger.kernel.org;
>> tglx@linutronix.de; Ghannam, Yazen <Yazen.Ghannam@amd.com>;
>> x86@kernel.org
>> Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 !=
>> thread 0
>>
>> On Wed, Jun 28, 2017 at 4:22 AM, Borislav Petkov <bp@suse.de> wrote:
>> > On Tue, Jun 27, 2017 at 07:06:30PM -0500, Jack Miller wrote:
>> >> After a call to firmware SwitchBSP(),
>> >
>> > What is that and who does that?
>>
>> SwitchBSP() is part of the UEFI MPServices Protocol which I believe is an
>> extension but it is supported by all of the firmwares I've tested on.
>>
>> In this case, I'm using a bootloader to SwitchBSP() so that hardware thread 0
>> (and thus core 0) can be offlined on AMD hardware (cpu0_hotplug
>> unsupported). This is currently working by passing 'nomce' to the kernel, but
>> obviously I'd prefer not to disable it.
>>
>
> Which core are you using as the BSP with SwitchBSP()?

Core 4, hardware thread 8 overall. I am testing on a Ryzen 7 machine.

>
>> >
>> >> Linux can be booted with a thread
>> >> that isn't the first in the system. That thread automatically becomes
>> >> CPU 0.
>> >
>> > Btw, you should be seeing other explosions too as a lot of code
>> > assumes CPU 0 is the BSP.
>>
>> Actually, with 'nomce' or this patch applied the system seems to chug along
>> merrily, no further errors in dmesg, no further BUGs. Linux still gets all of the
>> topology correct (i.e. CPU 0's core/thread/siblings are correctly identified) so
>> really, aside from userspace programs doing naive stuff with CPU affinity (like
>> expecting even,odd CPUs to be SMT pairs), I think the overall result here is
>> that most threads are interchangeable... except when probing certain
>> features like these MCA types.
>>
>
> Do you see 23 banks named in the new BSP's /sys/devices/system/machinecheck/
> folder? You should see non-core banks like l3_cache, umc, etc.

With my patch applied, I see entries like l3_cache under hardware
thread 0's directory (it's shifted to CPU 1, so machinecheck1).
Without my patch, only machinecheck0 has anything interesting in it
(insn_fetch, l2_cache etc.) because the init failed on CPU 1.

Jack

Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Borislav Petkov]

On Wed, Jun 28, 2017 at 06:51:08PM +0000, Ghannam, Yazen wrote:
> The non-core MCA banks are only visible to a "master thread" on each Die. The
> master thread is the first one on the Die. Since we have the same banks on each
> Die we only need to read them once, and I assumed that CPU0 would always be
> there.

Right, I believe this is similar to the node-base core thing. Or
something to that effect.

So is there a reliable way to find out which is the node-base core, or
the master thread? Because regardless what fw does, it should still
configure such master thread if the BSP is a different thread.

So if our code would be able to find that out, we should be good.

 [ Practically, a sane fw would simply go and make the new BSP also the master
   thread. But "sane" and "fw" in a sentence don't work. :-) ]

Thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

RE: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Ghannam, Yazen]

> -----Original Message-----
> From: themoken@gmail.com [mailto:themoken@gmail.com] On Behalf Of
> Jack Miller
> Sent: Wednesday, June 28, 2017 2:53 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: Jack Miller <jack@codezen.org>; Borislav Petkov <bp@suse.de>; linux-
> kernel@vger.kernel.org; tglx@linutronix.de; x86@kernel.org
> Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 !=
> thread 0
> 
> On Wed, Jun 28, 2017 at 1:00 PM, Ghannam, Yazen
> <Yazen.Ghannam@amd.com> wrote:
> >> -----Original Message-----
> >> From: themoken@gmail.com [mailto:themoken@gmail.com] On Behalf Of
> >> Jack Miller
> >> Sent: Wednesday, June 28, 2017 1:44 PM
> >> To: Borislav Petkov <bp@suse.de>
> >> Cc: Jack Miller <jack@codezen.org>; linux-kernel@vger.kernel.org;
> >> tglx@linutronix.de; Ghannam, Yazen <Yazen.Ghannam@amd.com>;
> >> x86@kernel.org
> >> Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU
> >> 0 != thread 0
> >>
> >> On Wed, Jun 28, 2017 at 4:22 AM, Borislav Petkov <bp@suse.de> wrote:
> >> > On Tue, Jun 27, 2017 at 07:06:30PM -0500, Jack Miller wrote:
> >> >> After a call to firmware SwitchBSP(),
> >> >
> >> > What is that and who does that?
> >>
> >> SwitchBSP() is part of the UEFI MPServices Protocol which I believe
> >> is an extension but it is supported by all of the firmwares I've tested on.
> >>
> >> In this case, I'm using a bootloader to SwitchBSP() so that hardware
> >> thread 0 (and thus core 0) can be offlined on AMD hardware
> >> (cpu0_hotplug unsupported). This is currently working by passing
> >> 'nomce' to the kernel, but obviously I'd prefer not to disable it.
> >>
> >
> > Which core are you using as the BSP with SwitchBSP()?
> 
> Core 4, hardware thread 8 overall. I am testing on a Ryzen 7 machine.
> 
> >
> >> >
> >> >> Linux can be booted with a thread
> >> >> that isn't the first in the system. That thread automatically
> >> >> becomes CPU 0.
> >> >
> >> > Btw, you should be seeing other explosions too as a lot of code
> >> > assumes CPU 0 is the BSP.
> >>
> >> Actually, with 'nomce' or this patch applied the system seems to chug
> >> along merrily, no further errors in dmesg, no further BUGs. Linux
> >> still gets all of the topology correct (i.e. CPU 0's
> >> core/thread/siblings are correctly identified) so really, aside from
> >> userspace programs doing naive stuff with CPU affinity (like
> >> expecting even,odd CPUs to be SMT pairs), I think the overall result
> >> here is that most threads are interchangeable... except when probing
> certain features like these MCA types.
> >>
> >
> > Do you see 23 banks named in the new BSP's
> > /sys/devices/system/machinecheck/ folder? You should see non-core banks
> like l3_cache, umc, etc.
> 
> With my patch applied, I see entries like l3_cache under hardware thread 0's
> directory (it's shifted to CPU 1, so machinecheck1).
> Without my patch, only machinecheck0 has anything interesting in it
> (insn_fetch, l2_cache etc.) because the init failed on CPU 1.
> 

What happens with SMT off?

Thanks,
Yazen

Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Jack Miller]

On Wed, Jun 28, 2017 at 1:58 PM, Ghannam, Yazen <Yazen.Ghannam@amd.com> wrote:
>> With my patch applied, I see entries like l3_cache under hardware thread 0's
>> directory (it's shifted to CPU 1, so machinecheck1).
>> Without my patch, only machinecheck0 has anything interesting in it
>> (insn_fetch, l2_cache etc.) because the init failed on CPU 1.
>>
>
> What happens with SMT off?

I haven't been able to test with SMT off (since it's apparent that
'nosmt' doesn't really do anything and I don't locally have a firmware
option to turn it off).

First things first though, like Boris I'd like to know if there's a
better way to detect this master thread, other than by APIC ID. Right
now I'm working on a v2 that will remove the CPU check, let each one
perform the rdmsr and only update empty bank info. I believe this call
is being serialized elsewhere (need to check), but if I could keep
this patch to a one-liner by detecting the right thread, I'd like to.

Jack

RE: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0

[Ghannam, Yazen]

> -----Original Message-----
> From: themoken@gmail.com [mailto:themoken@gmail.com] On Behalf Of
> Jack Miller
> Sent: Thursday, June 29, 2017 12:23 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: Jack Miller <jack@codezen.org>; Borislav Petkov <bp@suse.de>; linux-
> kernel@vger.kernel.org; tglx@linutronix.de; x86@kernel.org
> Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 !=
> thread 0
> 
> On Wed, Jun 28, 2017 at 1:58 PM, Ghannam, Yazen
> <Yazen.Ghannam@amd.com> wrote:
> >> With my patch applied, I see entries like l3_cache under hardware
> >> thread 0's directory (it's shifted to CPU 1, so machinecheck1).
> >> Without my patch, only machinecheck0 has anything interesting in it
> >> (insn_fetch, l2_cache etc.) because the init failed on CPU 1.
> >>
> >
> > What happens with SMT off?
> 
> I haven't been able to test with SMT off (since it's apparent that 'nosmt'
> doesn't really do anything and I don't locally have a firmware option to turn it
> off).
> 
> First things first though, like Boris I'd like to know if there's a better way to
> detect this master thread, other than by APIC ID. Right now I'm working on a
> v2 that will remove the CPU check, let each one perform the rdmsr and only
> update empty bank info. I believe this call is being serialized elsewhere (need
> to check), but if I could keep this patch to a one-liner by detecting the right
> thread, I'd like to.
> 

There is a master thread per Die, so a multi-Die system will have multiple master
threads. In which case, we still need to decide which master thread will populate
the array.

I have a solution that doesn't rely on using a specific thread. I'll send it up shortly.

Thanks,
Yazen

[PATCH] x86/mce/AMD: Allow any CPU to initialize smca_banks array

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

Current SMCA implementations have the same banks on each CPU with the
non-core banks only visible to a "master thread" on each Die. Practically,
this means the smca_banks array, which describes the banks, only needs to
be populated once by a single master thread.

CPU0 seemed like a good candidate to do the populating. However, it's
possible that CPU0 is not enabled in which case the smca_banks array won't
be populated.

Rather than try to figure out another master thread to do the populating,
we should just allow any CPU to populate the array.

Drop the CPU0 check and return early if the bank was already initialized.
Also, drop the WARNing about an already initialized bank, since this will
be a common, expected occurrence.

The smca_banks array is only populated at boot time and CPUs are brought
online sequentially. So there's no need for locking around the array.

If the first CPU up is a master thread, then it will populate the array
with all banks, core and non-core. Every CPU afterwards will return early.

If the first CPU up is not a master thread, then it will populate the array
with all core banks. The first CPU afterwards that is a master thread will
skip populating the core banks and continue populating the non-core banks.
Every CPU afterwards will then return early.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index b41466723f71..37dc43f820b4 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -214,8 +214,8 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 		wrmsr(smca_config, low, high);
 	}
 
-	/* Collect bank_info using CPU 0 for now. */
-	if (cpu)
+	/* Return early if this bank was already initialized. */
+	if (smca_banks[bank].hwid)
 		return;
 
 	if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_IPID(bank), &low, &high)) {
@@ -229,11 +229,6 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
 	for (i = 0; i < ARRAY_SIZE(smca_hwid_mcatypes); i++) {
 		s_hwid = &smca_hwid_mcatypes[i];
 		if (hwid_mcatype == s_hwid->hwid_mcatype) {
-
-			WARN(smca_banks[bank].hwid,
-			     "Bank %s already initialized!\n",
-			     smca_get_name(s_hwid->bank_type));
-
 			smca_banks[bank].hwid = s_hwid;
 			smca_banks[bank].id = low;
 			smca_banks[bank].sysfs_id = s_hwid->count++;
-- 
2.7.4

Re: [PATCH] x86/mce/AMD: Allow any CPU to initialize smca_banks array

[Jack Miller]

This patch works for me, thanks!

Jack

On Thu, Jun 29, 2017 at 1:08 PM, Yazen Ghannam <Yazen.Ghannam@amd.com> wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
>
> Current SMCA implementations have the same banks on each CPU with the
> non-core banks only visible to a "master thread" on each Die. Practically,
> this means the smca_banks array, which describes the banks, only needs to
> be populated once by a single master thread.
>
> CPU0 seemed like a good candidate to do the populating. However, it's
> possible that CPU0 is not enabled in which case the smca_banks array won't
> be populated.
>
> Rather than try to figure out another master thread to do the populating,
> we should just allow any CPU to populate the array.
>
> Drop the CPU0 check and return early if the bank was already initialized.
> Also, drop the WARNing about an already initialized bank, since this will
> be a common, expected occurrence.
>
> The smca_banks array is only populated at boot time and CPUs are brought
> online sequentially. So there's no need for locking around the array.
>
> If the first CPU up is a master thread, then it will populate the array
> with all banks, core and non-core. Every CPU afterwards will return early.
>
> If the first CPU up is not a master thread, then it will populate the array
> with all core banks. The first CPU afterwards that is a master thread will
> skip populating the core banks and continue populating the non-core banks.
> Every CPU afterwards will then return early.
>
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> index b41466723f71..37dc43f820b4 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
> @@ -214,8 +214,8 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
>                 wrmsr(smca_config, low, high);
>         }
>
> -       /* Collect bank_info using CPU 0 for now. */
> -       if (cpu)
> +       /* Return early if this bank was already initialized. */
> +       if (smca_banks[bank].hwid)
>                 return;
>
>         if (rdmsr_safe_on_cpu(cpu, MSR_AMD64_SMCA_MCx_IPID(bank), &low, &high)) {
> @@ -229,11 +229,6 @@ static void smca_configure(unsigned int bank, unsigned int cpu)
>         for (i = 0; i < ARRAY_SIZE(smca_hwid_mcatypes); i++) {
>                 s_hwid = &smca_hwid_mcatypes[i];
>                 if (hwid_mcatype == s_hwid->hwid_mcatype) {
> -
> -                       WARN(smca_banks[bank].hwid,
> -                            "Bank %s already initialized!\n",
> -                            smca_get_name(s_hwid->bank_type));
> -
>                         smca_banks[bank].hwid = s_hwid;
>                         smca_banks[bank].id = low;
>                         smca_banks[bank].sysfs_id = s_hwid->count++;
> --
> 2.7.4
>

Re: [PATCH] x86/mce/AMD: Allow any CPU to initialize smca_banks array

[Borislav Petkov]

On Thu, Jun 29, 2017 at 01:08:28PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> Current SMCA implementations have the same banks on each CPU with the
> non-core banks only visible to a "master thread" on each Die. Practically,
> this means the smca_banks array, which describes the banks, only needs to
> be populated once by a single master thread.

...

> If the first CPU up is not a master thread, then it will populate the array
> with all core banks. The first CPU afterwards that is a master thread will
> skip populating the core banks and continue populating the non-core banks.
> Every CPU afterwards will then return early.
> 
> Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce_amd.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--