[Qemu-devel] Re: [Bochs-developers] [PATCH 2/6] Add S3 state to DSDT. Handleresumeevent in the BIOS.

["Sebastian Herbszt" <herbszt@gmx.de>]

Gleb Natapov wrote:
> On Thu, Oct 30, 2008 at 11:41:28PM +0100, Sebastian Herbszt wrote:
>> Gleb Natapov wrote:
>>> diff --git a/bios/rombios.c b/bios/rombios.c
>>> index 88eac04..46996fa 100644
>>> --- a/bios/rombios.c
>>> +++ b/bios/rombios.c
>>> @@ -2198,6 +2198,31 @@ debugger_off()
>>>   outb(0xfedc, 0x00);
>>> }
>>>
>>> +void
>>> +s3_resume()
>>> +{
>>> +    Bit32u s3_wakeup_vector;
>>> +    Bit8u s3_resume_flag;
>>> +
>>> +    s3_resume_flag = read_byte(0x40, 0xb0);
>>> +    s3_wakeup_vector = read_dword(0x40, 0xb2);
>>> +
>>> +    BX_INFO("S3 resume called %x 0x%lx\n", s3_resume_flag, s3_wakeup_vector);
>>> +    if (s3_resume_flag != 0xFE || !s3_wakeup_vector)
>>> +     return;
>>> +
>>> +    write_byte(0x40, 0xb0, 0);
>>> +
>>> +    /* setup wakeup vector */
>>> +    write_word(0x40, 0xb6, (s3_wakeup_vector & 0xF)); /* IP */
>>> +    write_word(0x40, 0xb8, (s3_wakeup_vector >> 4)); /* CS */
>>
>> Any reason not to use 0040h:0067h instead?
>>
> It is OS visible. OS can assume that a value written there will stay
> there till overwritten by the OS itself.

0040h:00B6h is as much OS visiable as the "RESET RESTART ADDRESS".
The former has no standard definition tho and could or not be used
by the OS or BIOS. The "RESET RESTART ADDRESS" is used by the
(commercial) BIOS together with the CMOS "Shutdown Status Byte" for
internal purpose. I am not sure whether the OS expectation is valid.
This however is a minor detail which can be changed later if needed.

>>> +
>>> +    if (*shutdown_flag == 0xfe) {
>>> +        *s3_resume_vector = find_resume_vector();
>>> +        if (!*s3_resume_vector) {
>>> +         BX_INFO("This is S3 resume but wakeup vector is NULL\n");
>>> +        } else {
>>> +         BX_INFO("S3 resume vector %p\n", *s3_resume_vector);
>>> +            /* redirect bios read access to RAM */
>>> +            pci_for_each_device(find_440fx);
>>> +            bios_lock_shadow_ram(); /* bios is already copied */
>>
>> bios_shadow_init() is called in pci_bios_init(). Why do we need to lock it here?
>>
> Because we don't call pci_bios_init() if it is S3 resume.

If pci_bios_init() is not called on S3 resume, pci_bios_init_bridges()
is not called either. This way bios_shadow_init() is never called and the
bios is never shadowed. Since it's not shadowed there is no need to lock it.
What do i miss here?

>>> +            return;
>>> +        }
>>> +    }
>>> +
>>> +    uuid_probe();
>>> +
>>>     pci_bios_init();
>>>
>>>     if (bios_table_cur_addr != 0) {
>>>
>>
>> rombios32.c r1.32 got:
>>
>>    smp_probe();
>>
>>    pci_bios_init();
>>
>>    if (bios_table_cur_addr != 0) {
>>
>>        mptable_init();
>>
>>        uuid_probe();
>>
>> The patch should remove uuid_probe() from the if-case.
>>
> Why? uuid_probe() is needed only if SMBIOS tables are built. And
> smbios_init() call is in the same if.

Your patch does also add a uuid_probe() before the if-case. Since it does
not remove the one in the if-case, uuid_probe() will be called twice.

> BTW judging by this if it
> looks like BX_USE_EBDA_TABLES is not supported today too since
> bios_table_cur_addr is always zero in this case. 

find_bios_table_area() is supposed to also succeed in the
BX_USE_EBDA_TABLES case because bios_table_area_start in rombios.c
is unconditional. In the BX_USE_EBDA_TABLES case mptable_init() does use
mp_config_table and smbios_init() does use ebda_cur_addr which is set in
ram_probe().

- Sebastian