Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37478 )
Change subject: libpayload: Cache physical cbmem console address ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37478/1/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37478/1/payloads/libpayload/drivers... PS1, Line 64: cbmem_console_p
Yes, we're checking if the field in sysinfo was NULL. phys_to_virt() is not checking for NULL, so phys_to_virt(NULL) is just (void *)virt_offset. This code is correct because you're essentially checking (lib_sysinfo.cbmem_cons + virt_offset - virt_offset == NULL),
Naa, `cbmem_console_p` checked here is `lib_sysinfo.cbmem_cons - virt_offset`. However, it's the equivalent of `cbtable.cbmem_cons + virt_offset - virt_offset`.
I'm just saying I find the whole thing pretty hard to follow.
Yes. That is what convinced me to follow your advice and have a closer look at the pointers in `libsysinfo`.
It's a mess to be honest ;) we have pointers to cb table structs, replicated cb table structs, pointers to cbmem entries... all virtual. However, inside the structs, there may be more pointers and those have to be physical (because we refer to the original tables).
Here is an idea how to clean it up: * Don't point to anything outside the payload unless it is supposed to be written to (e.g. MMIO, CBMEM console). * For all the referenced table entries, copy the required data into payload space (heap or directly into the sysinfo struct). * Don't use pointers for anything outside the payload, use physical addresses and `uintptr_t` to make it more obvious that it needs conversion.