Message ID | 20200807142750.270548-1-kyle.meyer@hpe.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/MCE: Set the MCE's status and misc members | expand |
On Fri, Aug 07, 2020 at 09:27:50AM -0500, Kyle Meyer wrote: > The MCE's status and misc members are initialized to zero > within mce_setup. Set the MCE's status and misc members to the > corresponding values passed in by the mem_err argument. This provides > support for the RAS: Corrected Errors Collector (CEC) which uses the > MCE. This commit message is talking about the "how" but that is visible from the patch. It needs to talk more about the why, as to why are you doing this and what you're trying to achieve. I believe you alluded to some of that offlist. > Signed-off-by: Kyle Meyer <kyle.meyer@hpe.com> > --- > arch/x86/include/asm/mce.h | 6 ++++-- > arch/x86/kernel/cpu/mce/apei.c | 8 ++++++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h > index cf503824529c..2346d900a232 100644 > --- a/arch/x86/include/asm/mce.h > +++ b/arch/x86/include/asm/mce.h > @@ -79,8 +79,10 @@ > #define MCACOD_INSTR 0x0150 /* Instruction Fetch */ > > /* MCi_MISC register defines */ > -#define MCI_MISC_ADDR_LSB(m) ((m) & 0x3f) > -#define MCI_MISC_ADDR_MODE(m) (((m) >> 6) & 7) > +#define MCI_MISC_ADDR_LSB(m) ((m) & 0x3f) > +#define MCI_MISC_ADDR_LSB_SET(x) ((x) & 0x3f) > +#define MCI_MISC_ADDR_MODE(m) (((m) >> 6) & 7) > +#define MCI_MISC_ADDR_MODE_SET(x) (((x) & 7) << 6) > #define MCI_MISC_ADDR_SEGOFF 0 /* segment offset */ > #define MCI_MISC_ADDR_LINEAR 1 /* linear address */ > #define MCI_MISC_ADDR_PHYS 2 /* physical address */ > diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c > index af8d37962586..efdfb55b934a 100644 > --- a/arch/x86/kernel/cpu/mce/apei.c > +++ b/arch/x86/kernel/cpu/mce/apei.c > @@ -38,6 +38,14 @@ void apei_mce_report_mem_error(int severity, struct cper_sec_mem_err *mem_err) > /* Fake a memory read error with unknown channel */ > m.status = MCI_STATUS_VAL | MCI_STATUS_EN | MCI_STATUS_ADDRV | 0x9f; > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || > + boot_cpu_data.x86_vendor == X86_VENDOR_ZHAOXIN) { > + m.status |= MCI_STATUS_MISCV; > + m.misc |= MCI_MISC_ADDR_LSB_SET(ffs( > + mem_err->physical_addr_mask) - 1) | > + MCI_MISC_ADDR_MODE_SET(MCI_MISC_ADDR_PHYS); > + } This is ugly, especially that trailing opening brace - please use a helper variable.
There's one small code change I suggest below. But now it's time to think about the description. We need to zoom out to the 10,000 ft. view. IMHO, to understand what this patch does, your current description requires the reader to be more familiar with the involved functions than we should expect. To introduce the reader to what's going on, it should read more like: "When creating an MCE record from a CPER memory error, fill in the m.misc field and mark it as valid in the m.status field on processors where MCEs are expected to have this information. This ensures that mce_usable_address() will not interpret the address as invalid, which (for example) would cause cec_notifier() to ignore the created MCE records." In addition to giving an improved background picture, that gives direct "pointers" to the other functions you have to examine to understand what's going on. On Fri, Aug 07, 2020 at 09:27:50AM -0500, Kyle Meyer wrote: > The MCE's status and misc members are initialized to zero > within mce_setup. Set the MCE's status and misc members to the > corresponding values passed in by the mem_err argument. This provides > support for the RAS: Corrected Errors Collector (CEC) which uses the > MCE. > > Signed-off-by: Kyle Meyer <kyle.meyer@hpe.com> > --- > arch/x86/include/asm/mce.h | 6 ++++-- > arch/x86/kernel/cpu/mce/apei.c | 8 ++++++++ > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h > index cf503824529c..2346d900a232 100644 > --- a/arch/x86/include/asm/mce.h > +++ b/arch/x86/include/asm/mce.h > @@ -79,8 +79,10 @@ > #define MCACOD_INSTR 0x0150 /* Instruction Fetch */ > > /* MCi_MISC register defines */ > -#define MCI_MISC_ADDR_LSB(m) ((m) & 0x3f) > -#define MCI_MISC_ADDR_MODE(m) (((m) >> 6) & 7) > +#define MCI_MISC_ADDR_LSB(m) ((m) & 0x3f) > +#define MCI_MISC_ADDR_LSB_SET(x) ((x) & 0x3f) > +#define MCI_MISC_ADDR_MODE(m) (((m) >> 6) & 7) > +#define MCI_MISC_ADDR_MODE_SET(x) (((x) & 7) << 6) > #define MCI_MISC_ADDR_SEGOFF 0 /* segment offset */ > #define MCI_MISC_ADDR_LINEAR 1 /* linear address */ > #define MCI_MISC_ADDR_PHYS 2 /* physical address */ > diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c > index af8d37962586..efdfb55b934a 100644 > --- a/arch/x86/kernel/cpu/mce/apei.c > +++ b/arch/x86/kernel/cpu/mce/apei.c > @@ -38,6 +38,14 @@ void apei_mce_report_mem_error(int severity, struct cper_sec_mem_err *mem_err) > /* Fake a memory read error with unknown channel */ > m.status = MCI_STATUS_VAL | MCI_STATUS_EN | MCI_STATUS_ADDRV | 0x9f; > > + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || > + boot_cpu_data.x86_vendor == X86_VENDOR_ZHAOXIN) { > + m.status |= MCI_STATUS_MISCV; Concentrating on this statement: > + m.misc |= MCI_MISC_ADDR_LSB_SET(ffs( > + mem_err->physical_addr_mask) - 1) | > + MCI_MISC_ADDR_MODE_SET(MCI_MISC_ADDR_PHYS); I know you mentioned that the whole of structure m was cleared out by mce_setup(), but that's in the change comment, not the code. Since we know m.misc is already 0, the code would be clearer if this line started m.misc = MCI_MISC... instead of m.misc |= MCI_MISC... because the result is the same, and someone coming fresh into reading this code has to go into the definition of mce_setup() to know that m.misc started as zero for the |= version. Another way of stating this is there's nothing already in m.misc you're trying to preserve and add to, where seeing the |= operation could mislead a new reader into thinking there was something there you're trying to preserve. > + } > + > if (severity >= GHES_SEV_RECOVERABLE) > m.status |= MCI_STATUS_UC; > > -- > 2.26.2 > --> Steve
On Fri, Aug 07, 2020 at 10:05:50AM -0500, Steve Wahl wrote: > There's one small code change I suggest below. > > But now it's time to think about the description. We need to zoom out > to the 10,000 ft. view. IMHO, to understand what this patch does, > your current description requires the reader to be more familiar with > the involved functions than we should expect. Maybe also travel back in time to why apei_mce_report_mem_error() was created in the first place. This was a workaround for a couple of Intel CPUs (Nehalem, Westmere) that didn't provide a system address in MCi_ADDR. Since BIOS could figure out the address, it was tasked with reporting to the OS suing APEI. apei_mce_report_mem_error() did the minimum necessary to create an error log that mcelog(8) daemon could process. Jump forward in time to Sandybridge (and all successors) and the reason for the workaround is gone. All of them provide a system address in the machine check bank ... so this APEI path is a poor substitute for getting the data from the machine check bank (which will give you the channel, the corrected error count, and perhaps other useful information). So why are we applying band-aids to this decade old workaround? -Tony
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h index cf503824529c..2346d900a232 100644 --- a/arch/x86/include/asm/mce.h +++ b/arch/x86/include/asm/mce.h @@ -79,8 +79,10 @@ #define MCACOD_INSTR 0x0150 /* Instruction Fetch */ /* MCi_MISC register defines */ -#define MCI_MISC_ADDR_LSB(m) ((m) & 0x3f) -#define MCI_MISC_ADDR_MODE(m) (((m) >> 6) & 7) +#define MCI_MISC_ADDR_LSB(m) ((m) & 0x3f) +#define MCI_MISC_ADDR_LSB_SET(x) ((x) & 0x3f) +#define MCI_MISC_ADDR_MODE(m) (((m) >> 6) & 7) +#define MCI_MISC_ADDR_MODE_SET(x) (((x) & 7) << 6) #define MCI_MISC_ADDR_SEGOFF 0 /* segment offset */ #define MCI_MISC_ADDR_LINEAR 1 /* linear address */ #define MCI_MISC_ADDR_PHYS 2 /* physical address */ diff --git a/arch/x86/kernel/cpu/mce/apei.c b/arch/x86/kernel/cpu/mce/apei.c index af8d37962586..efdfb55b934a 100644 --- a/arch/x86/kernel/cpu/mce/apei.c +++ b/arch/x86/kernel/cpu/mce/apei.c @@ -38,6 +38,14 @@ void apei_mce_report_mem_error(int severity, struct cper_sec_mem_err *mem_err) /* Fake a memory read error with unknown channel */ m.status = MCI_STATUS_VAL | MCI_STATUS_EN | MCI_STATUS_ADDRV | 0x9f; + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || + boot_cpu_data.x86_vendor == X86_VENDOR_ZHAOXIN) { + m.status |= MCI_STATUS_MISCV; + m.misc |= MCI_MISC_ADDR_LSB_SET(ffs( + mem_err->physical_addr_mask) - 1) | + MCI_MISC_ADDR_MODE_SET(MCI_MISC_ADDR_PHYS); + } + if (severity >= GHES_SEV_RECOVERABLE) m.status |= MCI_STATUS_UC;
The MCE's status and misc members are initialized to zero within mce_setup. Set the MCE's status and misc members to the corresponding values passed in by the mem_err argument. This provides support for the RAS: Corrected Errors Collector (CEC) which uses the MCE. Signed-off-by: Kyle Meyer <kyle.meyer@hpe.com> --- arch/x86/include/asm/mce.h | 6 ++++-- arch/x86/kernel/cpu/mce/apei.c | 8 ++++++++ 2 files changed, 12 insertions(+), 2 deletions(-)