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