Re: [SeaBIOS] [PATCH 12/13] Add CSM support

[Kevin O'Connor]

On Mon, Jan 28, 2013 at 04:23:45PM +0000, David Woodhouse wrote:
> From: David Woodhouse

Thanks.  I have a few minor comments.

[...]
> --- /dev/null
> +++ b/README.CSM
> @@ -0,0 +1,20 @@
> +Enabling CONFIG_CSM allows SeaBIOS to be built as a Compatibility Support
> +Module for use with the OMVF/EDK-II UEFI firmware.

Ideally this would be added to the existing README instead of adding a
new file.

[...]
> --- a/src/boot.h
> +++ b/src/boot.h
> @@ -23,4 +23,9 @@ int bootprio_find_named_rom(const char *name, int instance);
>  struct usbdevice_s;
>  int bootprio_find_usb(struct usbdevice_s *usbdev, int lun);
>
> +// 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.

[...]
> --- /dev/null
> +++ b/src/csm.c
> @@ -0,0 +1,313 @@
> +// Compatibility Support Module (CSM) for UEFI / EDK-II
> +//
> +// Copyright © 2013 Intel Corporation

I think the copyright notice should be in ASCII unless there is a
compelling reason not to.
> +//
> +// This file may be distributed under the terms of the GNU LGPLv3 license.
>
> +#include "config.h" // CONFIG_*
> +#include "csm.h"
> +#include "util.h" // checksum
> +#include "bregs.h"
> +#include "optionroms.h"
> +#include "pci.h"
> +#include "memmap.h"
> +#include "biosvar.h"
> +#include "post.h"
> +#include "acpi.h"
> +#include "boot.h"
> +#include "smbios.h"
> +#include "pic.h"
>
> +#if CONFIG_CSM

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.
> +#define MAX_ACPI2_RSDP_SIZE 36
>
> +struct rsdp_descriptor VAR32FLATVISIBLE __aligned(16) csm_rsdp;
>
> +extern void entry_csm16(void);

This extern is dead code.
> +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.

[...]
> +/* 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.)
> /* 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.

Also, I don't understand the comment.

[...]
> // 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.

Also, SeaBIOS uses space indentation and no tabs.
> // 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?
> // EFI may have reinitialised the video using its *own* driver.
> enable_vga_console();
> // EFI fills this in for us. Zero it for now...
> struct bios_data_area_s *bda = MAKE_FLATPTR(SEG_BDA, 0);
> bda->hdcount = 0;
> device_hardware_setup();
> wait_threads();
> interactive_bootmenu();
> 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.

[...]
> +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.
> 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?
> case 0005: handle_csm_0005(regs); break;
> case 0006: handle_csm_0006(regs); break;
>
> +//    case 0007: handle_csm_0007(regs); break;
> +//    case 0008: hamdle_csm_0008(regs); break;
> default: regs->al = 1;
> }
> wait_threads();

Unless I'm missing something, the wait_threads() call shouldn't be
needed.

[...]
> +int csm_bootprio_ata(struct pci_device *pci, int chanid, int slave)
> +{
> if (!csm_boot_table)
>    return -1;
> BBS_TABLE *bbs = (void *)csm_boot_table->BbsTable;
> int index = 1 + (chanid * 2) + slave;
> dprintf(3, "CSM bootprio for ATA%d,%d (index %d) is %d\n", chanid, slave,
>    index, bbs[index].BootPriority);
> return bbs[index].BootPriority;

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.

[...]
> --- /dev/null
> +++ b/src/csm.h
> @@ -0,0 +1,13 @@
> +// Compatibility Support Module (CSM) for UEFI / EDK-II
> +//
> +// Copyright © 2013 Intel Corporation
> +//
> +// This file may be distributed under the terms of the GNU LGPLv3 license.
>
> +#include "types.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.

[...]
> --- a/src/pirtable.c
> +++ b/src/pirtable.c
> @@ -17,7 +17,7 @@ struct pir_table {
>  } PACKED;
>
> extern struct pir_table PIR_TABLE;
> -#if CONFIG_PIRTABLE && !CONFIG_COREBOOT
> +#if CONFIG_PIRTABLE && !CONFIG_COREBOOT && !CONFIG_CSM

Ideally these constraints would be set in Kconfig.

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

-Kevin