[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