Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Smita Koralahalli Channabasappa <skoralah@amd.com>]

On 11/6/20 6:09 AM, Borislav Petkov wrote:

> On Tue, Nov 03, 2020 at 10:49:52AM -0600, Smita Koralahalli wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
>> index af8d37962586..f56f0bc147e2 100644
>> --- a/arch/x86/kernel/cpu/mce/apei.c
>> +++ b/arch/x86/kernel/cpu/mce/apei.c
>> @@ -51,6 +51,62 @@ void apei_mce_report_mem_error(int severity, struct cper_sec_mem_err *mem_err)
>>   }
>>   EXPORT_SYMBOL_GPL(apei_mce_report_mem_error);
>>   
>> +int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
>> +{
>> +	const u64 *i_mce = ((const u64 *) (ctx_info + 1));
>> +	unsigned int cpu;
>> +	struct mce m;
>> +
>> +	if (!boot_cpu_has(X86_FEATURE_SMCA))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * The starting address of the Register Array extracted from BERT
>> +	 * must match with the first expected register in the register
>> +	 * layout of MCAX address space. In SMCA systems this address
>> +	 * corresponds to banks's MCA_STATUS register.
> So which is it "MCAX" or "SMCA"? They both denote the same thing but
> let's stick to one to avoid unnecessary confusion. I'm guessing to
> "SMCA" because it is more wide-spread in the kernel...

Okay I will change it to SMCA.

>> +	 *
>> +	 * The Register array size must be large enough to include all
>> +	 * the SMCA registers which we want to extract.
>> +	 *
>> +	 * The number of registers in the Register Array is determined
>> +	 * by Register Array Size/8 as defined in UEFI spec v2.8, sec
>> +	 * N.2.4.2.2. The register layout is fixed and currently the raw
>> +	 * data in the register array includes 6 SMCA registers which the
>> +	 * kernel can extract.
>> +	 */
>> +
>> +	if ((ctx_info->msr_addr & MSR_AMD64_SMCA_MC0_STATUS) !=
>> +	    MSR_AMD64_SMCA_MC0_STATUS || ctx_info->reg_arr_size < 48)
>> +		return -EINVAL;
> Split that if in two consecutive if-statements.

Okay.

>
> Also, why the ANDing and not simply do:
>
> 	if (ctx_info->msr_addr == MSR_AMD64_SMCA_MC0_STATUS)
>
> ?
>
> I'm guessing you wanna match *all* MCi_STATUS MSRs - not only MC0, yes?
>
> If so, document that with in the comment above it.
>
> Thx.

ANDing with MC0 avoids bank check by turning off the bits corresponding
to the bank number.

For example: MCA_STATUS in SMCA space is 0xC0002YY1 where YY is the bank
number. The bank number is "Dont care" so ANDing with MC0_STATUS
(0xC0002001) should match on any MCi_STATUS MSR.

I will include that in comments above it.

Thanks,

>