[RFC PATCH 0/2] seabios: add serial console support

[Gerd Hoffmann]

Hi,

Ok folks, finally took the time to put serial console support into
seabios natively, without requiring sgabios.  For now this will use the
first serial port in case no vgabios was found, i.e. use something along
the lines of "qemu -vga none -serial stdio" to check it out.

Design goal is to print as few control sequences as possible, so you
get readable logs when writing serial output to a file, but still allow
enough control that boot loader menus etc. can be used interactively.

The patches reached the first milestone now, you can boot into grub2
and use the builtin editor.  And unlike sgabios it will not go crazy in
case you try to move around the cursor quickly by holding down keys and
let autorepeat kick in.  ipxe command line editor worked too on a quick
test.

known issues / todo list:
 * isolinux/pxelinux seems to have trouble with this.
 * add support for sending output to both vga and serial.
 * add compile time config option.
 * add runtime config option.
 * figure reasonable plan for transition from sgabios.

enjoy,
  Gerd

Gerd Hoffmann (2):
  serial console, output
  serial console, input

src/clock.c      |   1 +
 src/misc.c       |   2 +
 src/optionroms.c |   4 +-
 src/serial.c     | 595 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/util.h       |   3 +
 5 files changed, 604 insertions(+), 1 deletion(-)

[RFC PATCH 1/2] serial console, output

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 src/misc.c       |   2 +
 src/optionroms.c |   4 +-
 src/serial.c     | 340 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/util.h       |   2 +
 4 files changed, 347 insertions(+), 1 deletion(-)

diff --git a/src/misc.c b/src/misc.c
index f02237c..f4b656d 100644
--- a/src/misc.c
+++ b/src/misc.c
@@ -11,6 +11,7 @@
 #include "output.h" // debug_enter
 #include "stacks.h" // call16_int
 #include "string.h" // memset
+#include "util.h" // serial_10

#define PORT_MATH_CLEAR        0x00f0

@@ -57,6 +58,7 @@ handle_10(struct bregs *regs)
 {
     debug_enter(regs, DEBUG_HDL_10);
     // don't do anything, since the VGA BIOS handles int10h requests
+    sercon_10(regs);
 }

// NMI handler
diff --git a/src/optionroms.c b/src/optionroms.c
index 65f7fe0..e6b308c 100644
--- a/src/optionroms.c
+++ b/src/optionroms.c
@@ -432,9 +432,11 @@ vgarom_setup(void)
     run_file_roms("vgaroms/", 1, NULL);
     rom_reserve(0);

-    if (rom_get_last() == BUILD_ROM_START)
+    if (rom_get_last() == BUILD_ROM_START) {
         // No VGA rom found
+        sercon_enable();
         return;
+    }

VgaROM = (void*)BUILD_ROM_START;
     enable_vga_console();
diff --git a/src/serial.c b/src/serial.c
index 88349c8..74b91bb 100644
--- a/src/serial.c
+++ b/src/serial.c
@@ -315,3 +315,343 @@ handle_17(struct bregs *regs)
     default:   handle_17XX(regs); break;
     }
 }
+
+/****************************************************************
+ * serial console output
+ ****************************************************************/
+
+VARLOW u16 sercon_port;
+VARLOW u8 sercon_mode;
+VARLOW u8 sercon_cols;
+VARLOW u8 sercon_rows;
+
+VARLOW u8 sercon_row;
+VARLOW u8 sercon_col;
+
+/*
+ * We have a small output buffer here, for lazy output.  That allows
+ * to avoid a whole bunch of control sequences for pointless cursor
+ * moves, so when logging the output it'll be *alot* less cluttered.
+ *
+ * sercon_char/attr  is the actual output buffer.
+ * sercon_col_lazy   is the column of the terminal's cursor, typically
+ *                   a few positions left of sercon_col.
+ * sercon_attr_last  is the most recent attribute sent to the terminal.
+ */
+VARLOW u8 sercon_attr_last;
+VARLOW u8 sercon_col_lazy;
+VARLOW u8 sercon_char[8];
+VARLOW u8 sercon_attr[8];
+
+VARLOW u8 sercon_cmap[8] = { '0', '4', '2', '6', '1', '5', '3', '7' };
+
+static void sercon_putchar(u8 chr)
+{
+    u16 addr = GET_LOW(sercon_port);
+    u32 end = irqtimer_calc_ticks(0x0a);
+
+    for (;;) {
+        u8 lsr = inb(addr+SEROFF_LSR);
+        if ((lsr & 0x60) == 0x60) {
+            // Success - can write data
+            outb(chr, addr+SEROFF_DATA);
+            break;
+        }
+        if (irqtimer_check(end)) {
+            break;
+        }
+        yield();
+    }
+}
+
+static void sercon_init(void)
+{
+    /* reset */
+    sercon_putchar('\x1b');
+    sercon_putchar('c');
+    /* clear screen */
+    sercon_putchar('\x1b');
+    sercon_putchar('[');
+    sercon_putchar('2');
+    sercon_putchar('J');
+}
+
+static void sercon_set_color(u8 fg, u8 bg, u8 bold)
+{
+    sercon_putchar('\x1b');
+    sercon_putchar('[');
+    sercon_putchar('0');
+    if (fg != 7) {
+        sercon_putchar(';');
+        sercon_putchar('3');
+        sercon_putchar(GET_LOW(sercon_cmap[fg & 7]));
+    }
+    if (bg != 0) {
+        sercon_putchar(';');
+        sercon_putchar('4');
+        sercon_putchar(GET_LOW(sercon_cmap[bg & 7]));
+    }
+    if (bold) {
+        sercon_putchar(';');
+        sercon_putchar('1');
+    }
+    sercon_putchar('m');
+}
+
+static void sercon_set_attr(u8 attr)
+{
+    if (attr == GET_LOW(sercon_attr_last))
+        return;
+
+    SET_LOW(sercon_attr_last, attr);
+    sercon_set_color((attr >> 0) & 7,
+                     (attr >> 4) & 7,
+                     attr & 0x08);
+}
+
+static void sercon_cursor_goto(u8 row, u8 col)
+{
+    row++; col++;
+    sercon_putchar('\x1b');
+    sercon_putchar('[');
+    sercon_putchar('0' + row / 10);
+    sercon_putchar('0' + row % 10);
+    sercon_putchar(';');
+    sercon_putchar('0' + col / 10);
+    sercon_putchar('0' + col % 10);
+    sercon_putchar('H');
+}
+
+static void sercon_print_char(u8 chr, u8 attr)
+{
+    if (chr != 0x00) {
+        sercon_set_attr(attr);
+        sercon_putchar(chr);
+    } else {
+        /* move cursor right */
+        sercon_putchar('\x1b');
+        sercon_putchar('[');
+        sercon_putchar('C');
+    }
+}
+
+static void sercon_flush_lazy(void)
+{
+    u8 count = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
+    u8 pos = 0;
+
+    if (!count && !GET_LOW(sercon_attr[0]))
+        return;
+
+    while (count) {
+        sercon_print_char(GET_LOW(sercon_char[pos]),
+                          GET_LOW(sercon_attr[pos]));
+        count--;
+        pos++;
+    }
+
+    if (pos < ARRAY_SIZE(sercon_char) && GET_LOW(sercon_char[pos])) {
+        sercon_print_char(GET_LOW(sercon_char[pos]),
+                          GET_LOW(sercon_attr[pos]));
+        /* move cursor left */
+        sercon_putchar('\x1b');
+        sercon_putchar('[');
+        sercon_putchar('D');
+    }
+
+    SET_LOW(sercon_col_lazy, GET_LOW(sercon_col));
+    for (pos = 0; pos < ARRAY_SIZE(sercon_char); pos++) {
+        SET_LOW(sercon_attr[pos], 0x07);
+        SET_LOW(sercon_char[pos], 0x00);
+    }
+}
+
+/* Set video mode */
+static void sercon_1000(struct bregs *regs)
+{
+    SET_LOW(sercon_mode, regs->al);
+    switch (regs->al) {
+    case 0x03:
+    default:
+        SET_LOW(sercon_cols, 80);
+        SET_LOW(sercon_rows, 25);
+        regs->al = 0x30;
+    }
+    SET_LOW(sercon_row, 0);
+    SET_LOW(sercon_col, 0);
+    SET_LOW(sercon_col_lazy, 0);
+    sercon_init();
+}
+
+/* Set cursor position */
+static void sercon_1002(struct bregs *regs)
+{
+    u8 row = regs->dh;
+    u8 col = regs->dl;
+
+    if (row == GET_LOW(sercon_row) &&
+        col >= GET_LOW(sercon_col_lazy) &&
+        col <  GET_LOW(sercon_col_lazy) + ARRAY_SIZE(sercon_char)) {
+        SET_LOW(sercon_col, col);
+        if (col+1 == GET_LOW(sercon_col_lazy) + ARRAY_SIZE(sercon_char))
+            sercon_flush_lazy();
+    } else {
+        sercon_flush_lazy();
+        if (row == GET_LOW(sercon_row) && col == 0) {
+            sercon_putchar('\r');
+        } else {
+            sercon_cursor_goto(row, col);
+        }
+        SET_LOW(sercon_row, row);
+        SET_LOW(sercon_col, col);
+        SET_LOW(sercon_col_lazy, col);
+    }
+}
+
+/* Get cursor position */
+static void sercon_1003(struct bregs *regs)
+{
+    regs->ax = 0;
+    regs->ch = 0;
+    regs->cl = 7;
+    regs->dh = GET_LOW(sercon_row);
+    regs->dl = GET_LOW(sercon_col);
+}
+
+/* Scroll up window */
+static void sercon_1006(struct bregs *regs)
+{
+    sercon_flush_lazy();
+    sercon_putchar('\r');
+    sercon_putchar('\n');
+}
+
+/* Read character and attribute at cursor position */
+static void sercon_1008(struct bregs *regs)
+{
+    regs->ah = 0x07;
+    regs->bh = ' ';
+}
+
+/* Write character and attribute at cursor position */
+static void sercon_1009(struct bregs *regs)
+{
+    u16 count = regs->cx;
+    u8 pos;
+
+    if (count == 1) {
+        pos = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
+        if (pos < ARRAY_SIZE(sercon_char)) {
+            sercon_char[pos] = regs->al;
+            sercon_attr[pos] = regs->bl;
+        }
+    } else if (regs->al == 0x20 &&
+               GET_LOW(sercon_rows) * GET_LOW(sercon_cols) == count &&
+               GET_LOW(sercon_row) == 0 &&
+               GET_LOW(sercon_col) == 0) {
+        /* override everything with spaces -> this is clear screen */
+        sercon_flush_lazy();
+        sercon_putchar('\x1b');
+        sercon_putchar('[');
+        sercon_putchar('2');
+        sercon_putchar('J');
+    } else {
+        sercon_flush_lazy();
+        sercon_set_attr(regs->bl);
+        while (count) {
+            sercon_putchar(regs->al);
+            count--;
+        }
+        sercon_cursor_goto(GET_LOW(sercon_row),
+                           GET_LOW(sercon_col));
+    }
+}
+
+/* Teletype output */
+static void sercon_100e(struct bregs *regs)
+{
+    u8 pos, row, col;
+
+    switch (regs->al) {
+    case 7:
+        // beep
+        break;
+    case 8:
+        sercon_flush_lazy();
+        sercon_putchar(regs->al);
+        col = GET_LOW(sercon_col);
+        if (col > 0) {
+            col--;
+            SET_LOW(sercon_col, col);
+            SET_LOW(sercon_col_lazy, col);
+        }
+        break;
+    case '\r':
+        sercon_flush_lazy();
+        sercon_putchar(regs->al);
+        SET_LOW(sercon_col, 0);
+        SET_LOW(sercon_col_lazy, 0);
+        break;
+    case '\n':
+        sercon_flush_lazy();
+        sercon_putchar(regs->al);
+        row = GET_LOW(sercon_row);
+        row++;
+        if (row >= GET_LOW(sercon_rows))
+            row = GET_LOW(sercon_rows)-1;
+        SET_LOW(sercon_row, row);
+        break;
+    default:
+        pos = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
+        sercon_char[pos] = regs->al;
+        SET_LOW(sercon_col, GET_LOW(sercon_col) + 1);
+        if (pos+1 == ARRAY_SIZE(sercon_char))
+            sercon_flush_lazy();
+        break;
+    }
+}
+
+/* Get current video mode */
+static void sercon_100f(struct bregs *regs)
+{
+    regs->al = GET_LOW(sercon_mode);
+    regs->ah = GET_LOW(sercon_cols);
+}
+
+/* VBE 2.0 */
+static void sercon_104f(struct bregs *regs)
+{
+    regs->ax = 0x0100;
+}
+
+void VISIBLE16
+sercon_10(struct bregs *regs)
+{
+    if (!GET_LOW(sercon_port))
+        return;
+
+    switch (regs->ah) {
+    case 0x00: sercon_1000(regs); break;
+    case 0x02: sercon_1002(regs); break;
+    case 0x03: sercon_1003(regs); break;
+    case 0x06: sercon_1006(regs); break;
+    case 0x08: sercon_1008(regs); break;
+    case 0x09: sercon_1009(regs); break;
+    case 0x0e: sercon_100e(regs); break;
+    case 0x0f: sercon_100f(regs); break;
+    case 0x4f: sercon_104f(regs); break;
+    default:
+        dprintf(1, "%s: ah 0x%02x, not implemented\n",
+                __func__, regs->ah);
+    }
+}
+
+void sercon_enable(void)
+{
+    u16 addr = PORT_SERIAL1;
+
+    SET_LOW(sercon_port, addr);
+    outb(0x03, addr + SEROFF_LCR); // 8N1
+    outb(0x01, addr + 0x02);       // enable fifo
+    enable_vga_console();
+}
diff --git a/src/util.h b/src/util.h
index 7b41207..29f17be 100644
--- a/src/util.h
+++ b/src/util.h
@@ -230,6 +230,8 @@ void code_mutable_preinit(void);
 // serial.c
 void serial_setup(void);
 void lpt_setup(void);
+void sercon_10(struct bregs *regs);
+void sercon_enable(void);

// vgahooks.c
 void handle_155f(struct bregs *regs);

[RFC PATCH 1/2] serial console, output

[Kevin O'Connor]

On Fri, Jul 01, 2016 at 12:54:30PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann

Thanks.  See my comments below.

[...]
> --- a/src/misc.c
> +++ b/src/misc.c
> @@ -11,6 +11,7 @@
>  #include "output.h" // debug_enter
>  #include "stacks.h" // call16_int
>  #include "string.h" // memset
> +#include "util.h" // serial_10
>
> #define PORT_MATH_CLEAR        0x00f0
>
> @@ -57,6 +58,7 @@ handle_10(struct bregs *regs)
>  {
>      debug_enter(regs, DEBUG_HDL_10);
>      // don't do anything, since the VGA BIOS handles int10h requests
> sercon_10(regs);
>
> }

Might as well remove handle_10 and call sercon_10 directly from
romlayout.S.

[...]
> --- a/src/serial.c
> +++ b/src/serial.c
> @@ -315,3 +315,343 @@ handle_17(struct bregs *regs)
>      default:   handle_17XX(regs); break;
>      }
>  }
>
> +/****************************************************************
> serial console output
> ****************************************************************/

I think this code should go in a new c file and not modify serial.c.

[...]
> +VARLOW u8 sercon_mode;
> +VARLOW u8 sercon_cols;
> +VARLOW u8 sercon_rows;
>
> +VARLOW u8 sercon_row;
> +VARLOW u8 sercon_col;

I think the code should use the BDA equivalents of the above instead
of declaring them in the private VARLOW space.  Some old programs may
rely on the equivalents in the BDA being updated.
> +/*
> We have a small output buffer here, for lazy output.  That allows
> to avoid a whole bunch of control sequences for pointless cursor
> moves, so when logging the output it'll be *alot* less cluttered.
> sercon_char/attr  is the actual output buffer.
> sercon_col_lazy   is the column of the terminal's cursor, typically
>               a few positions left of sercon_col.
> sercon_attr_last  is the most recent attribute sent to the terminal.
> */
>
> +VARLOW u8 sercon_attr_last;
> +VARLOW u8 sercon_col_lazy;
> +VARLOW u8 sercon_char[8];
> +VARLOW u8 sercon_attr[8];
>
> +VARLOW u8 sercon_cmap[8] = { '0', '4', '2', '6', '1', '5', '3', '7' };

For constant data (sercon_cmap) it would be preferable to use "static
VAR16" (see kbd.c:scan_to_keycode[] and mouse.c:sample_rates[] as
examples) and use GET_GLOBAL() instead of GET_LOW().

[...]
> +static void sercon_print_char(u8 chr, u8 attr)
> +{
> if (chr != 0x00) {
>    sercon_set_attr(attr);
>    sercon_putchar(chr);
> } else {
>    /* move cursor right */
>    sercon_putchar('\x1b');
>    sercon_putchar('[');
>    sercon_putchar('C');
> }
>
> +}
>
> +static void sercon_flush_lazy(void)
> +{
> u8 count = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
> u8 pos = 0;
> if (!count && !GET_LOW(sercon_attr[0]))
>    return;
> while (count) {
>    sercon_print_char(GET_LOW(sercon_char[pos]),
>                      GET_LOW(sercon_attr[pos]));
>    count--;
>    pos++;
> }
> if (pos < ARRAY_SIZE(sercon_char) && GET_LOW(sercon_char[pos])) {
>    sercon_print_char(GET_LOW(sercon_char[pos]),
>                      GET_LOW(sercon_attr[pos]));
>    /* move cursor left */
>    sercon_putchar('\x1b');
>    sercon_putchar('[');
>    sercon_putchar('D');
> }
> SET_LOW(sercon_col_lazy, GET_LOW(sercon_col));
> for (pos = 0; pos < ARRAY_SIZE(sercon_char); pos++) {
>    SET_LOW(sercon_attr[pos], 0x07);
>    SET_LOW(sercon_char[pos], 0x00);
> }
>
> +}

So, if I understand the above correctly, it's a buffer of recent
updates used to reduce serial bandwidth (and unclutter serial logs) in
the case where the application code issues a bunch of cursor moves /
cell updates that are mostly redundant.
> +/* Set video mode */
> +static void sercon_1000(struct bregs *regs)
> +{
> SET_LOW(sercon_mode, regs->al);
> switch (regs->al) {
> case 0x03:
> default:
>    SET_LOW(sercon_cols, 80);
>    SET_LOW(sercon_rows, 25);
>    regs->al = 0x30;
> }
> SET_LOW(sercon_row, 0);
> SET_LOW(sercon_col, 0);
> SET_LOW(sercon_col_lazy, 0);
> sercon_init();
>
> +}

FYI, the screen should only be cleared if the high bit of the mode is
not set.  I do wonder if more of the vgabios interface code from
vgasrc/vgabios.c could be reused here.
> +/* Set cursor position */
> +static void sercon_1002(struct bregs *regs)
> +{
> u8 row = regs->dh;
> u8 col = regs->dl;
> if (row == GET_LOW(sercon_row) &&
>    col >= GET_LOW(sercon_col_lazy) &&
>    col <  GET_LOW(sercon_col_lazy) + ARRAY_SIZE(sercon_char)) {
>    SET_LOW(sercon_col, col);
>    if (col+1 == GET_LOW(sercon_col_lazy) + ARRAY_SIZE(sercon_char))
>        sercon_flush_lazy();
> } else {
>    sercon_flush_lazy();
>    if (row == GET_LOW(sercon_row) && col == 0) {
>        sercon_putchar('\r');
>    } else {
>        sercon_cursor_goto(row, col);
>    }
>    SET_LOW(sercon_row, row);
>    SET_LOW(sercon_col, col);
>    SET_LOW(sercon_col_lazy, col);
> }
>
> +}
>
> +/* Get cursor position */
> +static void sercon_1003(struct bregs *regs)
> +{
> regs->ax = 0;
> regs->ch = 0;
> regs->cl = 7;
> regs->dh = GET_LOW(sercon_row);
> regs->dl = GET_LOW(sercon_col);
>
> +}
>
> +/* Scroll up window */
> +static void sercon_1006(struct bregs *regs)
> +{
> sercon_flush_lazy();
> sercon_putchar('\r');
> sercon_putchar('\n');
>
> +}
>
> +/* Read character and attribute at cursor position */
> +static void sercon_1008(struct bregs *regs)
> +{
> regs->ah = 0x07;
> regs->bh = ' ';
>
> +}

FYI, the sgabios code seems to indicate that sercon_1008() needs to be
implemented for some programs to work properly.  The sgabios code even
implements a cache of recent writes to try to get it to work.  It's
ugly.
> +/* Write character and attribute at cursor position */
> +static void sercon_1009(struct bregs *regs)
> +{
> u16 count = regs->cx;
> u8 pos;
> if (count == 1) {
>    pos = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
>    if (pos < ARRAY_SIZE(sercon_char)) {
>        sercon_char[pos] = regs->al;
>        sercon_attr[pos] = regs->bl;
>    }
> } else if (regs->al == 0x20 &&
>           GET_LOW(sercon_rows) * GET_LOW(sercon_cols) == count &&
>           GET_LOW(sercon_row) == 0 &&
>           GET_LOW(sercon_col) == 0) {
>    /* override everything with spaces -> this is clear screen */
>    sercon_flush_lazy();
>    sercon_putchar('\x1b');
>    sercon_putchar('[');
>    sercon_putchar('2');
>    sercon_putchar('J');
> } else {
>    sercon_flush_lazy();
>    sercon_set_attr(regs->bl);
>    while (count) {
>        sercon_putchar(regs->al);
>        count--;
>    }
>    sercon_cursor_goto(GET_LOW(sercon_row),
>                       GET_LOW(sercon_col));
> }
>
> +}
>
> +/* Teletype output */
> +static void sercon_100e(struct bregs *regs)
> +{
> u8 pos, row, col;
> switch (regs->al) {
> case 7:
>    // beep
>    break;
> case 8:
>    sercon_flush_lazy();
>    sercon_putchar(regs->al);
>    col = GET_LOW(sercon_col);
>    if (col > 0) {
>        col--;
>        SET_LOW(sercon_col, col);
>        SET_LOW(sercon_col_lazy, col);
>    }
>    break;
> case '\r':
>    sercon_flush_lazy();
>    sercon_putchar(regs->al);
>    SET_LOW(sercon_col, 0);
>    SET_LOW(sercon_col_lazy, 0);
>    break;
> case '\n':
>    sercon_flush_lazy();
>    sercon_putchar(regs->al);
>    row = GET_LOW(sercon_row);
>    row++;
>    if (row >= GET_LOW(sercon_rows))
>        row = GET_LOW(sercon_rows)-1;
>    SET_LOW(sercon_row, row);
>    break;
> default:
>    pos = GET_LOW(sercon_col) - GET_LOW(sercon_col_lazy);
>    sercon_char[pos] = regs->al;
>    SET_LOW(sercon_col, GET_LOW(sercon_col) + 1);
>    if (pos+1 == ARRAY_SIZE(sercon_char))
>        sercon_flush_lazy();
>    break;
> }
>
> +}

I'm finding it hard to understand how the "uncluttering" works in the
above.  I think it would be easier to understand if the vgabios
interface code was separated from the "uncluttering" code.

In particular, I wonder if it would be simpler if the interface code
was more similar to vgasrc/vgabios.c and it just translated requests
to set_cursor_pos(cursorpos) and write_char(cursorpos, charattr) type
calls.  Then the write_char() code could check if the position was
near previously written characters and buffer it if so - flushing if
not.  Thus the "uncluttering" could be mostly done in write_char().
The set_cursor_pos() implementation could be very lazy - it only needs
to update the BDA with the new position.  The sercon_check_event()
code could send an explicit cursor move for the case where the cursor
is idling at some position not after the last written character.

[...]
> +void VISIBLE16
> +sercon_10(struct bregs *regs)
> +{
> if (!GET_LOW(sercon_port))
>    return;
> switch (regs->ah) {
> case 0x00: sercon_1000(regs); break;
> case 0x02: sercon_1002(regs); break;
> case 0x03: sercon_1003(regs); break;
> case 0x06: sercon_1006(regs); break;
> case 0x08: sercon_1008(regs); break;
> case 0x09: sercon_1009(regs); break;
> case 0x0e: sercon_100e(regs); break;
> case 0x0f: sercon_100f(regs); break;
> case 0x4f: sercon_104f(regs); break;
> default:
>    dprintf(1, "%s: ah 0x%02x, not implemented\n",
>            __func__, regs->ah);

There is a warn_unimplemented(regs) for this.  Also, would be nice to
implement a sercon_10XX(regs) to match other areas of the code.

-Kevin

[RFC PATCH 1/2] serial console, output

[Gerd Hoffmann]

Hi,
> > @@ -57,6 +58,7 @@ handle_10(struct bregs *regs)
> >  {
> >      debug_enter(regs, DEBUG_HDL_10);
> >      // don't do anything, since the VGA BIOS handles int10h requests
> > sercon_10(regs);
> >
> > }
>
> Might as well remove handle_10 and call sercon_10 directly from
> romlayout.S.

Well, I expect this will change when adding support for parallel output
to both vga and serial console.
> > +/****************************************************************
> > serial console output
> > ****************************************************************/
>
> I think this code should go in a new c file and not modify serial.c.

Agree.  Was thinking about that already as I saw the code grow ;)
> [...]
> > +VARLOW u8 sercon_mode;
> > +VARLOW u8 sercon_cols;
> > +VARLOW u8 sercon_rows;
> >
> > +VARLOW u8 sercon_row;
> > +VARLOW u8 sercon_col;
>
> I think the code should use the BDA equivalents of the above instead
> of declaring them in the private VARLOW space.  Some old programs may
> rely on the equivalents in the BDA being updated.

Figured that meanwhile.  syslinux is one example.
> > +VARLOW u8 sercon_cmap[8] = { '0', '4', '2', '6', '1', '5', '3', '7' };
>
> For constant data (sercon_cmap) it would be preferable to use "static
> VAR16" (see kbd.c:scan_to_keycode[] and mouse.c:sample_rates[] as
> examples) and use GET_GLOBAL() instead of GET_LOW().

OK.
> > SET_LOW(sercon_col_lazy, GET_LOW(sercon_col));
> > for (pos = 0; pos < ARRAY_SIZE(sercon_char); pos++) {
> >    SET_LOW(sercon_attr[pos], 0x07);
> >    SET_LOW(sercon_char[pos], 0x00);
> > }
> >
> > +}
>
> So, if I understand the above correctly, it's a buffer of recent
> updates used to reduce serial bandwidth (and unclutter serial logs) in
> the case where the application code issues a bunch of cursor moves /
> cell updates that are mostly redundant.

Yep.  Typically happens for colored output.  Apps use int10/09 to set
character and attribute at the cursor position (but without moving the
cursor).  Then they move the cursor either using int10/02 (explicit set
cursor position) or by printing the same character again using int10/0e
(teletype, which prints character without updating attribute and moves
the cursor forward).
> > +/* Read character and attribute at cursor position */
> > +static void sercon_1008(struct bregs *regs)
> > +{
> > regs->ah = 0x07;
> > regs->bh = ' ';
> >
> > +}
>
> FYI, the sgabios code seems to indicate that sercon_1008() needs to be
> implemented for some programs to work properly.  The sgabios code even
> implements a cache of recent writes to try to get it to work.  It's
> ugly.

Didn't run into any issues yet, but also tested linux bootloaders only.

Maybe we can reuse the output buffer which we have anyway.  Logic needs
reworked a bit.  We can't just clear characters after printing them out
if we want be able to read them later, so we need a separate
pending-updates bit.  Also should be bigger I guess, maybe 80 chars so
it can cover a complete line.
> I'm finding it hard to understand how the "uncluttering" works in the
> above.  I think it would be easier to understand if the vgabios
> interface code was separated from the "uncluttering" code.
>
> In particular, I wonder if it would be simpler if the interface code
> was more similar to vgasrc/vgabios.c and it just translated requests
> to set_cursor_pos(cursorpos) and write_char(cursorpos, charattr) type
> calls.  Then the write_char() code could check if the position was
> near previously written characters and buffer it if so - flushing if
> not.  Thus the "uncluttering" could be mostly done in write_char().
> The set_cursor_pos() implementation could be very lazy - it only needs
> to update the BDA with the new position.  The sercon_check_event()
> code could send an explicit cursor move for the case where the cursor
> is idling at some position not after the last written character.

I'll have a look.
> > default:
> >    dprintf(1, "%s: ah 0x%02x, not implemented\n",
> >            __func__, regs->ah);
>
> There is a warn_unimplemented(regs) for this.  Also, would be nice to
> implement a sercon_10XX(regs) to match other areas of the code.

Ok.

cheers,
  Gerd

[RFC PATCH 1/2] serial console, output

[Paolo Bonzini]

On 04/07/2016 10:16, Gerd Hoffmann wrote:
> sercon_putchar('\x1b');
> sercon_putchar('c');
> /* clear screen */
> sercon_putchar('\x1b');
> sercon_putchar('[');
> sercon_putchar('2');
> sercon_putchar('J');
>
> +}
>
> +static void sercon_set_color(u8 fg, u8 bg, u8 bold)
> +{
> sercon_putchar('\x1b');
> sercon_putchar('[');
> sercon_putchar('0');
> if (fg != 7) {
>    sercon_putchar(';');
>    sercon_putchar('3');
>    sercon_putchar(GET_LOW(sercon_cmap[fg & 7]));
> }
> if (bg != 0) {
>    sercon_putchar(';');
>    sercon_putchar('4');
>    sercon_putchar(GET_LOW(sercon_cmap[bg & 7]));
> }
> if (bold) {
>    sercon_putchar(';');
>    sercon_putchar('1');
> }
> sercon_putchar('m');

Add a sercon_putstr perhaps?
> > > +/* Read character and attribute at cursor position */
> > > > > +static void sercon_1008(struct bregs *regs)
> > > > > +{
> > > > > regs->ah = 0x07;
> > > > > regs->bh = ' ';
> > > > >
> > > > > +}
> > >
> > > FYI, the sgabios code seems to indicate that sercon_1008() needs to be
> > > implemented for some programs to work properly.  The sgabios code even
> > > implements a cache of recent writes to try to get it to work.  It's
> > > ugly.
>
> Didn't run into any issues yet, but also tested linux bootloaders only.
>
> Maybe we can reuse the output buffer which we have anyway.  Logic needs
> reworked a bit.  We can't just clear characters after printing them out
> if we want be able to read them later, so we need a separate
> pending-updates bit.  Also should be bigger I guess, maybe 80 chars so
> it can cover a complete line.

80x25 is just 2K...  Perhaps it's simpler to just allocate the whole
video buffer from the UMB or EBDA when serial console is in use?

(GWBASIC in graphics modes, for one, uses 10/08 from the whole screen.
I don't know if it does that in text modes too).

Paolo

[RFC PATCH 1/2] serial console, output

[Gerd Hoffmann]

On Mo, 2016-07-04 at 11:11 +0200, Paolo Bonzini wrote:
> On 04/07/2016 10:16, Gerd Hoffmann wrote:
> > sercon_putchar('\x1b');
> > sercon_putchar('c');
> > /* clear screen */
> > sercon_putchar('\x1b');
> > sercon_putchar('[');
> > sercon_putchar('2');
> > sercon_putchar('J');
> >
> > +}
> >
> > +static void sercon_set_color(u8 fg, u8 bg, u8 bold)
> > +{
> > sercon_putchar('\x1b');
> > sercon_putchar('[');
> > sercon_putchar('0');
> > if (fg != 7) {
> >    sercon_putchar(';');
> >    sercon_putchar('3');
> >    sercon_putchar(GET_LOW(sercon_cmap[fg & 7]));
> > }
> > if (bg != 0) {
> >    sercon_putchar(';');
> >    sercon_putchar('4');
> >    sercon_putchar(GET_LOW(sercon_cmap[bg & 7]));
> > }
> > if (bold) {
> >    sercon_putchar(';');
> >    sercon_putchar('1');
> > }
> > sercon_putchar('m');
>
> Add a sercon_putstr perhaps?

We run in real mode, so passing around pointers isn't that easy.
Given this would make sense for constant strings only (like the 4-byte
clear-screen sequence) and not so much for variable stuff we might put
all strings in the global segment.

Would this ...

void sercon_putchar(char *ptr)
{
    char c = GET_GLOBAL(ptr[0]);
    [ ... ]

... work?
> > Maybe we can reuse the output buffer which we have anyway.  Logic needs
> > reworked a bit.  We can't just clear characters after printing them out
> > if we want be able to read them later, so we need a separate
> > pending-updates bit.  Also should be bigger I guess, maybe 80 chars so
> > it can cover a complete line.
>
> 80x25 is just 2K...  Perhaps it's simpler to just allocate the whole
> video buffer from the UMB or EBDA when serial console is in use?

4k, we need both character and attribute.  But, yes, if we can allocate
that somewhere in real mode address space without running into memory
pressure this might be the best option.

cheers,
  Gerd

[RFC PATCH 1/2] serial console, output

[Paolo Bonzini]

On 04/07/2016 14:46, Gerd Hoffmann wrote:
> We run in real mode, so passing around pointers isn't that easy.
> Given this would make sense for constant strings only (like the 4-byte
> clear-screen sequence) and not so much for variable stuff we might put
> all strings in the global segment.

Hmm you're right, pointers are messy here.

Paolo
> Would this ...
>
> void sercon_putchar(char *ptr)
> {
>     char c = GET_GLOBAL(ptr[0]);
>     [ ... ]
>
> ... work?

[RFC PATCH 1/2] serial console, output

[Kevin O'Connor]

On Mon, Jul 04, 2016 at 02:46:24PM +0200, Gerd Hoffmann wrote:
> On Mo, 2016-07-04 at 11:11 +0200, Paolo Bonzini wrote:
> > On 04/07/2016 10:16, Gerd Hoffmann wrote:
> > > +static void sercon_set_color(u8 fg, u8 bg, u8 bold)
> > > +{
> > > sercon_putchar('\x1b');
> > > sercon_putchar('[');
> > > sercon_putchar('0');
> >
> > Add a sercon_putstr perhaps?
>
> We run in real mode, so passing around pointers isn't that easy.
> Given this would make sense for constant strings only (like the 4-byte
> clear-screen sequence) and not so much for variable stuff we might put
> all strings in the global segment.
>
> Would this ...
>
> void sercon_putchar(char *ptr)
> {
>     char c = GET_GLOBAL(ptr[0]);
>     [ ... ]
>
> ... work?

Yes.  See output.c:puts_cs() as an example.  It only works if it's a
constant string (as opposed to a string built on the stack).
> > > Maybe we can reuse the output buffer which we have anyway.  Logic needs
> > > reworked a bit.  We can't just clear characters after printing them out
> > > if we want be able to read them later, so we need a separate
> > > pending-updates bit.  Also should be bigger I guess, maybe 80 chars so
> > > it can cover a complete line.
> >
> > 80x25 is just 2K...  Perhaps it's simpler to just allocate the whole
> > video buffer from the UMB or EBDA when serial console is in use?
>
> 4k, we need both character and attribute.  But, yes, if we can allocate
> that somewhere in real mode address space without running into memory
> pressure this might be the best option.

Unfortunately, the screen can be larger than 80x25.

If a large buffer is desired, it's also possible to malloc_high() it
and then use either: call32() to access it, or int1587 to read/write
it (see ramdisk.c:ramdisk_copy() as an example).  Either way it seems
ugly.

At one point I looked through the sgabios code and was able to
understand how it did caching.  I'll take another look and see if I
can describe it.

-Kevin

[RFC PATCH 1/2] serial console, output

[Paolo Bonzini]

On 04/07/2016 17:26, Kevin O'Connor wrote:
> > 4k, we need both character and attribute.  But, yes, if we can allocate
> > that somewhere in real mode address space without running into memory
> > pressure this might be the best option.
>
> Unfortunately, the screen can be larger than 80x25.

It can with SVGA BIOS, but Gerd here only supports mode 3, doesn't he?

Paolo
> If a large buffer is desired, it's also possible to malloc_high() it
> and then use either: call32() to access it, or int1587 to read/write
> it (see ramdisk.c:ramdisk_copy() as an example).  Either way it seems
> ugly.
>
> At one point I looked through the sgabios code and was able to
> understand how it did caching.  I'll take another look and see if I
> can describe it.

[RFC PATCH 1/2] serial console, output

[Gerd Hoffmann]

Hi,
> > Unfortunately, the screen can be larger than 80x25.
>
> It can with SVGA BIOS, but Gerd here only supports mode 3, doesn't he?

Current code yes, but that doesn't imply it'll stay that way forever.
Supporting other sizes is just a matter of making sercon_1000()
recognizing the mode numbers and set cols+rows accordingly.

cheers,
  Gerd

[RFC PATCH 1/2] serial console, output

[Kevin O'Connor]

On Mon, Jul 04, 2016 at 11:26:48AM -0400, Kevin O'Connor wrote:
> At one point I looked through the sgabios code and was able to
> understand how it did caching.  I'll take another look and see if I
> can describe it.

So, if I read the sgabios code correctly, it allocates a buffer of:

struct { u8 x, y; char c; } logbuf[256];
int logbuf_offset;

Every character sent on the serial port is appended to "logbuf" in
order, wrapping if necessary: logbuf[logbuf_offset++ % 256] = x, y, c.
On a read, it scans backwards from logbuf_offset to find the last
update to that cell.

Interestingly, it doesn't store the attribute with the character -
it's int1008 handler just returns the last attribute used anywhere on
the screen.

The code is only used if it is the sole vga code (as opposed to being
used in addition to an existing vgabios).

Does anyone know where one can find the original svn commit history
for sgabios?  Seems the original google code repo is no longer
present.

-Kevin

[RFC PATCH 1/2] serial console, output

[Paolo Bonzini]

On 04/07/2016 18:00, Kevin O'Connor wrote:
> So, if I read the sgabios code correctly, it allocates a buffer of:
>
> struct { u8 x, y; char c; } logbuf[256];
> int logbuf_offset;
>
> Every character sent on the serial port is appended to "logbuf" in
> order, wrapping if necessary: logbuf[logbuf_offset++ % 256] = x, y, c.
> On a read, it scans backwards from logbuf_offset to find the last
> update to that cell.
>
> Interestingly, it doesn't store the attribute with the character -
> it's int1008 handler just returns the last attribute used anywhere on
> the screen.
>
> The code is only used if it is the sole vga code (as opposed to being
> used in addition to an existing vgabios).
>
> Does anyone know where one can find the original svn commit history
> for sgabios?  Seems the original google code repo is no longer
> present.

There was no history as far as I remember, mostly a code drop.

Paolo

[RFC PATCH 1/2] serial console, output

[Kevin O'Connor]

On Mon, Jul 04, 2016 at 06:03:30PM +0200, Paolo Bonzini wrote:
> On 04/07/2016 18:00, Kevin O'Connor wrote:
> > Does anyone know where one can find the original svn commit history
> > for sgabios?  Seems the original google code repo is no longer
> > present.
>
> There was no history as far as I remember, mostly a code drop.

Ah, right.

I found what I was looking for though - it was in the sgabios
design.txt file instead of the revision history:

Summary of new enhancements
---------------------------
SGABIOS now keeps a log of the last 256 characters written to
the screen and where they were written in the event an application
like lilo asks for the current character under the cursor.  These
are currently stored in a 1KB EBDA allocation which can be expanded
as needed.  This method avoids having to store a 64KB buffer for
the largest possible serial terminal supported (255x255).

So, if I read the above correctly, it was lilo that inspired the
"feature".  Anyway, something to keep in mind.

BTW, lots of good info in the rest of design.txt.

Cheers,
-Kevin

[RFC PATCH 1/2] serial console, output

[Gerd Hoffmann]

Hi,
> I found what I was looking for though - it was in the sgabios
> design.txt file instead of the revision history:
> So, if I read the above correctly, it was lilo that inspired the
> "feature".  Anyway, something to keep in mind.

Oh.  lilo.  Interesting.

I didn't expect http://www.qemu-advent-calendar.org/#day-1 becomes a
useful testcase some day ;)
> BTW, lots of good info in the rest of design.txt.

Guess I should have a closer look at it.

thanks for the pointer,
  Gerd

[RFC PATCH 1/2] serial console, output

[Gerd Hoffmann]

Hi,
> > void sercon_putchar(char *ptr)
> > {
> >     char c = GET_GLOBAL(ptr[0]);
> >     [ ... ]
> >
> > ... work?
>
> Yes.  See output.c:puts_cs() as an example.  It only works if it's a
> constant string (as opposed to a string built on the stack).

After cleaning up the code only three fixed sequences are left: reset,
clear screen and move cursor right.  Don't think it is worth creating a
putstring function just for these.

cheers,
  Gerd

[RFC PATCH 2/2] serial console, input

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann 
---
 src/clock.c  |   1 +
 src/serial.c | 255 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/util.h   |   1 +
 3 files changed, 257 insertions(+)

diff --git a/src/clock.c b/src/clock.c
index e83e0f3..e44e112 100644
--- a/src/clock.c
+++ b/src/clock.c
@@ -295,6 +295,7 @@ clock_update(void)
     floppy_tick();
     usb_check_event();
     ps2_check_event();
+    sercon_check_event();
 }

// INT 08h System Timer ISR Entry Point
diff --git a/src/serial.c b/src/serial.c
index 74b91bb..d72dd01 100644
--- a/src/serial.c
+++ b/src/serial.c
@@ -655,3 +655,258 @@ void sercon_enable(void)
     outb(0x01, addr + 0x02);       // enable fifo
     enable_vga_console();
 }
+
+/****************************************************************
+ * serial input
+ ****************************************************************/
+
+VARLOW u8 rx_buf[16];
+VARLOW u8 rx_bytes;
+
+VARLOW struct {
+    char seq[4];
+    u8 len;
+    u8 scancode;
+} termseq[] = {
+    { .seq = "OP", .len = 2, .scancode = 0x3b },    // F1
+    { .seq = "OQ", .len = 2, .scancode = 0x3c },    // F2
+    { .seq = "OR", .len = 2, .scancode = 0x3d },    // F3
+    { .seq = "OS", .len = 2, .scancode = 0x3e },    // F4
+    { .seq = "[A", .len = 2, .scancode = 0xc8 },    // up
+    { .seq = "[B", .len = 2, .scancode = 0xd0 },    // down
+    { .seq = "[C", .len = 2, .scancode = 0xcd },    // right
+    { .seq = "[D", .len = 2, .scancode = 0xcb },    // left
+};
+
+#define FLAG_CTRL  (1<<0)
+#define FLAG_SHIFT (1<<1)
+
+VARLOW struct {
+    u8 flags;
+    u8 scancode;
+} termchr[256] = {
+    [ '1'        ] = { .scancode = 0x02,                      },
+    [ '!'        ] = { .scancode = 0x02, .flags = FLAG_SHIFT  },
+    [ '2'        ] = { .scancode = 0x03,                      },
+    [ '@'        ] = { .scancode = 0x03, .flags = FLAG_SHIFT  },
+    [ '3'        ] = { .scancode = 0x04,                      },
+    [ '#'        ] = { .scancode = 0x04, .flags = FLAG_SHIFT  },
+    [ '4'        ] = { .scancode = 0x05,                      },
+    [ '$'        ] = { .scancode = 0x05, .flags = FLAG_SHIFT  },
+    [ '5'        ] = { .scancode = 0x06,                      },
+    [ '%'        ] = { .scancode = 0x06, .flags = FLAG_SHIFT  },
+    [ '6'        ] = { .scancode = 0x07,                      },
+    [ '^'        ] = { .scancode = 0x07, .flags = FLAG_SHIFT  },
+    [ '7'        ] = { .scancode = 0x08,                      },
+    [ '&'        ] = { .scancode = 0x08, .flags = FLAG_SHIFT  },
+    [ '8'        ] = { .scancode = 0x09,                      },
+    [ '*'        ] = { .scancode = 0x09, .flags = FLAG_SHIFT  },
+    [ '9'        ] = { .scancode = 0x0a,                      },
+    [ '('        ] = { .scancode = 0x0a, .flags = FLAG_SHIFT  },
+    [ '0'        ] = { .scancode = 0x0b,                      },
+    [ ')'        ] = { .scancode = 0x0b, .flags = FLAG_SHIFT  },
+    [ '-'        ] = { .scancode = 0x0c,                      },
+    [ '='        ] = { .scancode = 0x0d,                      },
+    [ '+'        ] = { .scancode = 0x0d, .flags = FLAG_SHIFT  },
+
+    [ 'q'        ] = { .scancode = 0x10,                      },
+    [ 'Q'        ] = { .scancode = 0x10, .flags = FLAG_SHIFT  },
+    [ 'Q' & 0x1f ] = { .scancode = 0x10, .flags = FLAG_CTRL   },
+    [ 'w'        ] = { .scancode = 0x11,                      },
+    [ 'W'        ] = { .scancode = 0x11, .flags = FLAG_SHIFT  },
+    [ 'W' & 0x1f ] = { .scancode = 0x11, .flags = FLAG_CTRL   },
+    [ 'e'        ] = { .scancode = 0x12,                      },
+    [ 'E'        ] = { .scancode = 0x12, .flags = FLAG_SHIFT  },
+    [ 'E' & 0x1f ] = { .scancode = 0x12, .flags = FLAG_CTRL   },
+    [ 'r'        ] = { .scancode = 0x13,                      },
+    [ 'R'        ] = { .scancode = 0x13, .flags = FLAG_SHIFT  },
+    [ 'R' & 0x1f ] = { .scancode = 0x13, .flags = FLAG_CTRL   },
+    [ 't'        ] = { .scancode = 0x14,                      },
+    [ 'T'        ] = { .scancode = 0x14, .flags = FLAG_SHIFT  },
+    [ 'T' & 0x1f ] = { .scancode = 0x14, .flags = FLAG_CTRL   },
+    [ 'y'        ] = { .scancode = 0x15,                      },
+    [ 'Y'        ] = { .scancode = 0x15, .flags = FLAG_SHIFT  },
+    [ 'Y' & 0x1f ] = { .scancode = 0x15, .flags = FLAG_CTRL   },
+    [ 'u'        ] = { .scancode = 0x16,                      },
+    [ 'U'        ] = { .scancode = 0x16, .flags = FLAG_SHIFT  },
+    [ 'U' & 0x1f ] = { .scancode = 0x16, .flags = FLAG_CTRL   },
+    [ 'i'        ] = { .scancode = 0x17,                      },
+    [ 'I'        ] = { .scancode = 0x17, .flags = FLAG_SHIFT  },
+    [ 'I' & 0x1f ] = { .scancode = 0x17, .flags = FLAG_CTRL   },
+    [ 'o'        ] = { .scancode = 0x18,                      },
+    [ 'O'        ] = { .scancode = 0x18, .flags = FLAG_SHIFT  },
+    [ 'O' & 0x1f ] = { .scancode = 0x18, .flags = FLAG_CTRL   },
+    [ 'p'        ] = { .scancode = 0x19,                      },
+    [ 'P'        ] = { .scancode = 0x19, .flags = FLAG_SHIFT  },
+    [ 'P' & 0x1f ] = { .scancode = 0x19, .flags = FLAG_CTRL   },
+    [ '['        ] = { .scancode = 0x1a,                      },
+    [ '{'        ] = { .scancode = 0x1a, .flags = FLAG_SHIFT  },
+    [ ']'        ] = { .scancode = 0x1b,                      },
+    [ '}'        ] = { .scancode = 0x1b, .flags = FLAG_SHIFT  },
+
+    [ 'a'        ] = { .scancode = 0x1e,                      },
+    [ 'A'        ] = { .scancode = 0x1e, .flags = FLAG_SHIFT  },
+    [ 'A' & 0x1f ] = { .scancode = 0x1e, .flags = FLAG_CTRL   },
+    [ 's'        ] = { .scancode = 0x1f,                      },
+    [ 'S'        ] = { .scancode = 0x1f, .flags = FLAG_SHIFT  },
+    [ 'S' & 0x1f ] = { .scancode = 0x1f, .flags = FLAG_CTRL   },
+    [ 'd'        ] = { .scancode = 0x20,                      },
+    [ 'D'        ] = { .scancode = 0x20, .flags = FLAG_SHIFT  },
+    [ 'D' & 0x1f ] = { .scancode = 0x20, .flags = FLAG_CTRL   },
+    [ 'f'        ] = { .scancode = 0x21,                      },
+    [ 'F'        ] = { .scancode = 0x21, .flags = FLAG_SHIFT  },
+    [ 'F' & 0x1f ] = { .scancode = 0x21, .flags = FLAG_CTRL   },
+    [ 'g'        ] = { .scancode = 0x22,                      },
+    [ 'G'        ] = { .scancode = 0x22, .flags = FLAG_SHIFT  },
+    [ 'G' & 0x1f ] = { .scancode = 0x22, .flags = FLAG_CTRL   },
+    [ 'h'        ] = { .scancode = 0x23,                      },
+    [ 'H'        ] = { .scancode = 0x23, .flags = FLAG_SHIFT  },
+    [ 'H' & 0x1f ] = { .scancode = 0x23, .flags = FLAG_CTRL   },
+    [ 'j'        ] = { .scancode = 0x24,                      },
+    [ 'J'        ] = { .scancode = 0x24, .flags = FLAG_SHIFT  },
+    [ 'J' & 0x1f ] = { .scancode = 0x24, .flags = FLAG_CTRL   },
+    [ 'k'        ] = { .scancode = 0x25,                      },
+    [ 'K'        ] = { .scancode = 0x25, .flags = FLAG_SHIFT  },
+    [ 'K' & 0x1f ] = { .scancode = 0x25, .flags = FLAG_CTRL   },
+    [ 'l'        ] = { .scancode = 0x26,                      },
+    [ 'L'        ] = { .scancode = 0x26, .flags = FLAG_SHIFT  },
+    [ 'L' & 0x1f ] = { .scancode = 0x26, .flags = FLAG_CTRL   },
+    [ ';'        ] = { .scancode = 0x27,                      },
+    [ ':'        ] = { .scancode = 0x27, .flags = FLAG_SHIFT  },
+    [ '''       ] = { .scancode = 0x28,                      },
+    [ '"'        ] = { .scancode = 0x28, .flags = FLAG_SHIFT  },
+
+    [ '\'       ] = { .scancode = 0x2b,                      },
+    [ '|'        ] = { .scancode = 0x2b, .flags = FLAG_SHIFT  },
+    [ 'z'        ] = { .scancode = 0x2c,                      },
+    [ 'Z'        ] = { .scancode = 0x2c, .flags = FLAG_SHIFT  },
+    [ 'Z' & 0x1f ] = { .scancode = 0x2c, .flags = FLAG_CTRL   },
+    [ 'x'        ] = { .scancode = 0x2d,                      },
+    [ 'X'        ] = { .scancode = 0x2d, .flags = FLAG_SHIFT  },
+    [ 'X' & 0x1f ] = { .scancode = 0x2d, .flags = FLAG_CTRL   },
+    [ 'c'        ] = { .scancode = 0x2e,                      },
+    [ 'C'        ] = { .scancode = 0x2e, .flags = FLAG_SHIFT  },
+    [ 'C' & 0x1f ] = { .scancode = 0x2e, .flags = FLAG_CTRL   },
+    [ 'v'        ] = { .scancode = 0x2f,                      },
+    [ 'V'        ] = { .scancode = 0x2f, .flags = FLAG_SHIFT  },
+    [ 'V' & 0x1f ] = { .scancode = 0x2f, .flags = FLAG_CTRL   },
+    [ 'b'        ] = { .scancode = 0x30,                      },
+    [ 'B'        ] = { .scancode = 0x30, .flags = FLAG_SHIFT  },
+    [ 'B' & 0x1f ] = { .scancode = 0x30, .flags = FLAG_CTRL   },
+    [ 'n'        ] = { .scancode = 0x31,                      },
+    [ 'N'        ] = { .scancode = 0x31, .flags = FLAG_SHIFT  },
+    [ 'N' & 0x1f ] = { .scancode = 0x31, .flags = FLAG_CTRL   },
+    [ 'm'        ] = { .scancode = 0x32,                      },
+    [ 'M'        ] = { .scancode = 0x32, .flags = FLAG_SHIFT  },
+    [ 'M' & 0x1f ] = { .scancode = 0x32, .flags = FLAG_CTRL   },
+    [ ','        ] = { .scancode = 0x33,                      },
+    [ '<'        ] = { .scancode = 0x33, .flags = FLAG_SHIFT  },
+    [ '.'        ] = { .scancode = 0x34,                      },
+    [ '>'        ] = { .scancode = 0x34, .flags = FLAG_SHIFT  },
+    [ '?'        ] = { .scancode = 0x35,                      },
+    [ '|'        ] = { .scancode = 0x35, .flags = FLAG_SHIFT  },
+
+    [ '\x1b'     ] = { .scancode = 0x01,                      },
+    [ '\t'       ] = { .scancode = 0x0f,                      },
+    [ '\r'       ] = { .scancode = 0x1c,                      },
+    [ '\n'       ] = { .scancode = 0x1c,                      },
+    [ ' '        ] = { .scancode = 0x39,                      },
+};
+
+static void shiftbuf(int remove)
+{
+    int i, remaining;
+
+    remaining = GET_LOW(rx_bytes) - remove;
+    SET_LOW(rx_bytes, remaining);
+    for (i = 0; i < remaining; i++)
+        SET_LOW(rx_buf[i], GET_LOW(rx_buf[i + remove]));
+}
+
+static void sercon_sendkey(u8 scancode, u8 flags)
+{
+    if (flags & FLAG_CTRL)
+        process_key(0x1d);
+    if (flags & FLAG_SHIFT)
+        process_key(0x2a);
+
+    if (scancode & 0x80) {
+        process_key(0xe0);
+        process_key(scancode & ~0x80);
+        process_key(0xe0);
+        process_key(scancode);
+    } else {
+        process_key(scancode);
+        process_key(scancode | 0x80);
+    }
+
+    if (flags & FLAG_SHIFT)
+        process_key(0x2a | 0x80);
+    if (flags & FLAG_CTRL)
+        process_key(0x1d | 0x80);
+}
+
+void VISIBLE16
+sercon_check_event(void)
+{
+    u16 addr = GET_LOW(sercon_port);
+    u8 byte, scancode, flags, count = 0;
+    int seq, chr, len;
+
+    // check to see if there is a active serial port
+    if (!addr)
+        return;
+    if (inb(addr + SEROFF_LSR) == 0xFF)
+        return;
+
+    // flush pending output
+    sercon_flush_lazy();
+
+    // read all available data
+    while (inb(addr + SEROFF_LSR) & 0x01) {
+        byte = inb(addr + SEROFF_DATA);
+        if (GET_LOW(rx_bytes) < sizeof(rx_buf)) {
+            SET_LOW(rx_buf[rx_bytes], byte);
+            SET_LOW(rx_bytes, GET_LOW(rx_bytes) + 1);
+            count++;
+        }
+    }
+
+next_char:
+    // no (more) input data
+    if (!GET_LOW(rx_bytes))
+        return;
+
+    // lookup escape sequences
+    if (GET_LOW(rx_bytes) > 1 && GET_LOW(rx_buf[0]) == 0x1b) {
+        for (seq = 0; seq < ARRAY_SIZE(termseq); seq++) {
+            len = GET_LOW(termseq[seq].len);
+            if (GET_LOW(rx_bytes) < len + 1)
+                continue;
+            for (chr = 0; chr < len; chr++) {
+                if (GET_LOW(termseq[seq].seq[chr]) != GET_LOW(rx_buf[chr + 1]))
+                    break;
+            }
+            if (chr == len) {
+                scancode = GET_LOW(termseq[seq].scancode);
+                sercon_sendkey(scancode, 0);
+                shiftbuf(len + 1);
+                goto next_char;
+            }
+        }
+    }
+
+    // Seems we got a escape sequence we didn't recognise.
+    //  -> If we received data wait for more, maybe it is just incomplete.
+    if (GET_LOW(rx_buf[0]) == 0x1b && count)
+        return;
+
+    // Handle input as individual chars.
+    chr = GET_LOW(rx_buf[0]);
+    scancode = GET_LOW(termchr[chr].scancode);
+    flags = GET_LOW(termchr[chr].flags);
+    if (scancode)
+        sercon_sendkey(scancode, flags);
+    shiftbuf(1);
+    goto next_char;
+}
diff --git a/src/util.h b/src/util.h
index 29f17be..3e7366b 100644
--- a/src/util.h
+++ b/src/util.h
@@ -232,6 +232,7 @@ void serial_setup(void);
 void lpt_setup(void);
 void sercon_10(struct bregs *regs);
 void sercon_enable(void);
+void sercon_check_event(void);

// vgahooks.c
 void handle_155f(struct bregs *regs);

[RFC PATCH 2/2] serial console, input

[Kevin O'Connor]

On Fri, Jul 01, 2016 at 12:54:31PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann
>
> src/clock.c  |   1 +
>  src/serial.c | 255 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util.h   |   1 +
>  3 files changed, 257 insertions(+)
>
> diff --git a/src/clock.c b/src/clock.c
> index e83e0f3..e44e112 100644
> --- a/src/clock.c
> +++ b/src/clock.c
> @@ -295,6 +295,7 @@ clock_update(void)
>      floppy_tick();
>      usb_check_event();
>      ps2_check_event();
> sercon_check_event();
>
> }
>
> // INT 08h System Timer ISR Entry Point
> diff --git a/src/serial.c b/src/serial.c
> index 74b91bb..d72dd01 100644
> --- a/src/serial.c
> +++ b/src/serial.c
> @@ -655,3 +655,258 @@ void sercon_enable(void)
>      outb(0x01, addr + 0x02);       // enable fifo
>      enable_vga_console();
>  }
>
> +/****************************************************************
> serial input
> ****************************************************************/
>
> +VARLOW u8 rx_buf[16];
> +VARLOW u8 rx_bytes;
>
> +VARLOW struct {
> char seq[4];
> u8 len;
> u8 scancode;
>
> +} termseq[] = {
> { .seq = "OP", .len = 2, .scancode = 0x3b },    // F1
> { .seq = "OQ", .len = 2, .scancode = 0x3c },    // F2
> { .seq = "OR", .len = 2, .scancode = 0x3d },    // F3
> { .seq = "OS", .len = 2, .scancode = 0x3e },    // F4
> { .seq = "[A", .len = 2, .scancode = 0xc8 },    // up
> { .seq = "[B", .len = 2, .scancode = 0xd0 },    // down
> { .seq = "[C", .len = 2, .scancode = 0xcd },    // right
> { .seq = "[D", .len = 2, .scancode = 0xcb },    // left
>
> +};

It would be preferable to mark constant data with "static VAR16"
instead of VARLOW.
> +#define FLAG_CTRL  (1<<0)
> +#define FLAG_SHIFT (1<<1)
>
> +VARLOW struct {
> u8 flags;
> u8 scancode;
>
> +} termchr[256] = {
> [ '1'        ] = { .scancode = 0x02,                      },

I think this table should be generated at runtime from
kbd.c:scan_to_keycode[].  Since it doesn't change at runtime,
malloc_fseg() / GET_GLOBAL() could be used instead of VARLOW.

[...]
> +static void sercon_sendkey(u8 scancode, u8 flags)
> +{
> if (flags & FLAG_CTRL)
>    process_key(0x1d);
> if (flags & FLAG_SHIFT)
>    process_key(0x2a);
> if (scancode & 0x80) {
>    process_key(0xe0);
>    process_key(scancode & ~0x80);
>    process_key(0xe0);
>    process_key(scancode);
> } else {
>    process_key(scancode);
>    process_key(scancode | 0x80);
> }
> if (flags & FLAG_SHIFT)
>    process_key(0x2a | 0x80);
> if (flags & FLAG_CTRL)
>    process_key(0x1d | 0x80);
>
> +}

Is it necessary to use process_key() here instead of injecting the
keycode directly with enqueue_key()?  I think the only difference is
the CONFIG_KBD_CALL_INT15_4F stuff and I'm not sure if anything
interesting needs that.
> +void VISIBLE16
> +sercon_check_event(void)

Does this need VISIBLE16?
> +{
> u16 addr = GET_LOW(sercon_port);
> u8 byte, scancode, flags, count = 0;
> int seq, chr, len;
> // check to see if there is a active serial port
> if (!addr)
>    return;
> if (inb(addr + SEROFF_LSR) == 0xFF)
>    return;
> // flush pending output
> sercon_flush_lazy();
> // read all available data
> while (inb(addr + SEROFF_LSR) & 0x01) {
>    byte = inb(addr + SEROFF_DATA);
>    if (GET_LOW(rx_bytes) < sizeof(rx_buf)) {
>        SET_LOW(rx_buf[rx_bytes], byte);
>        SET_LOW(rx_bytes, GET_LOW(rx_bytes) + 1);
>        count++;
>    }
> }
>
> +next_char:
> // no (more) input data
> if (!GET_LOW(rx_bytes))
>    return;
> // lookup escape sequences
> if (GET_LOW(rx_bytes) > 1 && GET_LOW(rx_buf[0]) == 0x1b) {
>    for (seq = 0; seq < ARRAY_SIZE(termseq); seq++) {
>        len = GET_LOW(termseq[seq].len);
>        if (GET_LOW(rx_bytes) < len + 1)
>            continue;
>        for (chr = 0; chr < len; chr++) {
>            if (GET_LOW(termseq[seq].seq[chr]) != GET_LOW(rx_buf[chr + 1]))
>                break;
>        }
>        if (chr == len) {
>            scancode = GET_LOW(termseq[seq].scancode);
>            sercon_sendkey(scancode, 0);
>            shiftbuf(len + 1);
>            goto next_char;
>        }
>    }
> }
> // Seems we got a escape sequence we didn't recognise.
> //  -> If we received data wait for more, maybe it is just incomplete.
> if (GET_LOW(rx_buf[0]) == 0x1b && count)
>    return;
> // Handle input as individual chars.
> chr = GET_LOW(rx_buf[0]);
> scancode = GET_LOW(termchr[chr].scancode);
> flags = GET_LOW(termchr[chr].flags);
> if (scancode)
>    sercon_sendkey(scancode, flags);
> shiftbuf(1);
> goto next_char;
>
> +}

If I understand correctly, most keys are sent on the serial port as
single bytes, but there are a few keys that are sent as multi-byte
sequences.  There's a lot of complexity to implement buffering for
that unusual case.  I wonder if the buffer could be avoided - I played
with it a little and came up with the below (totally untested).  I'm
not sure if it's an improvement.

-Kevin

u8 multibyte_read_pos VARLOW;
u8 multibyte_read_count VARLOW;

void
sercon_check_event(void)
{
    u16 addr = GET_LOW(sercon_port);
    ...

u8 mb_pos = GET_LOW(multibyte_read_pos);
    u8 mb_count = GET_LOW(multibyte_read_count);
    u8 mustflush = mb_count != 0;

// read and process data
    while (inb(addr + SEROFF_LSR) & 0x01) {
        u8 byte = inb(addr + SEROFF_DATA);
        if (mb_count) {
            // In a multi-byte sequence
            while (GET_GLOBAL(termseq[mb_pos].seq[mb_count-1]) != byte) {
                // Byte didn't match this sequence - find one that does
                mb_pos++;
                if (mb_pos >= ARRAY_SIZE(termseq)
                    || memcmp_far(GLOBAL_SEG, termseq[mb_pos-1].seq
                                  , GLOBAL_SEG, termseq[mb_pos].seq
                                  , mb_count-1) != 0)
                    // No match - must flush previusly queued keys
                    dump_multibyte_sequence(mb_pos, mb_count);
                    mb_pos = mb_count = mustflush = 0;
                    break;
                }
            }
            if (mb_count) {
                if (!GET_GLOBAL(termseq[mb_pos].seq[mb_count])) {
                    // sequence complete
                    sercon_sendkey(GET_GLOBAL(termseq[seq].scancode), 0);
                    mb_pos = mb_count = mustflush = 0;
                } else {
                    // Got another key in this sequence - continue checking
                    mb_count++;
                }
                continue;
            }
        }
        if (byte == 0x1b) {
            // Start multi-byte sequence check;
            mb_pos = 0;
            mb_count = 1;
            continue;
        }
        // Send normal key
        sercon_sendkey(GET_LOW(termchr[chr].scancode), GET_LOW(termchr[chr].flags));
        mustflush = 0;
    }

if (mustflush && mb_count) {
        // Too long to read multi-byte sequence - must flush
        dump_multibyte_sequence(mb_pos, mb_count);
        mb_count = mb_pos = 0;
    }
    SET_LOW(multibyte_read_count, mb_count);
    SET_LOW(multibyte_read_pos, mb_pos);
}

static void
dump_multibyte_sequence(u8 mb_pos, u8 mb_count)
{
    sercon_sendkey(GET_LOW(termchr[0x1b].scancode), GET_LOW(termchr[0x1b].flags));
    int i;
    for (i=0; i<mb_count-1; i++) {
        u8 key = GET_GLOBAL(termseq[mb_pos].seq[i]);
        sercon_sendkey(GET_LOW(termchr[key].scancode), GET_LOW(termchr[key].flags));
    }
}

[RFC PATCH 2/2] serial console, input

[Kevin O'Connor]

On Fri, Jul 01, 2016 at 01:07:39PM -0400, Kevin O'Connor wrote:
> If I understand correctly, most keys are sent on the serial port as
> single bytes, but there are a few keys that are sent as multi-byte
> sequences.  There's a lot of complexity to implement buffering for
> that unusual case.  I wonder if the buffer could be avoided - I played
> with it a little and came up with the below (totally untested).  I'm
> not sure if it's an improvement.

The version below might be slightly easier to understand (still
totally untested).

-Kevin

u8 multibyte_read_count VARLOW;
u8 multibyte_read_pos VARLOW;

void
sercon_check_event(void)
{
    u16 addr = GET_LOW(sercon_port);
    ...

// read and process data
    int readdata = 0;
    while (inb(addr + SEROFF_LSR) & 0x01) {
        u8 byte = inb(addr + SEROFF_DATA);
        readdata = 1;
        int ret = sercon_check_multibyte(byte);
        if (ret)
            // byte part of multi-byte sequence
            continue;
        if (byte == 0x1b) {
            // Start multi-byte sequence check
            SET_LOW(multibyte_read_count, 1);
            continue;
        }
        // Send normal key
        sercon_sendkey(GET_LOW(termchr[chr].scancode), GET_LOW(termchr[chr].flags));
    }

if (!readdata && GET_LOW(multibyte_read_count))
        // Too long to read multi-byte sequence - must flush
        dump_multibyte_sequence();
}

static int
sercon_check_multibyte(u8 byte)
{
    int mb_count = GET_LOW(multibyte_read_count);
    if (!mb_count)
        // Not in a multi-byte sequence
        return 0;
    int mb_pos = GET_LOW(multibyte_read_pos);
    while (GET_GLOBAL(termseq[mb_pos].seq[mb_count-1]) != byte) {
        // Byte didn't match this sequence - find a sequence that does
        mb_pos++;
        if (mb_pos >= ARRAY_SIZE(termseq)
            || memcmp_far(GLOBAL_SEG, termseq[mb_pos-1].seq
                          , GLOBAL_SEG, termseq[mb_pos].seq, mb_count-1) != 0)
            // No match - must flush previusly queued keys
            dump_multibyte_sequence();
            return 0;
        }
    }
    mb_count++;
    if (!GET_GLOBAL(termseq[mb_pos].seq[mb_count-1])) {
        // sequence complete - send key
        sercon_sendkey(GET_GLOBAL(termseq[seq].scancode), 0);
        mb_count = mb_pos = 0;
    }
    SET_LOW(multibyte_read_count, mb_count);
    SET_LOW(multibyte_read_pos, mb_pos);
    return 1;
}

static void
dump_multibyte_sequence(void)
{
    sercon_sendkey(GET_LOW(termchr[0x1b].scancode), GET_LOW(termchr[0x1b].flags));
    int i, mb_count = GET_LOW(multibyte_read_count);
    for (i=0; i<mb_count-1; i++) {
        u8 key = GET_GLOBAL(termseq[mb_pos].seq[i]);
        sercon_sendkey(GET_LOW(termchr[key].scancode), GET_LOW(termchr[key].flags));
    }
    SET_LOW(multibyte_read_count, 0);
    SET_LOW(multibyte_read_pos, 0);
}

[RFC PATCH 2/2] serial console, input

[Gerd Hoffmann]

Hi,
> > +#define FLAG_CTRL  (1<<0)
> > +#define FLAG_SHIFT (1<<1)
> >
> > +VARLOW struct {
> > u8 flags;
> > u8 scancode;
> >
> > +} termchr[256] = {
> > [ '1'        ] = { .scancode = 0x02,                      },
>
> I think this table should be generated at runtime from
> kbd.c:scan_to_keycode[].  Since it doesn't change at runtime,
> malloc_fseg() / GET_GLOBAL() could be used instead of VARLOW.

Ah, ok.  Didn't notice this one can be used for ascii -> scancode
lookups.

I'm wondering whenever it makes sense to build a reverse table.  We
could simply search scan_to_keycode[] instead.  Sure it is slower than a
table lookup, but still _way_ faster than any human can type, and we
would save some real-mode memory.
> Is it necessary to use process_key() here instead of injecting the
> keycode directly with enqueue_key()?

From a brief look at the code I think I can use enqueue_key directly
without anything breaking.  I'll give it a try.
> > +void VISIBLE16
> > +sercon_check_event(void)
>
> Does this need VISIBLE16?

Don't think so, I'll drop it.
> If I understand correctly, most keys are sent on the serial port as
> single bytes, but there are a few keys that are sent as multi-byte
> sequences.

Yes.  Single-byte us-ascii for letters and numbers.  multibyte sequences
starting with ESC (0x1b) for almost everything else (function keys,
cursor keys, home, end, pageup, pagedown, ...)

The list in the patch isn't complete, I've only added the most important
keys for initial testing.

You may also see input with the 8th bit set, which has a high chance to
be a multibyte sequence (utf8) too these days.  But I think we can
safely ignore any input with the 8th bit set, we can't map that to us
keyboard layout anyway even if we manage to correctly identify the
character typed.
> There's a lot of complexity to implement buffering for
> that unusual case.  I wonder if the buffer could be avoided - I played
> with it a little and came up with the below (totally untested).  I'm
> not sure if it's an improvement.

Hmm, so you avoid the buffer by maintaining an index into termseq and
matched byte count instead.  Hmm.  Yes, avoids the buffer and also a few
cpu cycles because you can continue searching where you left off.  I
find the code flow harder to follow though.

cheers,
  Gerd

[RFC PATCH 2/2] serial console, input

[Kevin O'Connor]

On Mon, Jul 04, 2016 at 11:16:45AM +0200, Gerd Hoffmann wrote:
> Hi,
> > > +#define FLAG_CTRL  (1<<0)
> > > +#define FLAG_SHIFT (1<<1)
> > >
> > > +VARLOW struct {
> > > u8 flags;
> > > u8 scancode;
> > >
> > > +} termchr[256] = {
> > > [ '1'        ] = { .scancode = 0x02,                      },
> >
> > I think this table should be generated at runtime from
> > kbd.c:scan_to_keycode[].  Since it doesn't change at runtime,
> > malloc_fseg() / GET_GLOBAL() could be used instead of VARLOW.
>
> Ah, ok.  Didn't notice this one can be used for ascii -> scancode
> lookups.
>
> I'm wondering whenever it makes sense to build a reverse table.  We
> could simply search scan_to_keycode[] instead.  Sure it is slower than a
> table lookup, but still _way_ faster than any human can type, and we
> would save some real-mode memory.

Agreed.

[...]
> > There's a lot of complexity to implement buffering for
> > that unusual case.  I wonder if the buffer could be avoided - I played
> > with it a little and came up with the below (totally untested).  I'm
> > not sure if it's an improvement.
>
> Hmm, so you avoid the buffer by maintaining an index into termseq and
> matched byte count instead.  Hmm.  Yes, avoids the buffer and also a few
> cpu cycles because you can continue searching where you left off.  I
> find the code flow harder to follow though.

Right.  Yeah, also not sure it's an improvement.

Does the original code flush the multi-byte sequence on a timeout?  I
suspect it is important that one can hit ESC without having to type
another key.  Also, I'd prefer to avoid backwards gotos if possible.

-Kevin

[RFC PATCH 2/2] serial console, input

[Gerd Hoffmann]

Hi,
> Does the original code flush the multi-byte sequence on a timeout?  I
> suspect it is important that one can hit ESC without having to type
> another key.  Also, I'd prefer to avoid backwards gotos if possible.

Yes, sort of.  If it didn't match an escape sequence and hasn't seen
additional data on this particular sercon_handle_event call it goes on
interpret the buffer content as single byte.  So, effectively the
timeout is the clock_update() call interval.

cheers,
  Gerd

[Gerd Hoffmann]

Hi,
> known issues / todo list:
> isolinux/pxelinux seems to have trouble with this.

Found this one.  video_* fields in BDA need accurate info for syslinux
to work.

Also added cp437 -> utf8 mapping for proper box character drawings.

Stuff is here: https://www.kraxel.org/cgit/seabios/log/?h=serial

/me goes disappear into the weekend now.

cheers,
  Gerd