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.
