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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.05.2020 12:01, Roger Pau Monné wrote:
> 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>

Thanks.

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

I've added "No functional change from this" to the last paragraph.

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

Jan