Re: [PATCH v2 2/2] x86/APIC: modify error_interrupt() to output using single printk()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.03.2023 15:29, Elliott Mitchell wrote:
> On Mon, Mar 20, 2023 at 09:56:54AM +0100, Jan Beulich wrote:
>> On 17.03.2023 20:53, Elliott Mitchell wrote:
>>> This takes care of the issue of APIC errors tending to occur on multiple
>>> cores at one.  In turn this tends to causes the error messages to be
>>
>> Nit: "at once"?
> 
> https://en.wiktionary.org/wiki/at_once
> 
> Adverb #2, synonym of "simultaneously".

And that's what you mean, I think? Not being a native speaker, I have no
idea what "at one" is meaning here.

>>> @@ -1419,12 +1420,12 @@ static void cf_check error_interrupt(struct 
>>> cpu_user_regs *regs)
>>>      v1 = apic_read(APIC_ESR);
>>>      ack_APIC_irq();
>>>  
>>> -    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("%s", esr_fields[i]);
>>> -    printk("\n");
>>> +        entries[i] = v1 & (1 << i) ? esr_fields[i] : "";
>>> +    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)"
>>> +            "%s%s%s%s%s%s%s%s" "\n",
>>> +            smp_processor_id(), v , v1, entries[0], entries[1], entries[2],
>>> +            entries[3], entries[4], entries[5], entries[6], entries[7]);
>>
>> Two style nits: Indentation wants fixing here (it was wrong in the original
>> code already), and the stray blank between v and the comma also wants
>> dropping at this occasion.
> 
> There are several minor issues here which may be best handled during
> commit as they're very small items about how precisely you want this to
> look.
> 
> First, I later realized I goofed the argument order.  In order to match
> the original implementation, it needs to be entries[7] ... entries[0]
> (could though be the low-order bits should be reported first).

I'm not really concerned of the order. A change of order wants
mentioning in the description though.

> Second, the order of the for loop no longer matters.  Using
> ARRAY_SIZE(esr_fields) and increment should now be more maintainable
> (this would also allow i to be unsigned).

Indeed. But that would better done in a separate patch then anyway.

> Third, I'm simply unsure how you would prefer to format the printk().

About any way matching style guidelines is okay. There are two more
things to mention though (sorry for not noticing earlier): We aim at
keeping the entire format string on one line, for grep-ability. And
there's no need (and in fact no reason) to split the sequence of %s
from the \n. To summarize:

    printk(XENLOG_DEBUG
           "APIC error on CPU%u: %02x(%02x)%s%s%s%s%s%s%s%s\n",

(unless of course it all fits on one line, which it looks like it
does).

Jan