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

PATCH In review Jack Miller 3 participants 14 messages Jun 28, 2017 – Jul 17, 2017

Subsystems (2)

x86 EDAC-core

lore

Share & tools

lore

Jack Miller

author

2017-06-28 00:06 UTC #0 lore raw

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

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 <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -2

index 6e4a047e4b68..9d74adcf34d2 100644
@@ -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)) {

Signature · 1 line

2.13.2

Borislav Petkov

2017-06-28 09:22 UTC #1 lore raw

Quoted 1 line · Jack Miller

After a call to firmware SwitchBSP(),

What is that and who does that?

Quoted 3 lines

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.

...

Quoted 10 lines

Signed-off-by Jack Miller <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
index 6e4a047e4b68..9d74adcf34d2 100644

arch/x86/kernel/cpu/mcheck/mce_amd.c static void get_smca_bank_info(unsigned int bank) · line 174

Quoted 7 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -2

@@ -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?

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Jack Miller

author

2017-06-28 17:44 UTC #2 lore raw

Quoted 4 lines · Borislav Petkov

Quoted 1 line · Jack Miller
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.

Quoted 7 lines

Quoted 4 lines
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.

Quoted 23 lines

...
Quoted 10 lines
Signed-off-by Jack Miller <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0
index 6e4a047e4b68..9d74adcf34d2 100644
arch/x86/kernel/cpu/mcheck/mce_amd.c static void get_smca_bank_info(unsigned int bank) · line 174
Quoted 7 lines
arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -2
@@ -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

Quoted 4 lines

        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

Ghannam, Yazen

2017-06-28 18:00 UTC #3 lore raw

Quoted 17 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Jack Miller
Sent: Wednesday, June 28, 2017 1:44 PM
To: Borislav Petkov <[email protected]>
Cc: Jack Miller <[email protected]>; [email protected]; [email protected]; Ghannam, Yazen <[email protected]>; [email protected]
Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0
Quoted 4 lines · Borislav Petkov
Quoted 1 line · Jack Miller
After a call to firmware SwitchBSP(),
What is that and who does that?

Quoted 8 lines

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()?

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
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.

Quoted 8 lines

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

Jack Miller

author

2017-06-28 18:53 UTC #4 lore raw

Quoted 20 lines · Yazen

Quoted 20 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Jack Miller
Sent: Wednesday, June 28, 2017 1:44 PM
To: Borislav Petkov <[email protected]>
Cc: Jack Miller <[email protected]>; [email protected]; [email protected]; Ghannam, Yazen <[email protected]>; [email protected]
Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0
Quoted 4 lines · Borislav Petkov
Quoted 1 line · Jack Miller
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.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 18 lines

Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
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.
Quoted 8 lines
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.

Quoted 2 lines

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

Ghannam, Yazen

2017-06-28 18:59 UTC #5 lore raw

Quoted 61 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Jack Miller
Sent: Wednesday, June 28, 2017 2:53 PM
To: Ghannam, Yazen <[email protected]>
Cc: Jack Miller <[email protected]>; Borislav Petkov <[email protected]>; linux- [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0
Quoted 20 lines · Yazen
Quoted 20 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Jack Miller
Sent: Wednesday, June 28, 2017 1:44 PM
To: Borislav Petkov <[email protected]>
Cc: Jack Miller <[email protected]>; [email protected]; [email protected]; Ghannam, Yazen <[email protected]>; [email protected]
Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0
Quoted 4 lines · Borislav Petkov
Quoted 1 line · Jack Miller
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 7 lines
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.
Quoted 2 lines
Quoted 1 line

Quoted 9 lines

Quoted 2 lines
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

Jack Miller

author

2017-06-29 16:22 UTC #6 lore raw

Quoted 7 lines · Yazen

Quoted 5 lines
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

Ghannam, Yazen

2017-06-29 17:58 UTC #7 lore raw

Quoted 24 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Jack Miller
Sent: Thursday, June 29, 2017 12:23 PM
To: Ghannam, Yazen <[email protected]>
Cc: Jack Miller <[email protected]>; Borislav Petkov <[email protected]>; linux- [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0
Quoted 7 lines · Yazen
Quoted 5 lines
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).

Quoted 7 lines

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

Borislav Petkov

2017-06-28 18:17 UTC #8 lore raw

In reply to Jack Miller

Quoted 3 lines · Jack Miller

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

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?

Quoted 1 line

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.

Quoted 8 lines

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?

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Ghannam, Yazen

2017-06-28 18:51 UTC #9 lore raw

Quoted 62 lines

-----Original Message-----
From: Borislav Petkov [mailto:[email protected]]
Sent: Wednesday, June 28, 2017 2:17 PM
To: Jack Miller <[email protected]>; Ghannam, Yazen <[email protected]>
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] x86/mce/AMD: Fix partial SMCA bank init when CPU 0 != thread 0
Quoted 3 lines · Jack Miller
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.
Quoted 3 lines
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.
Quoted 2 lines
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.
Quoted 8 lines
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?
Quoted 1 line
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.
Quoted 8 lines
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.

Quoted 8 lines

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

Borislav Petkov

2017-06-28 19:01 UTC #10 lore raw

Quoted 4 lines · Yazen

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.

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Yazen Ghannam

2017-06-29 18:08 UTC #11 lore raw

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

In reply to Jack Miller

From: Yazen Ghannam <[email protected]>

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 <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -7

index b41466723f71..37dc43f820b4 100644
@@ -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++;

Jack Miller

author

2017-06-30 15:57 UTC #12 lore raw

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

This patch works for me, thanks!

Jack

Quoted 62 lines · Yazen Ghannam

From: Yazen Ghannam <[email protected]>
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 <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)
arch/x86/kernel/cpu/mcheck/mce_amd.c +2 -8
index b41466723f71..37dc43f820b4 100644
@@ -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++;
--

Quoted 2 lines

2.7.4

---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Borislav Petkov

Applied

2017-07-17 05:19 UTC #13 lore raw

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

In reply to Yazen Ghannam

Quoted 6 lines · Yazen Ghannam

From: Yazen Ghannam <[email protected]>
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.

...

Quoted 9 lines

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 <[email protected]>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Applied, thanks.