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

[Kevin O'Connor]

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.)

> --- /dev/null
> +++ b/src/pv.c

What is "pv"?  How about "qemu-cfg.c"?

> +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.

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.

Thanks,
-Kevin