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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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