Re: [coreboot] Multiboot bugfix (coreboot-v3)

[Jordan Crouse]

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.
>
> 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

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
> 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");
> }
>