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

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

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