[SeaBIOS] [PATCH v4 3/5] error out if present cpus count changed during SMP bringup
Igor Mammedov
imammedo at redhat.com
Fri Sep 16 15:10:18 CEST 2016
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
More information about the SeaBIOS
mailing list