[PATCH 0/2] SMCA CPER Fixes

[PATCH 0/2] SMCA CPER Fixes

[Yazen Ghannam]

Hi all,

This set is a follow up to the patch linked below. It fixes two issues
in SMCA CPER decoding. The original patch attempted to address both
issues, but those changes may cause issues during runtime CPER decoding.

I reused the commit message between the two patches in this set, since
the context is almost identical.

Thanks,
Yazen

Link:
https://lore.kernel.org/r/20230417162006.3292715-1-yazen.ghannam@amd.com

Yazen Ghannam (2):
  x86/mce: Disable preemption for CPER decoding
  x86/mce: Set correct PPIN for CPER decoding

 arch/x86/kernel/cpu/mce/apei.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.34.1

[PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Yazen Ghannam]

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

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;
-- 
2.34.1

RE: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Luck, Tony]

> 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

Re: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Yazen Ghannam]

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

RE: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Luck, Tony]

> 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

Re: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Yazen Ghannam]

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

RE: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Luck, Tony]

> 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

Re: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Yazen Ghannam]

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

RE: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Luck, Tony]

> 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

Re: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Borislav Petkov]

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. :-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Yazen Ghannam]

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

Re: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Borislav Petkov]

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.

:-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] x86/mce: Disable preemption for CPER decoding

[Yazen Ghannam]

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

[PATCH 2/2] x86/mce: Set correct PPIN for CPER decoding

[Yazen Ghannam]

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 second issue by using the PPIN value cached during CPU init.

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

diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
index 2a7a51ca2995..db16dc3c7b03 100644
--- a/arch/x86/kernel/cpu/mce/apei.c
+++ b/arch/x86/kernel/cpu/mce/apei.c
@@ -103,11 +103,13 @@ int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
 
 	m.extcpu = -1;
 	m.socketid = -1;
+	m.ppin = 0;
 
 	for_each_possible_cpu(cpu) {
 		if (cpu_data(cpu).initial_apicid == lapic_id) {
 			m.extcpu = cpu;
 			m.socketid = cpu_data(m.extcpu).phys_proc_id;
+			m.ppin = cpu_data(m.extcpu).ppin;
 			break;
 		}
 	}
-- 
2.34.1