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

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

[Tiejun Chen]

Just make this readable while debug.

Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
 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[] =
+{
+    "Send CS error",
+    "Receive CS error",
+    "Send accept error",
+    "Receive accept error",
+    "Reserved",
+    "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];
+}
+
 void error_interrupt(struct cpu_user_regs *regs)
 {
     unsigned long v, v1;
+    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);
 
-    /* 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",
+            smp_processor_id(), v , v1, reason);
 }
 
 /*
-- 
1.8.3.2

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

[Jan Beulich]

>>> On 04.05.15 at 04:03, <tiejun.chen@intel.com> wrote:
> --- 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[] =

If at all, then this should be const. But...

> +{
> +    "Send CS error",
> +    "Receive CS error",
> +    "Send accept error",
> +    "Receive accept error",
> +    "Reserved",
> +    "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];
> +}
> +
>  void error_interrupt(struct cpu_user_regs *regs)
>  {
>      unsigned long v, v1;
> +    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);

... I'm not convinced - you're treating errors represented by lower
bits "better" than higher ones. And if there are multiple errors, then
spelling out one but not the other may end up being confusing.
These errors should be rare enough to warrant manually looking up
the individual bits' meanings.

Jan

> -    /* 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",
> +            smp_processor_id(), v , v1, reason);
>  }
>  
>  /*

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

[Andrew Cooper]

On 04/05/2015 03:03, Tiejun Chen wrote:
> Just make this readable while debug.

"debugging"

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

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.

> +{
> +    "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.

> +    "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.

> +}
> +
>  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.

> +    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]);
printk("\n");

Which will decode all set bits, and guarantee not to access outside the
bounds of apic_fault_reasons.

~Andrew

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

[Chen, Tiejun]

On 2015/5/4 16:07, Jan Beulich wrote:
>>>> On 04.05.15 at 04:03, <tiejun.chen@intel.com> wrote:
>> --- 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[] =
>
> If at all, then this should be const. But...
>
>> +{
>> +    "Send CS error",
>> +    "Receive CS error",
>> +    "Send accept error",
>> +    "Receive accept error",
>> +    "Reserved",
>> +    "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];
>> +}
>> +
>>   void error_interrupt(struct cpu_user_regs *regs)
>>   {
>>       unsigned long v, v1;
>> +    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);
>
> ... I'm not convinced - you're treating errors represented by lower
> bits "better" than higher ones. And if there are multiple errors, then
> spelling out one but not the other may end up being confusing.

I just think APIC shouldn't record these multiple errors at the same 
time because these kinds of errors seem be exclusive... But I don't find 
this is described in SDM so I think you're right.

Fortunately, looks Andrew guides me a approach to cover this point, so 
just let me go there.

Thanks
Tiejun

> These errors should be rare enough to warrant manually looking up
> the individual bits' meanings.
>
> Jan
>
>> -    /* 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",
>> +            smp_processor_id(), v , v1, reason);
>>   }
>>
>>   /*
>
>
>
>

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

[Chen, Tiejun]

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@intel.com>
>
> 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

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

[Andrew Cooper]

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 ?

> +{
> +    "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
KERN_ for XENLOG_

>              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])

> +    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.

~Andrew

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

[Chen, Tiejun]

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

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

[Andrew Cooper]

On 05/05/2015 07:28, Chen, Tiejun wrote:
> 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...

Not worth changing for the sake of changing, but where we are already
altering code we attempt to make it consistent.

>
>> 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");

Yes, although you also need to drop the XENLOG_DEBUG from this and the
printk() inside the loop, as they are not printks from the start of a line.

so,
for ( i = 7; i >= 0; --i )
    if ( v1 & (1 << i) )
        printk(", %s", esr_fields[i]);
printk("\n");

~Andrew