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

login
register
mail settings
Submitter Gleb Natapov
Date Oct. 1, 2009, 4:35 p.m.
Message ID <20091001163555.GV9832@redhat.com>
Download mbox | patch
Permalink /patch/34753/
State Superseded
Headers show

Comments

Gleb Natapov - Oct. 1, 2009, 4:35 p.m.
> 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. This kind
of logic does not belong to general function like get_config_u32().
So how about another approach: qemu and core boot provide functions like
void cfg_get_uuid(u8 *uuid);
int cfg_show_boot_menu(void);
...
with all necessary fall-back logic and we use coreboot or qemu file during
build time depending what our target is. Something like attached patch.

--
			Gleb.
Kevin O'Connor - Oct. 2, 2009, 12:51 a.m.
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
Gleb Natapov - Oct. 2, 2009, 2:03 p.m.
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.
Kevin O'Connor - Oct. 2, 2009, 4:52 p.m.
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
Gleb Natapov - Oct. 2, 2009, 6:10 p.m.
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.
Kevin O'Connor - Oct. 2, 2009, 7:31 p.m.
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

Patch

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;