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

[Igor Mammedov]

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.

> 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.

> 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