From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][PATCH] xen/apic: refactor error_interrupt Date: Tue, 05 May 2015 14:28:33 +0800 Message-ID: <55486311.9040505@intel.com> References: <1430704984-6238-1-git-send-email-tiejun.chen@intel.com> <55472CDD.2010300@citrix.com> <55484B42.6030300@intel.com> <55485CFD.2060506@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55485CFD.2060506@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , keir@xen.org, jbeulich@suse.com Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 2015/5/5 14:02, Andrew Cooper wrote: > On 05/05/2015 05:46, Chen, Tiejun wrote: >> >>> A better approach might be: >>> >>> printk(KERN_DEBUG "APIC error on CPU%u: %02lx(%02lx)", ...) >>> for ( i = (1<<7); i; i >>= 1 ) >>> if ( v1 & i ) >>> printk(", %s", apic_fault_reasons[i]); >> >> I guess this should be apic_fault_reasons[ffs(i) - 1]. > > Indeed. > >> >>> printk("\n"); >>> >>> Which will decode all set bits, and guarantee not to access outside the >>> bounds of apic_fault_reasons. >>> >> >> Thanks you for all comments. So overall, what about this? > > Much better. A few further comments. > >> >> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c >> index 3217bdf..87684a2 100644 >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -1319,9 +1319,21 @@ out: ; >> * This interrupt should never happen with our APIC/SMP architecture >> */ >> >> +static const char * const fault_reasons_from_esr[] = > > It is not too important, but I was thinking of something rather shorter > as a name. "esr_fields[]" perhaps ? Looks good. > >> +{ >> + "Send CS error", >> + "Receive CS error", >> + "Send accept error", >> + "Receive accept error", >> + "Redirectable IPI", >> + "Send illegal vector", >> + "Received illegal vector", >> + "Illegal register address", >> +}; >> + >> void error_interrupt(struct cpu_user_regs *regs) >> { >> - unsigned long v, v1; >> + unsigned int v, v1, i; >> >> /* First tickle the hardware, only then report what went on. -- >> REW */ >> v = apic_read(APIC_ESR); >> @@ -1329,18 +1341,12 @@ void error_interrupt(struct cpu_user_regs *regs) >> v1 = apic_read(APIC_ESR); >> ack_APIC_irq(); >> >> - /* Here is what the APIC error bits mean: >> - 0: Send CS error >> - 1: Receive CS error >> - 2: Send accept error >> - 3: Receive accept error >> - 4: Reserved >> - 5: Send illegal vector >> - 6: Received illegal vector >> - 7: Illegal register address >> - */ >> - printk (KERN_DEBUG "APIC error on CPU%d: %02lx(%02lx)\n", >> + printk (KERN_DEBUG "APIC error on CPU%u: %02x(%02x)", > > While at this, please apply Xen style to all changed lines. In > particular, drop the space between printk and its opening bracket. Swap Okay. And actually we should clean this whole file completely since this is backported from Linux... > KERN_ for XENLOG_ Sure. > >> smp_processor_id(), v , v1); >> + for (i = (1<<7); i; i >>= 1) > > Spaces inside the brackets. > >> + if(v1 & i) > > And here. > >> + printk (KERN_DEBUG ", %s", fault_reasons_from_esr[ffs(i) >> - 1]); > > The code here now looks correct, but it is unlikely that the compiler > would be able to simplify the ffs() call. > > An alternate approach would be: > > int i; > for ( i = 7; i >= 0; --i ) > if ( v1 & (1 << i) ) > printk(... , fault_reasons_from_esr[i]) Then, - printk (KERN_DEBUG "APIC error on CPU%d: %02lx(%02lx)\n", + printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)", smp_processor_id(), v , v1); + for ( i = 7; i >= 0; --i ) + if ( v1 & (1 << i) ) + printk(XENLOG_DEBUG ", %s", esr_fields[i]); > >> + printk (KERN_DEBUG ".\n"); > > Please do not put full stops on messages like this. It serves no useful > purpose, but wasted space in the console ring and wasted time when > pushed over a serial console. Are you saying this? printk(XENLOG_DEBUG "\n"); Thanks Tiejun