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/qemu-devel@nongnu.org/msg390671.html
and I'm preparing rebased variant for posting next week to qemu-devel.

> 
> -Kevin


___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios

Re: [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 
> ---
>  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

___
SeaBIOS mailing list
SeaBIOS@seabios.org
https://www.coreboot.org/mailman/listinfo/seabios