| Message ID | 7771343b52e6769d7670ad73094f5276025a10fe.1689191941.git.ehem+xen@m5p.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixing ACPI error reporting display | expand |
On 17.03.2023 20:53, Elliott Mitchell wrote: > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -1410,6 +1410,7 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs) > ", Received illegal vector", > ", Illegal register address", > }; > + const char *entries[ARRAY_SIZE(esr_fields)]; > unsigned int v, v1; > int i; > > @@ -1419,12 +1420,13 @@ 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[7], entries[6], > + entries[5], entries[4], entries[3], entries[2], entries[1], entries[0]); While pre-existing in both cases, two nits: There's a stray blank before one of the commas, and indentation is wrong too. Furthermore there's no reason to split the format string, especially not ahead of the \n. Plus line wrapping for the 8 entries[] references could also be done more evenly. Since these are all cosmetic, I guess I'll do adjustments while committing: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On Thu, Jul 13, 2023 at 03:12:55PM +0200, Jan Beulich wrote: > On 17.03.2023 20:53, Elliott Mitchell wrote: > > --- a/xen/arch/x86/apic.c > > +++ b/xen/arch/x86/apic.c > > @@ -1419,12 +1420,13 @@ 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[7], entries[6], > > + entries[5], entries[4], entries[3], entries[2], entries[1], entries[0]); > > While pre-existing in both cases, two nits: There's a stray blank before one > of the commas, and indentation is wrong too. I don't see anything which could be called an indentation error. The very first added line is attached to the `for ()`, therefore it correctly has one more indent. I guess this is an advantage of rewriting the loop as part of this patch. The correct indentation is clearer. > Furthermore there's no reason to split the format string, especially not > ahead of the \n. Plus line wrapping for the 8 entries[] references could > also be done more evenly. > > Since these are all cosmetic, I guess I'll do adjustments while committing: > Acked-by: Jan Beulich <jbeulich@suse.com> Yeah, that seems the best approach here. I'm unsure how likely it is for additional bits in the ESR register to be allocated. Ideal is to format this in such a way that adding bits causes a smaller delta. I was thinking high-order bits were likely to be reportted at start, hence the last line would be best filled first. Though one does not know the future and trying to anticipate it may well yield worse results. This really does seem a place where it should be adjusted to taste during final commit. Otherwise there are too many minor nits and things get bogged down in trivial issues.
On 13.07.2023 18:47, Elliott Mitchell wrote: > On Thu, Jul 13, 2023 at 03:12:55PM +0200, Jan Beulich wrote: >> On 17.03.2023 20:53, Elliott Mitchell wrote: >>> --- a/xen/arch/x86/apic.c >>> +++ b/xen/arch/x86/apic.c >>> @@ -1419,12 +1420,13 @@ 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[7], entries[6], >>> + entries[5], entries[4], entries[3], entries[2], entries[1], entries[0]); >> >> While pre-existing in both cases, two nits: There's a stray blank before one >> of the commas, and indentation is wrong too. > > I don't see anything which could be called an indentation error. The > very first added line is attached to the `for ()`, therefore it correctly > has one more indent. It's the printk() invocation, where all continued lines lacked another 3 blanks. Jan
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c index 8cfb8cd71c..5b830b2312 100644 --- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -1410,6 +1410,7 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs) ", Received illegal vector", ", Illegal register address", }; + const char *entries[ARRAY_SIZE(esr_fields)]; unsigned int v, v1; int i; @@ -1419,12 +1420,13 @@ 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[7], entries[6], + entries[5], entries[4], entries[3], entries[2], entries[1], entries[0]); } /*
This takes care of the issue of APIC errors tending to occur on multiple cores at once. In turn this tends to causes the error messages to be merged together, making understanding them difficult. Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> --- v2: Splitting the loop adjustment off. Fixing the entry display order. --- xen/arch/x86/apic.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)