SeaBIOS modifies its internal e820 structure, but does not propagate these changes back to coreboot tables. This resulted in multiple errors in MemTest86 when run on 2 GB platforms, probably because of some memory-mapped devices.
This patch copies back modified e820 tables before booting an OS or a payload.
Change-Id: I16a3f059695717daedb1e997ce4e62018c7e02b3 Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com --- src/e820map.c | 2 ++ src/e820map.h | 2 ++ src/fw/coreboot.c | 50 +++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/src/e820map.c b/src/e820map.c index 39445cf6399d..2d2de6094b14 100644 --- a/src/e820map.c +++ b/src/e820map.c @@ -148,5 +148,7 @@ e820_remove(u64 start, u64 size) void e820_prepboot(void) { + // Synchronize memory maps. + coreboot_update_memtable(); dump_map(); } diff --git a/src/e820map.h b/src/e820map.h index de8b523003c5..78bf1ce76572 100644 --- a/src/e820map.h +++ b/src/e820map.h @@ -23,4 +23,6 @@ void e820_prepboot(void); extern struct e820entry e820_list[]; extern int e820_count;
+extern void coreboot_update_memtable(void); + #endif // e820map.h diff --git a/src/fw/coreboot.c b/src/fw/coreboot.c index 7c0954b5398c..aa07baad3a7f 100644 --- a/src/fw/coreboot.c +++ b/src/fw/coreboot.c @@ -189,10 +189,6 @@ coreboot_preinit(void) e820_add(m->start, m->size, type); }
- // Ughh - coreboot likes to set a map at 0x0000-0x1000, but this - // confuses grub. So, override it. - e820_add(0, 16*1024, E820_RAM); - struct cb_cbmem_ref *cbref = find_cb_subtable(cbh, CB_TAG_CBMEM_CONSOLE); if (cbref) { cbcon = (void*)(u32)cbref->cbmem_addr; @@ -567,3 +563,49 @@ cbfs_payload_setup(void) boot_add_cbfs(cfile->fhdr, desc, bootprio_find_named_rom(filename, 0)); } } + +// Mirror changes done on e820 to coreboot tables. +void +coreboot_update_memtable(void) +{ + if (!CONFIG_COREBOOT) + return; + + // Find coreboot table. + struct cb_header *cbh = find_cb_table(); + if (!cbh) { + dprintf(1, "Unable to find coreboot table!\n"); + return; + } + char *end = (char *)cbh + sizeof(*cbh) + cbh->table_bytes; + dprintf(3, "Now attempting to find coreboot memory map\n"); + struct cb_memory *dst = find_cb_subtable(cbh, CB_TAG_MEMORY); + if (!dst) { + dprintf(1, "Unable to find coreboot memory table!\n"); + return; + } + + // Remove old memory table, overwriting it with following subtables + // and append an updated memory table to the end. + u32 old_size = dst->size; + char *src = (char *)dst + old_size; + memcpy(dst, src, (end - src)); + dst = (struct cb_memory *)(end - old_size); + u32 new_size = e820_count * sizeof(struct e820entry); + dst->tag = CB_TAG_MEMORY; + // Tables are binary compatible, except that CB_MEM_TABLE became + // E820_RESERVED. + memcpy((char*)dst + sizeof(struct cb_memory), e820_list, new_size); + new_size += sizeof(struct cb_memory); + dst->size = new_size; + + // Update header fields (size and checksum). + cbh->table_bytes += new_size - old_size; + cbh->table_checksum = ipchksum((char*)cbh + sizeof(*cbh), cbh->table_bytes); + cbh->header_checksum = 0; + cbh->header_checksum = ipchksum((char*)cbh, sizeof(*cbh)); + + // Ughh - coreboot likes to set a map at 0x0000-0x1000, but this + // confuses grub. So, override it in e820 only. + e820_add(0, 16*1024, E820_RAM); +}
Dear Krystian,
On 11/05/18 13:27, Krystian Hebel wrote:
SeaBIOS modifies its internal e820 structure, but does not propagate these changes back to coreboot tables. This resulted in multiple errors in MemTest86 when run on 2 GB platforms, probably because of some memory-mapped devices.
This patch copies back modified e820 tables before booting an OS or a payload.
Thank you for the patch. If it’s the common problem, then it was fixed in “coreboot’s” MemTest86+ [1]. Can you reproduce the problem with that version?
Kind regards,
Paul
On 11/05/18 13:32, Paul Menzel wrote:
Thank you for the patch. If it’s the common problem, then it was > fixed in “coreboot’s” MemTest86+ [1]. Can you reproduce the problem with that version?
Yes, it is the version on which I tested. It worked OK before [1], because when no coreboot tables were found MemTest86+ resorted to e820.
It was reproduced on PC Engines apu3 (2 GB version) and apu1, the latter requires SPD fix [2]. 4 GB versions of apu doesn't seem to be affected.
[1]: https://review.coreboot.org/cgit/memtest86plus.git/commit/?id=0704ab381a7ebd...
[2]: https://review.coreboot.org/c/memtest86plus/+/29372
Regards,
Krystian
coreboot's memtest86+ payload is buggy compared to memtest86+ floppy (which could be downloaded from memtest.org and added to CBFS to be accessible as SeaBIOS menu entry). at AMD Lenovo G505S and maybe some other coreboot laptops, the USB devices like keyboard are not working at coreboot's memtest86+ while working fine at all the other payloads using "libpayload" (i.e. coreinfo or tint) - so it's not a libpayload problem - and also the same USB keyboards are working at memtest86+ floppy. So it is obvious something is broken at 86+ payload source code.
Also, with LZMA compression 86+ floppy occupies much less space than 86+ payload. So, aside from academic/research purposes, I do not see any advantages of 86+ payload compared to 86+ floppy - only disadvantages: larger size and troubled USB. I've seriously considered submitting a patch which replaces coreboot's memtest86+ payload with a floppy (download, compare its' checksum and then insert to CBFS), but then I thought that maybe someone could need 86+ payload as a coding example.
If you would like to try out a floppy (e.g. because something else - like 2GB support - could be also broken at 86+ payload, while working fine at 86+ floppy booted through SeaBIOS) , here are the instructions:
1) Download the latest 5.01 version of memtest86+ from memtest86.org :
wget https://www.memtest.org/download/5.01/memtest86+-5.01.floppy.zip
2) Calculate its' sha256 :
sha256sum ./memtest86+-5.01.floppy.zip
should be
2a2d4c1234c9130e1da5fea941ccfbaa343739d5b3302b5f3f9b24077868f8ee ./memtest86+-5.01.floppy.zip
3) If sha256 is correct, unzip ./memtest86+-5.01.floppy.zip . You'll get a "floppy" directory with these files: ls ./floppy/ dd.exe install64.bat install.bat memtestp.bin rawrite.exe README.txt You only need memtestp.bin , sha256 of which is " ddd4a2ba44c312aa4f2c7506a388cc2ca7f1caec60c3c6d80ed8a9f0b43d529c "
4) Size of memtestp.bin file is 150024 bytes. To be understood by SeaBIOS, it needs to be expanded to 1474560 bytes (by zeroes), which could be done with this command:
dd if=/dev/zero of=./memtestp.bin bs=1 count=1 seek=1474559 conv=notrunc
sha256 of expanded memtestp.bin file will be " 364535abd0d105da9396df6015e480c4d4c52b07dcc4e1d4756bde8ef87a30f1 "
5) Now it could be added to the compiled coreboot.rom with this command:
./build/cbfstool ./build/coreboot.rom add -f ./floppy/memtestp.bin -n floppyimg/memtestp.lzma -t raw -c lzma
If done everything correctly, it will be available at SeaBIOS boot menu as Ramdisk [memtestp]
Best regards, Mike Banon
On Mon, Nov 5, 2018 at 3:51 PM Krystian Hebel krystian.hebel@3mdeb.com wrote:
On 11/05/18 13:32, Paul Menzel wrote:
Thank you for the patch. If it’s the common problem, then it was > fixed in “coreboot’s” MemTest86+ [1]. Can you reproduce the problem > with that version?
Yes, it is the version on which I tested. It worked OK before [1], because when no coreboot tables were found MemTest86+ resorted to e820.
It was reproduced on PC Engines apu3 (2 GB version) and apu1, the latter requires SPD fix [2]. 4 GB versions of apu doesn't seem to be affected.
Regards,
Krystian
SeaBIOS mailing list SeaBIOS@seabios.org https://mail.coreboot.org/mailman/listinfo/seabios
On Mon, Nov 05, 2018 at 01:27:10PM +0100, Krystian Hebel wrote:
SeaBIOS modifies its internal e820 structure, but does not propagate these changes back to coreboot tables. This resulted in multiple errors in MemTest86 when run on 2 GB platforms, probably because of some memory-mapped devices.
This patch copies back modified e820 tables before booting an OS or a payload.
Thanks, but I'm not sure that at a "high level" this is a good idea. I don't think SeaBIOS should be altering coreboot information. (Doing so leads to all sorts of painful debugging problems, for example.) If memtest86 is having problems with the coreboot tables I suspect it would be best to alter memtest86 to not read the coreboot tables.
-Kevin
I don't think SeaBIOS should be altering coreboot information. (Doing so leads to all sorts of painful debugging problems, for example.)
Well, currently it is marking at least first couple of kilobytes of memory (4 if I recall correctly) as free to use RAM. There coreboot tables are located, or at least a pointer to tables in higher memory. Because of that these tables can get overwritten by SeaBIOS or OS that starts later.
Having a device marked as an available RAM is even worse - writing unchecked values to some device registers (e.g. by a coreboot-aware system which makes use of outdated memory tables) can lead to undefined behaviour (best case scenario) and even physically damage the platform or connected devices. Trying to debug this can be even worse, especially with address space randomisation implemented in most modern operating systems.
This patch affects not only memtest, it is just clearly and immediately visible there.
-
Kevin O'Connor -
Krystian Hebel -
Mike Banon -
Paul Menzel