Re: [SeaBIOS] [PATCH 3/3] add serial console support

[Kevin O'Connor]

On Mon, Jul 04, 2016 at 10:39:54PM +0200, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann

Not sure if this is still an RFC.  I think it needs to have a Kconfig
option.  It should probably also have runtime enable option.

[...]
> --- a/Makefile
> +++ b/Makefile
> @@ -29,7 +29,7 @@ LD32BIT_FLAG:=-melf_i386
>
> # Source files
>  SRCBOTH=misc.c stacks.c output.c string.c block.c cdrom.c disk.c mouse.c kbd.c \
> system.c serial.c clock.c resume.c pnpbios.c vgahooks.c pcibios.c apm.c \
> system.c serial.c sercon.c clock.c resume.c pnpbios.c vgahooks.c pcibios.c apm.c \

How about console.c instead of sercon.c?

[...]
> --- /dev/null
> +++ b/src/sercon.c
> @@ -0,0 +1,545 @@

There should be a copyright statement.

[...]
> +VARLOW u8 sercon_char[8];
> +VARLOW u8 sercon_attr[8];

Out of curiosity, is 8 needed for effective "uncluttering" or is it an
optimization?  (That is, would a size of 1 suffice for the common
case?)

[...]
> +static void sercon_lazy_putchar(u8 chr, u8 attr, u8 teletype)
> +{
> u8 idx;
> if (cursor_pos_row() != GET_LOW(sercon_row_last) ||
>    cursor_pos_col() <  GET_LOW(sercon_col_last) ||
>    cursor_pos_col() >= GET_LOW(sercon_col_last) + ARRAY_SIZE(sercon_char)) {
>    sercon_lazy_flush();
> }
> idx = cursor_pos_col() - GET_LOW(sercon_col_last);
> SET_LOW(sercon_char[idx], chr);
> if (teletype)
>    cursor_pos_set(cursor_pos_row(),
>                   cursor_pos_col()+1);

This doesn't handle end of column wrapping properly.
> else
>    SET_LOW(sercon_attr[idx], attr);
>
> +}

FYI, once sercon_1013 is implemented then 'teletype' is not mutually
exclusive with 'use_attr'.

[...]
> +static void sercon_1000(struct bregs *regs)
> +{
> int mode, rows, cols;
> mode = regs->al;
> switch (mode) {
> case 0x03:
> default:
>    cols = 80;
>    rows = 25;
>    regs->al = 0x30;
> }
> SET_LOW(sercon_col_last, 0);
> SET_LOW(sercon_row_last, 0);
> cursor_pos_set(0, 0);
> SET_BDA(video_mode, mode);
> SET_BDA(video_cols, cols);
> SET_BDA(video_rows, rows-1);
> SET_BDA(cursor_type, 0x0007);
> sercon_term_reset();
> sercon_term_clear_screen();

Should only clear screen if high-bit of regs->al not set.
> +/* Set text-mode cursor shape */
> +static void sercon_1001(struct bregs *regs)
> +{
> /* dummy (add show/hide cursor?) */
>
> +}

Not sure if hiding the cursor is necessary, but it should
SET_BDA(cursor_type, regs->cx)

[...]
> +static void sercon_1003(struct bregs *regs)
> +{
> regs->ax = 0;
> regs->ch = 0;
> regs->cl = 7;

regs->cx = GET_BDA(cursor_type)

Not sure why regs->ax is cleared.

[...]
> +static void sercon_1009(struct bregs *regs)
> +{
> u16 count = regs->cx;
> if (count == 1) {
>    sercon_lazy_putchar(regs->al, regs->bl, 0);
> } else if (regs->al == 0x20 &&
>           video_rows() * video_cols() == count &&
>           cursor_pos_row() == 0 &&
>           cursor_pos_col() == 0) {
>    /* override everything with spaces -> this is clear screen */
>    sercon_lazy_flush();
>    sercon_set_attr(regs->bl);
>    sercon_term_clear_screen();
> } else {
>    sercon_lazy_flush();
>    sercon_set_attr(regs->bl);
>    while (count) {
>        sercon_putchar(regs->al);

At a minimum that should be sercon_print_utf8(), though could
sercon_lazy_putchar() be used?

[...]
> +static void sercon_100e(struct bregs *regs)
> +{
> switch (regs->al) {
> case 7:
>    // beep
>    break;

sercon_putchar(0x07)
> +void VISIBLE16
> +sercon_10(struct bregs *regs)
> +{
> if (!GET_LOW(sercon_port))
>    return;
> switch (regs->ah) {
> case 0x00: sercon_1000(regs); break;
> case 0x01: sercon_1001(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:   sercon_10XX(regs); break;
> }
>
> +}

I noticed sercon_1007, sercon_100a, and sercon_1013 aren't
implemented.  I guess they aren't used frequently, but they've been
around since the original IBM MDA adapter.

[...]
> +void VISIBLE16
> +sercon_check_event(void)

Not sure VISIBLE16 is needed.
> +{
> u16 addr = GET_LOW(sercon_port);
> u16 keycode;
> u8 byte, 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_lazy_flush();
> // 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_GLOBAL(termseq[seq].len);
>        if (GET_LOW(rx_bytes) < len + 1)
>            continue;
>        for (chr = 0; chr < len; chr++) {
>            if (GET_GLOBAL(termseq[seq].seq[chr]) != GET_LOW(rx_buf[chr + 1]))
>                break;
>        }
>        if (chr == len) {
>            enqueue_key(GET_GLOBAL(termseq[seq].keycode));
>            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]);
> keycode = ascii_to_keycode(chr);
> if (keycode)
>    enqueue_key(keycode);
> shiftbuf(1);
> goto next_char;
>
> +}

I would prefer if we could avoid backwards gotos.

In general, looks good.
-Kevin