| Message ID | 20091001163555.GV9832@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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). [...] > --- /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); } 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. 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 =============================== -Kevin
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.
On Fri, Oct 02, 2009 at 04:03:11PM +0200, Gleb Natapov wrote: > > 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); > } It would look like: u32 get_config_u32(const char *name, u32 default) { if (CONFIG_COREBOOT) return get_cbfs_config_u32(name, default); else return get_qemu_config_u32(name, default); } It is a wrapper but there would be just one instead of one wrapper per config option. > > 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. [...] > 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. I agree it's not compelling for qemu - I'm bringing it up as a possibility. As above, I don't think it would require shared memory - the existing "stream" interface would be sufficient. > That is why qemu chose to provide name/value interface. I'm a bit confused here. As near as I can tell, qemu has an int_id->value mapping. What I'd like to see is a string_name->value mapping. The int_id isn't flexible because I can't share ids across products. If qemu is willing to add this to the backend (ie, the emulator passes the info to the bios as string_name->value), then great. The actual specifics of how it is done isn't of great concern to me. If not, then lets go forward with the basic int_id support. The internal API for coreboot can be figured out when the coreboot config support is added. -Kevin
On Fri, Oct 02, 2009 at 12:52:13PM -0400, Kevin O'Connor wrote: > On Fri, Oct 02, 2009 at 04:03:11PM +0200, Gleb Natapov wrote: > > > 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); > > } > > It would look like: > u32 get_config_u32(const char *name, u32 default) > { > if (CONFIG_COREBOOT) > return get_cbfs_config_u32(name, default); > else > return get_qemu_config_u32(name, default); > } Correct, wrong cut&paste on my part ;) > > It is a wrapper but there would be just one instead of one wrapper per > config option. > How many config options do you expect? And as I said having function like cfg_show_boot_menu() allow to have more flexible defaults handling. If we will go this route I prefer bool get_config_u32(const char *name, u32 *val) interface. And will still need common interface for table loading. I prefer to implement qemu way in qemu_table_load() and don't jump through the hoops trying to make it look like coreboot. > > > 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. > [...] > > 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. > > I agree it's not compelling for qemu - I'm bringing it up as a > possibility. As above, I don't think it would require shared memory - > the existing "stream" interface would be sufficient. Qemu already has interface. That is the interface that we have to use. Discussing purely theoretical possibilities just distract us from searching for solution. > > > That is why qemu chose to provide name/value interface. > > I'm a bit confused here. As near as I can tell, qemu has an > int_id->value mapping. What I'd like to see is a string_name->value What is the difference? Why do you care if it is int->value or string->value? I can easily map string to int in seabios qemu config functions so for the rest of seabios it will look just like string->value. I don't see the point of doing this though. > mapping. The int_id isn't flexible because I can't share ids across > products. What do you mean? What products? > > If qemu is willing to add this to the backend (ie, the emulator passes > the info to the bios as string_name->value), then great. The actual Qemu is not (or shouldn't be) developed in a lockstep with a bios. Bios should be flexible enough to support older or newer qemus. Seabios should treat qemu just like any other HW mobo. Even if we add something to qemu now we want to support older qemus too. > specifics of how it is done isn't of great concern to me. If not, > then lets go forward with the basic int_id support. The internal API > for coreboot can be figured out when the coreboot config support is > added. > You mean like in my first patch? I can resend it with all other points addressed. -- Gleb.
Hi Gleb, On Fri, Oct 02, 2009 at 08:10:10PM +0200, Gleb Natapov wrote: > How many config options do you expect? I expect about a dozen. > > > That is why qemu chose to provide name/value interface. > > I'm a bit confused here. As near as I can tell, qemu has an > > int_id->value mapping. What I'd like to see is a string_name->value > What is the difference? Why do you care if it is int->value or > string->value? We seem to have not been effectively communicating. The original patch you sent has a simple and sane abstraction around the existing qemu int->value configuration system. I liked it (with the few comments I sent earlier), and think it should be committed. I was raising the possibility/hope that qemu could be extended with a string->value system for passing parameters. Such a system would simplify SeaBIOS a little. I'm not familiar with qemu development, and if this was a "theoretical" distraction, then I'm sorry for raising it. To answer your question, the reason I prefer string->value is that it allows me to use the same namespace for both coreboot and qemu. Configuration enhancements made for one environment can automatically be utilized by the other. >I can easily map string to int in seabios qemu config > functions so for the rest of seabios it will look just like string->value. > I don't see the point of doing this though. Agreed - that would not be productive. > > If qemu is willing to add this to the backend (ie, the emulator passes > > the info to the bios as string_name->value), then great. The actual > Qemu is not (or shouldn't be) developed in a lockstep with a bios. Bios > should be flexible enough to support older or newer qemus. Seabios > should treat qemu just like any other HW mobo. Even if we add something to > qemu now we want to support older qemus too. Also agreed. As an example of this, qemu is currently passing some config parameters via nvram - in a "theoretical" way, I'd like to see these normalized to name->value - but even if that did happen SeaBIOS would need to continue to support the old method. > > specifics of how it is done isn't of great concern to me. If not, > > then lets go forward with the basic int_id support. The internal API > > for coreboot can be figured out when the coreboot config support is > > added. > > > You mean like in my first patch? I can resend it with all other points > addressed. Yes. Thanks. -Kevin
diff --git a/Makefile b/Makefile index c4016e8..8d5c913 100644 --- a/Makefile +++ b/Makefile @@ -10,11 +10,20 @@ VERSION=pre-0.4.3-$(shell date +"%Y%m%d_%H%M%S")-$(shell hostname) # Output directory OUT=out/ +# Configure as a coreboot payload. +COREBOOT=y + +ifdef COREBOOT +CFGSRC=cbcfg.c +else +CFGSRC=pv.c +endif + # Source files SRCBOTH=output.c util.c block.c floppy.c ata.c misc.c mouse.c kbd.c pci.c \ serial.c clock.c pic.c cdrom.c ps2port.c smp.c resume.c \ pnpbios.c pirtable.c vgahooks.c pmm.c ramdisk.c \ - usb.c usb-uhci.c usb-hid.c + usb.c usb-uhci.c usb-hid.c $(CFGSRC) SRC16=$(SRCBOTH) system.c disk.c apm.c pcibios.c font.c SRC32=$(SRCBOTH) post.c shadow.c memmap.c coreboot.c boot.c \ acpi.c smm.c mptable.c smbios.c pciinit.c optionroms.c mtrr.c \ diff --git a/src/boot.c b/src/boot.c index 7b74007..71d6e27 100644 --- a/src/boot.c +++ b/src/boot.c @@ -12,6 +12,7 @@ #include "bregs.h" // struct bregs #include "boot.h" // struct ipl_s #include "cmos.h" // inb_cmos +#include "cfg.h" struct ipl_s IPL; @@ -206,7 +207,7 @@ menu_show_cbfs(struct ipl_entry_s *ie, int menupos) static void interactive_bootmenu() { - if (! CONFIG_BOOTMENU) + if (! CONFIG_BOOTMENU || ! cfg_show_boot_menu()) return; while (get_keystroke(0) >= 0) diff --git a/src/cbcfg.c b/src/cbcfg.c new file mode 100644 index 0000000..56b6076 --- /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; +} diff --git a/src/cfg.h b/src/cfg.h new file mode 100644 index 0000000..1f6b488 --- /dev/null +++ b/src/cfg.h @@ -0,0 +1,6 @@ +#ifndef __CFG_H +#define __CFG_H + +void cfg_get_uuid(u8 *uuid); +int cfg_show_boot_menu(void); +#endif diff --git a/src/post.c b/src/post.c index f72e134..e8ae3f0 100644 --- a/src/post.c +++ b/src/post.c @@ -20,6 +20,7 @@ #include "mptable.h" // mptable_init #include "boot.h" // IPL #include "usb.h" // usb_setup +#include "pv.h" void __set_irq(int vector, void *loc) @@ -184,6 +185,8 @@ post() serial_setup(); mouse_setup(); + qemu_cfg_port_probe(); + init_bios_tables(); boot_setup(); diff --git a/src/pv.c b/src/pv.c new file mode 100644 index 0000000..fa57b5b --- /dev/null +++ b/src/pv.c @@ -0,0 +1,67 @@ +#include "config.h" +#include "ioport.h" +#include "pv.h" + +int qemu_cfg_present; + +static void +qemu_cfg_select(u16 f) +{ + outw(f, PORT_QEMU_CFG_CTL); +} + +static void +qemu_cfg_read(u8 *buf, int len) +{ + while (len--) + *(buf++) = inb(PORT_QEMU_CFG_DATA); +} + +static void +qemu_cfg_read_entry(void *buf, int e, int len) +{ + qemu_cfg_select(e); + qemu_cfg_read(buf, len); +} + +void qemu_cfg_port_probe(void) +{ + char *sig = "QEMU"; + int i; + + if (CONFIG_COREBOOT) + return; + + qemu_cfg_present = 1; + + qemu_cfg_select(QEMU_CFG_SIGNATURE); + + for (i = 0; i < 4; i++) + if (inb(PORT_QEMU_CFG_DATA) != sig[i]) { + qemu_cfg_present = 0; + break; + } + dprintf(4, "qemu_cfg_present=%d\n", qemu_cfg_present); +} + +void cfg_get_uuid(u8 *uuid) +{ + memset(uuid, 0, 16); + + if (!qemu_cfg_present || !CONFIG_UUID_BACKDOOR) + return; + + qemu_cfg_read_entry(uuid, QEMU_CFG_UUID, 16); +} + +int cfg_show_boot_menu(void) +{ + u16 v; + if (!qemu_cfg_present) + return 1; + + qemu_cfg_read_entry(&v, QEMU_CFG_BOOT_MENU, sizeof(v)); + + return v; +} + diff --git a/src/pv.h b/src/pv.h new file mode 100644 index 0000000..632a29c --- /dev/null +++ b/src/pv.h @@ -0,0 +1,42 @@ +#ifndef __PV_H +#define __PV_H + +#include "util.h" + +/* This CPUID returns the signature 'KVMKVMKVM' in ebx, ecx, and edx. It + * should be used to determine that a VM is running under KVM. + */ +#define KVM_CPUID_SIGNATURE 0x40000000 + +static inline int kvm_para_available(void) +{ + unsigned int eax, ebx, ecx, edx; + char signature[13]; + + cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx); + memcpy(signature + 0, &ebx, 4); + memcpy(signature + 4, &ecx, 4); + memcpy(signature + 8, &edx, 4); + signature[12] = 0; + + if (strcmp(signature, "KVMKVMKVM") == 0) + return 1; + + return 0; +} + +#define QEMU_CFG_SIGNATURE 0x00 +#define QEMU_CFG_ID 0x01 +#define QEMU_CFG_UUID 0x02 +#define QEMU_CFG_NUMA 0x0d +#define QEMU_CFG_BOOT_MENU 0x0e +#define QEMU_CFG_MAX_CPUS 0x0f +#define QEMU_CFG_ARCH_LOCAL 0x8000 +#define QEMU_CFG_ACPI_TABLES (QEMU_CFG_ARCH_LOCAL + 0) +#define QEMU_CFG_SMBIOS_ENTRIES (QEMU_CFG_ARCH_LOCAL + 1) + +extern int qemu_cfg_present; + +void qemu_cfg_port_probe(void); + +#endif diff --git a/src/smbios.c b/src/smbios.c index 6fbddd9..4caab19 100644 --- a/src/smbios.c +++ b/src/smbios.c @@ -7,50 +7,7 @@ #include "util.h" // dprintf #include "biosvar.h" // GET_EBDA - - -/**************************************************************** - * UUID probe - ****************************************************************/ - -#define QEMU_CFG_SIGNATURE 0x00 -#define QEMU_CFG_ID 0x01 -#define QEMU_CFG_UUID 0x02 - -static void -qemu_cfg_read(u8 *buf, u16 f, int len) -{ - outw(f, PORT_QEMU_CFG_CTL); - while (len--) - *(buf++) = inb(PORT_QEMU_CFG_DATA); -} - -static int -qemu_cfg_port_probe() -{ - u8 sig[4] = "QEMU"; - u8 buf[4]; - qemu_cfg_read(buf, QEMU_CFG_SIGNATURE, 4); - return *(u32*)buf == *(u32*)sig; -} - -static void -uuid_probe(u8 *bios_uuid) -{ - // Default to UUID not set - memset(bios_uuid, 0, 16); - - if (! CONFIG_UUID_BACKDOOR) - return; - if (CONFIG_COREBOOT) - return; - if (! qemu_cfg_port_probe()) - // Feature not available - return; - - qemu_cfg_read(bios_uuid, QEMU_CFG_UUID, 16); -} - +#include "cfg.h" /**************************************************************** * smbios tables @@ -304,7 +261,7 @@ smbios_type_1_init(void *start) p->version_str = 0; p->serial_number_str = 0; - uuid_probe(p->uuid); + cfg_get_uuid(p->uuid); p->wake_up_type = 0x06; /* power switch */ p->sku_number_str = 0;