Messages in this thread | | | Date | Mon, 4 May 2015 10:29:50 -0500 | From | Aravind Gopalakrishnan <> | Subject | Re: [PATCH 2/4] x86/mce/amd: Introduce deferred error interrupt handler |
| |
On 5/3/2015 4:22 AM, Borislav Petkov wrote: > On Thu, Apr 30, 2015 at 09:49:23AM -0500, Aravind Gopalakrishnan wrote: >> Changes introduced in the patch- >> - Assign vector number 0xf4 for Deferred errors >> - Declare deferred_interrupt, allocate gate and bind it >> to DEFERRED_APIC_VECTOR. >> - Declare smp_deferred_interrupt to be used as the >> entry point for the interrupt in mce_amd.c >> - Define trace_deferred_interrupt for tracing >> - Enable deferred error interrupt selectively upon detection >> of 'succor' bitfield >> - Setup amd_deferred_error_interrupt() to handle the interrupt >> and assign it to def_int_vector if feature is present in HW. >> Else, let default handler deal with it. >> - Provide Deferred error interrupt stats on >> /proc/interrupts by incrementing irq_deferred_count > This commit message should explain the feature in more high-level way, > what is it good for and so on, not what you're adding. > > That I can see. :-)
Okay, I'll include a short description of deferred errors here for V2.
>> +#endif >> #endif >> diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h >> index 0f5fb6b..448451c 100644 >> --- a/arch/x86/include/asm/hardirq.h >> +++ b/arch/x86/include/asm/hardirq.h >> @@ -33,6 +33,9 @@ typedef struct { >> #ifdef CONFIG_X86_MCE_THRESHOLD >> unsigned int irq_threshold_count; >> #endif >> +#ifdef CONFIG_X86_MCE_AMD >> + unsigned int irq_deferred_count; > Right > unsigned int irq_deferred_error_count;
Ack.
>> +static void deferred_error_interrupt_enable(struct cpuinfo_x86 *c) >> +{ >> + u32 low = 0, high = 0; >> + int def_offset = -1, def_new; >> + >> + if (rdmsr_safe(MSR_CU_DEF_ERR, &low, &high)) >> + return; >> + >> + def_new = (low & MASK_DEF_LVTOFF) >> 4; >> + if (c->x86 == 0x15 && c->x86_model == 0x60 && >> + !(low & MASK_DEF_LVTOFF)) { > What's the family check for? for BIOSes which don't set the LVT offset > to 2, as they should? > > If so, we probably should say > > pr_err(FW_BUG "Your BIOS is not setting up LVT offset 0x2 for deferred error IRQs correctly.\n"); > > or similar...
Yeah. I meant to provide a comment at least for this. Forgot to do that.
I'll print out a error message as you suggested (considering we do this in other places like threshold setup or IBS setup..)
>> +/* Apic interrupt handler for deferred errors */ >> +static void amd_deferred_error_interrupt(void) >> +{ >> + u64 status; >> + unsigned int bank; >> + struct mce m; >> + >> + for (bank = 0; bank < mca_cfg.banks; ++bank) { >> + rdmsrl(MSR_IA32_MCx_STATUS(bank), status); >> + >> + if (!(status & MCI_STATUS_VAL) || >> + !(status & MCI_STATUS_DEFERRED)) >> + continue; >> + >> + mce_setup(&m); >> + m.bank = bank; >> + m.status = status; >> + mce_log(&m); >> + wrmsrl(MSR_IA32_MCx_STATUS(bank), 0); >> + break; >> + } > That's very similar to what we do in the end of > amd_threshold_interrupt(). You could add a generic __log_error() static > helper in a pre-patch and then call it here. >
Right. I think a __log_error() is a good idea. Except, in amd_threshold_interrupt(), we have- m.misc = ((u64)high << 32) | low;
which, is actually useless as we don't use m.misc anywhere in amd_decode_mce() or anywhere else in the decoding pipeline AFAICT. We only print out if 'misc' is valid and we only need status bits for that- ((m->status & MCI_STATUS_MISCV) ? "MiscV" : "-"),
But, more importantly, we don't setup 'm.addr' here (in amd_threshold_interrupt() or in amd_deferred_error_interrupt()) Which means anytime we pass an error to be decoded from the interrupt handlers, we don't get any info about the error address.
So, we can do one of these- 1. Remove m.misc setup in amd_threshold_interrupt() and rdmsrl(MSR_IA32_MCx_ADDR(bank), m.addr) before we call mce_log() 2. Since we have mce_read_aux() that reads misc and addr registers, we can move the mce_[rd|wr]msrl wrappers and mce_read_aux() into mce.h and use it here in mce_amd.c
Thoughts?
Thanks, -Aravind.
| |