Hi,
The attached patch adds a build option so that one can choose between native coreboot tables and multiboot information (or both, or neither).
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 ;-)
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
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.
On 16/09/08 16:35 +0200, Robert Millan wrote:
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.
Indeed - step 1 is making libpayload grok the tables itself, I already have an untested patch that does that. Step 2 is passing it on to other interested entities, which we can do either by passing the pointer, or in an extreme case, saving off the tables and reconstructing them for the child payloads. None of this will be any concern at all for the payloads.
Jordan
Robert Millan wrote:
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 rmh@aybabtu.com
Sorry, but I still don't understand the motivation for this. Tell me why it's needed and I'll probably ack straight away. :)
//Peter
On Wed, Sep 17, 2008 at 11:50:07AM +0200, Peter Stuge wrote:
Robert Millan wrote:
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 rmh@aybabtu.com
Sorry, but I still don't understand the motivation for this. Tell me why it's needed and I'll probably ack straight away. :)
With my Multiboot patch, one can no longer assume native coreboot tables unconditionally, since Multiboot might be used instead. coreboot_table.c, which provides get_lb_mem(), might not be linked in.
However, arch_write_tables() uses get_lb_mem() unconditionally as its return value. But I observed that this value isn't actually used for anything. The caller (write_tables()) in turn uses it as return value, and then it is discarded by stage2().
So, rather than introducing a bunch of ifdefs to conditionalize this, my patches opt to remove it. This way both build options get the size benefit.
Also, IMHO it didn't make much sense for a function that generates different types of data structures (write_tables()) to have a return value that is specific to only one of these.
Robert Millan wrote:
On Wed, Sep 17, 2008 at 11:50:07AM +0200, Peter Stuge wrote:
Robert Millan wrote:
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 rmh@aybabtu.com
Sorry, but I still don't understand the motivation for this. Tell me why it's needed and I'll probably ack straight away. :)
With my Multiboot patch, one can no longer assume native coreboot tables unconditionally, since Multiboot might be used instead. coreboot_table.c, which provides get_lb_mem(), might not be linked in.
However, arch_write_tables() uses get_lb_mem() unconditionally as its return value. But I observed that this value isn't actually used for anything. The caller (write_tables()) in turn uses it as return value, and then it is discarded by stage2().
So, rather than introducing a bunch of ifdefs to conditionalize this, my patches opt to remove it. This way both build options get the size benefit.
Also, IMHO it didn't make much sense for a function that generates different types of data structures (write_tables()) to have a return value that is specific to only one of these.
Just my 2ct:
I have a bit of a hick-up with removing the coreboot table, as this renders utility like flashrom (in some cases) and nvramtool (in all cases) pretty much unusable.
Also, I think bootloader stuff is libpayload/filo/grub2 territory.
Glad to see your patch is able to reduce the code size though; even if it comes at some cost.
Stefan
On 17/09/08 17:05 +0200, Stefan Reinauer wrote:
Robert Millan wrote:
On Wed, Sep 17, 2008 at 11:50:07AM +0200, Peter Stuge wrote:
Robert Millan wrote:
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 rmh@aybabtu.com
Sorry, but I still don't understand the motivation for this. Tell me why it's needed and I'll probably ack straight away. :)
With my Multiboot patch, one can no longer assume native coreboot tables unconditionally, since Multiboot might be used instead. coreboot_table.c, which provides get_lb_mem(), might not be linked in.
However, arch_write_tables() uses get_lb_mem() unconditionally as its return value. But I observed that this value isn't actually used for anything. The caller (write_tables()) in turn uses it as return value, and then it is discarded by stage2().
So, rather than introducing a bunch of ifdefs to conditionalize this, my patches opt to remove it. This way both build options get the size benefit.
Also, IMHO it didn't make much sense for a function that generates different types of data structures (write_tables()) to have a return value that is specific to only one of these.
Just my 2ct:
I have a bit of a hick-up with removing the coreboot table, as this renders utility like flashrom (in some cases) and nvramtool (in all cases) pretty much unusable.
Also, I think bootloader stuff is libpayload/filo/grub2 territory.
I'm not groking the resistance to these tables. The coreboot tables are good, but they have the distinct disadvantage of not being groked by anything out side of our little ecosystem of payloads. Tables are cheap, and it doesn't hurt us at all to include as many as we can.
Jordan
Hi,
On Wed, Sep 17, 2008 at 05:05:03PM +0200, Stefan Reinauer wrote:
I have a bit of a hick-up with removing the coreboot table, as this renders utility like flashrom (in some cases) and nvramtool (in all cases) pretty much unusable.
I don't propose removing the coreboot table. In most cases it is highly desireable, but in a few ones it's not that useful and is just taking space. A build option that defaults to 'yes' sounds like a sane way to support all use cases.
Note that the code removed by my last patch doesn't disable any functionality, it just stops passing a pointer around which wasn't being used for anything.
Also, I think bootloader stuff is libpayload/filo/grub2 territory.
How do you define "bootloader stuff"? coreboot looks a lot like a simplified bootloader to me. And it does support loading things that are usually loaded by GRUB, like Invaders, Memtest86 or even Linux.
Glad to see your patch is able to reduce the code size though; even if it comes at some cost.
Indeed, things come at a price. Such cost would be bearable for some and unacceptable for others. I really think having the user evaluate it is the best way to serve everyone.
Hey, at least your users are system builders and have a clue on what they're doing ;-)
I would rather leave in support for both, and have a cmos bit enable one or the other. 4k at this point is not worth saving. The flash sizes are getting to be very large.
ron
On Wed, Sep 17, 2008 at 11:46:19AM -0700, ron minnich wrote:
I would rather leave in support for both,
You mean both enabled unconditionally?
and have a cmos bit enable one or the other.
Runtime selection wouldn't be necessary. The two protocols don't collide with each other in case both are built-in.
Here's a new patch.
- Leaves native coreboot tables untouched, as requested by Stefan and Ron.
- Misc other changes requested by Carl-Daniel.
On 18.09.2008 22:09, Robert Millan wrote:
Here's a new patch.
Leaves native coreboot tables untouched, as requested by Stefan and Ron.
Misc other changes requested by Carl-Daniel.
Thanks. I'm surprised by the unconditional usage of multiboot, though. I thought Ron and Stefan asked for unconditional lbtable and conditional multiboot. Especially the unconditional inline asm instead of the old C call to the payload worries me.
This patch lacks all the documentation contained in the earlier versions. Please fix.
Index: arch/x86/archtables.c
--- arch/x86/archtables.c (revision 867) +++ arch/x86/archtables.c (working copy) @@ -24,6 +24,7 @@ #include <console.h> #include <string.h> #include <tables.h> +#include <multiboot.h> //#include <cpu/cpu.h> //#include <pirq_routing.h> //#include <smp/mpspec.h> @@ -78,6 +79,14 @@
post_code(POST_STAGE2_ARCH_WRITE_TABLES_ENTER);
- /* The Multiboot information structure must be in 0xf0000 */
- rom_table_end = write_multiboot_info(
low_table_start, low_table_end,rom_table_start, rom_table_end);- /* FIXME: is this alignment needed for PIRQ table? */
- rom_table_end = (rom_table_end + 1023) & ~1023;
Please explain the two lines above. They are not mentioned in the changelog.
- /* This table must be betweeen 0xf0000 & 0x100000 */ /* we need to make a decision: create empty functions
- in .h files if the cpp variable is undefined, or #ifdef?
Index: arch/x86/stage1.c
--- arch/x86/stage1.c (revision 867) +++ arch/x86/stage1.c (working copy) @@ -28,6 +28,7 @@ #include <lib.h> #include <mc146818rtc.h> #include <cpu.h> +#include <multiboot.h>
#ifdef CONFIG_PAYLOAD_ELF_LOADER /* ah, well, what a mess! This is a hard code. FIX ME but how? @@ -139,6 +140,14 @@ } #endif /* CONFIG_PAYLOAD_ELF_LOADER */
+int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f));
- return ret;
+}
The function above belongs in multiboot.c
/**
- This function is called from assembler code with its argument on the
- stack. Force the compiler to generate always correct code for this case.
Regards, Carl-Daniel
On Thu, Sep 18, 2008 at 2:59 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Thanks. I'm surprised by the unconditional usage of multiboot, though. I thought Ron and Stefan asked for unconditional lbtable and conditional multiboot. Especially the unconditional inline asm instead of the old C call to the payload worries me.
it's me and my dislike for conditional code and conditional structures. It always seems to bite us when we do it that way.
ron
On 18.09.2008 23:59, Carl-Daniel Hailfinger wrote:
On 18.09.2008 22:09, Robert Millan wrote:
--- arch/x86/stage1.c (revision 867) +++ arch/x86/stage1.c (working copy) @@ -139,6 +140,14 @@ } #endif /* CONFIG_PAYLOAD_ELF_LOADER */
+int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f));
- return ret;
+}
The function above belongs in multiboot.c
I forgot to mention that the clobber list is incomplete, possibly leading to corruption if a payload returns.
Regards, Carl-Daniel
On 19/09/08 01:12 +0200, Carl-Daniel Hailfinger wrote:
On 18.09.2008 23:59, Carl-Daniel Hailfinger wrote:
On 18.09.2008 22:09, Robert Millan wrote:
--- arch/x86/stage1.c (revision 867) +++ arch/x86/stage1.c (working copy) @@ -139,6 +140,14 @@ } #endif /* CONFIG_PAYLOAD_ELF_LOADER */
+int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f));
- return ret;
+}
The function above belongs in multiboot.c
I forgot to mention that the clobber list is incomplete, possibly leading to corruption if a payload returns.
Lets be honest with ourselves, none of our code is capable of processing the return of a payload. Complaining about this is falling out of an airplane and on the way down telling the other guy that he should be wearing a helmet.
Technically, we should be saving and then restoring the kitchen sink when we come back - something that isn't practical to do in a clobber list. Yes, folks, sometimes assembly can be useful and this is one of those times.
Jordan
On Thu, Sep 18, 2008 at 11:59:13PM +0200, Carl-Daniel Hailfinger wrote:
I thought Ron and Stefan asked for unconditional lbtable
They did, and I changed that in my last patch. The rationale being:
- lbtable is useful in some situations (Stefan)
- size is not that important (Ron)
and conditional multiboot.
They didn't. You must be confused.
This patch lacks all the documentation contained in the earlier versions. Please fix.
Can you be more specific? I'm not sure which documentation are you referring to.
- /* FIXME: is this alignment needed for PIRQ table? */
- rom_table_end = (rom_table_end + 1023) & ~1023;
Please explain the two lines above. They are not mentioned in the changelog.
Before my patch, PIRQ tables were implicitly 1k-aligned due to their hardcoded location being 0xf0000. Inmediately after writing them, this 1k-alignment is enforced because it's apparantly needed for ACPI tables. However, existing code doesn't give any indication on whether PIRQ tables require the same alignment or not.
Therefore, since my code pushes them, I thought it's better to be safe than sorry and add the same alignment.
But if someone can answer the question on whether this alignment is needed or not, then we can remove either one or both of these lines.
Index: arch/x86/stage1.c
--- arch/x86/stage1.c (revision 867) +++ arch/x86/stage1.c (working copy) @@ -28,6 +28,7 @@ #include <lib.h> #include <mc146818rtc.h> #include <cpu.h> +#include <multiboot.h>
#ifdef CONFIG_PAYLOAD_ELF_LOADER /* ah, well, what a mess! This is a hard code. FIX ME but how? @@ -139,6 +140,14 @@ } #endif /* CONFIG_PAYLOAD_ELF_LOADER */
+int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f));
- return ret;
+}
The function above belongs in multiboot.c
Note the stage1/stage2 separation. It would require either:
- multiboot.c to be linked in twice, once for stage1 and once for stage2
- multiboot.c to be split in two files, one for stage1 and one for stage2
this is why I initially used multiboot.h and made the function inline (which surprisingly results in zero size increase).
Which way do we go?
On Fri, Sep 19, 2008 at 6:33 AM, Robert Millan rmh@aybabtu.com wrote:
Note the stage1/stage2 separation. It would require either:
multiboot.c to be linked in twice, once for stage1 and once for stage2
multiboot.c to be split in two files, one for stage1 and one for stage2
build multiboot into stage 1, and its functions can be called from stage 2 thanks to carl-daniel.
ron
On Fri, Sep 19, 2008 at 09:08:47AM -0700, ron minnich wrote:
On Fri, Sep 19, 2008 at 6:33 AM, Robert Millan rmh@aybabtu.com wrote:
Note the stage1/stage2 separation. It would require either:
multiboot.c to be linked in twice, once for stage1 and once for stage2
multiboot.c to be split in two files, one for stage1 and one for stage2
build multiboot into stage 1, and its functions can be called from stage 2 thanks to carl-daniel.
Can't do. multiboot.c's write_multiboot_info() depends on search_global_resources() which is in stage2.
On Fri, Sep 19, 2008 at 07:11:11PM +0200, Robert Millan wrote:
On Fri, Sep 19, 2008 at 09:08:47AM -0700, ron minnich wrote:
On Fri, Sep 19, 2008 at 6:33 AM, Robert Millan rmh@aybabtu.com wrote:
Note the stage1/stage2 separation. It would require either:
multiboot.c to be linked in twice, once for stage1 and once for stage2
multiboot.c to be split in two files, one for stage1 and one for stage2
build multiboot into stage 1, and its functions can be called from stage 2 thanks to carl-daniel.
Can't do. multiboot.c's write_multiboot_info() depends on search_global_resources() which is in stage2.
So what then? I move search_global_resources() into stage1?
On 18/09/08 23:59 +0200, Carl-Daniel Hailfinger wrote:
+int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f));
- return ret;
+}
The function above belongs in multiboot.c
But you have subsequently turned down every other alternative (which were far more complex, by the way). Would you like to re-examine your comment here?
I believe that the multiboot code makes us better, and I would like to see it committed soon. If you have a legitimate technical reason for rejecting this patch, then please, let us have it, but stop trying to nitpick this patch to death.
Jordan
Hi,
Here's a new version of the patch. Changes relative to the last one:
- Add "static" keyword to run_address_multiboot() and remove its header declaration.
- Add "edx" to clobber list. Together with eax and ecx which are already in input list, this makes the call able to return for any payload that complies with "cdecl" calling convention.
On 22/09/08 23:03 +0200, Robert Millan wrote:
Hi,
Here's a new version of the patch. Changes relative to the last one:
- Add "static" keyword to run_address_multiboot() and remove its header declaration.
Ack - the code should be defined where it is used - don't make things too complex.
- Add "edx" to clobber list. Together with eax and ecx which are already in input list, this makes the call able to return for any payload that complies with "cdecl" calling convention.
libpayload will use cdecl, as should the other payloads. This is a legitimate restriction to put on the payloads, i think.
Acked-by: Jordan Crouse jordan.crouse@amd.com
I'm slightly tired of the back and forth on this patch - so I'll give the opponents a chance to discussion the technical merits of the patch, otherwise I'm going to check it in and let cleanups happen as they may.
Jordan
Signed-off-by: Robert Millan rmh@aybabtu.com
Index: include/arch/x86/multiboot.h
--- include/arch/x86/multiboot.h (revision 0) +++ include/arch/x86/multiboot.h (revision 0) @@ -0,0 +1,179 @@ +/* multiboot.h - multiboot header file. */ +/*
- Copyright (C) 2003 Free Software Foundation, Inc.
- Copyright (C) 2008 Robert Millan
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, see http://www.gnu.org/licenses/.
- */
+#ifndef MULTIBOOT_H +#define MULTIBOOT_H
+#include <types.h> +typedef u16 uint16_t; +typedef u32 uint32_t; +typedef u64 uint64_t;
+/* How many bytes from the start of the file we search for the header. */ +#define MB_SEARCH 8192
+/* The magic field should contain this. */ +#define MB_MAGIC 0x1BADB002
+/* This should be in %eax. */ +#define MB_MAGIC2 0x2BADB002
+/* The bits in the required part of flags field we don't support. */ +#define MB_UNSUPPORTED 0x0000fffc
+/* Alignment of multiboot modules. */ +#define MB_MOD_ALIGN 0x00001000
+/*
- Flags set in the 'flags' member of the multiboot header.
- */
+/* Align all boot modules on i386 page (4KB) boundaries. */ +#define MB_PAGE_ALIGN 0x00000001
+/* Must pass memory information to OS. */ +#define MB_MEMORY_INFO 0x00000002
+/* Must pass video information to OS. */ +#define MB_VIDEO_MODE 0x00000004
+/* This flag indicates the use of the address fields in the header. */ +#define MB_AOUT_KLUDGE 0x00010000
+/*
- Flags to be set in the 'flags' member of the multiboot info structure.
- */
+/* is there basic lower/upper memory information? */ +#define MB_INFO_MEMORY 0x00000001 +/* is there a boot device set? */ +#define MB_INFO_BOOTDEV 0x00000002 +/* is the command-line defined? */ +#define MB_INFO_CMDLINE 0x00000004 +/* are there modules to do something with? */ +#define MB_INFO_MODS 0x00000008
+/* These next two are mutually exclusive */
+/* is there a symbol table loaded? */ +#define MB_INFO_AOUT_SYMS 0x00000010 +/* is there an ELF section header table? */ +#define MB_INFO_ELF_SHDR 0x00000020
+/* is there a full memory map? */ +#define MB_INFO_MEM_MAP 0x00000040
+/* Is there drive info? */ +#define MB_INFO_DRIVE_INFO 0x00000080
+/* Is there a config table? */ +#define MB_INFO_CONFIG_TABLE 0x00000100
+/* Is there a boot loader name? */ +#define MB_INFO_BOOT_LOADER_NAME 0x00000200
+/* Is there a APM table? */ +#define MB_INFO_APM_TABLE 0x00000400
+/* Is there video information? */ +#define MB_INFO_VIDEO_INFO 0x00000800
+struct multiboot_header +{
- /* Must be MB_MAGIC - see above. */
- uint32_t magic;
- /* Feature flags. */
- uint32_t flags;
- /* The above fields plus this one must equal 0 mod 2^32. */
- uint32_t checksum;
- /* These are only valid if MB_AOUT_KLUDGE is set. */
- uint32_t header_addr;
- uint32_t load_addr;
- uint32_t load_end_addr;
- uint32_t bss_end_addr;
- uint32_t entry_addr;
- /* These are only valid if MB_VIDEO_MODE is set. */
- uint32_t mode_type;
- uint32_t width;
- uint32_t height;
- uint32_t depth;
+};
+struct multiboot_info +{
- /* Multiboot info version number */
- uint32_t flags;
- /* Available memory from BIOS */
- uint32_t mem_lower;
- uint32_t mem_upper;
- /* "root" partition */
- uint32_t boot_device;
- /* Kernel command line */
- uint32_t cmdline;
- /* Boot-Module list */
- uint32_t mods_count;
- uint32_t mods_addr;
- uint32_t syms[4];
- /* Memory Mapping buffer */
- uint32_t mmap_length;
- uint32_t mmap_addr;
- /* Drive Info buffer */
- uint32_t drives_length;
- uint32_t drives_addr;
- /* ROM configuration table */
- uint32_t config_table;
- /* Boot Loader Name */
- uint32_t boot_loader_name;
- /* APM table */
- uint32_t apm_table;
- /* Video */
- uint32_t vbe_control_info;
- uint32_t vbe_mode_info;
- uint16_t vbe_mode;
- uint16_t vbe_interface_seg;
- uint16_t vbe_interface_off;
- uint16_t vbe_interface_len;
+};
+struct multiboot_mmap_entry +{
- uint32_t size;
- uint64_t addr;
- uint64_t len;
- uint32_t type;
+} __attribute__((packed));
+unsigned long write_multiboot_info(unsigned long, unsigned long, unsigned long, unsigned long);
+#endif Index: arch/x86/archtables.c =================================================================== --- arch/x86/archtables.c (revision 867) +++ arch/x86/archtables.c (working copy) @@ -24,6 +24,7 @@ #include <console.h> #include <string.h> #include <tables.h> +#include <multiboot.h> //#include <cpu/cpu.h> //#include <pirq_routing.h> //#include <smp/mpspec.h> @@ -78,6 +79,14 @@
post_code(POST_STAGE2_ARCH_WRITE_TABLES_ENTER);
- /* The Multiboot information structure must be in 0xf0000 */
- rom_table_end = write_multiboot_info(
low_table_start, low_table_end,rom_table_start, rom_table_end);- /* FIXME: is this alignment needed for PIRQ table? */
- rom_table_end = (rom_table_end + 1023) & ~1023;
- /* This table must be betweeen 0xf0000 & 0x100000 */ /* we need to make a decision: create empty functions
- in .h files if the cpp variable is undefined, or #ifdef?
Index: arch/x86/multiboot.c
--- arch/x86/multiboot.c (revision 0) +++ arch/x86/multiboot.c (revision 0) @@ -0,0 +1,70 @@ +/*
- support for Multiboot payloads
- Copyright (C) 2008 Robert Millan
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, see http://www.gnu.org/licenses/.
- */
+#include <multiboot.h> +#include <string.h> +#include <device/resource.h> +#include <console.h>
+static struct multiboot_mmap_entry *mb_mem;
+static void build_mb_mem_range(void *gp, struct device *dev, struct resource *res) +{
- mb_mem->addr = res->base;
- mb_mem->len = res->size;
- mb_mem->type = 1;
- mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size);
- mb_mem++;
+}
+unsigned long write_multiboot_info(
- unsigned long low_table_start, unsigned long low_table_end,
- unsigned long rom_table_start, unsigned long rom_table_end)
+{
- struct multiboot_info *mbi = rom_table_end;
- memset(mbi, 0, sizeof(*mbi));
- rom_table_end += sizeof(*mbi);
- mbi->mmap_addr = (u32) rom_table_end;
- mb_mem = rom_table_end;
- /* free regions */
- search_global_resources( IORESOURCE_MEM | IORESOURCE_CACHEABLE,
IORESOURCE_MEM | IORESOURCE_CACHEABLE, build_mb_mem_range, NULL);- /* reserved regions */
- mb_mem->addr = low_table_start;
- mb_mem->len = low_table_end - low_table_start;
- mb_mem->type = 2;
- mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size);
- mb_mem++;
- mb_mem->addr = rom_table_start;
- mb_mem->len = rom_table_end - rom_table_start;
- mb_mem->type = 2;
- mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size);
- mb_mem++;
- mbi->mmap_length = ((u32) mb_mem) - mbi->mmap_addr;
- mbi->flags |= MB_INFO_MEM_MAP;
- printk(BIOS_INFO, "Multiboot Information structure has been written.\n");
- return mb_mem;
+} Index: arch/x86/stage1.c =================================================================== --- arch/x86/stage1.c (revision 867) +++ arch/x86/stage1.c (working copy) @@ -28,6 +28,7 @@ #include <lib.h> #include <mc146818rtc.h> #include <cpu.h> +#include <multiboot.h>
#ifdef CONFIG_PAYLOAD_ELF_LOADER /* ah, well, what a mess! This is a hard code. FIX ME but how? @@ -139,6 +140,14 @@ } #endif /* CONFIG_PAYLOAD_ELF_LOADER */
+static int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx");
- return ret;
+}
/**
- This function is called from assembler code with its argument on the
- stack. Force the compiler to generate always correct code for this case.
@@ -259,7 +268,7 @@ if (entry != (void*)-1) { /* Final coreboot call before handing off to the payload. */ mainboard_pre_payload();
run_address(entry);
run_address_multiboot(entry);Index: arch/x86/Makefile
--- arch/x86/Makefile (revision 867) +++ arch/x86/Makefile (working copy) @@ -188,7 +188,7 @@ STAGE2_LIB_SRC = stage2.c clog2.c mem.c tables.c delay.c \ compute_ip_checksum.c string.c
-STAGE2_ARCH_X86_SRC = archtables.c coreboot_table.c udelay_io.c +STAGE2_ARCH_X86_SRC = archtables.c coreboot_table.c multiboot.c udelay_io.c STAGE2_ARCH_X86_SRC += pci_ops_auto.c STAGE2_ARCH_X86_SRC += keyboard.c i8259.c isa-dma.c
Hi,
Robert Millan wrote:
- Add "edx" to clobber list. Together with eax and ecx which are already in input list, this makes the call able to return for any payload that complies with "cdecl" calling convention.
...
+static int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx");
- return ret;
+}
this assembler inline is not quite correct to be able to let the call return because the compiler assumes that after this assembler statement %ecx still holds the value f which hasn't to be the case. It assumes the same for %ebx but this should be no problem since the calling function must preserve it's value if it follows the C calling convention. But %ecx isn't one of the registers that needs to be preserved, so the assumption the compiler makes here is wrong.
How about this one:
| static int run_address_multiboot(void *f) | { | int ret, dummy; | __asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory"); | return ret; | }
The memory clobber is needed since you cannot know what the called function will actually do with the memory and to ensure all cached values are actually written back to memory before calling f().
Mathias
On Tue, Sep 23, 2008 at 10:10:31AM +0200, Mathias Krause wrote:
Hi,
Robert Millan wrote:
- Add "edx" to clobber list. Together with eax and ecx which are already in input list, this makes the call able to return for any payload that complies with "cdecl" calling convention.
...
+static int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx");
- return ret;
+}
this assembler inline is not quite correct to be able to let the call return because the compiler assumes that after this assembler statement %ecx still holds the value f which hasn't to be the case. It assumes the same for %ebx but this should be no problem since the calling function must preserve it's value if it follows the C calling convention. But %ecx isn't one of the registers that needs to be preserved, so the assumption the compiler makes here is wrong.
How about this one:
| static int run_address_multiboot(void *f) | { | int ret, dummy; | __asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory"); | return ret; | }
I see. But why not just add "ecx" to clobber list instead? Then a dummy variable isn't needed.
The memory clobber is needed since you cannot know what the called function will actually do with the memory and to ensure all cached values are actually written back to memory before calling f().
Is this really a problem? If the payload expects to return, it isn't supposed to modify coreboot's memory at all. If it does, I'd say it's normal that things break.
On 23/09/08 15:19 +0200, Robert Millan wrote:
On Tue, Sep 23, 2008 at 10:10:31AM +0200, Mathias Krause wrote:
Hi,
Robert Millan wrote:
- Add "edx" to clobber list. Together with eax and ecx which are already in input list, this makes the call able to return for any payload that complies with "cdecl" calling convention.
...
+static int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx");
- return ret;
+}
this assembler inline is not quite correct to be able to let the call return because the compiler assumes that after this assembler statement %ecx still holds the value f which hasn't to be the case. It assumes the same for %ebx but this should be no problem since the calling function must preserve it's value if it follows the C calling convention. But %ecx isn't one of the registers that needs to be preserved, so the assumption the compiler makes here is wrong.
How about this one:
| static int run_address_multiboot(void *f) | { | int ret, dummy; | __asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory"); | return ret; | }
I see. But why not just add "ecx" to clobber list instead? Then a dummy variable isn't needed.
The memory clobber is needed since you cannot know what the called function will actually do with the memory and to ensure all cached values are actually written back to memory before calling f().
Is this really a problem? If the payload expects to return, it isn't supposed to modify coreboot's memory at all. If it does, I'd say it's normal that things break.
Thats exactly right - if the payload is expected to return, then the responsiblity is on it not to destroy memory. All coreboot can do is call the function and hope for the best.
Jordan
On 23.09.2008 15:19, Robert Millan wrote:
On Tue, Sep 23, 2008 at 10:10:31AM +0200, Mathias Krause wrote:
Robert Millan wrote:
- Add "edx" to clobber list. Together with eax and ecx which are already in input list, this makes the call able to return for any payload that complies with "cdecl" calling convention.
...
+static int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx");
- return ret;
+}
this assembler inline is not quite correct to be able to let the call return because the compiler assumes that after this assembler statement %ecx still holds the value f which hasn't to be the case. It assumes the same for %ebx but this should be no problem since the calling function must preserve it's value if it follows the C calling convention. But %ecx isn't one of the registers that needs to be preserved, so the assumption the compiler makes here is wrong.
How about this one:
| static int run_address_multiboot(void *f) | { | int ret, dummy; | __asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory"); | return ret; | }
I see. But why not just add "ecx" to clobber list instead? Then a dummy variable isn't needed.
Try it and you'll see.
Regards, Carl-Daniel
Hi again,
Robert Millan wrote:
On Tue, Sep 23, 2008 at 10:10:31AM +0200, Mathias Krause wrote:
Robert Millan wrote:
- Add "edx" to clobber list. Together with eax and ecx which are already in input list, this makes the call able to return for any payload that complies with "cdecl" calling convention.
...
+static int run_address_multiboot(void *f) +{
- int ret;
- __asm__ __volatile__ ("call *%3" : "=a" (ret) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx");
- return ret;
+}
this assembler inline is not quite correct to be able to let the call return because the compiler assumes that after this assembler statement %ecx still holds the value f which hasn't to be the case. It assumes the same for %ebx but this should be no problem since the calling function must preserve it's value if it follows the C calling convention. But %ecx isn't one of the registers that needs to be preserved, so the assumption the compiler makes here is wrong.
How about this one:
| static int run_address_multiboot(void *f) | { | int ret, dummy; | __asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory"); | return ret; | }
I see. But why not just add "ecx" to clobber list instead? Then a dummy variable isn't needed.
When doing so the compiler is unable to find another register called %ecx to assign the value of f as input value. He cannot use %ecx because by mentioning it in the clobber list you already told him that register may change it's value at any time within the asm statement. That for the "kudge" with the dummy output variable is needed but it should get optimized away since it's value is never used. It's only usage is to tell the compiler that the value of %ecx has changed.
The memory clobber is needed since you cannot know what the called function will actually do with the memory and to ensure all cached values are actually written back to memory before calling f().
Is this really a problem? If the payload expects to return, it isn't supposed to modify coreboot's memory at all. If it does, I'd say it's normal that things break.
Not the return of f() is the problem but missing memory writes _before_ calling it. If coreboot does some memory modifications that are relevant for f() they should be taken out of registers and written back to memory before calling f(). Otherwise f() may break.
On Tue, Sep 23, 2008 at 04:11:34PM +0200, Mathias Krause wrote:
I see. But why not just add "ecx" to clobber list instead? Then a dummy variable isn't needed.
When doing so the compiler is unable to find another register called %ecx to assign the value of f as input value. He cannot use %ecx because by mentioning it in the clobber list you already told him that register may change it's value at any time within the asm statement. That for the "kudge" with the dummy output variable is needed but it should get optimized away since it's value is never used. It's only usage is to tell the compiler that the value of %ecx has changed.
Ok.
The memory clobber is needed since you cannot know what the called function will actually do with the memory and to ensure all cached values are actually written back to memory before calling f().
Is this really a problem? If the payload expects to return, it isn't supposed to modify coreboot's memory at all. If it does, I'd say it's normal that things break.
Not the return of f() is the problem but missing memory writes _before_ calling it. If coreboot does some memory modifications that are relevant for f() they should be taken out of registers and written back to memory before calling f(). Otherwise f() may break.
Makes sense. Thanks for the explanation.
Here's a new patch incorporating your suggestions.
Robert Millan wrote:
On Tue, Sep 23, 2008 at 04:11:34PM +0200, Mathias Krause wrote:
I see. But why not just add "ecx" to clobber list instead? Then a dummy variable isn't needed.
When doing so the compiler is unable to find another register called %ecx to assign the value of f as input value. He cannot use %ecx because by mentioning it in the clobber list you already told him that register may change it's value at any time within the asm statement. That for the "kudge" with the dummy output variable is needed but it should get optimized away since it's value is never used. It's only usage is to tell the compiler that the value of %ecx has changed.
Ok.
The memory clobber is needed since you cannot know what the called function will actually do with the memory and to ensure all cached values are actually written back to memory before calling f().
Is this really a problem? If the payload expects to return, it isn't supposed to modify coreboot's memory at all. If it does, I'd say it's normal that things break.
Not the return of f() is the problem but missing memory writes _before_ calling it. If coreboot does some memory modifications that are relevant for f() they should be taken out of registers and written back to memory before calling f(). Otherwise f() may break.
Makes sense. Thanks for the explanation.
You're welcome.
Here's a new patch incorporating your suggestions.
Thanks.
Mathias
On 23/09/08 20:01 +0200, Robert Millan wrote:
On Tue, Sep 23, 2008 at 04:11:34PM +0200, Mathias Krause wrote:
I see. But why not just add "ecx" to clobber list instead? Then a dummy variable isn't needed.
When doing so the compiler is unable to find another register called %ecx to assign the value of f as input value. He cannot use %ecx because by mentioning it in the clobber list you already told him that register may change it's value at any time within the asm statement. That for the "kudge" with the dummy output variable is needed but it should get optimized away since it's value is never used. It's only usage is to tell the compiler that the value of %ecx has changed.
Ok.
The memory clobber is needed since you cannot know what the called function will actually do with the memory and to ensure all cached values are actually written back to memory before calling f().
Is this really a problem? If the payload expects to return, it isn't supposed to modify coreboot's memory at all. If it does, I'd say it's normal that things break.
Not the return of f() is the problem but missing memory writes _before_ calling it. If coreboot does some memory modifications that are relevant for f() they should be taken out of registers and written back to memory before calling f(). Otherwise f() may break.
Makes sense. Thanks for the explanation.
Here's a new patch incorporating your suggestions.
-- 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."
Signed-off-by: Robert Millan rmh@aybabtu.com
r870 with some minor whitespace cleanups. From this point on, please flame me about this rather then Robert. Thank you.
Jordan
On Wed, Sep 24, 2008 at 08:57:30AM -0600, Jordan Crouse wrote:
r870 with some minor whitespace cleanups.
I'm glad to see Multiboot supported by coreboot. I know some of you are skeptical, but I really think this is going to be a win-win for both GRUB and coreboot.
Surely, GRUB will get pushed off in some situations when its role would only be that of "middle man". I understood that from the beginning, and I don't see it as a problem. I wish people use GRUB for its own value, not simply as a redundant interface between two things that don't grok each other.
coreboot wins with this, because it expands its ecosystem. You don't lose anything, and you win a wider range of payloads/kernels/OSes that will become usable on coreboot "for free".
In GRUB, developers of payloads for coreboot might find a good testing ground. Since it is now possible to produce a payload image that will be loadable by either coreboot or GRUB with no modification, implementors seeking to support coreboot in their payloads can load them from a coreboot/GRUB stack without need to reflash every time (with the burden of using flash programmers, hotswap or risk to produce a brick).
The benefits for GRUB are similar. If Multiboot is used more, GRUB becomes more useful. But of course, that only depends on how much people will want to use Multiboot. My work simply allows it to be used, but doesn't set it as a "mandatory" option. I'm confident over time you'll see a benefit in using it by its merit.
Peter Stuge wrote:
Robert Millan wrote:
I'm glad to see Multiboot supported by coreboot.
I am too.
coreboot wins with this, because it expands its ecosystem.
Indeed. So Ward, have you tried booting xen directly already? :p
Yes, I'm curious, too. Which real world payloads work now that didn't work before?
I think XEN needs the actual DOM0 kernel loaded as a module (which we can't do yet, can we?)
On Thu, Sep 25, 2008 at 11:16:39AM +0200, Stefan Reinauer wrote:
Yes, I'm curious, too. Which real world payloads work now that didn't work before?
http://grub.enbug.org/MultibootSystems has a list of noteworthy payloads using Multiboot. As I mentioned before, this list shouldn't be considered exhaustive, since Multiboot is commonly used for new small projects, in part due to the wide deployment of GRUB as a bootloader.
I think XEN needs the actual DOM0 kernel loaded as a module (which we can't do yet, can we?)
Xen (and for that matter, the Hurd) would need support for loading multiple code images, a feature which will be necessary if you want these as payloads, regardless on whether you use their existing Multiboot support or you replace it with something else.
In other words, my work didn't solve the Xen problem, but it brings you a step closer as it only leaves the multi-object part to be implemented, and doesn't require you to modify Xen itself.
Robert Millan wrote:
On Thu, Sep 25, 2008 at 11:16:39AM +0200, Stefan Reinauer wrote:
Yes, I'm curious, too. Which real world payloads work now that didn't work before?
http://grub.enbug.org/MultibootSystems has a list of noteworthy payloads using Multiboot. As I mentioned before, this list shouldn't be considered exhaustive, since Multiboot is commonly used for new small projects, in part due to the wide deployment of GRUB as a bootloader.
Yes, the list is well known; Thanks for bringing it up again. My question aimed more at which of these, except grub invaders, can actually work now.
The idea of booting an AROS kernel from the ROM is pretty nice indeed, giving the old Amiga "Kickstart" feeling back.
I think XEN needs the actual DOM0 kernel loaded as a module (which we can't do yet, can we?)
Xen (and for that matter, the Hurd) would need support for loading multiple code images, a feature which will be necessary if you want these as payloads, regardless on whether you use their existing Multiboot support or you replace it with something else.
In other words, my work didn't solve the Xen problem, but it brings you a step closer as it only leaves the multi-object part to be implemented, and doesn't require you to modify Xen itself.
So, do you see a simple method to get that "multiple code images" (is that multiboot modules?) support in there? I think this would greatly increase the number of supported multiboot systems (including openbios btw :-)
Stefan
On Thu, Sep 25, 2008 at 02:04:34PM +0200, Stefan Reinauer wrote:
Robert Millan wrote:
On Thu, Sep 25, 2008 at 11:16:39AM +0200, Stefan Reinauer wrote:
Yes, I'm curious, too. Which real world payloads work now that didn't work before?
http://grub.enbug.org/MultibootSystems has a list of noteworthy payloads using Multiboot. As I mentioned before, this list shouldn't be considered exhaustive, since Multiboot is commonly used for new small projects, in part due to the wide deployment of GRUB as a bootloader.
Yes, the list is well known; Thanks for bringing it up again. My question aimed more at which of these, except grub invaders, can actually work now.
My expectation is that, aside from multi-image payloads, most of them will either work right away, or only depend on minor features.
The idea of booting an AROS kernel from the ROM is pretty nice indeed, giving the old Amiga "Kickstart" feeling back.
Got any direct link to a kernel image? I might give it a try.
So, do you see a simple method to get that "multiple code images" (is that multiboot modules?) support in there?
Once coreboot supports loading a single payload that is composed of more than one image, it's rather simple to fill the module list structure with details such as number of modules, etc, and include that in the MBI.
Which would convert "multiple code images" into "multiboot modules" (I guess this answers your other question).
I'm not particularly interested in this use case though. While I do think that it could be interesting to support it in coreboot, it's not one of the situations I mentioned before in which GRUB doesn't deliver any actual value.
But I don't go as far as you put it before ("this is bootloader stuff"). If someone is inclined to implement multi-image support, but not the missing Multiboot bits, I might be able to hack the Multiboot part in.
I think this would greatly increase the number of supported multiboot systems
Well, surely even if the number wouldn't increase a lot, it'd still be so due to the "greatness" of Xen and the Hurd, which are both very nice projects ;-)
(including openbios btw :-)
Haven't checked, but if openbios required multi-image support, coreboot wouldn't be able to support it, which I'm aware it already does.
-
Carl-Daniel Hailfinger -
Jordan Crouse -
Mathias Krause -
Peter Stuge -
Robert Millan -
ron minnich -
Stefan Reinauer