Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
console: Declare empty printk() for __ROMCC__
Change-Id: Ieb0c33b082b4c4453d29d917f46561c0e672d09a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/intel/microcode/microcode.c M src/include/console/console.h 2 files changed, 8 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35588/1
diff --git a/src/cpu/intel/microcode/microcode.c b/src/cpu/intel/microcode/microcode.c index feb73c8..80470bf 100644 --- a/src/cpu/intel/microcode/microcode.c +++ b/src/cpu/intel/microcode/microcode.c @@ -17,11 +17,11 @@ #include <stddef.h> #if !defined(__ROMCC__) #include <cbfs.h> -#include <console/console.h> #else #include <arch/cbfs.h> #endif #include <arch/cpu.h> +#include <console/console.h> #include <cpu/x86/msr.h> #include <cpu/intel/microcode.h> #include <smp/spinlock.h> @@ -84,9 +84,7 @@
/* No use loading the same revision. */ if (current_rev == m->rev) { -#if !defined(__ROMCC__) printk(BIOS_INFO, "microcode: Update skipped, already up-to-date\n"); -#endif return; }
@@ -104,18 +102,14 @@
current_rev = read_microcode_rev(); if (current_rev == m->rev) { -#if !defined(__ROMCC__) printk(BIOS_INFO, "microcode: updated to revision " "0x%x date=%04x-%02x-%02x\n", read_microcode_rev(), m->date & 0xffff, (m->date >> 24) & 0xff, (m->date >> 16) & 0xff); -#endif return; }
-#if !defined(__ROMCC__) printk(BIOS_INFO, "microcode: Update failed\n"); -#endif }
uint32_t get_current_microcode_rev(void) @@ -180,13 +174,9 @@ msr = rdmsr(IA32_PLATFORM_ID); pf = 1 << ((msr.hi >> 18) & 7); } -#if !defined(__ROMCC__) - /* If this code is compiled with ROMCC we're probably in - * the bootblock and don't have console output yet. - */ + printk(BIOS_DEBUG, "microcode: sig=0x%x pf=0x%x revision=0x%x\n", sig, pf, rev); -#endif
while (microcode_len >= sizeof(*ucode_updates)) { /* Newer microcode updates include a size field, whereas older @@ -194,17 +184,13 @@ if (ucode_updates->total_size) { update_size = ucode_updates->total_size; } else { - #if !defined(__ROMCC__) printk(BIOS_SPEW, "Microcode size field is 0\n"); - #endif update_size = 2048; }
/* Checkpoint 1: The microcode update falls within CBFS */ if (update_size > microcode_len) { -#if !defined(__ROMCC__) printk(BIOS_WARNING, "Microcode header corrupted!\n"); -#endif break; }
diff --git a/src/include/console/console.h b/src/include/console/console.h index e5b753e..28ade48 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -19,7 +19,6 @@ #include <stdint.h> #include <arch/cpu.h> #include <console/post_codes.h> -#include <console/vtxprintf.h>
/* console.h is supposed to provide the log levels defined in here: */ #include <commonlib/loglevel.h> @@ -29,6 +28,8 @@
#ifndef __ROMCC__
+#include <console/vtxprintf.h> + void post_code(u8 value); #if CONFIG(CMOS_POST_EXTRA) void post_log_extra(u32 value); @@ -93,6 +94,10 @@
int do_vprintk(int msg_level, const char *fmt, va_list args);
+#else + +#define printk(...) do { } while (0) + #endif /* !__ROMCC__ */
#endif /* CONSOLE_CONSOLE_H_ */
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 1: Code-Review+1
Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35588
to look at the new patch set (#2).
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
console: Declare empty printk() for __ROMCC__
The typical do { } while (0) did not work, so provide empty stub function instead.
Change-Id: Ieb0c33b082b4c4453d29d917f46561c0e672d09a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/intel/microcode/microcode.c M src/include/console/console.h 2 files changed, 9 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/35588/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG@11 PS2, Line 11: Follow-up work on the topic showed poor romcc-optimisation and lack of XMM register use that triggered a build failure. Which approach to take?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG@11 PS2, Line 11:
Follow-up work on the topic showed poor romcc-optimisation and lack of XMM register use that trigger […]
You mean this patch doesn't work adequately because of the empty inline? Just #define prinkt(...) to nothing?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG@11 PS2, Line 11:
You mean this patch doesn't work adequately because of the empty inline? Just #define prinkt(... […]
See CB:35616 where that 'do {} while (0)' increases romcc register usage. I am not quite sure why e.g. <arch/smp/spinlock.h> does not define empty macros if they would work everywhere?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG@11 PS2, Line 11:
See CB:35616 where that 'do {} while (0)' increases romcc register usage. I am not quite sure why e.g. <arch/smp/spinlock.h> does not define empty macros if they would work everywhere?
The empty inline doesn't cause the same problems as 'do {} while (0)'?
As for empty macros... I think it's largely microcode and console are likely the only places this was a problem w/ romcc. And as your patch shows those were being explicitly handled by open coding out the problematic sequences. But with empty macros it creates dangling ';' which can cause issues depending on how the code is done.
I personally am not a fan of romcc because of its shortcomings w.r.t. code portability. And for so long most things were #if'd out.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG@11 PS2, Line 11:
The empty inline doesn't cause the same problems as 'do {} while (0)'?
Right. You can see patchset #1 here failing (romcc run out of available registers without SSE).
As for empty macros... I think it's largely microcode and console are likely the only places this was a problem w/ romcc. And as your patch shows those were being explicitly handled by open coding out the problematic sequences. But with empty macros it creates dangling ';' which can cause issues depending on how the code is done.
I personally am not a fan of romcc because of its shortcomings w.r.t. code portability. And for so long most things were #if'd out.
I am not sure if romcc-bootblock deprecation with next release is possible since so many platforms are way behind with the transition. Here, I was mostly looking into ways to gradually reduce #ifdef __ROMCC__ cases but we may as well do a one-shot wipeout once tree is ready for it.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG@11 PS2, Line 11:
The empty inline doesn't cause the same problems as 'do {} while (0)'? […]
sgtm
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35588/2//COMMIT_MSG@11 PS2, Line 11:
sgtm
Ack
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35588 )
Change subject: console: Declare empty printk() for __ROMCC__ ......................................................................
console: Declare empty printk() for __ROMCC__
The typical do { } while (0) did not work, so provide empty stub function instead.
Change-Id: Ieb0c33b082b4c4453d29d917f46561c0e672d09a Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35588 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/cpu/intel/microcode/microcode.c M src/include/console/console.h 2 files changed, 9 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/cpu/intel/microcode/microcode.c b/src/cpu/intel/microcode/microcode.c index feb73c8..80470bf 100644 --- a/src/cpu/intel/microcode/microcode.c +++ b/src/cpu/intel/microcode/microcode.c @@ -17,11 +17,11 @@ #include <stddef.h> #if !defined(__ROMCC__) #include <cbfs.h> -#include <console/console.h> #else #include <arch/cbfs.h> #endif #include <arch/cpu.h> +#include <console/console.h> #include <cpu/x86/msr.h> #include <cpu/intel/microcode.h> #include <smp/spinlock.h> @@ -84,9 +84,7 @@
/* No use loading the same revision. */ if (current_rev == m->rev) { -#if !defined(__ROMCC__) printk(BIOS_INFO, "microcode: Update skipped, already up-to-date\n"); -#endif return; }
@@ -104,18 +102,14 @@
current_rev = read_microcode_rev(); if (current_rev == m->rev) { -#if !defined(__ROMCC__) printk(BIOS_INFO, "microcode: updated to revision " "0x%x date=%04x-%02x-%02x\n", read_microcode_rev(), m->date & 0xffff, (m->date >> 24) & 0xff, (m->date >> 16) & 0xff); -#endif return; }
-#if !defined(__ROMCC__) printk(BIOS_INFO, "microcode: Update failed\n"); -#endif }
uint32_t get_current_microcode_rev(void) @@ -180,13 +174,9 @@ msr = rdmsr(IA32_PLATFORM_ID); pf = 1 << ((msr.hi >> 18) & 7); } -#if !defined(__ROMCC__) - /* If this code is compiled with ROMCC we're probably in - * the bootblock and don't have console output yet. - */ + printk(BIOS_DEBUG, "microcode: sig=0x%x pf=0x%x revision=0x%x\n", sig, pf, rev); -#endif
while (microcode_len >= sizeof(*ucode_updates)) { /* Newer microcode updates include a size field, whereas older @@ -194,17 +184,13 @@ if (ucode_updates->total_size) { update_size = ucode_updates->total_size; } else { - #if !defined(__ROMCC__) printk(BIOS_SPEW, "Microcode size field is 0\n"); - #endif update_size = 2048; }
/* Checkpoint 1: The microcode update falls within CBFS */ if (update_size > microcode_len) { -#if !defined(__ROMCC__) printk(BIOS_WARNING, "Microcode header corrupted!\n"); -#endif break; }
diff --git a/src/include/console/console.h b/src/include/console/console.h index e5b753e..1c2a276 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -19,7 +19,6 @@ #include <stdint.h> #include <arch/cpu.h> #include <console/post_codes.h> -#include <console/vtxprintf.h>
/* console.h is supposed to provide the log levels defined in here: */ #include <commonlib/loglevel.h> @@ -29,6 +28,8 @@
#ifndef __ROMCC__
+#include <console/vtxprintf.h> + void post_code(u8 value); #if CONFIG(CMOS_POST_EXTRA) void post_log_extra(u32 value); @@ -93,6 +94,11 @@
int do_vprintk(int msg_level, const char *fmt, va_list args);
+#else + +static inline void romcc_printk(void) { } +#define printk(...) romcc_printk() + #endif /* !__ROMCC__ */
#endif /* CONSOLE_CONSOLE_H_ */
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)