Message-ID: <5547906E.3060701@amd.com>
Date: Mon, 4 May 2015 10:29:50 -0500
From: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Borislav Petkov <bp@alien8.de>
CC: <tglx@linutronix.de>, <mingo@redhat.com>, <hpa@zytor.com>,
        <tony.luck@intel.com>, <jiang.liu@linux.intel.com>,
        <yinghai@kernel.org>, <x86@kernel.org>, <dvlasenk@redhat.com>,
        <JBeulich@suse.com>, <slaoub@gmail.com>, <luto@amacapital.net>,
        <dave.hansen@linux.intel.com>, <oleg@redhat.com>,
        <rostedt@goodmis.org>, <rusty@rustcorp.com.au>, <prarit@redhat.com>,
        <linux@rasmusvillemoes.dk>, <jroedel@suse.de>,
        <andriy.shevchenko@linux.intel.com>, <macro@linux-mips.org>,
        <wangnan0@huawei.com>, <linux-kernel@vger.kernel.org>,
        <linux-edac@vger.kernel.org>
Subject: Re: [PATCH 2/4] x86/mce/amd: Introduce deferred error interrupt handler
References: <1430405365-4473-1-git-send-email-Aravind.Gopalakrishnan@amd.com>
 <1430405365-4473-3-git-send-email-Aravind.Gopalakrishnan@amd.com>
 <20150503092212.GC18048@pd.tnic>
In-Reply-To: <20150503092212.GC18048@pd.tnic>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 May 2015 15:29:54.6099
 (UTC)
X-MS-Exchange-CrossTenant-Id: fde4dada-be84-483f-92cc-e026cbee8e96
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=fde4dada-be84-483f-92cc-e026cbee8e96;Ip=[165.204.84.222];Helo=[atltwp02.amd.com]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR02MB1112
X-OriginatorOrg: amd4.onmicrosoft.com
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4292
Lines: 122

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/