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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel