Julius Werner 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
However, it's the equivalent of `cbtable.cbmem_cons + virt_offset - virt_offset`.
Yeah, that's what I meant. See, it's complicated. ;)
However, inside the structs, there may be more pointers and those have to be physical (because we refer to the original tables).
Are there really? I can't see any obvious examples right now. In general, pointers should never be directly in structures that are meant to be accessed across program boundaries (because pointer size might change), they should always be converted to uint32_t or uint64_t. If we have examples where they aren't, we should probably just fix that instead.
Don't point to anything outside the payload unless it is supposed to be written to (e.g. MMIO, CBMEM console).
Hmm... that's copying a lot of stuff even though most of it is often not needed (e.g. all the build information). I think it might also get confusing with the difference between "supposed to be written to" and the rest. Can't we just make it all uintptr_t instead?