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

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

[Gleb Natapov]

Move qemu config code from smbios.c to its own files. Add support for
-boot menu=on|off qemu option.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/Makefile b/Makefile
index ec30f39..a7dd8c8 100644
--- a/Makefile
+++ b/Makefile
@@ -13,7 +13,7 @@ OUT=out/
 # 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
+        pnpbios.c pirtable.c vgahooks.c pmm.c ramdisk.c pv.c
 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 b70d49c..083d5b9 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 "pv.h"
 
 struct ipl_s IPL;
 
@@ -297,7 +298,8 @@ boot_prep()
         return;
 
     // Allow user to modify BCV/IPL order.
-    interactive_bootmenu();
+    if (qemu_cfg_show_boot_menu())
+        interactive_bootmenu();
 
     // Setup floppy boot order
     int override = IPL.bev[0].subchoice;
diff --git a/src/post.c b/src/post.c
index e2569b0..bd638f8 100644
--- a/src/post.c
+++ b/src/post.c
@@ -19,6 +19,7 @@
 #include "bregs.h" // struct bregs
 #include "mptable.h" // mptable_init
 #include "boot.h" // IPL
+#include "pv.h"
 
 void
 __set_irq(int vector, void *loc)
@@ -182,6 +183,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..1e2033c
--- /dev/null
+++ b/src/pv.c
@@ -0,0 +1,62 @@
+#include "config.h"
+#include "ioport.h"
+#include "pv.h"
+
+int qemu_cfg_present;
+
+static void
+qemu_cfg_select(u16 f)
+{
+    outw(f, QEMU_CFG_CTL_PORT);
+}
+
+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;
+
+    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);
+}
+
+void qemu_cfg_get_uuid(u8 *uuid)
+{
+    if (!qemu_cfg_present)
+        return;
+
+    qemu_cfg_read_entry(uuid, QEMU_CFG_UUID, 16);
+}
+
+int qemu_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..d4bca80
--- /dev/null
+++ b/src/pv.h
@@ -0,0 +1,46 @@
+#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_CTL_PORT		0x510
+#define QEMU_CFG_DATA_PORT		0x511
+#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);
+int qemu_cfg_show_boot_menu(void);
+void qemu_cfg_get_uuid(u8 *uuid);
+
+#endif
diff --git a/src/smbios.c b/src/smbios.c
index 6fbddd9..3223b80 100644
--- a/src/smbios.c
+++ b/src/smbios.c
@@ -7,7 +7,7 @@
 
 #include "util.h" // dprintf
 #include "biosvar.h" // GET_EBDA
-
+#include "pv.h"
 
 /****************************************************************
  * UUID probe
@@ -18,23 +18,6 @@
 #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
@@ -44,11 +27,8 @@ uuid_probe(u8 *bios_uuid)
         return;
     if (CONFIG_COREBOOT)
         return;
-    if (! qemu_cfg_port_probe())
-        // Feature not available
-        return;
 
-    qemu_cfg_read(bios_uuid, QEMU_CFG_UUID, 16);
+    qemu_cfg_get_uuid(bios_uuid);
 }
 
 
--
			Gleb.

[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

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

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

[Kevin O'Connor]

On Tue, Sep 15, 2009 at 08:43:39AM +0300, Gleb Natapov wrote:
> On Mon, Sep 14, 2009 at 08:08:24PM -0400, Kevin O'Connor 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.
> > 
> 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?

My preference is strings - I think the long-term flexibility they
provide is worth the slight additional overhead they require.

Currently, SeaBIOS is just under 64K -- if features continue to get
added (eg, usb keyboard/boot) then it will likely creep into the
e-segment.  I don't foresee any hard limits on space coming up.

-Kevin

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

[Gleb Natapov]

On Tue, Sep 15, 2009 at 10:02:59PM -0400, Kevin O'Connor wrote:
> On Tue, Sep 15, 2009 at 08:43:39AM +0300, Gleb Natapov wrote:
> > On Mon, Sep 14, 2009 at 08:08:24PM -0400, Kevin O'Connor 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.
> > > 
> > 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?
> 
> My preference is strings - I think the long-term flexibility they
> provide is worth the slight additional overhead they require.
> 
> Currently, SeaBIOS is just under 64K -- if features continue to get
> added (eg, usb keyboard/boot) then it will likely creep into the
> e-segment.  I don't foresee any hard limits on space coming up.
> 
Fine by me.

The configuration interface should be able to read streams of data
though.  Qemu uses it to pass additional ACPI tables for instance. So
it not just simple key->value interface. get_config_u32("ShowBootMenu")
will work for simple cases but how can we express stream semantic? May be:

table_count = get_config_u16("AdditionalAcpiCount")
select_config("AdditionalAcpiTables");
for (i=0; i<table_count; i++) {
   len = config_read(table[i], table_len[i]);
}

--
			Gleb.

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

[Kevin O'Connor]

On Thu, Sep 17, 2009 at 12:57:28PM +0300, Gleb Natapov wrote:
> The configuration interface should be able to read streams of data
> though.  Qemu uses it to pass additional ACPI tables for instance. So
> it not just simple key->value interface. get_config_u32("ShowBootMenu")
> will work for simple cases but how can we express stream semantic? May be:
> 
> table_count = get_config_u16("AdditionalAcpiCount")
> select_config("AdditionalAcpiTables");
> for (i=0; i<table_count; i++) {
>    len = config_read(table[i], table_len[i]);
> }

On coreboot there is the Coreboot FileSystem (CBFS).  Basically, the
flash stores a series of named files, and SeaBIOS knows how to iterate
through them.  So, for instance, one might find the file
"pci1013,00b8.rom" which contains an option rom for pci device
1013:00b8, or one might find "floppyimg/FreeDOS" with an image of a
floppy to be emulated.

So, ideally qemu would do something similar.  Maybe something like:
copy_config_file("AdditionalAcpiTables", destfileptr, destfilemaxlen).
The exact mechanism for extracting the info is flexible.  To be
compatible with the CBFS interface, seabios just needs a way to "walk"
the list of files, find out how big a given file is, and be able to
copy the file to ram.

If anyone is curious, the cbfs functions are in src/coreboot.c - the
main interface is:

// Find the file with the given filename.
struct cbfs_file *cbfs_findfile(const char *fname)
// Find next file with the given filename prefix.
struct cbfs_file *cbfs_findprefix(const char *prefix, struct cbfs_file *last)
// Determine the uncompressed size of a datafile.
u32 cbfs_datasize(struct cbfs_file *file)
// Copy a file to memory (uncompressing if necessary)
int cbfs_copyfile(struct cbfs_file *file, void *dst, u32 maxlen)

-Kevin

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

[Gleb Natapov]

On Thu, Sep 17, 2009 at 09:24:11PM -0400, Kevin O'Connor wrote:
> On Thu, Sep 17, 2009 at 12:57:28PM +0300, Gleb Natapov wrote:
> > The configuration interface should be able to read streams of data
> > though.  Qemu uses it to pass additional ACPI tables for instance. So
> > it not just simple key->value interface. get_config_u32("ShowBootMenu")
> > will work for simple cases but how can we express stream semantic? May be:
> > 
> > table_count = get_config_u16("AdditionalAcpiCount")
> > select_config("AdditionalAcpiTables");
> > for (i=0; i<table_count; i++) {
> >    len = config_read(table[i], table_len[i]);
> > }
> 
> On coreboot there is the Coreboot FileSystem (CBFS).  Basically, the
> flash stores a series of named files, and SeaBIOS knows how to iterate
> through them.  So, for instance, one might find the file
> "pci1013,00b8.rom" which contains an option rom for pci device
> 1013:00b8, or one might find "floppyimg/FreeDOS" with an image of a
> floppy to be emulated.
> 
> So, ideally qemu would do something similar.  Maybe something like:
Qemu already does something different. For instance acpi tables are
transfered as stream formated like this:
<table count><1 table length><table data><2 table length><table data>
...<n table length><table data>

I don't think qemu should expose file system API to a BIOS.

> copy_config_file("AdditionalAcpiTables", destfileptr, destfilemaxlen).
> The exact mechanism for extracting the info is flexible.  To be
> compatible with the CBFS interface, seabios just needs a way to "walk"
> the list of files, find out how big a given file is, and be able to
> copy the file to ram.
> 
> If anyone is curious, the cbfs functions are in src/coreboot.c - the
> main interface is:
> 
> // Find the file with the given filename.
> struct cbfs_file *cbfs_findfile(const char *fname)
> // Find next file with the given filename prefix.
> struct cbfs_file *cbfs_findprefix(const char *prefix, struct cbfs_file *last)
> // Determine the uncompressed size of a datafile.
> u32 cbfs_datasize(struct cbfs_file *file)
> // Copy a file to memory (uncompressing if necessary)
> int cbfs_copyfile(struct cbfs_file *file, void *dst, u32 maxlen)
> 
> -Kevin

--
			Gleb.

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

[Kevin O'Connor]

On Fri, Sep 18, 2009 at 01:01:20PM +0300, Gleb Natapov wrote:
> On Thu, Sep 17, 2009 at 09:24:11PM -0400, Kevin O'Connor wrote:
> > On coreboot there is the Coreboot FileSystem (CBFS).  Basically, the
> > flash stores a series of named files, and SeaBIOS knows how to iterate
> > through them.  So, for instance, one might find the file
> > "pci1013,00b8.rom" which contains an option rom for pci device
> > 1013:00b8, or one might find "floppyimg/FreeDOS" with an image of a
> > floppy to be emulated.
> > 
> > So, ideally qemu would do something similar.  Maybe something like:
> Qemu already does something different. For instance acpi tables are
> transfered as stream formated like this:
> <table count><1 table length><table data><2 table length><table data>
> ...<n table length><table data>

Maybe a stream could be introduced with something like:
<name><len><data> <name2><len2><data2> ...

> I don't think qemu should expose file system API to a BIOS.

To be clear, I'm not proposing exposing the system's filesystem to the
guest.

It's really just a way of getting name=value pairs.  If there is a
different way to do this then that's fine.  Ideally, to fit with
SeaBIOS' current code, there would be a way to obtain the size and
data for a given "name" along with an ability to iterate through the
list of "names" available.

-Kevin

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

[Gleb Natapov]

On Sat, Sep 19, 2009 at 11:16:41AM -0400, Kevin O'Connor wrote:
> On Fri, Sep 18, 2009 at 01:01:20PM +0300, Gleb Natapov wrote:
> > On Thu, Sep 17, 2009 at 09:24:11PM -0400, Kevin O'Connor wrote:
> > > On coreboot there is the Coreboot FileSystem (CBFS).  Basically, the
> > > flash stores a series of named files, and SeaBIOS knows how to iterate
> > > through them.  So, for instance, one might find the file
> > > "pci1013,00b8.rom" which contains an option rom for pci device
> > > 1013:00b8, or one might find "floppyimg/FreeDOS" with an image of a
> > > floppy to be emulated.
> > > 
> > > So, ideally qemu would do something similar.  Maybe something like:
> > Qemu already does something different. For instance acpi tables are
> > transfered as stream formated like this:
> > <table count><1 table length><table data><2 table length><table data>
> > ...<n table length><table data>
> 
> Maybe a stream could be introduced with something like:
> <name><len><data> <name2><len2><data2> ...
> 
The format is already set. The are two ports. You write option id in
first port and you read option value from second one. The value format
is different for each option. Additional acpi table format is like I
described above. If we want to use the same APIs for config access for
coreboot and qemu the API will have to be general enough to accommodate
both approaches. Changing formats is not an option at this stage.

> > I don't think qemu should expose file system API to a BIOS.
> 
> To be clear, I'm not proposing exposing the system's filesystem to the
> guest.
> 
> It's really just a way of getting name=value pairs.  If there is a
> different way to do this then that's fine.  Ideally, to fit with
> SeaBIOS' current code, there would be a way to obtain the size and
> data for a given "name" along with an ability to iterate through the
> list of "names" available.
> 
You can obtain size for acpi tables by reading the whole data first time
just for calculating the data size and discarding the data. But I don't see
the point of doing it. The format was specifically designed to allow
reading one table at a time and placing it in its final place in memory.

--
			Gleb.

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

[Kevin O'Connor]

On Sat, Sep 19, 2009 at 08:56:55PM +0300, Gleb Natapov wrote:
> On Sat, Sep 19, 2009 at 11:16:41AM -0400, Kevin O'Connor wrote:
> > Maybe a stream could be introduced with something like:
> > <name><len><data> <name2><len2><data2> ...
> > 
> The format is already set. The are two ports. You write option id in
> first port and you read option value from second one. The value format
> is different for each option. Additional acpi table format is like I
> described above. If we want to use the same APIs for config access for
> coreboot and qemu the API will have to be general enough to accommodate
> both approaches. Changing formats is not an option at this stage.

Okay - it doesn't sound like there is much overlap here between qemu
and coreboot.  On coreboot cbfs is used for pulling out option roms,
executables, and floppy images - none of which have much use under
qemu anyway.

So, maybe we should just go back to the discussion of a config
interface.  I think it would be nice to have one api for getting
config items for both qemu and coreboot - something like
get_config_u32("ShowBootMenu").  On coreboot that info could then be
extracted from cbfs and qemu can get in from the "cfg port".

Does that make sense?

-Kevin

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

[Gleb Natapov]

On Tue, Sep 29, 2009 at 09:09:46PM -0400, Kevin O'Connor wrote:
> On Sat, Sep 19, 2009 at 08:56:55PM +0300, Gleb Natapov wrote:
> > On Sat, Sep 19, 2009 at 11:16:41AM -0400, Kevin O'Connor wrote:
> > > Maybe a stream could be introduced with something like:
> > > <name><len><data> <name2><len2><data2> ...
> > > 
> > The format is already set. The are two ports. You write option id in
> > first port and you read option value from second one. The value format
> > is different for each option. Additional acpi table format is like I
> > described above. If we want to use the same APIs for config access for
> > coreboot and qemu the API will have to be general enough to accommodate
> > both approaches. Changing formats is not an option at this stage.
> 
> Okay - it doesn't sound like there is much overlap here between qemu
> and coreboot.  On coreboot cbfs is used for pulling out option roms,
> executables, and floppy images - none of which have much use under
> qemu anyway.
> 
> So, maybe we should just go back to the discussion of a config
> interface.  I think it would be nice to have one api for getting
> config items for both qemu and coreboot - something like
> get_config_u32("ShowBootMenu").  On coreboot that info could then be
> extracted from cbfs and qemu can get in from the "cfg port".
> 
> Does that make sense?
> 
Yes. That is the direction I was going to take. Lest implement simple
name/value interface first and table loading code will be different.
If there will be much overlap in table loading code we will unify it
later.

--
			Gleb.

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

[Blue Swirl]

On Wed, Sep 30, 2009 at 9:35 AM, Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Sep 29, 2009 at 09:09:46PM -0400, Kevin O'Connor wrote:
>> On Sat, Sep 19, 2009 at 08:56:55PM +0300, Gleb Natapov wrote:
>> > On Sat, Sep 19, 2009 at 11:16:41AM -0400, Kevin O'Connor wrote:
>> > > Maybe a stream could be introduced with something like:
>> > > <name><len><data> <name2><len2><data2> ...
>> > >
>> > The format is already set. The are two ports. You write option id in
>> > first port and you read option value from second one. The value format
>> > is different for each option. Additional acpi table format is like I
>> > described above. If we want to use the same APIs for config access for
>> > coreboot and qemu the API will have to be general enough to accommodate
>> > both approaches. Changing formats is not an option at this stage.
>>
>> Okay - it doesn't sound like there is much overlap here between qemu
>> and coreboot.  On coreboot cbfs is used for pulling out option roms,
>> executables, and floppy images - none of which have much use under
>> qemu anyway.
>>
>> So, maybe we should just go back to the discussion of a config
>> interface.  I think it would be nice to have one api for getting
>> config items for both qemu and coreboot - something like
>> get_config_u32("ShowBootMenu").  On coreboot that info could then be
>> extracted from cbfs and qemu can get in from the "cfg port".
>>
>> Does that make sense?
>>
> Yes. That is the direction I was going to take. Lest implement simple
> name/value interface first and table loading code will be different.
> If there will be much overlap in table loading code we will unify it
> later.

The name/value interface could be useful for OpenBIOS too if we want
to avoid using NVRAM for OpenBIOS variables for compatibility. There
should be methods to iterate through all keys and get the byte size of
the value.

For example, current fw_cfg interface could be extended to allow
string keys in addition to integer keys.

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

[Gleb Natapov]

On Wed, Sep 30, 2009 at 08:17:26PM +0300, Blue Swirl wrote:
> On Wed, Sep 30, 2009 at 9:35 AM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Sep 29, 2009 at 09:09:46PM -0400, Kevin O'Connor wrote:
> >> On Sat, Sep 19, 2009 at 08:56:55PM +0300, Gleb Natapov wrote:
> >> > On Sat, Sep 19, 2009 at 11:16:41AM -0400, Kevin O'Connor wrote:
> >> > > Maybe a stream could be introduced with something like:
> >> > > <name><len><data> <name2><len2><data2> ...
> >> > >
> >> > The format is already set. The are two ports. You write option id in
> >> > first port and you read option value from second one. The value format
> >> > is different for each option. Additional acpi table format is like I
> >> > described above. If we want to use the same APIs for config access for
> >> > coreboot and qemu the API will have to be general enough to accommodate
> >> > both approaches. Changing formats is not an option at this stage.
> >>
> >> Okay - it doesn't sound like there is much overlap here between qemu
> >> and coreboot.  On coreboot cbfs is used for pulling out option roms,
> >> executables, and floppy images - none of which have much use under
> >> qemu anyway.
> >>
> >> So, maybe we should just go back to the discussion of a config
> >> interface.  I think it would be nice to have one api for getting
> >> config items for both qemu and coreboot - something like
> >> get_config_u32("ShowBootMenu").  On coreboot that info could then be
> >> extracted from cbfs and qemu can get in from the "cfg port".
> >>
> >> Does that make sense?
> >>
> > Yes. That is the direction I was going to take. Lest implement simple
> > name/value interface first and table loading code will be different.
> > If there will be much overlap in table loading code we will unify it
> > later.
> 
> The name/value interface could be useful for OpenBIOS too if we want
> to avoid using NVRAM for OpenBIOS variables for compatibility. There
> should be methods to iterate through all keys and get the byte size of
> the value.
> 
> For example, current fw_cfg interface could be extended to allow
> string keys in addition to integer keys.
There are two types of string keys. Those that you know the length in
advance (like uuid) and those you don't. fw_cfg can support second type
without additional change by passing <len><string> as a value. This will
be hidden from seabios behind get_config_string() interface of course.

--
			Gleb.

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

[Gleb Natapov]

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

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;
--
			Gleb.

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

[Kevin O'Connor]

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

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

[Kevin O'Connor]

On Wed, Sep 30, 2009 at 08:17:26PM +0300, Blue Swirl wrote:
> The name/value interface could be useful for OpenBIOS too if we want
> to avoid using NVRAM for OpenBIOS variables for compatibility. There
> should be methods to iterate through all keys and get the byte size of
> the value.

Yes - that's what I think would work well for SeaBIOS also.

-Kevin

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

[Gleb Natapov]

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.

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

[Kevin O'Connor]

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

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

[Gleb Natapov]

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.

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

[Kevin O'Connor]

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