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

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

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