Messages in this thread | | | Subject | Re: [PATCH v5] cper, apei, mce: Pass x86 CPER through the MCA handling chain | From | Smita Koralahalli Channabasappa <> | Date | Mon, 9 Nov 2020 12:36:54 -0600 |
| |
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,
>
| |