From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KwZpC-00077d-If for qemu-devel@nongnu.org; Sun, 02 Nov 2008 05:04:50 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KwZpA-00075q-Sr for qemu-devel@nongnu.org; Sun, 02 Nov 2008 05:04:49 -0500 Received: from [199.232.76.173] (port=38489 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KwZpA-00075X-60 for qemu-devel@nongnu.org; Sun, 02 Nov 2008 05:04:48 -0500 Received: from mx2.redhat.com ([66.187.237.31]:53585) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KwZp9-0006wj-Mf for qemu-devel@nongnu.org; Sun, 02 Nov 2008 05:04:47 -0500 Date: Sun, 2 Nov 2008 12:04:32 +0200 From: Gleb Natapov Message-ID: <20081102100432.GB16809@redhat.com> References: <20081027101249.21464.57377.stgit@gleb-debian.qumranet.com.qumranet.com> <20081027101259.21464.36016.stgit@gleb-debian.qumranet.com.qumranet.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [Bochs-developers] [PATCH 2/6] Add S3 state to DSDT. Handle resumeevent in the BIOS. Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sebastian Herbszt Cc: bochs-developers@lists.sourceforge.net, qemu-devel@nongnu.org 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.