Change in ...coreboot[master]: [WIP] console: Refactor printk() varargs prototypes

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31491

Change subject: [WIP] console: Refactor printk() varargs prototypes
......................................................................

[WIP] console: Refactor printk() varargs prototypes

Change-Id: I816641c2223c3079ad9c95c1380d4b250898ef93
Signed-off-by: Kyösti Mälkki 
---
M src/console/printk.c
M src/include/console/console.h
2 files changed, 16 insertions(+), 25 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/31491/1

diff --git a/src/console/printk.c b/src/console/printk.c
index 735876e..c53f030 100644
--- a/src/console/printk.c
+++ b/src/console/printk.c
@@ -39,9 +39,8 @@
    do_putchar(byte);
 }

-int do_printk(int msg_level, const char *fmt, ...)
+int do_printk_va_list(int msg_level, const char *fmt, va_list args)
 {
-	va_list args;
    int i;

if (IS_ENABLED(CONFIG_SQUELCH_EARLY_SMP) && ENV_CACHE_AS_RAM &&
@@ -60,9 +59,7 @@
    spin_lock(&console_lock);
 #endif

-	va_start(args, fmt);
    i = vtxprintf(wrap_putchar, fmt, args, NULL);
-	va_end(args);

console_tx_flush();

@@ -77,13 +74,3 @@

return i;
 }
-
-#if IS_ENABLED(CONFIG_VBOOT)
-void do_printk_va_list(int msg_level, const char *fmt, va_list args)
-{
-	if (!console_log_level(msg_level))
-		return;
-	vtxprintf(wrap_putchar, fmt, args, NULL);
-	console_tx_flush();
-}
-#endif /* CONFIG_VBOOT */
diff --git a/src/include/console/console.h b/src/include/console/console.h
index 7ab61ea..bdddc98 100644
--- a/src/include/console/console.h
+++ b/src/include/console/console.h
@@ -19,6 +19,7 @@
 #include <stdint.h>
 #include <arch/cpu.h>
 #include <console/post_codes.h>
+#include <console/vtxprintf.h>
 #include <commonlib/loglevel.h>

#define RAM_DEBUG (IS_ENABLED(CONFIG_DEBUG_RAM_SETUP) ? BIOS_DEBUG : BIOS_NEVER)
@@ -56,8 +57,6 @@
 #if __CONSOLE_ENABLE__
 asmlinkage void console_init(void);
 int console_log_level(int msg_level);
-int do_printk(int msg_level, const char *fmt, ...)
-	__attribute__((format(printf, 2, 3)));
 void do_putchar(unsigned char byte);

#define printk(LEVEL, fmt, args...) \
@@ -82,15 +81,20 @@
 static inline void do_putchar(unsigned char byte) {}
 #endif

-#if IS_ENABLED(CONFIG_VBOOT)
-/* FIXME: Collision of varargs with AMD headers without guard. */
-#include <console/vtxprintf.h>
-#if __CONSOLE_ENABLE__
-void do_printk_va_list(int msg_level, const char *fmt, va_list args);
-#else
-static inline void do_printk_va_list(int l, const char *fmt, va_list args) {}
-#endif
-#endif
+int do_printk_va_list(int msg_level, const char *fmt, va_list args);
+
+static inline __attribute__((format(printf, 2, 3)))
+int do_printk(int msg_level, const char *fmt, ...)
+{
+	va_list args;
+	int i;
+
+	va_start(args, fmt);
+	i = do_printk_va_list(msg_level, fmt, args);
+	va_end(args);
+
+	return i;
+}

#endif /* !__ROMCC__ */

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: [WIP] console: Refactor printk() varargs prototypes
......................................................................

Patch Set 1: Code-Review+2

LGTM conceptually. Unfortunately, it seems that GCC is (as usual) stupid and cannot inline vararg functions no matter what (I even tried with always_inline and it just flat out refuses), so this leads no slightly suboptimal code generation. No big deal I think, but you'll need to fix up those memlayouts again (for Veyron maybe it's time to just drop the pre-RAM console completely...).

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: [WIP] console: Refactor printk() varargs prototypes
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+2
>
> LGTM conceptually. Unfortunately, it seems that GCC is (as usual) stupid and cannot inline vararg functions no matter what (I even tried with always_inline and it just flat out refuses), so this leads no slightly suboptimal code generation. No big deal I think, but you'll need to fix up those memlayouts again (for Veyron maybe it's time to just drop the pre-RAM console completely...).

I did a fresh install of arm crossgcc from commit da7ffb4 and abuild passed. I did not feel like digging deeper into this and did not have older arm toolchain around to compare.

Being lazy, I think I just wait until buildbots have had their toolchains updated and rebase then.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: [WIP] console: Refactor printk() varargs prototypes
......................................................................

Patch Set 1:

If it's not inlined how is it linked then? a single implementation or
one per calling compilation unit? Could we safe space by placing it
into `printk.c`?

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: [WIP] console: Refactor printk() varargs prototypes
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31491/1/src/include/console/console.h 
File src/include/console/console.h:

https://review.coreboot.org/#/c/31491/1/src/include/console/console.h@92 
PS1, Line 92: 	va_start(args, fmt);
Aren't these implemented as macros? If we don't know how much
code this produces, is inlining a good choice?

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[Kyösti Mälkki (Code Review)]

Hello Julius Werner, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31491

to look at the new patch set (#3).

Change subject: console: Refactor printk() varargs prototypes
......................................................................

console: Refactor printk() varargs prototypes

Change-Id: I816641c2223c3079ad9c95c1380d4b250898ef93
Signed-off-by: Kyösti Mälkki 
---
M src/console/printk.c
M src/include/console/console.h
M src/security/vboot/vboot_logic.c
3 files changed, 22 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/31491/3

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: console: Refactor printk() varargs prototypes
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31491/3/src/console/printk.c 
File src/console/printk.c:

https://review.coreboot.org/#/c/31491/3/src/console/printk.c@42 
PS3, Line 42: static __inline
plain inline is preferred over __inline

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: console: Refactor printk() varargs prototypes
......................................................................

Patch Set 3: Code-Review+2

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: console: Refactor printk() varargs prototypes
......................................................................

Patch Set 3:

(1 comment)
> If it's not inlined how is it linked then? a single implementation or
> one per calling compilation unit? Could we safe space by placing it
> into `printk.c`?

OHH! Yeah, that would explain why the binary blew up so much from such a small function. Good point, pulling it into .c was the right fix.

https://review.coreboot.org/#/c/31491/3/src/console/printk.c 
File src/console/printk.c:

https://review.coreboot.org/#/c/31491/3/src/console/printk.c@42 
PS3, Line 42: static __inline
> plain inline is preferred over __inline

Wait why are we inlining this now? Doesn't that duplicate all this code?

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[Kyösti Mälkki (Code Review)]

Hello Aaron Durbin, Julius Werner, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31491

to look at the new patch set (#4).

Change subject: console: Refactor printk() varargs prototypes
......................................................................

console: Refactor printk() varargs prototypes

Change-Id: I816641c2223c3079ad9c95c1380d4b250898ef93
Signed-off-by: Kyösti Mälkki 
---
M src/console/printk.c
M src/include/console/console.h
M src/security/vboot/vboot_logic.c
3 files changed, 16 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/31491/4

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: console: Refactor printk() varargs prototypes
......................................................................

Patch Set 4: Code-Review+2

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: console: Refactor printk() varargs prototypes
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31491/3/src/console/printk.c 
File src/console/printk.c:

https://review.coreboot.org/#/c/31491/3/src/console/printk.c@42 
PS3, Line 42: static __inline
> Wait why are we inlining this now? Doesn't that duplicate all this code?

inline is a hint. the compiler will do whatever it wants to do anyway.

Change in ...coreboot[master]: console: Refactor printk() varargs prototypes

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31491 )

Change subject: console: Refactor printk() varargs prototypes
......................................................................

console: Refactor printk() varargs prototypes

Change-Id: I816641c2223c3079ad9c95c1380d4b250898ef93
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/31491
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/console/printk.c
M src/include/console/console.h
M src/security/vboot/vboot_logic.c
3 files changed, 16 insertions(+), 23 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/console/printk.c b/src/console/printk.c
index 735876e..0a9d3bf 100644
--- a/src/console/printk.c
+++ b/src/console/printk.c
@@ -39,9 +39,8 @@
    do_putchar(byte);
 }

-int do_printk(int msg_level, const char *fmt, ...)
+int vprintk(int msg_level, const char *fmt, va_list args)
 {
-	va_list args;
    int i;

if (IS_ENABLED(CONFIG_SQUELCH_EARLY_SMP) && ENV_CACHE_AS_RAM &&
@@ -60,9 +59,7 @@
    spin_lock(&console_lock);
 #endif

-	va_start(args, fmt);
    i = vtxprintf(wrap_putchar, fmt, args, NULL);
-	va_end(args);

console_tx_flush();

@@ -78,12 +75,14 @@
    return i;
 }

-#if IS_ENABLED(CONFIG_VBOOT)
-void do_printk_va_list(int msg_level, const char *fmt, va_list args)
+int do_printk(int msg_level, const char *fmt, ...)
 {
-	if (!console_log_level(msg_level))
-		return;
-	vtxprintf(wrap_putchar, fmt, args, NULL);
-	console_tx_flush();
+	va_list args;
+	int i;
+
+	va_start(args, fmt);
+	i = vprintk(msg_level, fmt, args);
+	va_end(args);
+
+	return i;
 }
-#endif /* CONFIG_VBOOT */
diff --git a/src/include/console/console.h b/src/include/console/console.h
index 7ab61ea..33fe2df 100644
--- a/src/include/console/console.h
+++ b/src/include/console/console.h
@@ -19,6 +19,7 @@
 #include <stdint.h>
 #include <arch/cpu.h>
 #include <console/post_codes.h>
+#include <console/vtxprintf.h>
 #include <commonlib/loglevel.h>

#define RAM_DEBUG (IS_ENABLED(CONFIG_DEBUG_RAM_SETUP) ? BIOS_DEBUG : BIOS_NEVER)
@@ -56,8 +57,6 @@
 #if __CONSOLE_ENABLE__
 asmlinkage void console_init(void);
 int console_log_level(int msg_level);
-int do_printk(int msg_level, const char *fmt, ...)
-	__attribute__((format(printf, 2, 3)));
 void do_putchar(unsigned char byte);

#define printk(LEVEL, fmt, args...) \
@@ -82,15 +81,10 @@
 static inline void do_putchar(unsigned char byte) {}
 #endif

-#if IS_ENABLED(CONFIG_VBOOT)
-/* FIXME: Collision of varargs with AMD headers without guard. */
-#include <console/vtxprintf.h>
-#if __CONSOLE_ENABLE__
-void do_printk_va_list(int msg_level, const char *fmt, va_list args);
-#else
-static inline void do_printk_va_list(int l, const char *fmt, va_list args) {}
-#endif
-#endif
+int vprintk(int msg_level, const char *fmt, va_list args);
+
+int do_printk(int msg_level, const char *fmt, ...)
+	__attribute__((format(printf, 2, 3)));

#endif /* !__ROMCC__ */

diff --git a/src/security/vboot/vboot_logic.c b/src/security/vboot/vboot_logic.c
index 00bbae6..8c3ba80 100644
--- a/src/security/vboot/vboot_logic.c
+++ b/src/security/vboot/vboot_logic.c
@@ -48,7 +48,7 @@
    	printk(BIOS_INFO, "VB2:%s() ", func);

va_start(args, fmt);
-	do_printk_va_list(BIOS_INFO, fmt, args);
+	vprintk(BIOS_INFO, fmt, args);
    va_end(args);

return;