[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [Bochs-developers] [PATCH 2/6] Add S3 state to DSDT. Ha
From: |
Gleb Natapov |
Subject: |
[Qemu-devel] Re: [Bochs-developers] [PATCH 2/6] Add S3 state to DSDT. Handle resumeevent in the BIOS. |
Date: |
Sun, 2 Nov 2008 12:04:32 +0200 |
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.
>> -void rombios32_init(void)
>> +static uint32_t find_resume_vector(void)
>> {
>> - BX_INFO("Starting rombios32\n");
>> + unsigned long addr;
>> +
>> + if (bios_table_cur_addr == 0)
>> + return 0;
>
> BX_USE_EBDA_TABLES case
>
>> +
OK.
>> + for (addr = bios_table_cur_addr; addr < bios_table_end_addr; addr++) {
>> + if (!memcmp((void*)addr, "RSD PTR ", 8)) {
>> + struct rsdp_descriptor *rsdp = (void*)addr;
>> + struct rsdt_descriptor_rev1 *rsdt =
>> (void*)rsdp->rsdt_physical_address;
>> + struct fadt_descriptor_rev1 *fadt =
>> (void*)rsdt->table_offset_entry[0];
>> + struct facs_descriptor_rev1 *facs = (void*)fadt->firmware_ctrl;
>> + return facs->firmware_waking_vector;
>> + }
>> + }
>> +
>
> RSDP is on a 16-byte boundary
>
OK.
>> + return 0;
>> +}
>> +
>> +static void find_440fx(PCIDevice *d)
>> +{
>> + uint16_t vendor_id, device_id;
>> +
>> + vendor_id = pci_config_readw(d, PCI_VENDOR_ID);
>> + device_id = pci_config_readw(d, PCI_DEVICE_ID);
>> +
>> + if (vendor_id == 0x8086 && device_id == 0x1237)
>> + i440_pcidev = *d;
>
> Please use PCI_VENDOR_ID_INTEL and PCI_DEVICE_ID_INTEL_82441.
>
OK.
>> +}
>> +
>> +void rombios32_init(uint32_t *s3_resume_vector, uint8_t *shutdown_flag)
>> +{
>> + BX_INFO("Starting rombios32 %p %p %x\n", s3_resume_vector,
>> shutdown_flag, *shutdown_flag);
>
> Maybe make it two lines and explain the values for the log:
>
> BX_INFO("Starting rombios32\n");
> BX_INFO("s3_resume_vector=%p ... \n", s3_resume_vector, ...
>
OK. Actually printing *shutdown_flag is enough. Pointer are always
same and were needed for debug only.
>>
>> #ifdef BX_QEMU
>> qemu_cfg_port = qemu_cfg_port_probe();
>> @@ -2025,6 +2070,24 @@ void rombios32_init(void)
>>
>> smp_probe();
>>
>> + if (find_bios_table_area() < 0)
>> + return;
>
> It should not return if BX_USE_EBDA_TABLES is defined.
> Is there a need to return at all?
>
Indeed. No need to return here.
>> +
>> + 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.
>> + 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. 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.
--
Gleb.
- [Qemu-devel] Re: [Bochs-developers] [PATCH 2/6] Add S3 state to DSDT. Handle resumeevent in the BIOS.,
Gleb Natapov <=