| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 23 Sep 2020 19:07:17 +0900
From: Punit Agrawal <punit1.agrawal@...hiba.co.jp>
To: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>
Cc: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
<linux-pm@...r.kernel.org>, <linux-edac@...r.kernel.org>,
<linux-efi@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
<devel@...ica.org>, Borislav Petkov <bp@...en8.de>,
Tony Luck <tony.luck@...el.com>,
"Rafael J . Wysocki" <rjw@...ysocki.net>,
Len Brown <len.brown@...el.com>,
"Ard Biesheuvel" <ardb@...nel.org>,
Yazen Ghannam <yazen.ghannam@....com>
Subject: Re: [PATCH v4] cper, apei, mce: Pass x86 CPER through the MCA handling chain
Hi Smita,
A few comments below.
Smita Koralahalli <Smita.KoralahalliChannabasappa@....com> writes:
> Linux Kernel uses ACPI Boot Error Record Table (BERT) to report fatal
> errors that occurred in a previous boot. The MCA errors in the BERT are
> reported using the x86 Processor Error Common Platform Error Record (CPER)
> format. Currently, the record prints out the raw MSR values and AMD relies
> on the raw record to provide MCA information.
>
> Extract the raw MSR values of MCA registers from the BERT and feed it into
> the standard mce_log() function through the existing x86/MCA RAS
> infrastructure. This will result in better decoding from the EDAC MCE
> decoder or the default notifier.
>
> The implementation is SMCA specific as the raw MCA register values are
> given in the register offset order of the MCAX address space.
>
> [ Build error in patch v1. ]
>
> Reported-by: kernel test robot <lkp@...el.com>
I know Boris asked you to add the reason for the Reported-by, but
usually we don't track version differences in the committed patch.
Boris, can you confirm if you want the Reported-by to be retained?
> Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>
> ---
> Link:
> https://lkml.kernel.org/r/20200903234531.162484-2-Smita.KoralahalliChannabasappa@amd.com
>
> v4:
> Included what kernel test robot reported.
> Changed function name from apei_mce_report_x86_error ->
> apei_smca_report_x86_error.
> Added comment for MASK_MCA_STATUS definition.
> Wrapped apei_smca_report_x86_error() with CONFIG_X86_MCE in
> arch/x86/include/asm/mce.h
> v3:
> Moved arch specific declarations from generic headers to arch
> specific headers.
> Cleaned additional declarations which are unnecessary.
> Included the check for context type.
> Added additional check to verify for appropriate MSR address in
> the register layout.
> v2:
> Fixed build error reported by kernel test robot.
> Passed struct variable as function argument instead of entire struct.
> ---
> arch/x86/include/asm/acpi.h | 11 ++++++++
> arch/x86/include/asm/mce.h | 5 ++++
> arch/x86/kernel/acpi/apei.c | 5 ++++
> arch/x86/kernel/cpu/mce/apei.c | 49 +++++++++++++++++++++++++++++++++
> drivers/firmware/efi/cper-x86.c | 10 +++++--
> 5 files changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index 6d2df1ee427b..65064d9f7fa6 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -159,6 +159,8 @@ static inline u64 x86_default_get_root_pointer(void)
> extern int x86_acpi_numa_init(void);
> #endif /* CONFIG_ACPI_NUMA */
>
> +struct cper_ia_proc_ctx;
> +
> #ifdef CONFIG_ACPI_APEI
> static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> {
> @@ -177,6 +179,15 @@ static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
> */
> return PAGE_KERNEL_NOENC;
> }
> +
> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
> + u64 lapic_id);
> +#else
> +static inline int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
> + u64 lapic_id)
> +{
> + return -EINVAL;
> +}
> #endif
>
> #define ACPI_TABLE_UPGRADE_MAX_PHYS (max_low_pfn_mapped << PAGE_SHIFT)
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 109af5c7f515..d07bd635acfd 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -173,17 +173,22 @@ extern void mce_unregister_decode_chain(struct notifier_block *nb);
> #include <linux/atomic.h>
>
> extern int mce_p5_enabled;
> +struct cper_ia_proc_ctx;
>
> #ifdef CONFIG_X86_MCE
> int mcheck_init(void);
> void mcheck_cpu_init(struct cpuinfo_x86 *c);
> void mcheck_cpu_clear(struct cpuinfo_x86 *c);
> void mcheck_vendor_init_severity(void);
> +int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
> + u64 lapic_id);
> #else
> static inline int mcheck_init(void) { return 0; }
> static inline void mcheck_cpu_init(struct cpuinfo_x86 *c) {}
> static inline void mcheck_cpu_clear(struct cpuinfo_x86 *c) {}
> static inline void mcheck_vendor_init_severity(void) {}
> +static inline int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
> + u64 lapic_id) { return -EINVAL; }
> #endif
>
> #ifdef CONFIG_X86_ANCIENT_MCE
> diff --git a/arch/x86/kernel/acpi/apei.c b/arch/x86/kernel/acpi/apei.c
> index c22fb55abcfd..0916f00a992e 100644
> --- a/arch/x86/kernel/acpi/apei.c
> +++ b/arch/x86/kernel/acpi/apei.c
> @@ -43,3 +43,8 @@ void arch_apei_report_mem_error(int sev, struct cper_sec_mem_err *mem_err)
> apei_mce_report_mem_error(sev, mem_err);
> #endif
> }
> +
> +int arch_apei_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
> +{
> + return apei_smca_report_x86_error(ctx_info, lapic_id);
> +}
> diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c
> index af8d37962586..d4b3a2053eef 100644
> --- a/arch/x86/kernel/cpu/mce/apei.c
> +++ b/arch/x86/kernel/cpu/mce/apei.c
> @@ -51,6 +51,55 @@ void apei_mce_report_mem_error(int severity, struct cper_sec_mem_err *mem_err)
> }
> EXPORT_SYMBOL_GPL(apei_mce_report_mem_error);
>
> +/*
> + * The first expected register in the register layout of MCAX address space.
> + * The address defined must match with the first MSR address extracted from
> + * BERT which in SMCA systems is the bank's MCA_STATUS register.
> + *
> + * Note that the decoding of the raw MSR values in BERT is implementation
> + * specific and follows register offset order of MCAX address space.
> + */
> +#define MASK_MCA_STATUS 0xC0002001
The macro value is already defined in mce.h as
MSR_AMD64_SMCA_MC0_STATUS. Is there any reason to not use it?
You can move the comment to where you check the status register.
> +
> +int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info, u64 lapic_id)
> +{
> + const u64 *i_mce = ((const void *) (ctx_info + 1));
This can directly be cast into (const u64 *).
> + unsigned int cpu;
> + struct mce m;
> +
> + if (!boot_cpu_has(X86_FEATURE_SMCA))
> + return -EINVAL;
> +
> + if ((ctx_info->msr_addr & MASK_MCA_STATUS) != MASK_MCA_STATUS)
> + return -EINVAL;
> +
> + mce_setup(&m);
> +
> + m.extcpu = -1;
> + m.socketid = -1;
> +
> + 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;
> + break;
> + }
> + }
> +
> + m.apicid = lapic_id;
> + m.bank = (ctx_info->msr_addr >> 4) & 0xFF;
> + m.status = *i_mce;
> + m.addr = *(i_mce + 1);
> + m.misc = *(i_mce + 2);
> + /* Skipping MCA_CONFIG */
> + m.ipid = *(i_mce + 4);
> + m.synd = *(i_mce + 5);
Instead of using the raw pointer arithmetic, it is better to define a
structure for the MCA registers? Something like -
struct {
u64 addr;
u64 misc;
u64 config;
u64 ipid;
...
}
Checking back, this was mentioned in the previous review comments as
well. Please address all comments before posting a new version - either
by following the suggestion or explaining why it is not a good idea.
Thanks,
Punit
> +
> + mce_log(&m);
> +
> + return 0;
> +}
> +
> #define CPER_CREATOR_MCE \
> GUID_INIT(0x75a574e3, 0x5052, 0x4b29, 0x8a, 0x8e, 0xbe, 0x2c, \
> 0x64, 0x90, 0xb8, 0x9d)
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index 2531de49f56c..2f2b0c431c18 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -2,6 +2,7 @@
> // Copyright (C) 2018, Advanced Micro Devices, Inc.
>
> #include <linux/cper.h>
> +#include <linux/acpi.h>
>
> /*
> * We don't need a "CPER_IA" prefix since these are all locally defined.
> @@ -347,9 +348,12 @@ void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
> ctx_info->mm_reg_addr);
> }
>
> - printk("%sRegister Array:\n", newpfx);
> - print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
> - (ctx_info + 1), ctx_info->reg_arr_size, 0);
> + if (ctx_info->reg_ctx_type != CTX_TYPE_MSR ||
> + arch_apei_report_x86_error(ctx_info, proc->lapic_id)) {
> + printk("%sRegister Array:\n", newpfx);
> + print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, groupsize,
> + (ctx_info + 1), ctx_info->reg_arr_size, 0);
> + }
>
> ctx_info = (struct cper_ia_proc_ctx *)((long)ctx_info + size);
> }
Powered by blists - more mailing lists