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

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

[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