[SeaBIOS] [PATCH v4 3/5] error out if present cpus count changed during SMP bringup

[Igor Mammedov]

On Fri, 16 Sep 2016 08:55:49 -0400
"Kevin O'Connor" <kevin at koconnor.net> wrote:

> On Fri, Sep 16, 2016 at 01:54:16PM +0200, Igor Mammedov wrote:
> > if during SMP bringup a cpu is hotplugged, Seabios might
> > silently hung in
> > 
> >   while (cmos_smp_count != CountCPUs)
> > 
> > loop as cmos_smp_count might be less then CountCPUs if
> > SIPI were delivered to the hotplugged CPU.
> > 
> > Warn user about it and ask for reboot.
> > 
> > While at it rename CountCPUs to BroughtUpCPUs to make clear
> > what it is counting.
> > 
> > Signed-off-by: Igor Mammedov <imammedo at redhat.com>
> > ---
> >  src/fw/smp.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/fw/smp.c b/src/fw/smp.c
> > index 31bcc6a..9040b01 100644
> > --- a/src/fw/smp.c
> > +++ b/src/fw/smp.c
> > @@ -46,7 +46,7 @@ smp_write_msrs(void)
> >  }
> >  
> >  u32 MaxCountCPUs;
> > -static u32 CountCPUs;
> > +static u32 BroughtUpCPUs;
> >  // 256 bits for the found APIC IDs
> >  static u32 FoundAPICIDs[256/32];
> >  
> > @@ -78,7 +78,7 @@ handle_smp(void)
> >  
> >      smp_write_msrs();
> >  
> > -    CountCPUs++;
> > +    BroughtUpCPUs++;
> >  }
> >  
> >  // Atomic lock for shared stack across processors.
> > @@ -95,13 +95,13 @@ smp_scan(void)
> >      if (eax < 1 || !(cpuid_features & CPUID_APIC)) {
> >          // No apic - only the main cpu is present.
> >          dprintf(1, "No apic - only the main cpu is present.\n");
> > -        CountCPUs= 1;
> > +        BroughtUpCPUs= 1;
> >          return;
> >      }
> >  
> >      // mark the BSP initial APIC ID as found, too:
> >      apic_id_init();
> > -    CountCPUs = 1;
> > +    BroughtUpCPUs = 1;
> >  
> >      // Setup jump trampoline to counter code.
> >      u64 old = *(u64*)BUILD_AP_BOOT_ADDR;
> > @@ -131,8 +131,8 @@ smp_scan(void)
> >      writel(APIC_ICR_LOW, 0x000C4600 | sipi_vector);
> >  
> >      // Wait for other CPUs to process the SIPI.
> > -    u8 cmos_smp_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
> > -    while (cmos_smp_count != CountCPUs)
> > +    u8 smp_count, smp_count_initial = rtc_read(CMOS_BIOS_SMP_COUNT) + 1;
> > +    while ((smp_count = rtc_read(CMOS_BIOS_SMP_COUNT) + 1) != BroughtUpCPUs) {
> >          asm volatile(
> >              // Release lock and allow other processors to use the stack.
> >              "  movl %%esp, %1\n"
> > @@ -143,12 +143,15 @@ smp_scan(void)
> >              "  jc 1b\n"
> >              : "+m" (SMPLock), "+m" (SMPStack)
> >              : : "cc", "memory");
> > +        if (smp_count != smp_count_initial)
> > +            dprintf(1, "Error: count of present cpus changed, reboot manually");
> > +    }  
> 
> I'm not sure about this patch.  No one reads the debug log, so this
> error message seems unlikely to be seen; in the case someone did
> forward the debug log to disk, this loop could rapidly fill it up.
> 
> Perhaps separate out this patch from the rest of the series as it
> seems separate.
Sure, we can drop it for now and think about a beet way to handle race
later.
I can post [v5 5/5] as reply here since dropping this patch will cause
some trivial conflicts or respin whole series with amended 5/5
(whichever you prefer).
 
> The rest of the series looks good to me.  Is there a dependency on
> pending QEMU patches?
There is older version of QEMU patches
 https://www.mail-archive.com/qemu-devel@nongnu.org/msg390671.html
and I'm preparing rebased variant for posting next week to qemu-devel.

> 
> -Kevin