| Message ID | 20230622131841.3153672-2-yazen.ghannam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SMCA CPER Fixes | expand |
> All the above is done when the BERT is processed during late init. This > can be scheduled on any CPU, and it may be preemptible. > 2) mce_setup() will pull info from the executing CPU, so some info in > struct mce may be incorrect for the CPU with the error. For example, > in a dual-socket system, an error logged in socket 1 CPU but > processed by a socket 0 CPU will save the PPIN of the socket 0 CPU. > Fix the first issue by locally disabling preemption before calling > mce_setup(). It doesn't really fix the issue, it just makes the warnings go away. The BERT record was created because some error crashed the system. It's being parsed by a CPU that likely had nothing to do with the actual error that occurred in the previous incarnation of the OS. If there is a CPER record in the BERT data that includes CPU information, that would be the right thing to use. Alternatively is there some invalid CPU value that could be loaded into the "struct mce"? -Tony
On 6/22/2023 11:35 AM, Luck, Tony wrote: >> All the above is done when the BERT is processed during late init. This >> can be scheduled on any CPU, and it may be preemptible. > >> 2) mce_setup() will pull info from the executing CPU, so some info in >> struct mce may be incorrect for the CPU with the error. For example, >> in a dual-socket system, an error logged in socket 1 CPU but >> processed by a socket 0 CPU will save the PPIN of the socket 0 CPU. > >> Fix the first issue by locally disabling preemption before calling >> mce_setup(). > > It doesn't really fix the issue, it just makes the warnings go away. > > The BERT record was created because some error crashed the > system. It's being parsed by a CPU that likely had nothing > to do with the actual error that occurred in the previous incarnation > of the OS. > Yes, these are true statements. > If there is a CPER record in the BERT data that includes CPU > information, that would be the right thing to use. Alternatively > is there some invalid CPU value that could be loaded into the > "struct mce"? > This is the reason we search for the logical CPU number using the Local APIC ID provided in the CPER. And fill in relevant data using that CPU number. Thanks, Yazen
> This is the reason we search for the logical CPU number using the Local > APIC ID provided in the CPER. And fill in relevant data using that CPU > number. So you don't care which CPU number mce_setup() used because you are going to update it with the right one from CPER? Then maybe the fix for part 1 is just to use raw_smp_processor_id() instead of smp_processor_id() to avoid the warning for calling with pre-emption enabled, instead of disabling premption with the get_cpu() ... put_cpu() wrap around the call to mce_setup()? -Tony
On 6/22/2023 1:05 PM, Luck, Tony wrote: >> This is the reason we search for the logical CPU number using the Local >> APIC ID provided in the CPER. And fill in relevant data using that CPU >> number. > > So you don't care which CPU number mce_setup() used because you are > going to update it with the right one from CPER? > That's right. > Then maybe the fix for part 1 is just to use raw_smp_processor_id() instead of > smp_processor_id() to avoid the warning for calling with pre-emption enabled, > instead of disabling premption with the get_cpu() ... put_cpu() wrap around the > call to mce_setup()? You mean use raw_smp_processor_id() in mce_setup()? I thought about that, but decided against it. I figure the preemption warning is helpful to catch issues when mce_setup() *is* supposed to run on the current CPU but doesn't. This BERT decoding path is the only exception AFAIK. So I didn't want to change the common code for a single exception. I just noticed a similar potential issue with mce_setup() in apei_mce_report_mem_error(). How is the CPU number decided there? Is it always "don't care", since the mce record is "fake"? Here are another couple of solutions for the preemption issue. 1) Don't use mce_setup() at all. Instead, do the memset(), etc. in the local function. This would result in some code duplication. 2) Split mce_setup() into global and per_cpu parts. The memset(), cpuid, etc. would be global, and the cpu_data()* and rdmsr() would be per_cpu. Option #2 can also be used in apei_mce_report_mem_error(), I think. Thanks, Yazen
> 2) Split mce_setup() into global and per_cpu parts. The memset(), cpuid, > etc. would be global, and the cpu_data()* and rdmsr() would be per_cpu. That sounds good. So global is: memset(m, 0, sizeof(struct mce)); /* need the internal __ version to avoid deadlocks */ m->time = __ktime_get_real_seconds(); m->cpuvendor = boot_cpu_data.x86_vendor; m->mcgcap = __rdmsr(MSR_IA32_MCG_CAP); m->microcode = boot_cpu_data.microcode; m->cpuid = cpuid_eax(1); Though that last one is perhaps per-cpu if you want to allow for mixed-stepping systems. Perhaps m->time also? Questionable whether it is useful to log time this record was created, when it refers to something much earlier in the BERT case. and per-cpu is: m->cpu = m->extcpu = smp_processor_id(); m->socketid = cpu_data(m->extcpu).phys_proc_id; m->apicid = cpu_data(m->extcpu).initial_apicid; m->ppin = cpu_data(m->extcpu).ppin; > Option #2 can also be used in apei_mce_report_mem_error(), I think. Agreed. -Tony
On 6/22/2023 3:42 PM, Luck, Tony wrote: >> 2) Split mce_setup() into global and per_cpu parts. The memset(), cpuid, >> etc. would be global, and the cpu_data()* and rdmsr() would be per_cpu. > > That sounds good. So global is: > > memset(m, 0, sizeof(struct mce)); > /* need the internal __ version to avoid deadlocks */ > m->time = __ktime_get_real_seconds(); > m->cpuvendor = boot_cpu_data.x86_vendor; > m->mcgcap = __rdmsr(MSR_IA32_MCG_CAP); MCG_CAP would be per_cpu, because the bank count can vary. But I don't think this matters in practice. So leaving it global is okay, I think. > m->microcode = boot_cpu_data.microcode; > m->cpuid = cpuid_eax(1); > > Though that last one is perhaps per-cpu if you want to allow for mixed-stepping systems. > Perhaps m->time also? Questionable whether it is useful to log time this record > was created, when it refers to something much earlier in the BERT case. > I agree about m->time. It doesn't seem useful in this case. But I don't know about m->cpuid. Mixing processor revisions is not allowed on AMD systems, and I don't know about other vendors. So I'd leave m->cpuid as global unless there's a strong case otherwise. > and per-cpu is: > > m->cpu = m->extcpu = smp_processor_id(); > m->socketid = cpu_data(m->extcpu).phys_proc_id; > m->apicid = cpu_data(m->extcpu).initial_apicid; > m->ppin = cpu_data(m->extcpu).ppin; > >> Option #2 can also be used in apei_mce_report_mem_error(), I think. > > Agreed. > Okay, I'll update that too. Thanks, Yazen
> But I don't know about m->cpuid. Mixing processor revisions is not > allowed on AMD systems, and I don't know about other vendors. So I'd > leave m->cpuid as global unless there's a strong case otherwise. There is (or was) support for mixed stepping in the microcode update code. Not sure if Boris and Ashok came to any agreement on keeping it. -Tony
On Fri, Jun 23, 2023 at 03:44:06PM +0000, Luck, Tony wrote: > There is (or was) support for mixed stepping in the microcode update > code. Not sure if Boris and Ashok came to any agreement on keeping it. Yap, needs to stay on AMD as the loader has always supported it. Btw, you might wanna update your bookmarks - bp@suse.de doesn't work anymore. :-)
On 6/23/2023 12:01 PM, Borislav Petkov wrote: > On Fri, Jun 23, 2023 at 03:44:06PM +0000, Luck, Tony wrote: >> There is (or was) support for mixed stepping in the microcode update >> code. Not sure if Boris and Ashok came to any agreement on keeping it. > > Yap, needs to stay on AMD as the loader has always supported it. > I don't understand this. Maybe it's a wording thing. I see the following in a PPR document. Section: Mixed Processor Revision Supports AMD Family XXh Models XXh processors with different OPNs or different revisions cannot be mixed in a multiprocessor system. If the BIOS detects an unsupported configuration, the system will halt prior to X86 core release and signal a port 80 error code. Is stepping not included in this statement? Or do you mean that we can support mixed microcode systems? Meaning the processors are identical but with different microcode versions. Thanks, Yazen
On Fri, Jun 23, 2023 at 12:14:00PM -0400, Yazen Ghannam wrote:
> Or do you mean that we can support mixed microcode systems?
We can and always have. I can chase down hw guys at some point and have
them make a definitive statement about mixed silicon steppings but we
have bigger fish to fry right now.
:-)
On 6/23/2023 12:42 PM, Borislav Petkov wrote: > On Fri, Jun 23, 2023 at 12:14:00PM -0400, Yazen Ghannam wrote: >> Or do you mean that we can support mixed microcode systems? > > We can and always have. I can chase down hw guys at some point and have > them make a definitive statement about mixed silicon steppings but we > have bigger fish to fry right now. > > :-) > No problem. I'll work on a solution that covers this case then. Thanks, Yazen
diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c index 8ed341714686..2a7a51ca2995 100644 --- a/arch/x86/kernel/cpu/mce/apei.c +++ b/arch/x86/kernel/cpu/mce/apei.c @@ -97,7 +97,9 @@ int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id) if (ctx_info->reg_arr_size < 48) return -EINVAL; + get_cpu(); mce_setup(&m); + put_cpu(); m.extcpu = -1; m.socketid = -1;
Scalable MCA systems may report errors found during boot-time polling through the ACPI Boot Error Record Table (BERT). The errors are logged in an "x86 Processor" Common Platform Error Record (CPER). The format of the x86 CPER does not include a logical CPU number, but it does provide the logical APIC ID for the logical CPU. Also, it does not explicitly provide MCA error information, but it can share this information using an "MSR Context" defined in the CPER format. The MCA error information is parsed by 1) Checking that the context matches the Scalable MCA register space. 2) Finding the logical CPU that matches the logical APIC ID from the CPER. 3) Filling in struct mce with the relevant data and logging it. All the above is done when the BERT is processed during late init. This can be scheduled on any CPU, and it may be preemptible. This results in two issues. 1) mce_setup() includes a call to smp_processor_id(). This will throw a warning if preemption is enabled. 2) mce_setup() will pull info from the executing CPU, so some info in struct mce may be incorrect for the CPU with the error. For example, in a dual-socket system, an error logged in socket 1 CPU but processed by a socket 0 CPU will save the PPIN of the socket 0 CPU. Fix the first issue by locally disabling preemption before calling mce_setup(). Fixes: 4a24d80b8c3e ("x86/mce, cper: Pass x86 CPER through the MCA handling chain") Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com> Cc: stable@vger.kernel.org --- arch/x86/kernel/cpu/mce/apei.c | 2 ++ 1 file changed, 2 insertions(+)