On Mon, Sep 15, 2008 at 10:54:20PM +0200, Carl-Daniel Hailfinger wrote:
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?
Yes. For testing, I recommend using the example Multiboot kernel from GRUB Legacy sources. You have to configure with --enable-example-kernel, and an image is produced in docs/kernel.
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?
Actually, NetBSD's kernel supports Multiboot now. This information is part of GRUB Legacy and is outdated.
If so, which real-world payloads are multiboot compliant?
See http://grub.enbug.org/MultibootSystems
Though, we probably miss many small ones. It is common to use Multiboot in new projects due the widespread of a readily available loader.
Ah, and before someone asks, yes memory map support is implemented ;-)
And it does not conform to the multiboot spec.
Yes, it does. The relevant paragraph is in section 3.3, titled "Boot information format", and starts with "If bit 6 in the `flags' word is set,".
I understand it's easy to miss details, specially in a new text one isn't used to work with. So please read that paragraph completely before continuing with this discussion.
Now, about the patch iself:
- You change a few prototypes. Please provide a separate patch for that.
Attached.
- Once multiboot support is active, payloads can't return anymore. This
is a change in behaviour and needs documentation, review and function annotation (noreturn).
Actually, I didn't have a specific reason to use a jump instead of a call. I'll change that.
- Source code in header files. Please move to .c files.
Ok.
- One call to get_lb_mem is removed for both multiboot and classic.
Separate patch please.
This is related to the prototype change; included in attached patch.
- Any reasons for the slightly modified licence header in multiboot.c?
I believe the reason using an URL instead of a snail address is recommended is that it is legally more solid (e.g. in case the FSF office moves or something). Anyway, it doesn't make a big difference; I'll just use the other text if you like that better.
- Total absence of debug printk() statements. Please include at least a
success message.
Ok.
- Multiboot suport will not work if the payload is not preparsed.
I don't understand what you mean here.
- 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.
Once base Multiboot support is in coreboot, if libpayload/bayou don't overwrite the structures it'd be reasonably simple to reuse them by simply passing on a pointer. Just 3-4 lines of code I think. I can have a look later if you like.