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.
--
Robert Millan
The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
how) you may access your data; but nobody's threatening your freedom: we
still allow you to remove your data and not access it at all."
Void-ify a few return types that assume presence of a native coreboot table
(and weren't actually used for anything).
Signed-off-by: Robert Millan <[EMAIL PROTECTED]>
Index: include/tables.h
===================================================================
--- include/tables.h (revision 861)
+++ include/tables.h (working copy)
@@ -30,7 +30,7 @@
* defined here.
*/
-struct lb_memory *write_tables(void);
+void write_tables(void);
/* The coreboot table information is for conveying information
* from the firmware to the loaded OS image. Primarily this
@@ -270,7 +270,7 @@
#define CHECKSUM_PCBIOS 1
};
-struct lb_memory *arch_write_tables(void);
+void arch_write_tables(void);
unsigned long write_coreboot_table(
unsigned long low_table_start, unsigned long low_table_end,
unsigned long rom_table_start, unsigned long rom_table_end);
Index: lib/tables.c
===================================================================
--- lib/tables.c (revision 861)
+++ lib/tables.c (working copy)
@@ -29,7 +29,7 @@
// #include <cpu.h>
#include <tables.h>
-struct lb_memory *write_tables(void)
+void write_tables(void)
{
- return arch_write_tables();
+ arch_write_tables();
}
Index: arch/x86/archtables.c
===================================================================
--- arch/x86/archtables.c (revision 861)
+++ arch/x86/archtables.c (working copy)
@@ -58,7 +58,7 @@
printk(BIOS_DEBUG,"OK\n");
}
#endif
-struct lb_memory *arch_write_tables(void)
+void arch_write_tables(void)
{
#if 0
#if HAVE_MP_TABLE==1
@@ -144,6 +144,4 @@
write_coreboot_table(
low_table_start, low_table_end,
rom_table_start, rom_table_end);
-
- return get_lb_mem();
}
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
