[Qemu-devel] Re: [PATCH][SEABIOS] Move qemu config port access functions into separate file.

[Gleb Natapov]

On Mon, Sep 14, 2009 at 08:08:24PM -0400, Kevin O'Connor wrote:
> On Mon, Sep 14, 2009 at 03:51:41PM +0300, Gleb Natapov wrote:
> > Move qemu config code from smbios.c to its own files. Add support for
> > -boot menu=on|off qemu option.
> 
> Hi Gleb,
> 
> A couple of comments:
> 
> >      // Allow user to modify BCV/IPL order.
> > -    interactive_bootmenu();
> > +    if (qemu_cfg_show_boot_menu())
> > +        interactive_bootmenu();
> 
> Can you move this test into interactive_bootmenu()?  (For non-qemu
> users the flow control looks odd otherwise.)
> 
OK. The flow control will not go away though just move to other place so
non-qemu user will see odd flow control anyway :)

> > --- /dev/null
> > +++ b/src/pv.c
> 
> What is "pv"?  How about "qemu-cfg.c"?
> 
pv == ParaVirtualization. I want to put non qemu specific thing there
to. For instance I put kvm_para_available() function there that checks
cpuid signature. I want to use it to replace if(CONFIG_KVM) in
ram_probe().

> > +void qemu_cfg_port_probe(void)
> > +{
> > +    char *sig = "QEMU";
> > +    int i;
> > +
> > +    qemu_cfg_present = 1;
> > +
> > +    qemu_cfg_select(QEMU_CFG_SIGNATURE);
> > +
> > +    for (i = 0; i < 4; i++)
> > +        if (inb(QEMU_CFG_DATA_PORT) != sig[i]) {
> > +            qemu_cfg_present = 0;
> > +            break;
> > +        }
> > +    dprintf(4, "qemu_cfg_present=%d\n", qemu_cfg_present);
> > +}
> 
> This needs to have a "if (! CONFIG_COREBOOT) return;" - as the current
> check for qemu is not safe on real hardware.
> 
Will add.

> Otherwise it looks good to me.
> 
> As an aside, it would be good to have a conversation on general BIOS
> configuration options.  These types of settings are going to be useful
> on real hardware also - it would be nice to come up with a scheme that
> would work on qemu and coreboot.  Maybe something like
> get_config_u32("ShowBootMenu") - where on qemu it would get the info
> from the qemu port but on coreboot it would pull the setting from the
> coreboot flash filesystem.
> 
Lets have conversation now. Sounds useful to me. Do you want to use
strings as option names though? They add to BIOS image size and it is
limited, no?

--
                        Gleb.