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 12:46:58 +0800 Message-ID: <55484B42.6030300@intel.com> References: <1430704984-6238-1-git-send-email-tiejun.chen@intel.com> <55472CDD.2010300@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55472CDD.2010300@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/4 16:25, Andrew Cooper wrote: > On 04/05/2015 03:03, Tiejun Chen wrote: >> Just make this readable while debug. > > "debugging" Fixed. > >> >> Signed-off-by: Tiejun Chen > > In principle, I fully agree with the change. (I had an item on my todo > list to make a change like this anyway). > >> --- >> xen/arch/x86/apic.c | 33 +++++++++++++++++++++------------ >> 1 file changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c >> index 3217bdf..0b21ed1 100644 >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -1319,28 +1319,37 @@ out: ; >> * This interrupt should never happen with our APIC/SMP architecture >> */ >> >> +static const char *apic_fault_reasons[] = > > static const char * const > > It might be better to have "esr" in the name to make it clear that these > are string representations of the esr fields. Sure. > >> +{ >> + "Send CS error", >> + "Receive CS error", >> + "Send accept error", >> + "Receive accept error", >> + "Reserved", > > "Redirectable IPI" > > This field is not reserved on all processors which can run Xen. Changed. > >> + "Send illegal vector", >> + "Received illegal vector", >> + "Illegal register address", >> +}; >> + >> +static const char *apic_get_fault_reason(u8 fault_reason) >> +{ >> + return apic_fault_reasons[fault_reason]; > > This can easily overflow the array. I don't see much value in the wrapper. Right. > >> +} >> + >> void error_interrupt(struct cpu_user_regs *regs) >> { >> unsigned long v, v1; > > These can be unsigned int rather than unsigned long, saving a bit of space. Okay. > >> + const char *reason; >> >> /* First tickle the hardware, only then report what went on. -- REW */ >> v = apic_read(APIC_ESR); >> apic_write(APIC_ESR, 0); >> v1 = apic_read(APIC_ESR); >> ack_APIC_irq(); >> + reason = apic_get_fault_reason(ffs(v1) - 1); > > All but the bottom byte of v1 is reserved, and not guaranteed to be read > as 0. > > Also, more than a single error can be latched at once, which this code > discards. > >> >> - /* 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", >> - smp_processor_id(), v , v1); >> + printk (KERN_DEBUG "APIC error on CPU%d: %02lx(%02lx):%s.\n", > > Please can we avoid adding redundant punctuation to error messages. The > full stop serves no semantic purpose. > > Also, please correct %d to %u as smp_processor_id() is an unsigned integer. > >> + smp_processor_id(), v , v1, reason); > > 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]. > 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? 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[] = +{ + "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)", smp_processor_id(), v , v1); + for (i = (1<<7); i; i >>= 1) + if(v1 & i) + printk (KERN_DEBUG ", %s", fault_reasons_from_esr[ffs(i) - 1]); + printk (KERN_DEBUG ".\n"); } /* Thanks Tiejun