[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