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

[Elliott Mitchell <ehem+xen@xxxxxxx>]

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".


> > @@ -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).

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).

Third, I'm simply unsure how you would prefer to format the printk().
I imagine at some future point the register may have additional bits
allocated.  At such point esr_fields[] would grow, but this would also
require adding to the format string and adding an argument.

Seems like several fiddly bits which mostly effect look and don't really
effect the implementation.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@xxxxxxx  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445