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: Code-Review+2
(2 comments)
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 60: just keep the physical address which won't break on relocation. */
Only few drivers that cache pointers are affected...
Are you sure (maybe I'm still not understanding this right)? If this code runs before relocation then the phys_to_virt() in get_cbmem_ptr() also runs before relocation, right? So the pointer in lib_sysinfo still has the value of a physical pointer, even though it pretends to be a virtual pointer. So if some other driver doesn't cache it but instead just tries to access the pointer directly at a point after relocation, it'll also die.
but that would affect a lot of code and potentially payloads too.
I think our general policy for affecting payloads with libpayload changes is "they'll have to deal with it". ;) At least I've never been too worried about that when cleaning things up.
I think doing it that way would make more sense than this back-and-forth, but I'll leave it up to you.
https://review.coreboot.org/c/coreboot/+/37478/1/payloads/libpayload/drivers... PS1, Line 64: cbmem_console_p (This is another good reason to keep it physical as long as possible, because it really screws with NULL checks... it's not super obvious why you're checking this instead of cbmem_con here.)