[PATCH] Add an option for debug output on the vga console.

[Karl Semich]

Only available if threads are disabled.  Only displays after the console
has been loaded and from within 32bit flat regions of code.

Signed-off-by: Karl Semich 
---
 src/Kconfig      |  9 +++++++++
 src/bootsplash.c |  9 +++++++++
 src/output.c     | 32 +++++++++++++++++++++++++-------
 src/util.h       |  1 +
 4 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/src/Kconfig b/src/Kconfig
index 3a8ffa1..5d2c7ab 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -571,6 +571,15 @@ menu "Debugging"
             provide the 32 bit address. E.g. 0xFEDC6000 for the AMD Kern
             (a.k.a Hudson UART).

+    config DEBUG_VGA_CONSOLE
+        depends on DEBUG_LEVEL != 0 && !THREADS
+        bool "VGA console debugging"
+        default y
+        help
+            Send debugging information to vga console. This is only displayed
+            after the VGA console has loaded, and only in 32 bit flat regions
+            of code. Requires that threads are disabled.
+
     config DEBUG_IO
         depends on QEMU_HARDWARE && DEBUG_LEVEL != 0
         bool "Special IO port debugging"
diff --git a/src/bootsplash.c b/src/bootsplash.c
index 538b316..2a93c8a 100644
--- a/src/bootsplash.c
+++ b/src/bootsplash.c
@@ -36,6 +36,8 @@ call16_int10(struct bregs *br)
  * VGA text / graphics console
  ****************************************************************/

+static int VgaConsoleActive = 0;
+
 void
 enable_vga_console(void)
 {
@@ -47,11 +49,18 @@ enable_vga_console(void)
     br.ax = 0x0003;
     call16_int10(&br);

+    VgaConsoleActive = 1;
+
     // Write to screen.
     printf("SeaBIOS (version %s)\n", VERSION);
     display_uuid();
 }

+int vga_console_active(void)
+{
+    return VgaConsoleActive;
+}
+
 static int
 find_videomode(struct vbe_info *vesa_info, struct vbe_mode_info *mode_info
                , int width, int height, int bpp_req)
diff --git a/src/output.c b/src/output.c
index 0184444..27935bc 100644
--- a/src/output.c
+++ b/src/output.c
@@ -17,12 +17,15 @@
 #include "output.h" // dprintf
 #include "stacks.h" // call16_int
 #include "string.h" // memset
-#include "util.h" // ScreenAndDebug
+#include "util.h" // ScreenAndDebug vga_console_active

struct putcinfo {
     void (*func)(struct putcinfo *info, char c);
 };

+static void __debug_putc(char c);
+static void __screen_putc(char c);
+

/****************************************************************
  * Debug output
@@ -35,9 +38,8 @@ debug_banner(void)
     dprintf(1, "BUILD: %s\n", BUILDINFO);
 }

-// Write a character to debug port(s).
 static void
-debug_putc(struct putcinfo *action, char c)
+__debug_putc(char c)
 {
     if (! CONFIG_DEBUG_LEVEL)
         return;
@@ -47,6 +49,16 @@ debug_putc(struct putcinfo *action, char c)
     serial_debug_putc(c);
 }

+// Write a character to debug port(s).
+static void
+debug_putc(struct putcinfo *action, char c)
+{
+    __debug_putc(c);
+    if (CONFIG_DEBUG_LEVEL && CONFIG_DEBUG_VGA_CONSOLE && !MODESEGMENT
+        && vga_console_active())
+        __screen_putc(c);
+}
+
 // Flush any pending output to debug port(s).
 static void
 debug_flush(void)
@@ -86,17 +98,23 @@ screenc(char c)
     call16_int(0x10, &br);
 }

-// Handle a character from a printf request.
 static void
-screen_putc(struct putcinfo *action, char c)
+__screen_putc(char c)
 {
-    if (ScreenAndDebug)
-        debug_putc(&debuginfo, c);
     if (c == '\n')
         screenc('\r');
     screenc(c);
 }

+// Handle a character from a printf request.
+static void
+screen_putc(struct putcinfo *action, char c)
+{
+    if (ScreenAndDebug)
+        __debug_putc(c);
+    __screen_putc(c);
+}
+
 static struct putcinfo screeninfo = { screen_putc };

diff --git a/src/util.h b/src/util.h
index aff8e88..8219e0c 100644
--- a/src/util.h
+++ b/src/util.h
@@ -50,6 +50,7 @@ int boot_lchs_find_ata_device(struct pci_device *pci, int chanid, int slave,

// bootsplash.c
 void enable_vga_console(void);
+int vga_console_active(void);
 void enable_bootsplash(void);
 void disable_bootsplash(void);

[Paul Menzel]

Dear Karl,

Thank you for your patch.

Am 12.03.22 um 02:04 schrieb Karl Semich:

One nit: Could you please remove the dot/period from the end of the summary.
> Only available if threads are disabled.

Could you please elaborate, why it does not work with enabled threads.
> Only displays after the console has been loaded and from within 32bit
> flat regions of code.

How did you test this?

Kind regards,

Paul
> Signed-off-by: Karl Semich
>
> src/Kconfig      |  9 +++++++++
>   src/bootsplash.c |  9 +++++++++
>   src/output.c     | 32 +++++++++++++++++++++++++-------
>   src/util.h       |  1 +
>   4 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/src/Kconfig b/src/Kconfig
> index 3a8ffa1..5d2c7ab 100644
> --- a/src/Kconfig
> +++ b/src/Kconfig
> @@ -571,6 +571,15 @@ menu "Debugging"
>               provide the 32 bit address. E.g. 0xFEDC6000 for the AMD Kern
>               (a.k.a Hudson UART).
> config DEBUG_VGA_CONSOLE
>    depends on DEBUG_LEVEL != 0 && !THREADS
>    bool "VGA console debugging"
>    default y
>    help
>        Send debugging information to vga console. This is only displayed
>        after the VGA console has loaded, and only in 32 bit flat regions
>        of code. Requires that threads are disabled.
> config DEBUG_IO
>     depends on QEMU_HARDWARE && DEBUG_LEVEL != 0
>     bool "Special IO port debugging"
>
> diff --git a/src/bootsplash.c b/src/bootsplash.c
> index 538b316..2a93c8a 100644
> --- a/src/bootsplash.c
> +++ b/src/bootsplash.c
> @@ -36,6 +36,8 @@ call16_int10(struct bregs *br)
> VGA text / graphics console
>
> ****************************************************************/
>
> +static int VgaConsoleActive = 0;
> void
> enable_vga_console(void)
> {
>
> @@ -47,11 +49,18 @@ enable_vga_console(void)
>       br.ax = 0x0003;
>       call16_int10(&br);
> VgaConsoleActive = 1;
> // Write to screen.
> printf("SeaBIOS (version %s)\n", VERSION);
> display_uuid();
> }
>
> +int vga_console_active(void)
> +{
> return VgaConsoleActive;
>
> +}
> static int
> find_videomode(struct vbe_info *vesa_info, struct vbe_mode_info *mode_info
>                , int width, int height, int bpp_req)
>
> diff --git a/src/output.c b/src/output.c
> index 0184444..27935bc 100644
> --- a/src/output.c
> +++ b/src/output.c
> @@ -17,12 +17,15 @@
>   #include "output.h" // dprintf
>   #include "stacks.h" // call16_int
>   #include "string.h" // memset
> -#include "util.h" // ScreenAndDebug
> +#include "util.h" // ScreenAndDebug vga_console_active
>
> struct putcinfo {
>       void (*func)(struct putcinfo *info, char c);
>   };
>
> +static void __debug_putc(char c);
> +static void __screen_putc(char c);
>
> /****************************************************************
> Debug output
>
> @@ -35,9 +38,8 @@ debug_banner(void)
>       dprintf(1, "BUILD: %s\n", BUILDINFO);
>   }
>
> -// Write a character to debug port(s).
>   static void
> -debug_putc(struct putcinfo *action, char c)
> +__debug_putc(char c)
>   {
>       if (! CONFIG_DEBUG_LEVEL)
>           return;
> @@ -47,6 +49,16 @@ debug_putc(struct putcinfo *action, char c)
>       serial_debug_putc(c);
>   }
>
> +// Write a character to debug port(s).
> +static void
> +debug_putc(struct putcinfo *action, char c)
> +{
> __debug_putc(c);
> if (CONFIG_DEBUG_LEVEL && CONFIG_DEBUG_VGA_CONSOLE && !MODESEGMENT
>    && vga_console_active())
>    __screen_putc(c);
>
> +}
> // Flush any pending output to debug port(s).
> static void
> debug_flush(void)
>
> @@ -86,17 +98,23 @@ screenc(char c)
>       call16_int(0x10, &br);
>   }
>
> -// Handle a character from a printf request.
>   static void
> -screen_putc(struct putcinfo *action, char c)
> +__screen_putc(char c)
>   {
> if (ScreenAndDebug)
>    debug_putc(&debuginfo, c);
> if (c == '\n')
>     screenc('\r');
> screenc(c);
> }
>
> +// Handle a character from a printf request.
> +static void
> +screen_putc(struct putcinfo *action, char c)
> +{
> if (ScreenAndDebug)
>    __debug_putc(c);
> __screen_putc(c);
>
> +}
> static struct putcinfo screeninfo = { screen_putc };
>
> diff --git a/src/util.h b/src/util.h
> index aff8e88..8219e0c 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -50,6 +50,7 @@ int boot_lchs_find_ata_device(struct pci_device *pci, int chanid, int slave,
>
> // bootsplash.c
>   void enable_vga_console(void);
> +int vga_console_active(void);
>   void enable_bootsplash(void);
>   void disable_bootsplash(void);

[Karl Semich]

On Sat, Mar 12, 2022, 2:27 AM Paul Menzel  wrote:
> Dear Karl,
>
> Thank you for your patch.

Thanks for your review.
> Am 12.03.22 um 02:04 schrieb Karl Semich:
>
> One nit: Could you please remove the dot/period from the end of the
> summary.

Will do soon, once I further learn git-send-email.
> Only available if threads are disabled.
>
> Could you please elaborate, why it does not work with enabled threads.

Honestly, I just get panic("call16 with invalid stack\n") immediately after
the first thread is created, if these are both enabled.

I'm still learning seabios, and it doesn't yet seem to make sense for me to
diagnose this in depth. I still have basic things to do like find non-vga
ways to see debug output.

I note that threads have mutated stack pointers, and the panic happens in
response to a check against the stack pointer. I disabled the panic and
experienced a crash instead.

Do you have any thoughts around it?
> Only displays after the console has been loaded and from within 32bit
> > flat regions of code.
>
> How did you test this?

I'm running seabios as part of setting up a kfsn4 board with coreboot, and
am trying changes live on the board.

No text displays before the vga console is loaded, but there can be crashes
if screenc is called too early in boot, so I added the vga_console_active()
check in the patch to prevent early calls.

Regarding 32bit flat, the code is guarded by !MODESEGMENT, but I can add
ASSERT32FLAT() to __screen_putc() to test it.

I'll also see how reasonable it is to do a test run in qemu on my raspberry
pi or kfsn4.

Kind regards,
> Paul

Thanks Paul.
>