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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.04.2020 19:55, Andrew Cooper wrote:
> On 13/03/2020 09:26, 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.
> 
> On further consideration, this is introducing a latent bug.
> 
> In a theoretical world where we could take the BSP offline, it is still
> the CPU with the ID 0 which needs various of these things setting back up.

No. If we took the BSP offline, no CPU with ID 0 would remain
(until such time that the ID would be re-used).

> You could argue that we could move ExtINT/NMI handling to a different
> CPU, and in this case, BSP still isn't the right distinction.  We'd want
> something to signify "the processor which is the target of legacy
> interrupts", as in such a case, it would specifically no longer be the
> CPU we booted on.

I see nothing wrong with calling this new "focus" processor the
BSP again. Post boot the significance of BSP, afaict, reduces
to aspects that aren't really tied to having been the processor
to lead bootup.

The important point is - if we were to allow offlining the BSP,
whichever other one would take its position would _not_ come
through setup_local_APIC(). The adjustments needs would need
doing by other means (quite likely by splitting out some of the
code into a new helper). Hence in setup_local_APIC() itself the
BSP / non-BSP distinction is correct.

> OTOH, the adjustment for the NMI watchdog does look to be different. 
> AFAICT, that is for deferring the watchdog setup until later in boot, at
> which point "the BSP" is the appropriate distinction to use.  (That said
> - I'm not sure why anything should need delaying.  I suspect this is
> misplaced code to begin with.)

Well, in any event - an unrelated aspect, and I think the switch
from smp_processor_id() to bsp is correct there as well.

> As for the messages being printed, I think that is fine to restrict to
> the BSP.

As per above I understand this isn't an ack for the patch. However,
I then don't understand what concrete changes you mean me to make
for it to stand a chance of getting your ack.

Jan