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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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.