Re: [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"  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
> >
> > 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/[e-mail]/msg390671.html
and I'm preparing rebased variant for posting next week to qemu-devel.
> -Kevin