Re: [PATCH 4/4] x86/APIC: restrict certain messages to BSP

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 14, 2020 at 02:31:33PM +0200, Jan Beulich wrote:
> On 14.05.2020 12:01, Roger Pau Monné wrote:
> > On Fri, Mar 13, 2020 at 10:26:47AM +0100, Jan Beulich wrote:
> >> --- a/xen/arch/x86/apic.c
> >> +++ b/xen/arch/x86/apic.c
> >> @@ -499,7 +499,7 @@ static void resume_x2apic(void)
> >>      __enable_x2apic();
> >>  }
> >>  
> >> -void setup_local_APIC(void)
> >> +void setup_local_APIC(bool bsp)
> >>  {
> >>      unsigned long oldvalue, value, maxlvt;
> >>      int i, j;
> >> @@ -598,8 +598,8 @@ void setup_local_APIC(void)
> >>      if ( directed_eoi_enabled )
> >>      {
> >>          value |= APIC_SPIV_DIRECTED_EOI;
> >> -        apic_printk(APIC_VERBOSE, "Suppress EOI broadcast on CPU#%d\n",
> >> -                    smp_processor_id());
> >> +        if ( bsp )
> >> +            apic_printk(APIC_VERBOSE, "Suppressing EOI broadcast\n");
> >>      }
> >>  
> >>      apic_write(APIC_SPIV, value);
> >> @@ -615,21 +615,22 @@ void setup_local_APIC(void)
> >>       * TODO: set up through-local-APIC from through-I/O-APIC? --macro
> >>       */
> >>      value = apic_read(APIC_LVT0) & APIC_LVT_MASKED;
> >> -    if (!smp_processor_id() && (pic_mode || !value)) {
> >> +    if (bsp && (pic_mode || !value)) {
> >>          value = APIC_DM_EXTINT;
> >>          apic_printk(APIC_VERBOSE, "enabled ExtINT on CPU#%d\n",
> >>                      smp_processor_id());
> >>      } else {
> >>          value = APIC_DM_EXTINT | APIC_LVT_MASKED;
> >> -        apic_printk(APIC_VERBOSE, "masked ExtINT on CPU#%d\n",
> >> -                    smp_processor_id());
> >> +        if (bsp)
> >> +            apic_printk(APIC_VERBOSE, "masked ExtINT on CPU#%d\n",
> >> +                        smp_processor_id());
> > 
> > You might want to also drop the CPU#%d from the above messages, since
> > they would only be printed for the BSP.
> 
> I want to specifically keep them, so that once (if ever) we introduce
> hot-unplug support for the BSP, the same or similar messages can be
> used and matched against earlier ones in the log.
> 
> >>      }
> >>      apic_write(APIC_LVT0, value);
> >>  
> >>      /*
> >>       * only the BP should see the LINT1 NMI signal, obviously.
> >>       */
> >> -    if (!smp_processor_id())
> >> +    if (bsp)
> >>          value = APIC_DM_NMI;
> >>      else
> >>          value = APIC_DM_NMI | APIC_LVT_MASKED;
> > 
> > This would be shorter as:
> > 
> > value = APIC_DM_NMI | (bsp ? 0 : APIC_LVT_MASKED);
> 
> Indeed, at the expense of larger code churn. Seems like an at least
> partially unrelated change to me (at risk of obscuring the actual
> purpose of the change here).

FTR, I'm happy with both of the above and my RB stands.

Roger.