Re: [PATCH 2/4] x86/mce/amd: Introduce deferred error interrupt handler

Aravind Gopalakrishnan
2015-05-04T15:30:06Z

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.