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

[Smita Koralahalli Channabasappa]

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,

>