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

[Kevin O'Connor]

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.

The rest of the series looks good to me.  Is there a dependency on
pending QEMU patches?

-Kevin