From 1f1619dcfd12baa2f8023b17dee7c9ae8985bceb Mon Sep 17 00:00:00 2001 From: Carl-Daniel Hailfinger Date: Sun, 10 Feb 2008 16:44:32 +0000 Subject: [PATCH] I found another bug in LAR handling of the option table by staring at the logs. (Yes, I read logs.) Looking at the log of a qemu x86 boot with a 1024 kB image, I'd say something is really broken. Excerpts of my log quoted below: > [...] > LAR: Attempting to open 'fallback/initram/segment0'. > LAR: Start 0xfff00000 len 0x100000 > LAR: seen member normal/option_table > LAR: seen member normal/stage2/segment0 > LAR: seen member normal/stage2/segment1 > LAR: seen member normal/stage2/segment2 > LAR: seen member normal/initram/segment0 > LAR: seen member bootblock > LAR: File not found! > [...] > LAR: Attempting to open 'normal/initram/segment0'. > LAR: Start 0xfff00000 len 0x100000 > LAR: seen member normal/option_table > LAR: seen member normal/stage2/segment0 > LAR: seen member normal/stage2/segment1 > LAR: seen member normal/stage2/segment2 > LAR: seen member normal/initram/segment0 > LAR: CHECK normal/initram/segment0 @ 0xfff040d0 > [...] > LAR: Attempting to open 'normal/option_table'. > LAR: Start 0xfffc0000 len 0x3c000 WTF?!? This start address is obviously very wrong. > LAR: seen member bootblock > LAR: File not found! Which results in not finding the option table. > [...] > LAR: Attempting to open 'normal/payload'. > LAR: Start 0xfff00000 len 0x100000 > LAR: seen member normal/option_table > LAR: seen member normal/stage2/segment0 > LAR: seen member normal/stage2/segment1 > LAR: seen member normal/stage2/segment2 > LAR: seen member normal/initram/segment0 > LAR: seen member bootblock > LAR: File not found! > [...] The bug is in arch/x86/mc146818rtc.c: > struct cmos_option_table *get_option_table(void) > { > struct mem_file result, archive; > int ret; > > // FIXME - i want to be dynamic. > archive.len=(CONFIG_COREBOOT_ROMSIZE_KB-16)*1024; We can't calculate len like that. Reasons: - We can't know at compile time how big the archive is going to be. - Subtracting 16 kB from the ROM size was needed when the bootblock was not part of the LAR. These times are long gone. > archive.start=(void *)(0UL-(CONFIG_COREBOOT_ROMSIZE_KB*1024)); Since the len calculation above is invalid, start is wrong as well. > ret = find_file(&archive, "normal/option_table", &result); > if (ret) { > printk(BIOS_ERR, "No such file '%s'.\n", > "normal/option_table"); > return (struct cmos_option_table *)NULL; > } > return (struct cmos_option_table *) result.start; > } Use the existing init_archive function to find the LAR in memory. This fixes the case where the option table was not found depending on a few unrelated parameters. Signed-off-by: Carl-Daniel Hailfinger Acked-by: Stefan Reinauer git-svn-id: svn://coreboot.org/repository/coreboot-v3@584 f3766cd6-281f-0410-b1cd-43a5c92072e9 --- arch/x86/mc146818rtc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/arch/x86/mc146818rtc.c b/arch/x86/mc146818rtc.c index 3ca57ea557..e945de753c 100644 --- a/arch/x86/mc146818rtc.c +++ b/arch/x86/mc146818rtc.c @@ -183,9 +183,7 @@ struct cmos_option_table *get_option_table(void) struct mem_file result, archive; int ret; - // FIXME - i want to be dynamic. - archive.len=(CONFIG_COREBOOT_ROMSIZE_KB-16)*1024; - archive.start=(void *)(0UL-(CONFIG_COREBOOT_ROMSIZE_KB*1024)); + init_archive(&archive); ret = find_file(&archive, "normal/option_table", &result); if (ret) {