Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31370
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
[WIP] console: Split loglevel for fast and slow
For fast CBMEM console use minimum BIOS_DEBUG level. For other consoles, Kconfig and/or nvram settings apply.
Change-Id: Iff56a0a3182f258200cac80e013957d598cc2130 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/console.c M src/console/init.c M src/console/printk.c M src/include/console/console.h M src/include/console/streams.h 5 files changed, 38 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31370/1
diff --git a/src/console/console.c b/src/console/console.c index 1a6aec1..73a1937 100644 --- a/src/console/console.c +++ b/src/console/console.c @@ -36,9 +36,12 @@ __flashconsole_init(); }
-void console_tx_byte(unsigned char byte) +void console_tx_byte_split(int fast_only, unsigned char byte) { __cbmemc_tx_byte(byte); + if (fast_only) + return; + __spkmodem_tx_byte(byte); __qemu_debugcon_tx_byte(byte);
@@ -57,8 +60,16 @@ __flashconsole_tx_byte(byte); }
-void console_tx_flush(void) +void console_tx_byte(unsigned char byte) { + console_tx_byte_split(0, byte); +} + +void console_tx_flush(int fast_only) +{ + if (fast_only) + return; + __uart_tx_flush(); __ne2k_tx_flush(); __usb_tx_flush(); @@ -67,18 +78,19 @@
void console_write_line(uint8_t *buffer, size_t number_of_bytes) { + int fast_only = 0; + /* Finish displaying all of the console data if requested */ if (number_of_bytes == 0) { - console_tx_flush(); + console_tx_flush(fast_only); return; }
/* Output the console data */ while (number_of_bytes--) - console_tx_byte(*buffer++); + console_tx_byte_split(fast_only, *buffer++); }
- #if IS_ENABLED(CONFIG_GDB_STUB) && (ENV_ROMSTAGE || ENV_RAMSTAGE) void gdb_hw_init(void) { diff --git a/src/console/init.c b/src/console/init.c index 7d1f31a..6640fb8 100644 --- a/src/console/init.c +++ b/src/console/init.c @@ -62,11 +62,18 @@ set_log_level(debug_level); }
-int console_log_level(int msg_level) +int console_slowlog_level(int msg_level) { return (get_log_level() >= msg_level); }
+int console_log_level(int msg_level) +{ + if (car_get_var(console_inited) == 0) + return 0; + return (BIOS_DEBUG >= msg_level); +} + asmlinkage void console_init(void) { init_log_level(); diff --git a/src/console/printk.c b/src/console/printk.c index 735876e..74094fa 100644 --- a/src/console/printk.c +++ b/src/console/printk.c @@ -36,13 +36,14 @@
static void wrap_putchar(unsigned char byte, void *data) { - do_putchar(byte); + int fast_only = *(int *)data; + console_tx_byte_split(fast_only, byte); }
int do_printk(int msg_level, const char *fmt, ...) { va_list args; - int i; + int i, fast_only;
if (IS_ENABLED(CONFIG_SQUELCH_EARLY_SMP) && ENV_CACHE_AS_RAM && !boot_cpu()) @@ -50,6 +51,7 @@
if (!console_log_level(msg_level)) return 0; + fast_only = !console_slowlog_level(msg_level);
DISABLE_TRACE; #ifdef __PRE_RAM__ @@ -61,10 +63,10 @@ #endif
va_start(args, fmt); - i = vtxprintf(wrap_putchar, fmt, args, NULL); + i = vtxprintf(wrap_putchar, fmt, args, &fast_only); va_end(args);
- console_tx_flush(); + console_tx_flush(fast_only);
#ifdef __PRE_RAM__ #if IS_ENABLED(CONFIG_HAVE_ROMSTAGE_CONSOLE_SPINLOCK) @@ -83,7 +85,8 @@ { if (!console_log_level(msg_level)) return; - vtxprintf(wrap_putchar, fmt, args, NULL); - console_tx_flush(); + int fast_only = !console_slowlog_level(msg_level); + vtxprintf(wrap_putchar, fmt, args, &fast_only); + console_tx_flush(fast_only); } #endif /* CONFIG_VBOOT */ diff --git a/src/include/console/console.h b/src/include/console/console.h index 7ab61ea..91a0c61 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -56,6 +56,7 @@ #if __CONSOLE_ENABLE__ asmlinkage void console_init(void); int console_log_level(int msg_level); +int console_slowlog_level(int msg_level); int do_printk(int msg_level, const char *fmt, ...) __attribute__((format(printf, 2, 3))); void do_putchar(unsigned char byte); @@ -78,6 +79,7 @@ #else static inline void console_init(void) {} static inline int console_log_level(int msg_level) { return 0; } +static inline int console_slowlog_level(int msg_level) { return 0; } static inline void printk(int LEVEL, const char *fmt, ...) {} static inline void do_putchar(unsigned char byte) {} #endif diff --git a/src/include/console/streams.h b/src/include/console/streams.h index 6e944a6..051fa08 100644 --- a/src/include/console/streams.h +++ b/src/include/console/streams.h @@ -19,7 +19,8 @@
void console_hw_init(void); void console_tx_byte(unsigned char byte); -void console_tx_flush(void); +void console_tx_byte_split(int fast_only, unsigned char byte); +void console_tx_flush(int fast_only);
/* * Write number_of_bytes data bytes from buffer to the serial device.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31370/1/src/console/init.c File src/console/init.c:
https://review.coreboot.org/#/c/31370/1/src/console/init.c@74 PS1, Line 74: return (BIOS_DEBUG >= msg_level); Comparisons should place the constant on the right side of the test
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 1:
Ron, Julius: Let me know if this is something generally desireable. I can try some inlining to make it fit tegra210 bootblock (google/smaug failing build).
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 1:
(2 comments)
Ron, Julius: Let me know if this is something generally desireable. I can try some inlining to make it fit tegra210 bootblock (google/smaug failing build).
For Chrome OS I don't think we'd be interested in using this, but I don't mind you adding it if you want it. For the Tegra210 you can probably just shift the memory map around a bit (e.g. steal another KB from TIMESTAMP) to make it fit.
https://review.coreboot.org/#/c/31370/1/src/console/init.c File src/console/init.c:
https://review.coreboot.org/#/c/31370/1/src/console/init.c@73 PS1, Line 73: return 0; I think you may also want to return 0 here if CONFIG_CBMEM_CONSOLE is not enabled, so you don't waste time vtxprintf'ing all those messages if they don't get output anywhere.
https://review.coreboot.org/#/c/31370/1/src/console/printk.c File src/console/printk.c:
https://review.coreboot.org/#/c/31370/1/src/console/printk.c@34 PS1, Line 34: console_tx_byte(byte); Note that this seems to get called directly in some cases (looks like output from some old Intel MRCs?). Probably not worth fixing (I guess we don't want to call console_slowlog_level() for every single char), but maybe add a comment to point out this is an intentional shortcoming?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31370/1/src/console/init.c File src/console/init.c:
https://review.coreboot.org/#/c/31370/1/src/console/init.c@73 PS1, Line 73: return 0;
I think you may also want to return 0 here if CONFIG_CBMEM_CONSOLE is not enabled, so you don't wast […]
Something like that, yes, but simply returning 0 here would wipe out slow consoles entirely.
https://review.coreboot.org/#/c/31370/1/src/console/printk.c File src/console/printk.c:
https://review.coreboot.org/#/c/31370/1/src/console/printk.c@34 PS1, Line 34: console_tx_byte(byte);
Note that this seems to get called directly in some cases (looks like output from some old Intel MRC […]
Yes, old intel MRC. Sadly logging is an on/off situation here, like with FSP using console_line(); neither exposes loglevels. So for the imaginary thing of having FSP builds with debugging available, we cannot make this fast/slow console logging split unless we improve the API further.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/#/c/31370/1/src/console/init.c File src/console/init.c:
https://review.coreboot.org/#/c/31370/1/src/console/init.c@73 PS1, Line 73: return 0;
Something like that, yes, but simply returning 0 here would wipe out slow consoles entirely.
Right, that wouldn't work. Maybe it's easier to just hardcode this to the CBMEM console rather than trying to have this generic fast/slow split? (Do we ever really expect to have another fast console anyway?) Then you could just do something like this in do_printk()
tx_byte_t tx_byte = wrap_putchar; if (!console_log_level(msg_level)) { if (!IS_ENABLED(CONFIG_CBMEM_CONSOLE)) return 0; tx_byte = wrap_cbmemc_tx_byte; }
and the overall footprint of this patch would be way smaller.
https://review.coreboot.org/#/c/31370/1/src/console/printk.c File src/console/printk.c:
https://review.coreboot.org/#/c/31370/1/src/console/printk.c@34 PS1, Line 34: console_tx_byte(byte);
Yes, old intel MRC. […]
Oh... right, I misread console_write_line(). Okay, I guess those things were never log-level controlled anyway so that doesn't need to change with this patch. Fair enough.
Hello ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31370
to look at the new patch set (#2).
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
[WIP] console: Split loglevel for fast and slow
For fast CBMEM console use minimum BIOS_DEBUG level. For other consoles, Kconfig and/or nvram settings apply.
Change-Id: Iff56a0a3182f258200cac80e013957d598cc2130 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/console.c M src/console/init.c M src/console/printk.c M src/include/console/console.h M src/include/console/streams.h 5 files changed, 47 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31370/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31370/1/src/console/printk.c File src/console/printk.c:
https://review.coreboot.org/#/c/31370/1/src/console/printk.c@34 PS1, Line 34: console_tx_byte(byte);
Oh... right, I misread console_write_line(). […]
We should, however, apply AXI_X86 attribute to do_putchar() and collect these together with copies of send_to_console() in one common place.
Hello ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31370
to look at the new patch set (#3).
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
[WIP] console: Split loglevel for fast and slow
For fast CBMEM console use minimum BIOS_DEBUG level. For other consoles, Kconfig and/or nvram settings apply.
Change-Id: Iff56a0a3182f258200cac80e013957d598cc2130 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/console.c M src/console/init.c M src/console/printk.c M src/include/console/console.h M src/include/console/streams.h 5 files changed, 47 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31370/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/31370/2/src/console/init.c File src/console/init.c:
https://review.coreboot.org/#/c/31370/2/src/console/init.c@73 PS2, Line 73: int log_level = get_log_level(); Hmm... it still bugs me a bit that this will call get_log_level() twice for every print now, and since it's split across two files I don't think the compiler can optimize much out there either.
How about you instead change the result of console_log_level() to an enum with CONSOLE_LOG_NONE = 0, CONSOLE_LOG_FAST = 1 and CONSOLE_LOG_ALL = 2?
https://review.coreboot.org/#/c/31370/2/src/console/printk.c File src/console/printk.c:
https://review.coreboot.org/#/c/31370/2/src/console/printk.c@44 PS2, Line 44: console_tx_byte_cbmemc(byte); nit: why not just cbmemc_tx_byte() here right away?
https://review.coreboot.org/#/c/31370/2/src/console/printk.c@104 PS2, Line 104: do_vtxprintf(msg_level, fmt, args); Really, I'm pretty sure this should also go through all the extra checks and locks in do_printk(), it just wasn't done right when this was added (or maybe those things were added to do_printk() later and people didn't notice).
I think the cleaner solution to all of this would be to put everything that's currently in do_printk() into do_printk_va_list (maybe rename that to vprintk() because that's the standard naming for this kind of thing), and then change printk/do_printk to a static inline which unpacks the varargs and calls into vprintk().
I know this is only tangentially related to your stuff, but doing that would make the code you're adding simpler and it would be better if we clean this up at some point before we keep throwing more and more logic into a wrong design.
Hello ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31370
to look at the new patch set (#4).
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
[WIP] console: Split loglevel for fast and slow
For fast CBMEM console use minimum BIOS_DEBUG level. For other consoles, Kconfig and/or nvram settings apply.
Change-Id: Iff56a0a3182f258200cac80e013957d598cc2130 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/init.c M src/console/printk.c M src/include/console/console.h 3 files changed, 31 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31370/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: [WIP] console: Split loglevel for fast and slow ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31370/4/src/include/console/console.h File src/include/console/console.h:
https://review.coreboot.org/#/c/31370/4/src/include/console/console.h@65 PS4, Line 65: CONSOLE_LOG_NONE nit: I'd at least anchor NONE as = 0 so it's clear that you can still evaluate console_log_level() as a boolean if you want to. (Also, do you know whether the use in soc/intel/quark can deal with truth values other than 1, or should we maybe change it to !!console_log_level() to be sure?)
Hello ron minnich, Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31370
to look at the new patch set (#6).
Change subject: console: Split loglevel for fast and slow ......................................................................
console: Split loglevel for fast and slow
For fast CBMEM console use minimum BIOS_DEBUG level. For other consoles, Kconfig and/or nvram settings apply.
Change-Id: Iff56a0a3182f258200cac80e013957d598cc2130 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/console/init.c M src/console/printk.c M src/include/console/console.h M src/soc/intel/quark/romstage/fsp2_0.c 4 files changed, 32 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31370/6
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: console: Split loglevel for fast and slow ......................................................................
Patch Set 6: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: console: Split loglevel for fast and slow ......................................................................
Patch Set 7:
I don't fully understand what this patches is doing. Do you prevent log messages > BIOS_DEBUG from being written to CBMEM?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: console: Split loglevel for fast and slow ......................................................................
Patch Set 7:
Patch Set 7:
I don't fully understand what this patches is doing. Do you prevent log messages > BIOS_DEBUG from being written to CBMEM?
No, with this patch all messages with level <= BIOS_DEBUG are written to CBMEM console, regardless of nvram/Kconfig setting. So if one has "slow" consoles enabled the amount of lines logged does not take such a bad impact on boot speed and you still get useful debugging from cbmem utility.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31370 )
Change subject: console: Split loglevel for fast and slow ......................................................................
console: Split loglevel for fast and slow
For fast CBMEM console use minimum BIOS_DEBUG level. For other consoles, Kconfig and/or nvram settings apply.
Change-Id: Iff56a0a3182f258200cac80e013957d598cc2130 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/31370 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M src/console/init.c M src/console/printk.c M src/include/console/console.h M src/soc/intel/quark/romstage/fsp2_0.c 4 files changed, 32 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/src/console/init.c b/src/console/init.c index 7d1f31a..b7cc43a 100644 --- a/src/console/init.c +++ b/src/console/init.c @@ -15,6 +15,7 @@ */
#include <arch/early_variables.h> +#include <commonlib/helpers.h> #include <console/console.h> #include <console/uart.h> #include <console/streams.h> @@ -64,7 +65,18 @@
int console_log_level(int msg_level) { - return (get_log_level() >= msg_level); + int log_level = get_log_level(); + + if (log_level < 0) + return CONSOLE_LOG_NONE; + + if (msg_level <= log_level) + return CONSOLE_LOG_ALL; + + if (IS_ENABLED(CONFIG_CONSOLE_CBMEM) && (msg_level <= BIOS_DEBUG)) + return CONSOLE_LOG_FAST; + + return 0; }
asmlinkage void console_init(void) diff --git a/src/console/printk.c b/src/console/printk.c index 0a9d3bf..6050620 100644 --- a/src/console/printk.c +++ b/src/console/printk.c @@ -17,6 +17,7 @@ * blatantly copied from linux/kernel/printk.c */
+#include <console/cbmem_console.h> #include <console/console.h> #include <console/streams.h> #include <console/vtxprintf.h> @@ -36,18 +37,24 @@
static void wrap_putchar(unsigned char byte, void *data) { - do_putchar(byte); + console_tx_byte(byte); +} + +static void wrap_putchar_cbmemc(unsigned char byte, void *data) +{ + __cbmemc_tx_byte(byte); }
int vprintk(int msg_level, const char *fmt, va_list args) { - int i; + int i, log_this;
if (IS_ENABLED(CONFIG_SQUELCH_EARLY_SMP) && ENV_CACHE_AS_RAM && !boot_cpu()) return 0;
- if (!console_log_level(msg_level)) + log_this = console_log_level(msg_level); + if (log_this < CONSOLE_LOG_FAST) return 0;
DISABLE_TRACE; @@ -59,9 +66,12 @@ spin_lock(&console_lock); #endif
- i = vtxprintf(wrap_putchar, fmt, args, NULL); - - console_tx_flush(); + if (log_this == CONSOLE_LOG_FAST) { + i = vtxprintf(wrap_putchar_cbmemc, fmt, args, NULL); + } else { + i = vtxprintf(wrap_putchar, fmt, args, NULL); + console_tx_flush(); + }
#ifdef __PRE_RAM__ #if IS_ENABLED(CONFIG_HAVE_ROMSTAGE_CONSOLE_SPINLOCK) diff --git a/src/include/console/console.h b/src/include/console/console.h index 33fe2df..2b02334 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -62,6 +62,8 @@ #define printk(LEVEL, fmt, args...) \ do { do_printk(LEVEL, fmt, ##args); } while (0)
+enum { CONSOLE_LOG_NONE = 0, CONSOLE_LOG_FAST, CONSOLE_LOG_ALL }; + #if IS_ENABLED(CONFIG_CONSOLE_OVERRIDE_LOGLEVEL) /* * This function should be implemented at mainboard level. diff --git a/src/soc/intel/quark/romstage/fsp2_0.c b/src/soc/intel/quark/romstage/fsp2_0.c index 23051bd..c237da5 100644 --- a/src/soc/intel/quark/romstage/fsp2_0.c +++ b/src/soc/intel/quark/romstage/fsp2_0.c @@ -173,7 +173,7 @@ upd->RankMask = config->RankMask; upd->RmuBaseAddress = (uintptr_t)rmu_data; upd->RmuLength = rmu_data_len; - upd->SerialPortWriteChar = console_log_level(BIOS_SPEW) + upd->SerialPortWriteChar = !!console_log_level(BIOS_SPEW) ? (uintptr_t)fsp_write_line : 0; upd->SmmTsegSize = IS_ENABLED(CONFIG_HAVE_SMI_HANDLER) ? config->SmmTsegSize : 0;
-
build bot (Jenkins) (Code Review) -
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)