Re: [SeaBIOS] [PATCH 3/3] Accept file supplied as multiboot modules

[Kevin O'Connor]

On Mon, May 18, 2015 at 08:07:56PM +0000, Vladimir 'phcoder' Serbinenko wrote:
>
> diff --git a/src/fw/coreboot.c b/src/fw/coreboot.c
> index 7815de4..df940cb 100644
> --- a/src/fw/coreboot.c
> +++ b/src/fw/coreboot.c
> @@ -17,6 +17,7 @@
>  #include "stacks.h" // yield
>  #include "string.h" // memset
>  #include "util.h" // coreboot_preinit
> +#include "multiboot.h"
>
> /****************************************************************
> @@ -462,6 +463,67 @@ coreboot_cbfs_init(void)
>      process_links_file();
>  }
>
> +static int
> +extract_filename(char *dest, char *src, size_t lim)
> +{
> char *ptr;
> for (ptr = src; *ptr; ptr++)
> {
>  if (!(ptr == src || ptr[-1] == ' ' || ptr[-1] == '\t'))
> continue;

This code would need to be in the seabios coding style.
>  /* memcmp stops early if it encounters \0 as it doesn't match name=.  */
>  if (memcmp(ptr, "name=", 5) == 0)
> {
>  int i;
>  char *optr = dest;
>  for (i = 0, ptr += 5; *ptr && *ptr != ' ' && i < lim; i++)
>    *optr++ = *ptr++;
>  *optr++ = '\0';
>  return 1;
> }
> }
> return 0;
>
> +}
>
> +void
> +coreboot_multiboot_init(u32 mbptr)
> +{
> struct multiboot_info *mbi = (void *)mbptr;
> dprintf (1, "mbptr=0x%x\n", mbptr);
> if (mbptr == NO_MULTIBOOT)
> return;
> mbi = (void *)mbptr;
> dprintf (1, "flags=0x%x, mods=0x%x, mods_c=%d\n", mbi->flags, mbi->mods_addr,
>   mbi->mods_count);
> if (!(mbi->flags & MULTIBOOT_INFO_MODS))
> return;
> int i;
> struct multiboot_mod_list *mod = (void *)mbi->mods_addr;
> for (i = 0; i < mbi->mods_count; i++) {
> struct cbfs_romfile_s *cfile;
> u8 *copy;
> u32 len;
> if (!mod[i].cmdline)
>  continue;
> len = mod[i].mod_end - mod[i].mod_start;
> cfile = malloc_tmp(sizeof(*cfile));
> memset(cfile, 0, sizeof(*cfile));
> dprintf (1, "module %s, size 0x%x\n", (char *)mod[i].cmdline, len);
> if (!extract_filename(cfile->file.name, (char *)mod[i].cmdline, sizeof(cfile->file.name)))
>  {
> free (cfile);
> continue;
>  }
> dprintf (1, "assigned file name <%s>\n", cfile->file.name);
> cfile->file.size = cfile->rawsize = len;
> copy = malloc_tmp (len);
> memcpy(copy, (void *)mod[i].mod_start, len);
> cfile->file.copy = cbfs_copyfile;
> cfile->data = copy;
> romfile_add(&cfile->file);
> }
>
> +}
>
> struct cbfs_payload_segment {
>      u32 type;
>      u32 compression;
> diff --git a/src/fw/csm.c b/src/fw/csm.c
> index 7cdb398..9382aa4 100644
> --- a/src/fw/csm.c
> +++ b/src/fw/csm.c
> @@ -61,7 +61,7 @@ csm_return(struct bregs *regs)
>  static void
>  csm_maininit(struct bregs *regs)
>  {
> interface_init();
>
> interface_init(NO_MULTIBOOT);
> pci_probe_devices();
>
> csm_compat_table.PnPInstallationCheckSegment = SEG_BIOS;
>
> diff --git a/src/post.c b/src/post.c
> index 9ea5620..5585340 100644
> --- a/src/post.c
> +++ b/src/post.c
> @@ -108,7 +108,7 @@ bda_init(void)
>  }
>
> void
> -interface_init(void)
> +interface_init(u32 mbptr)
>  {
>      // Running at new code address - do code relocation fixups
>      malloc_init();
> @@ -116,6 +116,7 @@ interface_init(void)
>      // Setup romfile items.
>      qemu_cfg_init();
>      coreboot_cbfs_init();
>
> coreboot_multiboot_init(mbptr);
>
> // Setup ivt/bda/ebda
> ivt_init();
>
> @@ -208,10 +209,10 @@ startBoot(void)
>
> // Main setup code.
>  static void
> -maininit(void)
> +maininit(u32 mbptr)
>  {
>      // Initialize internal interfaces.
> interface_init();
>
> interface_init(mbptr);
>
> // Setup platform devices.
> platform_hardware_setup();
>
> @@ -302,7 +303,7 @@ reloc_preinit(void *f, void *arg)
>
> // Setup for code relocation and then relocate.
>  void VISIBLE32INIT
> -dopost(void)
> +dopost(u32 mbptr)
>  {
>      // Detect ram and setup internal malloc.
>      qemu_preinit();
> @@ -310,14 +311,14 @@ dopost(void)
>      malloc_preinit();
>  // Relocate initialization code and call maininit().
> reloc_preinit(maininit, NULL);
> reloc_preinit(maininit, (void *) mbptr);
>
> }
>
> // Entry point for Power On Self Test (POST) - the BIOS initilization
>  // phase.  This function makes the memory at 0xc0000-0xfffff
>  // read/writable and then calls dopost().
>  void VISIBLE32FLAT
> -handle_post(void)
> +handle_post(u32 mbptr)
>  {
>      if (!CONFIG_QEMU && !CONFIG_COREBOOT)
>          return;
> @@ -332,5 +333,5 @@ handle_post(void)
>      make_bios_writable();
>  // Now that memory is read/writable - start post process.
> dopost();
> dopost(mbptr);

Instead of passing this variable all the way through these
initialization functions, I suspect it would be much easier to just
stash the value in a global variable in the assembler and then read it
from that global in the coreboot.c code.
> }
> diff --git a/src/romlayout.S b/src/romlayout.S
> index 93b6874..071f71b 100644
> --- a/src/romlayout.S
> +++ b/src/romlayout.S
> @@ -372,6 +372,7 @@ entry_bios32:
>          DECLFUNC entry_elf
>          .code32
>  entry_elf:
>    movl %eax, %ecx
>    cli
>    cld
>    lidtl (BUILD_BIOS_ADDR + pmode_IDT_info)
>
> @@ -383,6 +384,19 @@ entry_elf:
>          movw %ax, %gs
>          movw %ax, %ss
>          movl $BUILD_STACK_ADDR, %esp
>    movl $0x2BADB002, %eax
>    cmpl %ecx, %eax
>    je  multiboot

Why detect a multiboot entry in entry_elf - wouldn't it be much
simpler to point the multiboot header to a new entry_multiboot
assembler function?
>    xorl %ebx, %ebx
>    decl %ebx
>
> +multiboot:
> /* When compiled with -mregparm=1 or higher first argument is in
>   %eax, otherwise it's on stack. Support both.
> */

I don't understand this comment - seabios always uses -mregparam=3 and
nearly all the assembler entry stubs rely on this.
>    movl  %ebx, %eax
>    pushl %ebx
> /* Fake return address.  */
>    pushl %ebx
>    ljmpl $SEG32_MODE32_CS, $_cfunc32flat_handle_post
>    .code16
>
> diff --git a/src/util.h b/src/util.h
> index 704ae0a..e65f0ae 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -90,6 +90,7 @@ void coreboot_platform_setup(void);
>  void cbfs_payload_setup(void);
>  void coreboot_preinit(void);
>  void coreboot_cbfs_init(void);
> +void coreboot_multiboot_init(u32 mbptr);
>  struct cb_header;
>  void *find_cb_subtable(struct cb_header *cbh, u32 tag);
>  struct cb_header *find_cb_table(void);
> @@ -213,7 +214,8 @@ u16 get_pnp_offset(void);
>  void pnp_init(void);
>
> // post.c
> -void interface_init(void);
> +#define NO_MULTIBOOT 0xffffffff
> +void interface_init(u32 mbptr);
>  void device_hardware_setup(void);
>  void prepareboot(void);
>  void startBoot(void);

-Kevin