Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
libpayload: Read from cbmem console
To support the new recovery ui (the Groot project) and show cbmem log on the screen, add a function cbmem_console_read() to read from cbmem console. Non-printable characters are automatically replaced with '?' to ensure that the returned string is printable.
BRANCH=none BUG=b:146105976 TEST=emerge-nami libpayload
Change-Id: Ie324055f5fd8276f1d833fc9d04f60a792dbb9f6 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/libpayload.h 2 files changed, 48 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/37667/1
diff --git a/payloads/libpayload/drivers/cbmem_console.c b/payloads/libpayload/drivers/cbmem_console.c index 9435e1c..c6d7ee1 100644 --- a/payloads/libpayload/drivers/cbmem_console.c +++ b/payloads/libpayload/drivers/cbmem_console.c @@ -75,3 +75,50 @@
do_write(buffer, count); } + +void cbmem_console_read(void **buffer_p, size_t *count) +{ + const struct cbmem_console *console_p = cbmem_console_p; + char *console_c; + uint32_t size, cursor, overflow; + + if (!console_p) { + printf("ERROR: No cbmem console found in coreboot table.\n"); + return; + } + + cursor = console_p->cursor & CURSOR_MASK; + overflow = console_p->cursor & OVERFLOW; + if (!overflow && cursor < console_p->size) + size = cursor; + else + size = console_p->size; + + console_c = malloc(size + 1); + if (!console_c) { + printf("ERROR: Not enough memory for console.\n"); + return; + } + console_c[size] = '\0'; + + if (overflow) { + if (cursor >= size) { + printf("cbmem: ERROR: CBMEM console struct is illegal, " + "output may be corrupt or out of order!\n\n"); + cursor = 0; + } + memcpy(console_c, console_p->body + cursor, size - cursor); + memcpy(console_c + size - cursor, console_p->body, cursor); + } else { + memcpy(console_c, console_p->body, size); + } + + /* Slight memory corruption may occur between reboots and give us a few + unprintable characters like '\0'. Replace them with '?' on output. */ + for (cursor = 0; cursor < size; cursor++) + if (!isprint(console_c[cursor]) && !isspace(console_c[cursor])) + console_c[cursor] = '?'; + + *buffer_p = console_c; + *count = size; +} diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 934c368..6d814f1 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -313,6 +313,7 @@ */ void cbmem_console_init(void); void cbmem_console_write(const void *buffer, size_t count); +void cbmem_console_read(void **buffer_p, size_t *count); /** @} */
/* drivers/option.c */
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37667/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37667/1//COMMIT_MSG@9 PS1, Line 9: To support the new recovery ui (the Groot project) That is a Chrome OS specific thing so we should replace this with some generic term.
"To support showing cbmem logs on screen" should be good enough.
Hello Julius Werner, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37667
to look at the new patch set (#2).
Change subject: libpayload: Read from cbmem console ......................................................................
libpayload: Read from cbmem console
To support showing cbmem logs on recovery screen, add a function cbmem_console_read() to read from cbmem console. Non-printable characters are automatically replaced with '?' to ensure that the returned string is printable.
BRANCH=none BUG=b:146105976 TEST=emerge-nami libpayload
Change-Id: Ie324055f5fd8276f1d833fc9d04f60a792dbb9f6 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/libpayload.h 2 files changed, 48 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/37667/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37667/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37667/1//COMMIT_MSG@9 PS1, Line 9: To support the new recovery ui (the Groot project)
That is a Chrome OS specific thing so we should replace this with some generic term. […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
Patch Set 2:
(6 comments)
I'm not very convinced if this must be inside libpayload, since it'll need to copy whole buffer; maybe the payload can be more efficient on reading only the contents of one page, and scroll/update on demand.
But if you think this is really the right thing to do, I think it's no harm to have an extra API in libpayload.
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: size_t uint32_t ?
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: cbmem_console_read I'll probably call this cbmem_console_readback
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 85: if (!console_p) { : printf("ERROR: No cbmem console found in coreboot table.\n"); : return; : } : should we still set *buffer_p to NULL or *count to 0? especially there's no return value
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 99: enough also print the expected size.
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 100: return should we let caller know we've failed getting logs?
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 107: corrupt corrupted
should we just insert this into beginning of the output buffer? since the print here won't be noticed by users
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@7 PS2, Line 7: libpayload: Read from cbmem console Implement reading from CBMEM console
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@11 PS2, Line 11: that Could be removed.
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@12 PS2, Line 12: returned string is printable. I was wondering, why there might be unprintable characters, and found your comment in the implementation.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
Patch Set 2:
(2 comments)
Patch Set 2:
(6 comments)
I'm not very convinced if this must be inside libpayload, since it'll need to copy whole buffer; maybe the payload can be more efficient on reading only the contents of one page, and scroll/update on demand.
In this case, we'll have to remember the offset that has been read. However, it's possible that the data from the offset is overwritten before the next scroll/update call.
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@12 PS2, Line 12: returned string is printable.
I was wondering, why there might be unprintable characters, and found your comment in the implementa […]
Ack
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: size_t
uint32_t ?
I just want to be consistent with cbmem_console_write().
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Read from cbmem console ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: cbmem_console_read
I'll probably call this cbmem_console_readback
Does it make more sense with char *cbmem_console_read(size_t *count), or even discard the 'count' argument since the '\0's are all removed.
Hello Julius Werner, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37667
to look at the new patch set (#3).
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
libpayload: Implement reading from CBMEM console
To support showing CBMEM logs on recovery screen, add a function cbmem_console_read() to read from CBMEM console. Non-printable characters are automatically replaced with '?' to ensure the returned string is printable.
BRANCH=none BUG=b:146105976 TEST=emerge-nami libpayload
Change-Id: Ie324055f5fd8276f1d833fc9d04f60a792dbb9f6 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/libpayload.h 2 files changed, 55 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/37667/3
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@7 PS2, Line 7: libpayload: Read from cbmem console
Implement reading from CBMEM console
Done
https://review.coreboot.org/c/coreboot/+/37667/2//COMMIT_MSG@11 PS2, Line 11: that
Could be removed.
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 85: if (!console_p) { : printf("ERROR: No cbmem console found in coreboot table.\n"); : return; : } :
should we still set *buffer_p to NULL or *count to 0? especially there's no return value
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 99: enough
also print the expected size.
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 100: return
should we let caller know we've failed getting logs?
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 107: corrupt
corrupted […]
How about treating this the same as other errors?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: cbmem_console_read
Does it make more sense with […]
I like the idea of simplifying param type, but we also have to make the function name very clear about that caller must free the returned buffer.
What about
char *cbmem_snapshot_console(void);
That it's very clear this is a snapshot of console messages (which implies this is an extra buffer not sharing real console).
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: size_t
I just want to be consistent with cbmem_console_write().
Well let's remove the count.
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 107: corrupt
How about treating this the same as other errors?
Ack
Hello Julius Werner, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37667
to look at the new patch set (#4).
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
libpayload: Implement reading from CBMEM console
To support showing CBMEM logs on recovery screen, add a function cbmem_console_snapshot() to copy the CBMEM console to an allocated buffer. Non-printable characters are automatically replaced with '?' to ensure the returned string is printable.
BRANCH=none BUG=b:146105976 TEST=emerge-nami libpayload
Change-Id: Ie324055f5fd8276f1d833fc9d04f60a792dbb9f6 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/libpayload.h 2 files changed, 47 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/37667/4
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... File payloads/libpayload/drivers/cbmem_console.c:
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: cbmem_console_read
I like the idea of simplifying param type, but we also have to make the function name very clear abo […]
Done
https://review.coreboot.org/c/coreboot/+/37667/2/payloads/libpayload/drivers... PS2, Line 79: size_t
Well let's remove the count.
Ack
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
Patch Set 4:
ok, the implementation itself is good enough, but I think we need some document in comment describing how to use this function. Can you add that?
Hello Julius Werner, Hung-Te Lin, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37667
to look at the new patch set (#5).
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
libpayload: Implement reading from CBMEM console
To support showing CBMEM logs on recovery screen, add a function cbmem_console_snapshot() to copy the CBMEM console to an allocated buffer. Non-printable characters are automatically replaced with '?' to ensure the returned string is printable.
BRANCH=none BUG=b:146105976 TEST=emerge-nami libpayload
Change-Id: Ie324055f5fd8276f1d833fc9d04f60a792dbb9f6 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/libpayload.h 2 files changed, 53 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/37667/5
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
Patch Set 5:
Patch Set 4:
ok, the implementation itself is good enough, but I think we need some document in comment describing how to use this function. Can you add that?
Description added.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37667 )
Change subject: libpayload: Implement reading from CBMEM console ......................................................................
libpayload: Implement reading from CBMEM console
To support showing CBMEM logs on recovery screen, add a function cbmem_console_snapshot() to copy the CBMEM console to an allocated buffer. Non-printable characters are automatically replaced with '?' to ensure the returned string is printable.
BRANCH=none BUG=b:146105976 TEST=emerge-nami libpayload
Change-Id: Ie324055f5fd8276f1d833fc9d04f60a792dbb9f6 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37667 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Hung-Te Lin hungte@chromium.org --- M payloads/libpayload/drivers/cbmem_console.c M payloads/libpayload/include/libpayload.h 2 files changed, 53 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/payloads/libpayload/drivers/cbmem_console.c b/payloads/libpayload/drivers/cbmem_console.c index 9435e1c..62aabfb 100644 --- a/payloads/libpayload/drivers/cbmem_console.c +++ b/payloads/libpayload/drivers/cbmem_console.c @@ -75,3 +75,49 @@
do_write(buffer, count); } + +char *cbmem_console_snapshot(void) +{ + const struct cbmem_console *console_p = cbmem_console_p; + char *console_c; + uint32_t size, cursor, overflow; + + if (!console_p) { + printf("ERROR: No cbmem console found in coreboot table\n"); + return NULL; + } + + cursor = console_p->cursor & CURSOR_MASK; + overflow = console_p->cursor & OVERFLOW; + if (!overflow && cursor < console_p->size) + size = cursor; + else + size = console_p->size; + + console_c = malloc(size + 1); + if (!console_c) { + printf("ERROR: Not enough memory for console (size = %u)\n", + size); + return NULL; + } + console_c[size] = '\0'; + + if (overflow) { + if (cursor >= size) { + printf("ERROR: CBMEM console struct is corrupted\n"); + return NULL; + } + memcpy(console_c, console_p->body + cursor, size - cursor); + memcpy(console_c + size - cursor, console_p->body, cursor); + } else { + memcpy(console_c, console_p->body, size); + } + + /* Slight memory corruption may occur between reboots and give us a few + unprintable characters like '\0'. Replace them with '?' on output. */ + for (cursor = 0; cursor < size; cursor++) + if (!isprint(console_c[cursor]) && !isspace(console_c[cursor])) + console_c[cursor] = '?'; + + return console_c; +} diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 934c368..4b6a250 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -313,6 +313,13 @@ */ void cbmem_console_init(void); void cbmem_console_write(const void *buffer, size_t count); +/** + * Take a snapshot of the CBMEM memory console. This function will allocate a + * range of memory. Callers must free the returned buffer by themselves. + * + * @return The allocated buffer on success, NULL on failure. + */ +char *cbmem_console_snapshot(void); /** @} */
/* drivers/option.c */
-
Hung-Te Lin (Code Review) -
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Yu-Ping Wu (Code Review)