[SeaBIOS] [PATCH 1/3] Halt if number of started cpus are more then expected

[Gleb Natapov]

On Mon, Mar 12, 2012 at 05:04:48PM +0100, Igor Mammedov wrote:
> On 03/12/2012 02:27 PM, Gleb Natapov wrote:
> >On Mon, Mar 12, 2012 at 02:09:02PM +0100, Igor Mammedov wrote:
> >>On 03/12/2012 11:14 AM, Gleb Natapov wrote:
> >>>On Mon, Mar 12, 2012 at 10:52:18AM +0100, Igor Mammedov wrote:
> >>>>On 03/11/2012 11:36 AM, Gleb Natapov wrote:
> >>>>>On Sat, Mar 10, 2012 at 12:47:26PM +0100, Igor Mammedov wrote:
> >>>>>>Reduce amount of consumed cpu time (i.e. don't spin forever) if
> >>>>>>number of started cpus are more then expected. And print a bug
> >>>>>>message into debug port.
> >>>>>>
> >>>>>>Signed-off-by: Igor Mammedov<imammedo at redhat.com>
> >>>>>>---
> >>>>>>  src/smp.c |    8 +++++++-
> >>>>>>  1 files changed, 7 insertions(+), 1 deletions(-)
> >>>>>>
> >>>>>>diff --git a/src/smp.c b/src/smp.c
> >>>>>>index 8c077a1..9933ac6 100644
> >>>>>>--- a/src/smp.c
> >>>>>>+++ b/src/smp.c
> >>>>>>@@ -115,8 +115,14 @@ smp_probe(void)
> >>>>>>          msleep(10);
> >>>>>>      } else {
> >>>>>>          u8 cmos_smp_count = inb_cmos(CMOS_BIOS_SMP_COUNT);
> >>>>>>-        while (cmos_smp_count + 1 != readl(&CountCPUs))
> >>>>>>+        while (cmos_smp_count + 1 != readl(&CountCPUs)) {
> >>>>>>+            if (cmos_smp_count + 1<    readl(&CountCPUs)) {
> >>>>>>+                dprintf(1, "BUG: Expected %d cpu(s) but %d cpus started\n",
> >>>>>>+                        cmos_smp_count + 1, readl(&CountCPUs));
> >>>>>Shouldn't we print it to the console too?
> >>>>I'm not sure if it's possible to print at this stage, but I'll try and
> >>>>re-post if it works.
> >>>>
> >>>If it is not possible currently may be we should move smp detection to
> >>>later stage. Or do not hlt here and report error later. The easier for a
> >>>user to see this error the easier it is for us to debug user's problems.
> >>
> >>It looks safe to move vga_setup() right after pci_setup() in post().
> >>However I can't tell what structures various option roms might access so
> >>I'm not completely sure if it right thing to do.
> >>
> >Well you move it above "Setup interfaces that option roms may need"
> >comment, so we can be sure it is not safe :)
> Yep, sorry my bad.
> 
> >
> >I think moving smp detection to later stage should be safer. But now I am
> we can't move it past init_bios_tables() and this function is is under comment
> "// Setup interfaces that option roms may need" so I'm not sure we can move
> vga_setup before it.
> 
I think init_bios_tables() is not required for option rom to work. Not
sure though. Kevin?

> >not even sure we should halt at all. May be warn the user and continue
> 
> We need to stop because otherwise user won't event notice that there was
> an error and only later may notice not expected cpus count in OS. BIOSes
> usually stop in case of error and you have to override this behavior in
> settings (something like this: boot if keyboard is not present).
> 
> Perhaps we could safely clamp CountCPUs to 1 + alter smp_ap_boot_code()
> to stop counting in case of error and continue to boot till we will be able
> to print error message and stop after that.
> Yep, error message is nice to have but I'm not sure if this one case is
> worth of inventing deferred print infrastructure. Support personnel might
> just enable logging from debug port if BIOS is suspected (it is easy to
> spot, we hang right on boot). That's what they have to do now in case of
> any BIOS related problems.
> 
Since this error should not really happen IRL I agree we should not spend to
much time on new infrastructure.

> >booting with cmos_smp_count = CountCPUs?
> 
> I guess is was this way some time ago 31bfad632b0ca and with large cpu counts
> it wasn't reliable to wait XX ms while all cpus initialize, that why cmos_smp_count
> was introduced and bios waits on exact count.
> 
> The halt condition is meant for buggy emulators so if we get more cpus then
> expected it is safer to just stop then experience inexplicable bugs later
> (I can imagine having mp, smbios and acpi tables built with different CountCPUs,
> and maybe there is other dependencies).
> 
> 
> >--
> >			Gleb.
> 
> -- 
> -----
>  Igor

--
			Gleb.