[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.