[PATCH] x86: Update cbmem driver
This driver is not actually built since a Kconfig was never created for it.
Add a Kconfig (which is already implied by COREBOOT) and update the implementation to avoid using unnecessary memory. Drop the #ifdef at the top since we can rely on Kconfig to get that right.
To enable it (in addition to serial and video), use:
setenv stdout serial,vidconsole,cbmem
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/misc/Kconfig | 8 ++++++++ drivers/misc/cbmem_console.c | 26 +++++++++----------------- 2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index b9f5c7a37aed..3df57a5bfbd9 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -122,6 +122,14 @@ config VEXPRESS_CONFIG configuration bus on the Arm Versatile Express boards via a sysreg driver.
+config CBMEM_CONSOLE + bool "Write console output to coreboot cbmem" + depends on X86 + help + Enables console output to the cbmem console, which is a memory + region set up by coreboot to hold a record of all console output. + Enable this only if booting from coreboot. + config CMD_CROS_EC bool "Enable crosec command" depends on CROS_EC diff --git a/drivers/misc/cbmem_console.c b/drivers/misc/cbmem_console.c index 8bbe33d414da..6ae1389a23e0 100644 --- a/drivers/misc/cbmem_console.c +++ b/drivers/misc/cbmem_console.c @@ -5,27 +5,20 @@
#include <common.h> #include <console.h> -#ifndef CONFIG_SYS_COREBOOT -#error This driver requires coreboot -#endif - #include <asm/cb_sysinfo.h>
-struct cbmem_console { - u32 buffer_size; - u32 buffer_cursor; - u8 buffer_body[0]; -} __attribute__ ((__packed__)); - -static struct cbmem_console *cbmem_console_p; - void cbmemc_putc(struct stdio_dev *dev, char data) { - int cursor; + const struct sysinfo_t *sysinfo = cb_get_sysinfo(); + struct cbmem_console *cons = sysinfo ? sysinfo->cbmem_cons : NULL; + int pos; + + if (!cons) + return;
- cursor = cbmem_console_p->buffer_cursor++; - if (cursor < cbmem_console_p->buffer_size) - cbmem_console_p->buffer_body[cursor] = data; + pos = cons->cursor++; + if (pos < cons->size) + cons->body[pos] = data; }
void cbmemc_puts(struct stdio_dev *dev, const char *str) @@ -40,7 +33,6 @@ int cbmemc_init(void) { int rc; struct stdio_dev cons_dev; - cbmem_console_p = lib_sysinfo.cbmem_cons;
memset(&cons_dev, 0, sizeof(cons_dev));
+Paul Menzel pmenzel@molgen.mpg.de
On Sun, 13 Aug 2023 at 14:10, Simon Glass sjg@chromium.org wrote:
This driver is not actually built since a Kconfig was never created for it.
Add a Kconfig (which is already implied by COREBOOT) and update the implementation to avoid using unnecessary memory. Drop the #ifdef at the top since we can rely on Kconfig to get that right.
To enable it (in addition to serial and video), use:
setenv stdout serial,vidconsole,cbmem
Signed-off-by: Simon Glass sjg@chromium.org
drivers/misc/Kconfig | 8 ++++++++ drivers/misc/cbmem_console.c | 26 +++++++++----------------- 2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index b9f5c7a37aed..3df57a5bfbd9 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -122,6 +122,14 @@ config VEXPRESS_CONFIG configuration bus on the Arm Versatile Express boards via a sysreg driver.
+config CBMEM_CONSOLE
bool "Write console output to coreboot cbmem"depends on X86helpEnables console output to the cbmem console, which is a memoryregion set up by coreboot to hold a record of all console output.Enable this only if booting from coreboot.config CMD_CROS_EC bool "Enable crosec command" depends on CROS_EC diff --git a/drivers/misc/cbmem_console.c b/drivers/misc/cbmem_console.c index 8bbe33d414da..6ae1389a23e0 100644 --- a/drivers/misc/cbmem_console.c +++ b/drivers/misc/cbmem_console.c @@ -5,27 +5,20 @@
#include <common.h> #include <console.h> -#ifndef CONFIG_SYS_COREBOOT -#error This driver requires coreboot -#endif
#include <asm/cb_sysinfo.h>
-struct cbmem_console {
u32 buffer_size;u32 buffer_cursor;u8 buffer_body[0];-} __attribute__ ((__packed__));
-static struct cbmem_console *cbmem_console_p;
void cbmemc_putc(struct stdio_dev *dev, char data) {
int cursor;
const struct sysinfo_t *sysinfo = cb_get_sysinfo();struct cbmem_console *cons = sysinfo ? sysinfo->cbmem_cons : NULL;int pos;if (!cons)return;
cursor = cbmem_console_p->buffer_cursor++;if (cursor < cbmem_console_p->buffer_size)cbmem_console_p->buffer_body[cursor] = data;
pos = cons->cursor++;if (pos < cons->size)cons->body[pos] = data;}
void cbmemc_puts(struct stdio_dev *dev, const char *str) @@ -40,7 +33,6 @@ int cbmemc_init(void) { int rc; struct stdio_dev cons_dev;
cbmem_console_p = lib_sysinfo.cbmem_cons; memset(&cons_dev, 0, sizeof(cons_dev));-- 2.41.0.640.ga95def55d0-goog
Dear Simon and other developers,
- cursor = cbmem_console_p->buffer_cursor++;
- if (cursor < cbmem_console_p->buffer_size)
cbmem_console_p->buffer_body[cursor] = data;
- pos = cons->cursor++;
- if (pos < cons->size)
cons->body[pos] = data;
While at it, is it OK to increment cons->cursor unconditionally, even when the buffer is full?
It's better to do it after the check, isn't it? E.g.:
if (cons->cursor < cons->size) cons->body[cons->cursor++] = data;
Cheers, Alex.
Hi Alex,
On Sun, 13 Aug 2023 at 21:03, Alex Sadovsky nable.maininbox@googlemail.com wrote:
Dear Simon and other developers,
cursor = cbmem_console_p->buffer_cursor++;if (cursor < cbmem_console_p->buffer_size)cbmem_console_p->buffer_body[cursor] = data;
pos = cons->cursor++;if (pos < cons->size)cons->body[pos] = data;While at it, is it OK to increment cons->cursor unconditionally, even when the buffer is full?
It's better to do it after the check, isn't it? E.g.:
if (cons->cursor < cons->size) cons->body[cons->cursor++] = data;
I believe the original intent was to indicate that the buffer had overflowed. But prompted by your review I took a look at the coreboot implementation and it now has an overflow flag.
So I will send a v2 incorporating this.
Thanks, Simon
On 15.08.2023 01:42, Simon Glass:
I believe the original intent was to indicate that the buffer had overflowed. But prompted by your review I took a look at the coreboot implementation and it now has an overflow flag.
So I will send a v2 incorporating this.
Thank you for this investigation, v2 approach definitely makes sense. Btw, is there any reason to mark 'struct cbmem_console' as packed?
Hi Nable,
On Tue, 15 Aug 2023 at 19:41, Nable nable.maininbox@googlemail.com wrote:
On 15.08.2023 01:42, Simon Glass:
I believe the original intent was to indicate that the buffer had overflowed. But prompted by your review I took a look at the coreboot implementation and it now has an overflow flag.
So I will send a v2 incorporating this.
Thank you for this investigation, v2 approach definitely makes sense. Btw, is there any reason to mark 'struct cbmem_console' as packed?
Not that I can see. I'll drop that too for v3.
Regards, Simon
participants (3)
-
Alex Sadovsky -
Nable
-
Simon Glass