Re: [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