Re: [coreboot] [PATCH] Multiboot

[Carl-Daniel Hailfinger]

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