[coreboot] Multiboot bugfix (coreboot-v3)

Jordan Crouse jordan.crouse at amd.com
Sat Oct 18 00:47:19 CEST 2008


On 27/09/08 12:14 +0200, Robert Millan wrote:
> 
> Hi,
> 
> I noticed a problem in the multiboot memory map in v3.  Instead of the
> reserved region:
> 
>   addr=0x0, len=0x500
> 
> we get:
> 
>   addr=0x0, len=0x10
> 
> the reason being that the multiboot map is generated too early in
> arch_write_tables(), before a number of routines that write/reserve
> stuff are executed (in my test this only affects the 0x0-0x500 region
> but I notice there's other stuff too).
> 
> Attached patch moves it down, solving the problem.  Because stage1 can no
> longer assume the MBI is at 0xf0000, I had to add a return path for stage2
> to give it a pointer, using its exit status value.
> 
> As collateral benefit, my patch removes the FIXME about PIRQ alignment.
> 
> -- 
> 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 at aybabtu.com>

This seems to fix the disappearing coreboot tables problem, but doesn't
fix the corruption in the boot log that Myles and I are seeing.  I didn't
think it was related, but it didn't hurt to check.

Acked-by: Jordan Crouse <jordan.crouse at amd.com>
> Index: include/tables.h
> ===================================================================
> --- include/tables.h	(revision 870)
> +++ 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 870)
> +++ 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();
>  }
> Index: lib/stage2.c
> ===================================================================
> --- lib/stage2.c	(revision 870)
> +++ lib/stage2.c	(working copy)
> @@ -41,8 +41,10 @@
>   * TODO:
>   * - Check whether this documentation is still correct. Improve it.
>   */
> -int stage2(void)
> +void *stage2(void)
>  {
> +	void *mbi;
> +
>  	/* TODO: Add comment. */
>  	void show_all_devs(void);
>  
> @@ -88,9 +90,9 @@
>  
>  	/* TODO: Add comment. */
>  	post_code(POST_STAGE2_WRITE_TABLES);
> -	write_tables();
> +	mbi = write_tables();
>  	show_all_devs();
>  
> -	return 0;
> +	return mbi;
>  }
>  EXPORT_SYMBOL(stage2);
> Index: arch/x86/archtables.c
> ===================================================================
> --- arch/x86/archtables.c	(revision 870)
> +++ arch/x86/archtables.c	(working copy)
> @@ -59,7 +59,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
> @@ -69,6 +69,8 @@
>  	unsigned long low_table_start, low_table_end;
>  	unsigned long rom_table_start, rom_table_end;
>  
> +	void *mbi;
> +
>  	rom_table_start = 0xf0000;
>  	rom_table_end =   0xf0000;
>  	/* Start low addr at 16 bytes instead of 0 because of a buglet
> @@ -79,14 +81,6 @@
>  
>  	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?
> @@ -149,10 +143,17 @@
>  	move_gdt(low_table_end);
>  	low_table_end += &gdt_end - &gdt;
>  #endif
> +
> +	/* The Multiboot information structure */
> +	mbi = rom_table_end;
> +	rom_table_end = write_multiboot_info(
> +			      low_table_start, low_table_end,
> +			      rom_table_start, rom_table_end);
> +
>  	/* The coreboot table must be in 0-4K or 960K-1M */
>  	write_coreboot_table(
>  			      low_table_start, low_table_end,
>  			      rom_table_start, rom_table_end);
>  
> -	return get_lb_mem();
> +	return mbi;
>  }
> Index: arch/x86/stage1.c
> ===================================================================
> --- arch/x86/stage1.c	(revision 870)
> +++ arch/x86/stage1.c	(working copy)
> @@ -141,10 +141,10 @@
>  #endif /* CONFIG_PAYLOAD_ELF_LOADER */
>  
>  
> -static int run_address_multiboot(void *f)
> +static int run_address_multiboot(void *f, struct multiboot_info *mbi)
>  {
>  	int ret, dummy;
> -	__asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (0xf0000), "c" (f) : "edx", "memory");
> +	__asm__ __volatile__ ("call *%4" : "=a" (ret), "=c" (dummy) : "a" (MB_MAGIC2), "b" (mbi), "c" (f) : "edx", "memory");
>  	return ret;
>  }
>  
> @@ -165,6 +165,7 @@
>  	int ret;
>  	struct mem_file archive;
>  	void *entry;
> +	struct multiboot_info *mbi;
>  #ifdef CONFIG_PAYLOAD_ELF_LOADER
>  	struct mem_file result;
>  	int elfboot_mem(struct lb_memory *mem, void *where, int size);
> @@ -252,8 +253,8 @@
>  	entry = load_file_segments(&archive, "normal/stage2");
>  	if (entry == (void *)-1)
>  		die("FATAL: Failed loading stage2.");
> -	ret = run_address(entry);
> -	if (ret)
> +	mbi = run_address(entry);
> +	if (! mbi)
>  		die("FATAL: Failed in stage2 code.");
>  
>  	printk(BIOS_DEBUG, "Stage2 code done.\n");
> @@ -268,7 +269,7 @@
>  	if (entry != (void*)-1) {
>  		/* Final coreboot call before handing off to the payload. */
>  		mainboard_pre_payload();
> -		run_address_multiboot(entry);
> +		run_address_multiboot(entry, mbi);
>  	} else {
>  		die("FATAL: No usable payload found.\n");
>  	}

> --
> coreboot mailing list: coreboot at coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot


-- 
Jordan Crouse
Systems Software Development Engineer 
Advanced Micro Devices, Inc.





More information about the coreboot mailing list