[v2][PATCH] xen/apic: refactor error_interrupt

[v2][PATCH] xen/apic: refactor error_interrupt

[Tiejun Chen]

Just make this readable while debugging.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
v2:

* Tried to refine code according to comments.

 xen/arch/x86/apic.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 3217bdf..b9f53df 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 esr_fields[] =
+{
+    "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(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(", %s", esr_fields[i]);
+    printk("\n");
 }
 
 /*
-- 
1.9.1

Re: [v2][PATCH] xen/apic: refactor error_interrupt

[Andrew Cooper]

On 06/05/2015 07:27, Tiejun Chen wrote:
> Just make this readable while debugging.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Almost, but there is one issue.

> ---
> v2:
>
> * Tried to refine code according to comments.
>
>  xen/arch/x86/apic.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index 3217bdf..b9f53df 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 esr_fields[] =
> +{
> +    "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(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)",
>              smp_processor_id(), v , v1);
> +    for ( i = 7; i >= 0; --i )

As the patch currently goes, i is unsigned making this an infinite loop.

Swap i to be a regular int, and Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

> +        if ( v1 & (1 << i) )
> +            printk(", %s", esr_fields[i]);
> +    printk("\n");
>  }
>  
>  /*