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, I'd say something is really broken. Look at the 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/stage2/segment0'. LAR: Start 0xfff00000 len 0x100000 LAR: seen member normal/option_table LAR: seen member normal/stage2/segment0 LAR: CHECK normal/stage2/segment0 @ 0xfff00400 [...] LAR: Attempting to open 'normal/stage2/segment1'. LAR: Start 0xfff00000 len 0x100000 LAR: seen member normal/option_table LAR: seen member normal/stage2/segment0 LAR: seen member normal/stage2/segment1 LAR: CHECK normal/stage2/segment1 @ 0xfff004c0 [...] LAR: Attempting to open 'normal/stage2/segment2'. 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: CHECK normal/stage2/segment2 @ 0xfff03f40 [...] LAR: Attempting to open 'normal/stage2/segment3'. 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/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! [...] LAR: Attempting to open 'normal/payload/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! [...]
Short version of the above:
LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfffc0000 len 0x3c000 <------BUG! LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000
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;
len is wrong.
archive.start=(void *)(0UL-(CONFIG_COREBOOT_ROMSIZE_KB*1024));
start is even more wrong.
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
Index: LinuxBIOSv3-optiontable/arch/x86/mc146818rtc.c =================================================================== --- LinuxBIOSv3-optiontable/arch/x86/mc146818rtc.c (Revision 583) +++ LinuxBIOSv3-optiontable/arch/x86/mc146818rtc.c (Arbeitskopie) @@ -183,9 +183,7 @@ 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) {
Regards, Carl-Daniel
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080210 13:02]:
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
Thanks for spotting this. It was on my todo since a while actually. But since the lar segment stuff got in I can not build v3 anymore, so I didn't get around it yet.
Acked-by: Stefan Reinauer stepan@coresystems.de
* Stefan Reinauer stepan@coresystems.de [080210 14:15]:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080210 13:02]:
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.
The parameters are obviously very related. The functionality was just moved into the lar utility ;-)
LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfffc0000 len 0x3c000 <------BUG! LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000
Your init_archive function is pretty broken, so the above line with the "BUG" mentioned is the only one that is half ways correct.
You mentioned this is a default build, made with:
- 256 KB (COREBOOT_ROMSIZE_KB_256) (NEW)
So what I don't understand is why only the BUG line really assumes a 256k ROM while all others assume a 1M ROM.
Maybe this introduced all the access speed problems we were seeing lately? Scanning 4x the needed area is pretty overkill.
On Sun, Feb 10, 2008 at 02:27:51PM +0100, Stefan Reinauer wrote:
LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfffc0000 len 0x3c000 <------BUG!
..
So what I don't understand is why only the BUG line really assumes a 256k ROM while all others assume a 1M ROM.
Actually 256k would be 0x40000 so neither of the two algorithms seem to be spot on. :\
//Peter
Peter Stuge wrote:
On Sun, Feb 10, 2008 at 02:27:51PM +0100, Stefan Reinauer wrote:
LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfffc0000 len 0x3c000 <------BUG!
..
So what I don't understand is why only the BUG line really assumes a 256k ROM while all others assume a 1M ROM.
Actually 256k would be 0x40000 so neither of the two algorithms seem to be spot on. :\
Agreed.
The reason it is 16k less was that at the time this code was originally written the 16k (back then) bootblock was not part of the lar.
Stefan
On 10.02.2008 14:27, Stefan Reinauer wrote:
- Stefan Reinauer stepan@coresystems.de [080210 14:15]:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080210 13:02]:
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.
The parameters are obviously very related. The functionality was just moved into the lar utility ;-)
LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfffc0000 len 0x3c000 <------BUG! LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000
Your init_archive function is pretty broken, so the above line with the "BUG" mentioned is the only one that is half ways correct.
init_archive() is just moved code which was initially committed in r427. Changelog follows:
r427 | stepan | 2007-07-01 00:03:39 +0200 (So, 01 Jul 2007) | 13 lines
this patch puts the lar size in the bootblock and reads it from there. Why? This way we don't need to recompile the image when the size of the LinuxBIOS image changes. This alows building images for 50 motherboards and equipping each with 10 payloads, resulting in 500 images while you only have to build each payload once and each motherboard, too.
You mentioned this is a default build, made with:
- 256 KB (COREBOOT_ROMSIZE_KB_256) (NEW)
So what I don't understand is why only the BUG line really assumes a 256k ROM while all others assume a 1M ROM.
Ouch. arch/x86/stage1.c:init_archive() tries to read the data written by util/lar/bootblock.c:fixup_bootblock(). Let's find out where the garbage is introduced.
Maybe this introduced all the access speed problems we were seeing lately? Scanning 4x the needed area is pretty overkill.
That would indeed be bad.
Regards, Carl-Daniel
Problem solved.
On 10.02.2008 15:22, Carl-Daniel Hailfinger wrote:
On 10.02.2008 14:27, Stefan Reinauer wrote:
- Stefan Reinauer stepan@coresystems.de [080210 14:15]:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080210 13:02]:
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.
The parameters are obviously very related. The functionality was just moved into the lar utility ;-)
LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfffc0000 len 0x3c000 <------BUG! LAR: Start 0xfff00000 len 0x100000 LAR: Start 0xfff00000 len 0x100000
Your init_archive function is pretty broken, so the above line with the "BUG" mentioned is the only one that is half ways correct.
No, it is the only line being wrong.
You mentioned this is a default build, made with:
defconfig build, not default build. I'd like to blame the people for the confusion.
- 256 KB (COREBOOT_ROMSIZE_KB_256) (NEW)
So what I don't understand is why only the BUG line really assumes a 256k ROM while all others assume a 1M ROM.
Ouch. arch/x86/stage1.c:init_archive() tries to read the data written by util/lar/bootblock.c:fixup_bootblock(). Let's find out where the garbage is introduced.
No garbage, the defconfig image is 1024k and init_archive() reads exactly that value from the ROM.
Regards, Carl-Daniel
On 10.02.2008 14:15, Stefan Reinauer wrote:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080210 13:02]:
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
Thanks for spotting this. It was on my todo since a while actually. But since the lar segment stuff got in I can not build v3 anymore, so I didn't get around it yet.
Acked-by: Stefan Reinauer stepan@coresystems.de
Thanks, r584 with improved changelog.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Peter Stuge -
Stefan Reinauer