[coreboot] [PATCH] Multiboot
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Sep 18 23:59:13 CEST 2008
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
--
http://www.hailfinger.org/
More information about the coreboot
mailing list