erebion/coreboot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

I found another bug in LAR handling of the option table by staring at

Browse source 
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 <c-d.hailfinger.devel.2006@gmx.net>
Acked-by: Stefan Reinauer <stepan@coresystems.de>


git-svn-id: svn://coreboot.org/repository/coreboot-v3@584 f3766cd6-281f-0410-b1cd-43a5c92072e9
This commit is contained in:
Carl-Daniel Hailfinger 2008-02-10 16:44:32 +00:00
commit 1f1619dcfd
1 changed files with 1 additions and 3 deletions
Download patch file Download diff file Expand all files Collapse all files

4
arch/x86/mc146818rtc.c
View file

@ -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) {