On Sat, 2013-02-02 at 20:21 -0500, Kevin O'Connor wrote:
On Mon, Jan 28, 2013 at 04:23:45PM +0000, David Woodhouse wrote:
+++ b/README.CSM
Ideally this would be added to the existing README instead of adding a new file.
OK, will fix.
+// csm.c +int csm_bootprio_fdc(struct pci_device *pci, int port, int fdid); +int csm_bootprio_ata(struct pci_device *pci, int chanid, int slave); +int csm_bootprio_pci(struct pci_device *pci);
Ideally this would be added to util.h or csm.h instead of boot.h.
OK.
+// Copyright © 2013 Intel Corporation
I think the copyright notice should be in ASCII unless there is a compelling reason not to.
Really? This far into the 21st century? When the only "failure mode" for the Luddites who *are* stuck in the 20th century is cosmetic? I'd really rather not ☺
I'd prefer to avoid #if statements. Instead, just add "if (!CONFIG_CSM) return" to handle_csm32() and let gcc and the build weed out the unused code.
OK.
+#define MAX_ACPI2_RSDP_SIZE 36 +struct rsdp_descriptor VAR32FLATVISIBLE __aligned(16) csm_rsdp; +extern void entry_csm16(void);
This extern is dead code.
As is MAX_ACPI2_RSDP_SIZE. Will fix.
+EFI_COMPATIBILITY16_TABLE csm_compat_table VAR16EXPORT __aligned(16) = {
Instead of declaring this VAR16EXPORT I think this can be declared as VAR32FLATVISIBLE. (Technically it should be a new definition as VAR32FLATEXPORT, but it wont matter in practice.) Once this is done csm.c can be moved from SRCBOTH to SRC32FLAT in the Makefile and patch 10 is no longer needed.
I thought I'd experimented with that and it hadn't been sufficient. Will re-test, and make it work if it doesn't. I have a little more clue about the linking process now.
[...]
+/* Legacy16InitializeYourself */ +void handle_csm_0000(struct bregs *regs) +{
- dprintf(3, "Legacy16InitializeYourself table %04x:%04x\n", regs->es,
regs->bx);- csm_init_table = MAKE_FLATPTR(regs->es, regs->bx);
- dprintf(3, "BiosLessThan1MB %08x\n", csm_init_table->BiosLessThan1MB);
- dprintf(3, "HiPmmMemory %08x\n", csm_init_table->HiPmmMemory);
- dprintf(3, "HiPmmMemorySize %08x\n", csm_init_table->HiPmmMemorySizeInBytes);
- dprintf(3, "ReverseThunk %04x:%04x\n", csm_init_table->ReverseThunkCallSegment,
csm_init_table->ReverseThunkCallOffset);- dprintf(3, "NumE820Entries %08x\n", csm_init_table->NumberE820Entries);
- dprintf(3, "OsMemoryAbove1M %08x\n", csm_init_table->OsMemoryAbove1Mb);
- dprintf(3, "ThunkStart %08x\n", csm_init_table->ThunkStart);
- dprintf(3, "ThunkSize %08x\n", csm_init_table->ThunkSizeInBytes);
- dprintf(3, "LoPmmMemory %08x\n", csm_init_table->LowPmmMemory);
- dprintf(3, "LoPmmMemorySize %08x\n", csm_init_table->LowPmmMemorySizeInBytes);
- csm_malloc_setup(csm_init_table->LowPmmMemory, csm_init_table->LowPmmMemorySizeInBytes,
csm_init_table->HiPmmMemory, csm_init_table->HiPmmMemorySizeInBytes);- reloc_preinit();
- interface_init();
- timer_setup();
- pci_probe_devices();
- csm_compat_table.PnPInstallationCheckSegment = SEG_BIOS;
- csm_compat_table.PnPInstallationCheckOffset = get_pnp_offset();
- regs->ax = 0;
+}
[...]
+/* PrepareToBoot */ +void handle_csm_0002(struct bregs *regs) +{
- dprintf(3, "PrepareToBoot table %04x:%04x\n", regs->es, regs->bx);
Out of curiosity, does the e820 map, pirtable init, and pmtimer init need to happen here instead of in handle_csm_0000()? Normally these types of things would be done earlier. (I can also envision optionroms trying to access some of this info.)
The E820 map and PIR table are allocated by calls to our GetTableAddress() function, so they're not present when the InitializeYourself() function is first called. The ACPI table is different, since it's copied into a static buffer that we provide, but still it's not done until later.
In general, the CSM design seems to be assuming very well-behaved option ROMs that don't do *anything* out of the ordinary. I'm receiving reports that even doing timer_init() in the first call is causing problems, and we shouldn't be touching real hardware until we are actually booting. If an option ROM requires us to have functional timers, or even keyboard, then I think it's just not expected to work.
I don't really know a lot more than you about it though, except what I can glean from the specification PDF (not a lot), and from reading through the code paths on the OVMF side (only a little bit more).
There might be *some* scope for making things a little saner, but I don't think there's a massive appetite for it.
- /* La la la */
- dprintf(3, "%d e820 entries (%d bytes)`n", csm_compat_table.E820Length / sizeof(struct e820entry), csm_compat_table.E820Length);
- memcpy(e820_list, (void *)csm_compat_table.E820Pointer, csm_compat_table.E820Length);
- e820_count = csm_compat_table.E820Length / sizeof(struct e820entry);
I think it would be better to loop through the table and call add_e820() - this will ensure the seabios invariants (sort order, overlap checks, CONFIG_MAX_E820 checks, etc) are still run.
OK, I'll have a go at that.
Also, I don't understand the comment.
Oh, that's a hangover from when the function just said /* la la la */ and returned success without doing anything. I'll remove it.
[...]
- // For find_resume_vector()
- if (csm_rsdp.signature == RSDP_SIGNATURE) {
RsdpAddr = &csm_rsdp;dprintf(3, "CSM ACPI RSDP at %p\n", PirAddr);// Find PM timer from ACPI tables provided by UEFI// XXX: Theoretically, we should use XSDT and X_PM_TMR_BLK.if (csm_rsdp.rsdt_physical_address) {u32 *rsdt = (u32 *)csm_rsdp.rsdt_physical_address;int i;for (i = 9; i < rsdt[1] / 4; i++) {struct fadt_descriptor_rev1 *fadt = (void *)rsdt[i];u32 pm_tmr;if (fadt->signature != FACP_SIGNATURE)continue;pm_tmr = fadt->pm_tmr_blk;if (!pm_tmr)break;pmtimer_setup(PORT_ACPI_PM_BASE + 0x08, 3579);}- }
- }
Ideally this would be put in a function as I think Xen and Coreboot could make use of this as well.
OK.
Also, SeaBIOS uses space indentation and no tabs.
Oops. I'd fixed that once (but not taught emacs about it), and then remembered that I had to go back and fix this instead of just pulling the numbers out of my posterior for calling pmtimer_init(). Will fix.
- // SMBIOS table needs to be copied into the f-seg
- // XX: OVMF doesn't seem to set SmbiosTableLength so don't check it
- if (csm_boot_table->SmbiosTable && !SMBiosAddr)
copy_smbios((void *)csm_boot_table->SmbiosTable);Out of curiosity, why does the smbios table need to be copied when the pir and mptables do not?
$DEITY knows.
- wait_threads();
Minor, but there's no need for the additional wait_threads() call. It's only in the maininit() code path because of the possibility that CONFIG_THREAD_OPTIONROMS is set.
OK, will kill it.
[...]
+static u8 pic_mask[2] = { ~PIC1_IRQ2, ~0 };
+void VISIBLE32FLAT +handle_csm32(struct bregs *regs) +{
- ASSERT32FLAT();
- dprintf(3, "handle_csm16 regs %p AX=%04x (PIC %02x%02x)\n", regs, regs->ax,
pic_mask[0], pic_mask[1]);- outb(pic_mask[0], PORT_PIC1_DATA);
- outb(pic_mask[1], PORT_PIC2_DATA);
Wow, that's ugly. Ideally OVMF would do that. But, I understand why it's needed. I think it would be better to make a pic_save() and pic_restore() which lived in pic.c/h though.
Yeah, this is horrid. I wonder if we could declare it to be a bug on the OVMF side that it *doesn't* save and restore the mask. At least between PrepareToBoot() and Boot() calls. Or maybe we're not *supposed* to have enabled interrupts until we finally get the Boot() call?
- switch(regs->ax) {
- case 0000: handle_csm_0000(regs); break;
- case 0001: handle_csm_0001(regs); break;
- case 0002: handle_csm_0002(regs); break;
- case 0003: handle_csm_0003(regs); break;
+// case 0004: handle_csm_0004(regs); break;
Is there a plan to implement the 4/7/8 calls?
Eventually, once we work out what they're actually for and when they get called. So far I've just gone through and implemented the functions as and when I've seen OVMF actually call them...
- wait_threads();
Unless I'm missing something, the wait_threads() call shouldn't be needed.
Right. I'll kill it.
+int csm_bootprio_ata(struct pci_device *pci, int chanid, int slave)
Does UEFI really guarantee the order of the entries in the table (floppy, then ata, then pci)? Otherwise, the code could loop through the table (as csm_bootprio_pci does) and inspect DeviceType.
The BBS_TABLE has no 'lun' or equivalent field. You can't tell the difference between the various devices that might be attached to a given PCI bus/dev/func. This is insane and I'm trying to get it fixed. For now, the *only* way we can infer which IDE device they're talking about, is from its position in the array. This is not supported by the spec, but it's all we have for now.
+++ b/src/csm.h
Due to quirks in the SeaBIOS build (it textually includes files together at build) all headers really should have an include guard.
Also, I'd prefer not to put copyright notices on header files that just contain boilerplate definitions.
OK.
-#if CONFIG_PIRTABLE && !CONFIG_COREBOOT +#if CONFIG_PIRTABLE && !CONFIG_COREBOOT && !CONFIG_CSM
Ideally these constraints would be set in Kconfig.
OK. I think that just making PIRTABLE depend on !CSM should suffice, although I have no idea about the COREBOOT case.
[...]
--- a/src/romlayout.S +++ b/src/romlayout.S @@ -468,6 +468,43 @@ irqentryarg: DECL_IRQ_ENTRY hwpic1 DECL_IRQ_ENTRY hwpic2
+#if CONFIG_CSM
I'd avoid the #if and waste the few bytes of assembler - just make sure handle_csm32() has a "if (!CONFIG_CSM) return" test.
I think we also mentioned that this entry point didn't really need to be in the F-segment?
Will put together a new patch with most of the above fixed; thanks for the detailed review.