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

[Aravind Gopalakrishnan]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/