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

[Kevin O'Connor]

On Tue, May 19, 2015 at 08:12:46AM +0200, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> > This code would need to be in the seabios coding style.
> > 
> Could you point to style guide or to tell exact points you dislike? STFW
> "seabios style guide" returns no relevant results

http://seabios.org/Contributing

Basically, it's similar to the QEMU style - no tabs, 4 space
indentation, 80 column max, braces on same line as if/while/etc.

Also, the patches should contain a "signed-off-by" line as described
in the QEMU patch submission process.

> > 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.
> > 
> Done.
> > 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?
> > 
> Done.
> > 
> > I don't understand this comment - seabios always uses -mregparam=3 and
> > nearly all the assembler entry stubs rely on this.
> > 
> Removed.
> 

Thanks.  Some additional comments below.

> --- 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"
> 
> 
>  /****************************************************************
> @@ -464,6 +465,69 @@ 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;
> +      /* 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;
> +}
> +
> +u32 VISIBLE32FLAT multiboot_info;

The VISIBLE32FLAT macro is for functions (not variables).  I think you
can get away with just using __VISIBLE here (and removing the
"_cfunc32flat_" prefix from the romlayout.S code).

> +
> +void
> +coreboot_multiboot_init(void)
> +{
> +  struct multiboot_info *mbi;
> +  dprintf (1, "mbptr=0x%x\n", multiboot_info);
> +  if (multiboot_info == 0xffffffff)
> +    return;
> +  mbi = (void *)multiboot_info;
> +  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;
> @@ -546,6 +610,8 @@ cbfs_payload_setup(void)
>              break;
>          struct cbfs_romfile_s *cfile;
>          cfile = container_of(file, struct cbfs_romfile_s, file);
> +       if (!cfile->fhdr)
> +         continue;

Is there a reason to use 'struct cbfs_romfile_s' instead of
introducing a new 'struct multiboot_romfile_s'.  Could all this new
code go into a new file src/fw/multiboot.c?

>          const char *filename = file->name;
>          char *desc = znprintf(MAXDESCSIZE, "Payload [%s]", &filename[4]);
>          boot_add_cbfs(cfile->fhdr, desc, bootprio_find_named_rom(filename, 0));
> @@ -565,5 +631,5 @@ mb_head(u32 mbptr)
>         ".long _reloc_abs_start\n"
>         ".long 0\n"
>         ".long 0\n"
> -       ".long entry_elf\n");
> +       ".long entry_multiboot\n");
>  }
> diff --git a/src/multiboot.h b/src/multiboot.h
> new file mode 100644
> index 0000000..c143441
> --- /dev/null
> +++ b/src/multiboot.h
> @@ -0,0 +1,260 @@
> +/*  multiboot.h - Multiboot header file.  */
> +/*  Copyright (C) 1999,2003,2007,2008,2009,2010  Free Software Foundation, Inc.

This file should go into: src/std/

-Kevin