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

From: Gerd Hoffmann <kraxel@redhat.com>
To: Kevin O'Connor <kevin@koconnor.net>
Cc: seabios@seabios.org, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [SeaBIOS] [PATCH 3/3] add serial console support
Date: Tue, 05 Jul 2016 17:07:08 +0200	[thread overview]
Message-ID: <1467731228.12358.45.camel@redhat.com> (raw)
In-Reply-To: <20160705143041.GB2939@morn.lan>

On Di, 2016-07-05 at 10:30 -0400, Kevin O'Connor wrote:
> On Mon, Jul 04, 2016 at 10:39:54PM +0200, Gerd Hoffmann wrote:
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> 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.

Yes, still RfC.  I want have feature parity with sgabios before merging
this.  Big missing piece is output to both vga and serial, and various
little things like implementing the missing interrupts.

runtime option is there meanwhile, using NOGRAPHIC as suggested by
paolo:

https://www.kraxel.org/cgit/seabios/commit/?h=serial&id=21e51b3e0a37733e8cb5236f6ca1a91ae166791a

compile time option will be added too.

I also hacked up a patch to send output to both vga + serial:

https://www.kraxel.org/cgit/seabios/commit/?h=serial&id=3afd7b8bb96126b00989f3ae09f451bbec4f00f7

Not working stable though, seems to corrupt memory, not sure why.
I'm storing the vgabios int10 vector at 0x5f, then chain-call into
vgabios via "int 5f" instruction.  Anything obviously wrong with that?

> >  # 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?

sercon.c is consistent with the sercon_* naming of most functions.

> [...]
> > +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?)

I think size 1 (maybe 2) would catch (almost?) all cases too.

I've started with a complete line (80), then decided to try something
smaller so the flushing is triggered more often and I can easily see
whenever it works as intended or not.  So I removed the '0', '8' left
and it still looks that way ;)

I've also tried to use the sgabios approach of not buffering any
characters and be only lazy on cursor updates instead.  That didn't work
well with ipxe though which uses int10/09 (set char + attr, don't move
cursor) + int10/03 (set char only, move cursor) combination to print
colored + bold characters.

> > +    } 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?

The use patterns I've seen are either single chars (which already go
through sercon_lazy_putchar) or bulky output to fill/clear screen areas
much larger than our 8 char buffer, so using sercon_lazy_putchar looks
pointless for those.

I'll go over the other comments too and fix up things for the next
round.

cheers,
  Gerd