[coreboot] [PATCH] Multiboot

[Carl-Daniel Hailfinger]

On 15.09.2008 21:49, Robert Millan wrote:
> The attached patch adds a build option so that one can choose between
> native coreboot tables and multiboot information (or both, or neither).
>   

Have you tested it with a real preparsed payload?

http://www.gnu.org/software/grub/manual/html_node/Features.html says
that FreeBSD, NetBSD, OpenBSD, and Linux all lack multiboot compliance.
Is that still true? If so, which real-world payloads are multiboot
compliant?


> As explained, the purpose of this was that Multiboot payloads can be
> supported without reliing on intermediate tools/interfaces/code that take
> unnecessary space.  The benefit in size is quite good:
>
> Default build with native table:	55942   build/coreboot.stage2
>
> Build with multiboot information:	51193   build/coreboot.stage2
>
> Relative to the build in which neither is provided, this amounts to 4928 B
> in the first case and 179 B in the latter.
>
> Ah, and before someone asks, yes memory map support is implemented ;-)
>   

And it does not conform to the multiboot spec. Had you stated in advance
that you intend to provide memory map support contrary to/in extension
of the spec, we could have avoided the whole argument.
I don't care about spec conformance as long as it is indicated that the
code does not conform to the spec.

Now, about the patch iself:
- You change a few prototypes. Please provide a separate patch for that.
- Once multiboot support is active, payloads can't return anymore. This
is a change in behaviour and needs documentation, review and function
annotation (noreturn).
- Source code in header files. Please move to .c files.
- One call to get_lb_mem is removed for both multiboot and classic.
Separate patch please.
- Any reasons for the slightly modified licence header in multiboot.c?
- The multiboot spec extension/non-conformance mentioned above needs to
be visible in code comments and/or Kconfig help text.
- Total absence of debug printk() statements. Please include at least a
success message.
- Multiboot suport will not work if the payload is not preparsed.
- Please investigate the possibility to add that multiboot code to
libpayload so that Bayou can use it. Bayou is our recommended default
payload chooser and it would be nice if Bayou could support multiboot.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/