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

[Gleb Natapov <gleb@redhat.com>]

On Thu, Oct 01, 2009 at 08:51:27PM -0400, Kevin O'Connor wrote:
> On Thu, Oct 01, 2009 at 06:35:55PM +0200, Gleb Natapov wrote:
> > > 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.
> > > 
> > I started to implement this approach, but found a serious disadvantage:
> > What if config option is not known to qemu or coreboot? What is it
> > present only in qemu and meaningful default behaviour is required
> > for coreboot? Like ShowBootMenu for instance. We want to return qemu
> > setting or coreboot setting or 1 if neither is present.
> 
> I think passing in a default parameter like:
> get_config_u32("ShowBootMenu", 1) would work (return the config value
> or "1" if not found).
> 
Brrr, ugly.

> [...]
> > --- /dev/null
> > +++ b/src/cbcfg.c
> > @@ -0,0 +1,13 @@
> > +#include "config.h"
> > +#include "util.h"
> > +#include "cfg.h"
> > +
> > +void cfg_get_uuid(u8 *uuid)
> > +{
> > +    memset(uuid, 0, 16);
> > +}
> > +
> > +int cfg_show_boot_menu(void)
> > +{
> > +    return 1;
> > +}
> 
> What this would end up looking like is:
> 
> int cfg_show_boot_menu(void)
> {
>     return get_cbfs_config_u32("ShowBootMenu", 1);
> }
> 
Something like this will be better:

int cfg_show_boot_menu(void)
{
	if (cbf_config_exists("ShowBootMenu"))
		return get_cbfs_config_u32("ShowBootMenu");
	else
		return 1;
}

The default value logic may be more complex than this. For instance:
int cfg_show_boot_menu(void)
{
	if (cbf_config_exists("ShowBootMenu"))
		return get_cbfs_config_u32("ShowBootMenu");
	else if (cbf_config_exists("DefaultBootDevice"))
		return 0;
	else
		return 1;
}

The other way to achieve this flexibility is to have interface like
bool get_config_u32(const char *name, u32 *val). This will return true
if name was found and val is meaningful. Caller will implement fallback
to default.
 
> Having to write these wrapper functions is tedious, which is why it
> would be nice if I could get a name/value pair directly from qemu.
> 
And proposed get_config_u32() will look like this:
u32 get_config_u32(const char *name, u32 default)
{
	if (COREBOOT)
		return get_cbfs_config_u32("ShowBootMenu", 1);
	else
		return get_qemu_config_u32("ShowBootMenu", 1);
}
just another kind of wrapper. And get_qemu_config_u32 will have to map
string to config id since we are trying to adapt qemu to coreboot way.

> If qemu can provide a "stream" with a text config file in it, that's
> okay too.  Basically, that's what I'm thinking of doing on coreboot.
> Something like:
> 
> ===============================
> ShowBootMenu=1
> BootMenuDelayMS=5000
> ATA1-0-translation=2
> DefaultBootDevice=2
> ===============================
> 
This kind of interface doesn't make any sense to qemu. Qemu doesn't have
shared memory with a guest to store config as a text file like coreboot does.
That is why qemu chose to provide name/value interface. Now you are saying since
we have this text file in coreboot that will have to be parsed to get
name/value interface from it lets do the same for qemu. But this is just
because you want to do abstraction on a wrong level. Qemu already
provides you with name/value so why do you want to transform it to plane
text and then pars to name/value back. Doesn't make any sense.

What makes sense it functional interface:
 Does Boot menu should be shown?
 What drive to boot from by default?
 Load additional ACPI/SMBIOS tables.
And coreboot/qemu will implement them in a platform specific ways.

--
			Gleb.