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

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

On Fri, Mar 13, 2020 at 10:26:47AM +0100, Jan Beulich wrote:
> All CPUs get an equal setting of EOI broadcast suppression; no need to
> log one message per CPU, even if it's only in verbose APIC mode.
> 
> Only the BSP is eligible to possibly get ExtINT enabled; no need to log
> that it gets disabled on all APs, even if - again - it's only in verbose
> APIC mode.
> 
> Take the opportunity and introduce a "bsp" parameter to the function, to
> stop using smp_processor_id() to tell BSP from APs.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

AFAICT this doesn't introduce any functional change in APIC setup or
behavior, the only functional change is the log message reduction.
Might be good to add a note to that effect to make this clear, since
the change from smp_processor_id() -> bsp might make this not obvious.

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

>      }
>      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);

Not specially trilled anyway.

Thanks, Roger.