Re: [SeaBIOS] [PATCH v2] qemu: fast boot when linuxboot optionrom is used

[Kevin O'Connor]

On Fri, Nov 30, 2018 at 03:27:21PM +0100, Stefano Garzarella wrote:
> On Fri, Nov 30, 2018 at 2:06 PM Gerd Hoffmann  wrote:
> > > > Note: All these tests are done with CONFIG_DEBUG_LEVEL=0, using
> > > > CONFIG_DEBUG_LEVEL=1 the boot time grows up to 24 ms, maybe we should
> > > > put CONFIG_DEBUG_LEVEL=0 in the SeaBIOS configuration used in QEMU.
> > >
> > > I think the main seabios binary should have CONFIG_DEBUG_LEVEL=1 as it
> > > helps with debug reports.  I suppose an additional binary could be
> > > made for those looking for the fastest possible speed.  (The sole cost
> > > of the debugging is the additional hardware accesses that results from
> > > those debug messages.)
> >
> > The qemu debugcon (CONFIG_DEBUG_IO) is detecable at runtime, it returns
> > 0xe9 on port reads.  So we should be able to skip that too.  IIRC it
> > isn't *that* straightforward as seabios is initially mapped read/only so
> > a simple probe-on-first-putchar, then cache the result in a variable
> > doesn't work.  We could probe after make_bios_writable though which
> > should still avoid printing most of the messages.
>
> Great! I just tried the patch below and it works as you said. If I
> don't have debugcon in QEMU the outs are avoided and the speed is not
> bad (11.4 ms).
> If I have debugcon (iobase=0x402), I can see the output.
>
> Do you think can be an acceptable trade-off?

Makes sense to me.
> index 0770c47..31c080e 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -121,6 +121,10 @@ qemu_preinit(void)
>          kvm_detect();
>      }
> // Detect qemu debugcon
> if (inb(GET_GLOBAL(DebugOutputPort)) != QEMU_DEBUGCON_READBACK)
>    DebugOutputPort = 0;

As a minor quibble, this needs to be "if (CONFIG_DEBUG_IO && ...)" to
ensure it gets compiled out if debug io is not in use.  Also, I think
it would be preferable to introduce a serial_debug_postram_preinit()
method in src/hw/serialio.c that does this check.
> --- a/src/hw/serialio.c
> +++ b/src/hw/serialio.c
> @@ -107,7 +107,7 @@ u16 DebugOutputPort VARFSEG = 0x402;
>  void
>  qemu_debug_putc(char c)
>  {
> if (CONFIG_DEBUG_IO && runningOnQEMU())
> if (CONFIG_DEBUG_IO && runningOnQEMU() && GET_GLOBAL(DebugOutputPort))
>     // Send character to debug port.
>     outb(c, GET_GLOBAL(DebugOutputPort));
>
> }

As a minor nit, it would be preferable to invoke
GET_GLOBAL(DebugOutputPort) only once in the success case.

-Kevin

[PATCH v2] qemu: fast boot when linuxboot optionrom is used

[Stefano Garzarella]

On Fri, Nov 30, 2018 at 8:36 PM Kevin O'Connor  wrote:
> On Fri, Nov 30, 2018 at 03:27:21PM +0100, Stefano Garzarella wrote:
> > On Fri, Nov 30, 2018 at 2:06 PM Gerd Hoffmann  wrote:
> > > > > Note: All these tests are done with CONFIG_DEBUG_LEVEL=0, using
> > > > > CONFIG_DEBUG_LEVEL=1 the boot time grows up to 24 ms, maybe we should
> > > > > put CONFIG_DEBUG_LEVEL=0 in the SeaBIOS configuration used in QEMU.
> > > >
> > > > I think the main seabios binary should have CONFIG_DEBUG_LEVEL=1 as it
> > > > helps with debug reports.  I suppose an additional binary could be
> > > > made for those looking for the fastest possible speed.  (The sole cost
> > > > of the debugging is the additional hardware accesses that results from
> > > > those debug messages.)
> > >
> > > The qemu debugcon (CONFIG_DEBUG_IO) is detecable at runtime, it returns
> > > 0xe9 on port reads.  So we should be able to skip that too.  IIRC it
> > > isn't *that* straightforward as seabios is initially mapped read/only so
> > > a simple probe-on-first-putchar, then cache the result in a variable
> > > doesn't work.  We could probe after make_bios_writable though which
> > > should still avoid printing most of the messages.
> >
> > Great! I just tried the patch below and it works as you said. If I
> > don't have debugcon in QEMU the outs are avoided and the speed is not
> > bad (11.4 ms).
> > If I have debugcon (iobase=0x402), I can see the output.
> >
> > Do you think can be an acceptable trade-off?
>
> Makes sense to me.
> > index 0770c47..31c080e 100644
> > --- a/src/fw/paravirt.c
> > +++ b/src/fw/paravirt.c
> > @@ -121,6 +121,10 @@ qemu_preinit(void)
> >          kvm_detect();
> >      }
> > // Detect qemu debugcon
> > if (inb(GET_GLOBAL(DebugOutputPort)) != QEMU_DEBUGCON_READBACK)
> >    DebugOutputPort = 0;
>
> As a minor quibble, this needs to be "if (CONFIG_DEBUG_IO && ...)" to
> ensure it gets compiled out if debug io is not in use.  Also, I think
> it would be preferable to introduce a serial_debug_postram_preinit()
> method in src/hw/serialio.c that does this check.
> > --- a/src/hw/serialio.c
> > +++ b/src/hw/serialio.c
> > @@ -107,7 +107,7 @@ u16 DebugOutputPort VARFSEG = 0x402;
> >  void
> >  qemu_debug_putc(char c)
> >  {
> > if (CONFIG_DEBUG_IO && runningOnQEMU())
> > if (CONFIG_DEBUG_IO && runningOnQEMU() && GET_GLOBAL(DebugOutputPort))
> >     // Send character to debug port.
> >     outb(c, GET_GLOBAL(DebugOutputPort));
> >
> > }
>
> As a minor nit, it would be preferable to invoke
> GET_GLOBAL(DebugOutputPort) only once in the success case.
>
> -Kevin

Hi Kevin,
thank you very much, I sent a patch following your suggestions.

Cheers,
Stefano