Re: [SeaBIOS] [PATCH v2 4/6] add serial console support

[Kevin O'Connor]

On Thu, Sep 14, 2017 at 09:08:20AM +0200, Gerd Hoffmann wrote:
> Redirect int10 calls to serial console output.
> Parse serial input and queue key events.
> The serial console can work both as primary display
> and in parallel to another vga display (splitmode).

Thanks.  Looks good.  I have a few minor comments, but they can all be
fixed post commit.  (Let me know if you prefer otherwise.)
> --- 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 \
> cp437.c \

Line wrap.
> --- a/src/util.h
> +++ b/src/util.h
> @@ -234,6 +234,9 @@ void code_mutable_preinit(void);
>  // serial.c
>  void serial_setup(void);
>  void lpt_setup(void);
> +void sercon_10(struct bregs *regs);
> +void sercon_setup(u16 iobase);
> +void sercon_check_event(void);

These should be in a "// sercon.c" section (which should be above the
"serial.c" block to keep the alphabetical ordering).  Also,
sercon_10() doesn't need to be exported here.

[...]
> --- a/src/optionroms.c
> +++ b/src/optionroms.c
> @@ -12,6 +12,7 @@
>  #include "hw/pcidevice.h" // foreachpci
>  #include "hw/pci_ids.h" // PCI_CLASS_DISPLAY_VGA
>  #include "hw/pci_regs.h" // PCI_ROM_ADDRESS
> +#include "hw/serialio.h" // PORT_SERIAL1
>  #include "malloc.h" // rom_confirm
>  #include "output.h" // dprintf
>  #include "romfile.h" // romfile_loadint
> @@ -404,6 +405,8 @@ struct rom_header *VgaROM;
>  void
>  vgarom_setup(void)
>  {
> u16 ret, iobase = 0;
> if (! CONFIG_OPTIONROMS)
>        return;
>
> @@ -432,11 +435,22 @@ vgarom_setup(void)
>      run_file_roms("vgaroms/", 1, NULL);
>      rom_reserve(0);
> if (rom_get_last() == BUILD_ROM_START)
>
> ret = romfile_loadint("etc/sercon-enable", 0);
>
> if (ret)
>    iobase = PORT_SERIAL1;
>
> if (rom_get_last() == BUILD_ROM_START) {
>     // No VGA rom found
>    if (iobase) {
>        sercon_setup(iobase);
>        enable_vga_console();
>    }
>    return;
>
> }
>
> VgaROM = (void*)BUILD_ROM_START;
>
> if (iobase)
>    sercon_setup(iobase);
>
> enable_vga_console();
>
> }

It would be preferable to not implement sercon logic in optionroms.c.
Instead, the call to enable_vga_console() can be moved into
post.c:maininit() and both it and sercon_setup() can be called
unconditionally there.

[...]
> +void VISIBLE16
> +sercon_10(struct bregs *regs)
> +{

[...]
> --- a/src/romlayout.S
> +++ b/src/romlayout.S
> @@ -414,6 +414,53 @@ __csm_return:

[...]
>    // call sercon_10
>    popl %eax
>    popw %ds
>
> +2:      pushl $sercon_10

In keeping with the other assembler entry points, it would be
preferable to use the naming entry_sercon / handle_sercon().

-Kevin