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

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

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 <tiejun.chen@xxxxxxxxx>
> 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

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