Hello Julius Werner, Reto Buerki,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37478
to review the following change.
Change subject: libpayload: Cache physical cbmem console address ......................................................................
libpayload: Cache physical cbmem console address
Same as with other consoles and drivers that cache an address outside the payload (e.g. video/corebootfb), we should store the physical address, so we can derive the virtual address on demand. This makes it save to use the address across relocations.
Fixes the default build of FILO, tested with Qemu/i440FX and Qemu/Q35.
Change-Id: I4b8434af69e0526f78523ae61981a15abb1295b0 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/cbmem_console.c 1 file changed, 17 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37478/1
diff --git a/payloads/libpayload/drivers/cbmem_console.c b/payloads/libpayload/drivers/cbmem_console.c index 9435e1c..35a8d3a 100644 --- a/payloads/libpayload/drivers/cbmem_console.c +++ b/payloads/libpayload/drivers/cbmem_console.c @@ -39,7 +39,7 @@ #define CURSOR_MASK ((1 << 28) - 1) #define OVERFLOW (1 << 31)
-static struct cbmem_console *cbmem_console_p; +static uintptr_t cbmem_console_p;
static struct console_output_driver cbmem_console_driver = { @@ -48,27 +48,32 @@
static void do_write(const void *buffer, size_t count) { - memcpy(cbmem_console_p->body + (cbmem_console_p->cursor & CURSOR_MASK), - buffer, count); - cbmem_console_p->cursor += count; + struct cbmem_console *const cbmem_con = phys_to_virt(cbmem_console_p); + + memcpy(cbmem_con->body + (cbmem_con->cursor & CURSOR_MASK), buffer, count); + cbmem_con->cursor += count; }
void cbmem_console_init(void) { - cbmem_console_p = lib_sysinfo.cbmem_cons; - if (cbmem_console_p && cbmem_console_p->size) + /* We might have been called before relocation (like FILO does). So + just keep the physical address which won't break on relocation. */ + cbmem_console_p = virt_to_phys(lib_sysinfo.cbmem_cons); + + struct cbmem_console *const cbmem_con = phys_to_virt(cbmem_console_p); + if (cbmem_console_p && cbmem_con->size) console_add_output_driver(&cbmem_console_driver); }
void cbmem_console_write(const void *buffer, size_t count) { - while ((cbmem_console_p->cursor & CURSOR_MASK) + count >= - cbmem_console_p->size) { - size_t still_fits = cbmem_console_p->size - - (cbmem_console_p->cursor & CURSOR_MASK); + struct cbmem_console *const cbmem_con = phys_to_virt(cbmem_console_p); + + while ((cbmem_con->cursor & CURSOR_MASK) + count >= cbmem_con->size) { + size_t still_fits = cbmem_con->size - (cbmem_con->cursor & CURSOR_MASK); do_write(buffer, still_fits); - cbmem_console_p->cursor &= ~CURSOR_MASK; - cbmem_console_p->cursor |= OVERFLOW; + cbmem_con->cursor &= ~CURSOR_MASK; + cbmem_con->cursor |= OVERFLOW; buffer += still_fits; count -= still_fits; }
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 60: just keep the physical address which won't break on relocation. */ So if this is a thing that happens, then why do we call phys_to_virt() in get_cbmem_ptr() from the coreboot table parsing code, put the result of that in lib_sysinfo, then have all these individual drivers virt_to_phys() that again, and then phys_to_virt() it again on every use? Wouldn't it make much more sense to change lib_sysinfo to only store physical uintptr_ts?
Paul Menzel 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//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37478/1//COMMIT_MSG@14 PS1, Line 14: Fixes the default build of FILO, tested with Qemu/i440FX and Qemu/Q35. The hang issue like below?
Current location: 0x100000-0x17e2ff Relocating to 0xbff0e000-0xbff8c2ff...
The FILO banner is shown, but no prompt?
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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37478/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37478/1//COMMIT_MSG@14 PS1, Line 14: Fixes the default build of FILO, tested with Qemu/i440FX and Qemu/Q35.
The hang issue like below? […]
Yep.
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. */
So if this is a thing that happens, then why do we call phys_to_virt() in get_cbmem_ptr() from the c […]
Only few drivers that cache pointers are affected...
I don't recall why we decided to have virtual pointers. But I unified `sysinfo` at some point: CB:1855. I wouldn't mind to make them all physical pointers, but that would affect a lot of code and potentially payloads too.
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.)
Reto Buerki 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+1
I can confirm that this fixes the "relocate" issue in FILO.
Reto Buerki 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
I just found out that using FILO with DEBUG_ALL enabled still shows the issue.
Reto Buerki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37478 )
Change subject: libpayload: Cache physical cbmem console address ......................................................................
Patch Set 1:
Patch Set 1: -Code-Review
I just found out that using FILO with DEBUG_ALL enabled still shows the issue.
This seems independent of the cbmem_console, as the problem persists even with
# CONFIG_CONSOLE_CBMEM is not set
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:
(2 comments)
I just found out that using FILO with DEBUG_ALL enabled still shows the issue.
This seems independent of the cbmem_console, as the problem persists even with
# CONFIG_CONSOLE_CBMEM is not set
As I feared, more debug options enabled => more bugs ;) Maybe there is another console driver suffering in a similar way. If it's the last debug() call in x86/segment.c, a console driver might rely on outdated sysinfo.
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... […]
It makes much more sense, I just don't have the time for this, right now and don't want to leave the default config broken.
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 […]
Yeah, this is fishy. I did it on purpose but I'm not sure anymore. What do we check here? if the field in sysinfo is NULL or if the value read from coreboot tables was NULL?
Making all sysinfo pointers physical, would solve this too.
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 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.
At some point we decided to re-initialize sysinfo after the relocation, to support its virtual pointers. I don't remember why. Might just have been the worst idea ;)
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:
(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... […]
Oh okay, yeah, then all this makes a bit more sense. And yeah, that was probably not the best solution. ;)
https://review.coreboot.org/c/coreboot/+/37478/1/payloads/libpayload/drivers... PS1, Line 64: cbmem_console_p
Yeah, this is fishy. I did it on purpose but I'm not sure anymore. What […]
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), I'm just saying I find the whole thing pretty hard to follow.
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.
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?
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
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.
Sorry, I didn't mean C pointers. Just addresses, they are already uints.
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?
I know it would leave some potential for confusion. But I just fail to see the point of `struct sysinfo` if we don't import the information. If it's merely a collection of pointers to CB stuff without any abstraction, we'll depend on the CB structures everywhere. We could as well just drop `sysinfo` and discover things on demand. Then nobody would have to ask why we keep all those physical pointers :)
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
Sorry, I didn't mean C pointers. Just addresses, they are already uints.
Okay, but then when it's an integer type doesn't that provide enough of a hint that it's a physical address?
But I just fail to see the point of `struct sysinfo` if we don't import the information. If it's merely a collection of pointers to CB stuff without any abstraction, we'll
depend on the CB structures everywhere.
I don't see how that would be a problem? I'm not sure what the point would be in copying things around that you already have elsewhere.
We could as well just drop `sysinfo` and discover things on demand.
Well, in my understanding lib_sysinfo is just a directory of all the stuff discovered from the coreboot table. So that you don't have to parse the table over and over again for every lookup. I think the more detailed interpretation of what that data means can belong in the individual drivers dealing with it.
Hello build bot (Jenkins), Reto Buerki, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37478
to look at the new patch set (#2).
Change subject: libpayload: Cache physical cbmem console address ......................................................................
libpayload: Cache physical cbmem console address
Same as with other consoles and drivers that cache an address outside the payload (e.g. video/corebootfb), we should store the physical address, so we can derive the virtual address on demand. This makes it save to use the address across relocations.
As a first step in migrating `libsysinfo` to `uintptr_t`, we also switch to the physical address there.
Fixes the default build of FILO, tested with Qemu/i440FX and Qemu/Q35.
Change-Id: I4b8434af69e0526f78523ae61981a15abb1295b0 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 4 files changed, 26 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37478/2
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 2:
(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
Sorry, I didn't mean C pointers. Just addresses, they are already uints. […]
At the end of the train, it's all physical pointers now. Let me know what you think.
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 2: Code-Review+2
(1 comment)
Sorry, I pretty much forgot everything about these patches since I last saw them. ;) Current version looks reasonable to me.
https://review.coreboot.org/c/coreboot/+/37478/2/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/37478/2/payloads/libpayload/include... PS2, Line 104: nit: tab
Hello Felix Singer, build bot (Jenkins), Reto Buerki, Julius Werner,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37478
to look at the new patch set (#3).
Change subject: libpayload: Cache physical cbmem console address ......................................................................
libpayload: Cache physical cbmem console address
Same as with other consoles and drivers that cache an address outside the payload (e.g. video/corebootfb), we should store the physical address, so we can derive the virtual address on demand. This makes it save to use the address across relocations.
As a first step in migrating `libsysinfo` to `uintptr_t`, we also switch to the physical address there.
Fixes the default build of FILO, tested with Qemu/i440FX and Qemu/Q35.
Change-Id: I4b8434af69e0526f78523ae61981a15abb1295b0 Signed-off-by: Nico Huber nico.h@gmx.de --- M payloads/coreinfo/bootlog_module.c M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 5 files changed, 30 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/37478/3
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 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37478/2/payloads/libpayload/include... File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/37478/2/payloads/libpayload/include... PS2, Line 104:
nit: tab
The surrounding fields are update in follow-up commits, probably not worth the effort to keep it aligned in between.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37478 )
Change subject: libpayload: Cache physical cbmem console address ......................................................................
Patch Set 3: Code-Review+1
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 3: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37478 )
Change subject: libpayload: Cache physical cbmem console address ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37478/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37478/1//COMMIT_MSG@14 PS1, Line 14: Fixes the default build of FILO, tested with Qemu/i440FX and Qemu/Q35.
Yep.
Ack
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 3:
(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. */
Oh okay, yeah, then all this makes a bit more sense. […]
Resolved by keeping physical pointers in sysinfo.
https://review.coreboot.org/c/coreboot/+/37478/1/payloads/libpayload/drivers... PS1, Line 64: cbmem_console_p
At the end of the train, it's all physical pointers now. Let me know […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37478 )
Change subject: libpayload: Cache physical cbmem console address ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37478 )
Change subject: libpayload: Cache physical cbmem console address ......................................................................
libpayload: Cache physical cbmem console address
Same as with other consoles and drivers that cache an address outside the payload (e.g. video/corebootfb), we should store the physical address, so we can derive the virtual address on demand. This makes it save to use the address across relocations.
As a first step in migrating `libsysinfo` to `uintptr_t`, we also switch to the physical address there.
Fixes the default build of FILO, tested with Qemu/i440FX and Qemu/Q35.
Change-Id: I4b8434af69e0526f78523ae61981a15abb1295b0 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/37478 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M payloads/coreinfo/bootlog_module.c M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/coreboot_tables.h M payloads/libpayload/include/sysinfo.h M payloads/libpayload/libc/coreboot.c 5 files changed, 30 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/payloads/coreinfo/bootlog_module.c b/payloads/coreinfo/bootlog_module.c index 1502a55..da9860a 100644 --- a/payloads/coreinfo/bootlog_module.c +++ b/payloads/coreinfo/bootlog_module.c @@ -110,7 +110,7 @@ return -1; }
- struct cbmem_console *console = lib_sysinfo.cbmem_cons; + struct cbmem_console *console = phys_to_virt(lib_sysinfo.cbmem_cons); if (console == NULL) { return -1; } diff --git a/payloads/libpayload/drivers/cbmem_console.c b/payloads/libpayload/drivers/cbmem_console.c index 053802c..22d5312 100644 --- a/payloads/libpayload/drivers/cbmem_console.c +++ b/payloads/libpayload/drivers/cbmem_console.c @@ -38,7 +38,7 @@ #define CURSOR_MASK ((1 << 28) - 1) #define OVERFLOW (1 << 31)
-static struct cbmem_console *cbmem_console_p; +static uintptr_t cbmem_console_p;
static struct console_output_driver cbmem_console_driver = { @@ -47,27 +47,32 @@
static void do_write(const void *buffer, size_t count) { - memcpy(cbmem_console_p->body + (cbmem_console_p->cursor & CURSOR_MASK), - buffer, count); - cbmem_console_p->cursor += count; + struct cbmem_console *const cbmem_cons = phys_to_virt(cbmem_console_p); + + memcpy(cbmem_cons->body + (cbmem_cons->cursor & CURSOR_MASK), buffer, count); + cbmem_cons->cursor += count; }
void cbmem_console_init(void) { + const struct cbmem_console *const cbmem_cons = phys_to_virt(lib_sysinfo.cbmem_cons); + cbmem_console_p = lib_sysinfo.cbmem_cons; - if (cbmem_console_p && cbmem_console_p->size) + + if (cbmem_console_p && cbmem_cons->size) console_add_output_driver(&cbmem_console_driver); }
void cbmem_console_write(const void *buffer, size_t count) { - while ((cbmem_console_p->cursor & CURSOR_MASK) + count >= - cbmem_console_p->size) { - size_t still_fits = cbmem_console_p->size - - (cbmem_console_p->cursor & CURSOR_MASK); + struct cbmem_console *const cbmem_cons = phys_to_virt(cbmem_console_p); + + while ((cbmem_cons->cursor & CURSOR_MASK) + count >= + cbmem_cons->size) { + size_t still_fits = cbmem_cons->size - (cbmem_cons->cursor & CURSOR_MASK); do_write(buffer, still_fits); - cbmem_console_p->cursor &= ~CURSOR_MASK; - cbmem_console_p->cursor |= OVERFLOW; + cbmem_cons->cursor &= ~CURSOR_MASK; + cbmem_cons->cursor |= OVERFLOW; buffer += still_fits; count -= still_fits; } @@ -77,7 +82,7 @@
char *cbmem_console_snapshot(void) { - const struct cbmem_console *console_p = cbmem_console_p; + const struct cbmem_console *const console_p = phys_to_virt(cbmem_console_p); char *console_c; uint32_t size, cursor, overflow;
diff --git a/payloads/libpayload/include/coreboot_tables.h b/payloads/libpayload/include/coreboot_tables.h index 91d3520..c281417 100644 --- a/payloads/libpayload/include/coreboot_tables.h +++ b/payloads/libpayload/include/coreboot_tables.h @@ -31,6 +31,7 @@
#include <arch/types.h> #include <ipchksum.h> +#include <stdint.h>
enum { CB_TAG_UNUSED = 0x0000, @@ -396,4 +397,5 @@
/* Helper functions */ void *get_cbmem_ptr(unsigned char *ptr); +uintptr_t get_cbmem_addr(const void *cbmem_tab_entry); #endif diff --git a/payloads/libpayload/include/sysinfo.h b/payloads/libpayload/include/sysinfo.h index 6e83f68..3b1b9c9 100644 --- a/payloads/libpayload/include/sysinfo.h +++ b/payloads/libpayload/include/sysinfo.h @@ -29,6 +29,8 @@ #ifndef _SYSINFO_H #define _SYSINFO_H
+#include <stdint.h> + /* Maximum number of memory range definitions. */ #define SYSINFO_MAX_MEM_RANGES 32 /* Allow a maximum of 8 GPIOs */ @@ -101,7 +103,7 @@ #endif
void *tstamp_table; - void *cbmem_cons; + uintptr_t cbmem_cons; void *mrc_cache; void *acpi_gnvs;
diff --git a/payloads/libpayload/libc/coreboot.c b/payloads/libpayload/libc/coreboot.c index c6fb57f..25812e5 100644 --- a/payloads/libpayload/libc/coreboot.c +++ b/payloads/libpayload/libc/coreboot.c @@ -47,6 +47,12 @@ return phys_to_virt(cbmem->cbmem_tab); }
+uintptr_t get_cbmem_addr(const void *const cbmem_tab_entry) +{ + const struct cb_cbmem_tab *const cbmem = cbmem_tab_entry; + return cbmem->cbmem_tab; +} + static void cb_parse_memory(void *ptr, struct sysinfo_t *info) { struct cb_memory *mem = ptr; @@ -135,7 +141,7 @@
static void cb_parse_cbmem_cons(unsigned char *ptr, struct sysinfo_t *info) { - info->cbmem_cons = get_cbmem_ptr(ptr); + info->cbmem_cons = get_cbmem_addr(ptr); }
static void cb_parse_acpi_gnvs(unsigned char *ptr, struct sysinfo_t *info)
-
Angel Pons (Code Review) -
Julius Werner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Reto Buerki (Code Review)