[SeaBIOS] [PATCH] coreboot: update memory table before handling execution to payload/system

Krystian Hebel posted 1 patch 5 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/20181105122710.19319-1-krystian.hebel@3mdeb.com
src/e820map.c     |  2 ++
src/e820map.h     |  2 ++
src/fw/coreboot.c | 50 +++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 50 insertions(+), 4 deletions(-)

[SeaBIOS] [PATCH] coreboot: update memory table before handling execution to payload/system

Posted by Krystian Hebel 5 weeks ago
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);
+}
-- 
2.17.1


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] coreboot: update memory table before handling execution to payload/system

Posted by Paul Menzel 5 weeks ago
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


[1]: https://review.coreboot.org/cgit/memtest86plus.git

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] coreboot: update memory table before handling execution to payload/system

Posted by Krystian Hebel 5 weeks ago

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=0704ab381a7ebd3c0100d2d7be9359076f713c43

[2]: https://review.coreboot.org/c/memtest86plus/+/29372

Regards,

Krystian


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

[SeaBIOS] coreboot's memtest86+ is buggy compared to memtest86+ floppy

Posted by Mike Banon 5 weeks ago
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.
>
> [1]: https://review.coreboot.org/cgit/memtest86plus.git/commit/?id=0704ab381a7ebd3c0100d2d7be9359076f713c43
>
> [2]: https://review.coreboot.org/c/memtest86plus/+/29372
>
> Regards,
>
> Krystian
>
>
> _______________________________________________
> SeaBIOS mailing list
> SeaBIOS@seabios.org
> https://mail.coreboot.org/mailman/listinfo/seabios

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] coreboot: update memory table before handling execution to payload/system

Posted by Kevin O'Connor 4 weeks ago
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

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios

Re: [SeaBIOS] [PATCH] coreboot: update memory table before handling execution to payload/system

Posted by Krystian Hebel 4 weeks ago
> 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.


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios