Re: [SeaBIOS] [PATCH v3 2/2] Boot Linux using QEMU fw_cfg DMA interface

[Kevin O'Connor]

On Fri, Sep 18, 2015 at 10:59:15AM +0200, Marc Marí wrote:
> Reading Linux from the fw_cfg interface is faster using the DMA interface.
> For this reason, add a Linux loader that can benefit from this interface.
> Signed-off-by: Marc Marí
>
> src/boot.c        | 32 ++++++++++++++++-----
>  src/fw/paravirt.c | 85 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  src/fw/paravirt.h | 36 +++++++++++++++++++++++
>  src/post.c        |  1 +
>  src/romlayout.S   | 20 +++++++++++++
>  src/util.h        |  1 +
>  6 files changed, 152 insertions(+), 23 deletions(-)
>
> diff --git a/src/boot.c b/src/boot.c
> index e0f73a3..52918f8 100644
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -300,13 +300,14 @@ struct bootentry_s {
>  };
>  static struct hlist_head BootList VARVERIFY32INIT;
>
> -#define IPL_TYPE_FLOPPY      0x01
> -#define IPL_TYPE_HARDDISK    0x02
> -#define IPL_TYPE_CDROM       0x03
> -#define IPL_TYPE_CBFS        0x20
> -#define IPL_TYPE_BEV         0x80
> -#define IPL_TYPE_BCV         0x81
> -#define IPL_TYPE_HALT        0xf0
> +#define IPL_TYPE_FLOPPY         0x01
> +#define IPL_TYPE_HARDDISK       0x02
> +#define IPL_TYPE_CDROM          0x03
> +#define IPL_TYPE_CBFS           0x20
> +#define IPL_TYPE_QEMU_VMLINUX   0x21
> +#define IPL_TYPE_BEV            0x80
> +#define IPL_TYPE_BCV            0x81
> +#define IPL_TYPE_HALT           0xf0
>
> static void
>  bootentry_add(int type, int prio, u32 data, const char *desc)
> @@ -398,6 +399,12 @@ boot_add_cbfs(void *data, const char *desc, int prio)
>      bootentry_add(IPL_TYPE_CBFS, defPrio(prio, DEFAULT_PRIO), (u32)data, desc);
>  }
>
> +// Add a FW_CFG DMA boot
> +void
> +boot_add_qemu_vmlinux(const char *desc, int prio)
> +{
> bootentry_add(IPL_TYPE_QEMU_VMLINUX, defPrio(prio, DEFAULT_PRIO), 0, desc);
>
> +}
>
> /****************************************************************
> Keyboard calls
>
> @@ -684,6 +691,14 @@ boot_rom(u32 vector)
>      call_boot_entry(so, 0);
>  }
>
> +// Boot from a linuxboot ROM when QEMU cfg is in DMA mode
> +static void
> +boot_qemu_vmlinux_dma(void)
> +{
> printf("Booting Linux from fw_cfg...\n");
> qemu_vmlinux_dma_boot();
>
> +}
>
> // Unable to find bootable device - warn user and eventually retry.
>  static void
>  boot_fail(void)
> @@ -734,6 +749,9 @@ do_boot(int seq_nr)
>      case IPL_TYPE_BEV:
>          boot_rom(ie->vector);
>          break;
> case IPL_TYPE_QEMU_VMLINUX:
>    boot_qemu_vmlinux_dma();
>    break;
> case IPL_TYPE_HALT:
>        boot_fail();
>        break;
>
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 65f9fba..6896511 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -23,7 +23,8 @@
>  #include "util.h" // pci_setup
>  #include "x86.h" // cpuid
>  #include "xen.h" // xen_biostable_setup
> -#include "stacks.h" // yield
> +#include "bregs.h" // struct bregs
> +#include "stacks.h" // farcall16big, yield
>
> // Amount of continuous ram under 4Gig
>  u32 RamSize;
> @@ -185,21 +186,6 @@ qemu_platform_setup(void)
> QEMU firmware config (fw_cfg) interface
>
> ****************************************************************/
>
> -// List of QEMU fw_cfg entries.  DO NOT ADD MORE.  (All new content
> -// should be passed via the fw_cfg "file" interface.)
> -#define QEMU_CFG_SIGNATURE              0x00
> -#define QEMU_CFG_ID                     0x01
> -#define QEMU_CFG_UUID                   0x02
> -#define QEMU_CFG_NUMA                   0x0d
> -#define QEMU_CFG_BOOT_MENU              0x0e
> -#define QEMU_CFG_MAX_CPUS               0x0f
> -#define QEMU_CFG_FILE_DIR               0x19
> -#define QEMU_CFG_ARCH_LOCAL             0x8000
> -#define QEMU_CFG_ACPI_TABLES            (QEMU_CFG_ARCH_LOCAL + 0)
> -#define QEMU_CFG_SMBIOS_ENTRIES         (QEMU_CFG_ARCH_LOCAL + 1)
> -#define QEMU_CFG_IRQ0_OVERRIDE          (QEMU_CFG_ARCH_LOCAL + 2)
> -#define QEMU_CFG_E820_TABLE             (QEMU_CFG_ARCH_LOCAL + 3)
>
> static void
>  qemu_cfg_select(u16 f)
>  {
> @@ -509,3 +495,70 @@ void qemu_cfg_init(void)
>          dprintf(1, "Moving pm_base to 0x%x\n", acpi_pm_base);
>      }
>  }
>
> +void qemu_vmlinux_dma_boot(void)
> +{
> dprintf(1, "Loading kernel\n");
> irq_disable();

SeaBIOS always disabled interrupts in C code (see
http://www.seabios.org/Execution_and_code_flow#Hardware_interrupts ),
so this irq_disable() is not needed.
> void *setup_addr;
> u32 setup_size;
> qemu_cfg_read_entry(&setup_addr, QEMU_CFG_SETUP_ADDR, 4);
> qemu_cfg_read_entry(&setup_size, QEMU_CFG_SETUP_SIZE, 4);
> qemu_cfg_read_entry(setup_addr, QEMU_CFG_SETUP_DATA, setup_size);
> u16 protocol = readw(setup_addr + 0x206);
> if (protocol < 0x203) {
>    /* Assume initrd_max 0x37ffffff */
>    writel(setup_addr + 0x22c, 0x37ffffff);
> }
> void *initrd_addr;
> u32 initrd_size;
> qemu_cfg_read_entry(&initrd_addr, QEMU_CFG_INITRD_ADDR, 4);
> qemu_cfg_read_entry(&initrd_size, QEMU_CFG_INITRD_SIZE, 4);
> u32 initrd_end_page = (u32)(initrd_addr + initrd_size) & -4096;
> u32 max_allowed_page = readl(setup_addr + 0x22c) & -4096;

Use of the ALIGN_DOWN() macro would be preferable here.
> if (initrd_end_page != max_allowed_page) {
>    /* Initrd at the end of memory. Compute better initrd address
>     * based on e801 data
>     */
>    initrd_addr = (void *)((LegacyRamSize - initrd_size) & -4096);
>    writel(setup_addr + 0x218, (u32)initrd_addr);
> }
> qemu_cfg_read_entry(initrd_addr, QEMU_CFG_INITRD_DATA, initrd_size);
> void *kernel_addr;
> u32 kernel_size;
> qemu_cfg_read_entry(&kernel_addr, QEMU_CFG_KERNEL_ADDR, 4);
> qemu_cfg_read_entry(&kernel_size, QEMU_CFG_KERNEL_SIZE, 4);
> qemu_cfg_read_entry(kernel_addr, QEMU_CFG_KERNEL_DATA, kernel_size);
> void *cmdline_addr;
> u32 cmdline_size;
> qemu_cfg_read_entry(&cmdline_addr, QEMU_CFG_CMDLINE_ADDR, 4);
> qemu_cfg_read_entry(&cmdline_size, QEMU_CFG_CMDLINE_SIZE, 4);
> qemu_cfg_read_entry(cmdline_addr, QEMU_CFG_CMDLINE_DATA, cmdline_size);
> dprintf(1, "Jumping to kernel %d@%x %d@%x %d@%x %d@%x\n"
>        , setup_size, (u32)setup_addr, cmdline_size, (u32)cmdline_addr
>        , kernel_size, (u32)kernel_addr, initrd_size, (u32)initrd_addr);
> struct bregs br;
> memset(&br, 0, sizeof(br));
> extern void kernel_stub(void);
> br.ebx = (u32)setup_addr >> 4;
> br.edx = (u32)cmdline_addr - (u32)setup_addr - 16;
> br.code = SEGOFF(SEG_BIOS, (u32)kernel_stub - BUILD_BIOS_ADDR);
> farcall16big(&br);
>
> +}
>
> +void qemu_vmlinux_dma_setup(void) {
> if (qemu_cfg_dma_enabled()) {

I don't think this feature should depend on DMA (it works fine even if
fw_cfg DMA is not available).  I also don't think "dma" should be in
the function names - qemu_vmlinux_setup() is fine.

Also, the code should check if there is a kernel before registering an
IPL - for example:

u32 kernel_size;
    qemu_cfg_read_entry(&kernel_size, QEMU_CFG_KERNEL_SIZE, sizeof(kernel_size));
    if (!kernel_size)
        return;
>    boot_add_qemu_vmlinux("QEMU Kernel image",
>                    bootprio_find_named_rom("genroms/linuxboot.bin", 0));

Using "genroms/linuxboot.bin" works for testing, but I think a new
bootorder id (eg, "vmlinux") will need to be defined between QEMU and
SeaBIOS.  I don't think it would be good to overload the meaning of
the "genroms/" prefix.

Otherwise the series looks good to me.  Once the QEMU series is
committed we can commit this series to SeaBIOS.

-Kevin