Re: [Xen-devel] [RFC][PATCH] xen/apic: refactor error_interrupt

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel