On Fri, 16 Sep 2016 08:55:49 -0400 "Kevin O'Connor" <[email protected]> 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 <[email protected]> > > --- > > 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/[email protected]/msg390671.html and I'm preparing rebased variant for posting next week to qemu-devel. > > -Kevin _______________________________________________ SeaBIOS mailing list [email protected] https://www.coreboot.org/mailman/listinfo/seabios
