[Qemu-devel] [v4 PATCH 00/12] SMBIOS: build full tables in QEMU

[Qemu-devel] [v4 PATCH 00/12] SMBIOS: build full tables in QEMU

[Gabriel L. Somlo]

Here's version 4 of moving smbios table construction into QEMU.

New in this version:

   - 9/12 builds all memory tables (16, 17, 19, 20) and IMHO has much
     cleaner and easier to understand code, as well as extensive comments
     re. how tables fit together, and how they're generated.

   - after 10/12, we're 100% bug-for-bug SeaBIOS compatible

   - 11/12 updates tables (most importantly type 17) to v2.4 compatibility
     by adding the v2.3 fields missing in SeaBIOS

   - 12/12 removes bug-for-bug compatibility, cleans up a few of
     the arbitrarily hard coded fields, and counts CPUs starting from 0
     to match the rest of the table types with multiple instances.

Re. e820: I believe it's premature to build smbios tables from e820.

Both e820 and smbios are currently built from three "primitives":
ram_size, and [below|above]_4g_mem_size, and it is uncertain (at least
to me, at least right now) what e820 will end up looking like in the
future. I would humbly suggest we switch smbios generation over to using
e820 as a source once that becomes clearer.

See below for more e820 related thoughts and questions.


By adding support for the inclusion of a type 2 (baseboard) table, and
by generating v2.4 compliant type 17 (dimm) tables, this patch series
allows QEMU to more or less officially support OS X guests (v10.8 and
later, using Chameleon as a bootloader, but without further out of tree
patches).



On Fri, Mar 14, 2014 at 06:51:05PM +0100, Igor Mammedov wrote:
> They might overlap, grep for e820_add_entry(). If you interested in
> what kernel does with such table look for sanitize_e820_map() there.

I looked, and it appears sanitize_e820_map() will turn whatever is
collected from the hardware into a set of non-overlapping entries.
I am now wondering if QEMU isn't capable of directly generating a
sanitized e820 map from start.
 
> Does SMBIOS/t17 actually care about shadowing parts of it by something
> else in unrelated e820?

AFAICT, we only care about E820_RAM entries, so that we can map them
to DIMMs representing the total memory (ram_size) configured on the system.

> Once we have DIMMDevices, I'm planning to convert below-4g and above-4g to
> a set of DIMMDevices, there will be at least 1 device per node but there could
> be more of them to satisfy different backend requirements like hugepage
> size, alignment, e.t.c.
> 
> BTW why do we care how smbios tables are build in relation to NUMA mapping,
> they seem to be totally independent.

Right now, both the e820 table and smbios tables are populated from the same
set of "primitives", i.e. ram_size, below_4g_mem_size, and above_4g_mem_size.

If, in the future, we decide to build smbios memory tables indirectly, i.e.
from e820 (based on the fact that e820 will contain more complete information
supplied from more than just pc_q35_init() or pc_init1(), if using piix) it
will be good to have an idea of how to programatically deal with whatever
combinations of nodes, DIMMs, etc. will be present. Mainly, how can I tell
if a bunch of E820_RAM entries from the e820 table all belong to the same
or different nodes ?

At this point, can anyone with access to a real, physical, NUMA system dump
the smbios tables with dmidecode and post them here ? I think that would
be very informative.

Thanks again,
--Gabriel


Gabriel L. Somlo (12):
  SMBIOS: Rename smbios_set_type1_defaults() for more general use
  SMBIOS: Use macro to set smbios defaults
  SMBIOS: Use bitmaps to check for smbios table collisions
  SMBIOS: Add code to build full smbios tables; build type 2 table
  SMBIOS: Build full tables for types 0 and 1
  SMBIOS: Remove unused code for passing individual fields to bios
  SMBIOS: Build full type 3 table
  SMBIOS: Build full type 4 tables
  SMBIOS: Build full smbios memory tables (type 16, 17, 19, and 20)
  SMBIOS: Build full tables for type 32 and 127
  SMBIOS: Update all table definitions to smbios spec v2.3
  SMBIOS: Remove SeaBIOS compatibility quirks

 hw/i386/pc.c             |   3 +
 hw/i386/pc_piix.c        |  15 +-
 hw/i386/pc_q35.c         |  11 +-
 hw/i386/smbios.c         | 758 +++++++++++++++++++++++++++++++++++++++++------
 include/hw/i386/smbios.h |  47 ++-
 5 files changed, 717 insertions(+), 117 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 01/12] SMBIOS: Rename smbios_set_type1_defaults() for more general use

[Gabriel L. Somlo]

Subsequent patches will utilize this function to set defaults for
more smbios types than just type 1, so the function name should
reflect this.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/pc_piix.c        | 12 ++++++------
 hw/i386/pc_q35.c         |  8 ++++----
 hw/i386/smbios.c         |  4 ++--
 include/hw/i386/smbios.h |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7930a26..8513de0 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -60,7 +60,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
 static bool has_pci_info;
 static bool has_acpi_build = true;
-static bool smbios_type1_defaults = true;
+static bool smbios_defaults = true;
 /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to
  * host addresses aligned at 1Gbyte boundaries.  This way we can use 1GByte
  * pages in the host.
@@ -143,9 +143,9 @@ static void pc_init1(QEMUMachineInitArgs *args,
     guest_info->has_pci_info = has_pci_info;
     guest_info->isapc_ram_fw = !pci_enabled;
 
-    if (smbios_type1_defaults) {
+    if (smbios_defaults) {
         /* These values are guest ABI, do not change */
-        smbios_set_type1_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
+        smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
                                   args->machine->name);
     }
 
@@ -264,7 +264,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
 
 static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
-    smbios_type1_defaults = false;
+    smbios_defaults = false;
     gigabyte_align = false;
     option_rom_has_mr = true;
     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
@@ -345,7 +345,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
     has_acpi_build = false;
-    smbios_type1_defaults = false;
+    smbios_defaults = false;
     x86_cpu_compat_disable_kvm_features(FEAT_KVM, KVM_FEATURE_PV_EOI);
     enable_compat_apic_id_mode();
     pc_init1(args, 1, 0);
@@ -355,7 +355,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
     has_acpi_build = false;
-    smbios_type1_defaults = false;
+    smbios_defaults = false;
     if (!args->cpu_model) {
         args->cpu_model = "486";
     }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c844dc2..eacec53 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -50,7 +50,7 @@
 
 static bool has_pci_info;
 static bool has_acpi_build = true;
-static bool smbios_type1_defaults = true;
+static bool smbios_defaults = true;
 /* Make sure that guest addresses aligned at 1Gbyte boundaries get mapped to
  * host addresses aligned at 1Gbyte boundaries.  This way we can use 1GByte
  * pages in the host.
@@ -130,9 +130,9 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     guest_info->isapc_ram_fw = false;
     guest_info->has_acpi_build = has_acpi_build;
 
-    if (smbios_type1_defaults) {
+    if (smbios_defaults) {
         /* These values are guest ABI, do not change */
-        smbios_set_type1_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
+        smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
                                   args->machine->name);
     }
 
@@ -242,7 +242,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 
 static void pc_compat_1_7(QEMUMachineInitArgs *args)
 {
-    smbios_type1_defaults = false;
+    smbios_defaults = false;
     gigabyte_align = false;
     option_rom_has_mr = true;
     x86_cpu_compat_disable_kvm_features(FEAT_1_ECX, CPUID_EXT_X2APIC);
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index e8f41ad..89dc070 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -256,8 +256,8 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
-void smbios_set_type1_defaults(const char *manufacturer,
-                               const char *product, const char *version)
+void smbios_set_defaults(const char *manufacturer,
+                         const char *product, const char *version)
 {
     if (!type1.manufacturer) {
         type1.manufacturer = manufacturer;
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 18fb970..e088aae 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -16,8 +16,8 @@
 #include "qemu/option.h"
 
 void smbios_entry_add(QemuOpts *opts);
-void smbios_set_type1_defaults(const char *manufacturer,
-                               const char *product, const char *version);
+void smbios_set_defaults(const char *manufacturer,
+                         const char *product, const char *version);
 uint8_t *smbios_get_table(size_t *length);
 
 /*
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 02/12] SMBIOS: Use macro to set smbios defaults

[Gabriel L. Somlo]

The function smbios_set_defaults() uses a repeating code pattern
for each field. This patch replaces that pattern with a macro.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 89dc070..f4ee7b4 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -256,18 +256,17 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
+#define SMBIOS_SET_DEFAULT(field, value)                                  \
+    if (!field) {                                                         \
+        field = value;                                                    \
+    }
+
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
-    if (!type1.manufacturer) {
-        type1.manufacturer = manufacturer;
-    }
-    if (!type1.product) {
-        type1.product = product;
-    }
-    if (!type1.version) {
-        type1.version = version;
-    }
+    SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer);
+    SMBIOS_SET_DEFAULT(type1.product, product);
+    SMBIOS_SET_DEFAULT(type1.version, version);
 }
 
 uint8_t *smbios_get_table(size_t *length)
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 03/12] SMBIOS: Use bitmaps to check for smbios table collisions

[Gabriel L. Somlo]

Replace existing smbios_check_collision() functionality with
a pair of bitmaps: have_binfile_bitmap and have_fields_bitmap.
Bits corresponding to each smbios type are set by smbios_entry_add(),
which also uses the bitmaps to ensure that binary blobs and field
values are never accepted for the same type.

These bitmaps will also be used in the future to decide whether
or not to build a full table for a given smbios type.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c         | 51 ++++++++++++++++++++----------------------------
 include/hw/i386/smbios.h |  2 ++
 2 files changed, 23 insertions(+), 30 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index f4ee7b4..6889332 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -49,11 +49,8 @@ static size_t smbios_entries_len;
 static int smbios_type4_count = 0;
 static bool smbios_immutable;
 
-static struct {
-    bool seen;
-    int headertype;
-    Location loc;
-} first_opt[2];
+static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
+static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
 
 static struct {
     const char *vendor, *version, *date;
@@ -164,29 +161,6 @@ static void smbios_validate_table(void)
     }
 }
 
-/*
- * To avoid unresolvable overlaps in data, don't allow both
- * tables and fields for the same smbios type.
- */
-static void smbios_check_collision(int type, int entry)
-{
-    if (type < ARRAY_SIZE(first_opt)) {
-        if (first_opt[type].seen) {
-            if (first_opt[type].headertype != entry) {
-                error_report("Can't mix file= and type= for same type");
-                loc_push_restore(&first_opt[type].loc);
-                error_report("This is the conflicting setting");
-                loc_pop(&first_opt[type].loc);
-                exit(1);
-            }
-        } else {
-            first_opt[type].seen = true;
-            first_opt[type].headertype = entry;
-            loc_save(&first_opt[type].loc);
-        }
-    }
-}
-
 static void smbios_add_field(int type, int offset, const void *data, size_t len)
 {
     struct smbios_field *field;
@@ -331,7 +305,14 @@ void smbios_entry_add(QemuOpts *opts)
         }
 
         header = (struct smbios_structure_header *)(table->data);
-        smbios_check_collision(header->type, SMBIOS_TABLE_ENTRY);
+
+        if (test_bit(header->type, have_fields_bitmap)) {
+            error_report("Can't add binary type %d table! "
+                         "(fields already specified)", header->type);
+            exit(1);
+        }
+        set_bit(header->type, have_binfile_bitmap);
+
         if (header->type == 4) {
             smbios_type4_count++;
         }
@@ -346,7 +327,17 @@ void smbios_entry_add(QemuOpts *opts)
     if (val) {
         unsigned long type = strtoul(val, NULL, 0);
 
-        smbios_check_collision(type, SMBIOS_FIELD_ENTRY);
+        if (type > SMBIOS_MAX_TYPE) {
+            error_report("smbios type (%ld) out of range!", type);
+            exit(1);
+        }
+
+        if (test_bit(type, have_binfile_bitmap)) {
+            error_report("Can't add fields for type %ld table! "
+                         "(binary file already loaded)", type);
+            exit(1);
+        }
+        set_bit(type, have_fields_bitmap);
 
         switch (type) {
         case 0:
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index e088aae..3425d40 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -15,6 +15,8 @@
 
 #include "qemu/option.h"
 
+#define SMBIOS_MAX_TYPE 127
+
 void smbios_entry_add(QemuOpts *opts);
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version);
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 04/12] SMBIOS: Add code to build full smbios tables; build type 2 table

[Gabriel L. Somlo]

This patch adds a set of macros which build full smbios tables
of a given type, including the logic to decide whether a given
table type should be built or not.

To illustrate this new functionality, we introduce and optionally
build a table of type 2 (base board), which is required by some
versions of OS X (10.7 and 10.8).

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c         | 158 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/smbios.h |  18 +++++-
 2 files changed, 175 insertions(+), 1 deletion(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 6889332..06f572d 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -48,6 +48,7 @@ static uint8_t *smbios_entries;
 static size_t smbios_entries_len;
 static int smbios_type4_count = 0;
 static bool smbios_immutable;
+static bool smbios_have_defaults;
 
 static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
 static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
@@ -63,6 +64,10 @@ static struct {
     /* uuid is in qemu_uuid[] */
 } type1;
 
+static struct {
+    const char *manufacturer, *product, *version, *serial, *asset, *location;
+} type2;
+
 static QemuOptsList qemu_smbios_opts = {
     .name = "smbios",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
@@ -146,6 +151,39 @@ static const QemuOptDesc qemu_smbios_type1_opts[] = {
     { /* end of list */ }
 };
 
+static const QemuOptDesc qemu_smbios_type2_opts[] = {
+    {
+        .name = "type",
+        .type = QEMU_OPT_NUMBER,
+        .help = "SMBIOS element type",
+    },{
+        .name = "manufacturer",
+        .type = QEMU_OPT_STRING,
+        .help = "manufacturer name",
+    },{
+        .name = "product",
+        .type = QEMU_OPT_STRING,
+        .help = "product name",
+    },{
+        .name = "version",
+        .type = QEMU_OPT_STRING,
+        .help = "version number",
+    },{
+        .name = "serial",
+        .type = QEMU_OPT_STRING,
+        .help = "serial number",
+    },{
+        .name = "asset",
+        .type = QEMU_OPT_STRING,
+        .help = "asset tag number",
+    },{
+        .name = "location",
+        .type = QEMU_OPT_STRING,
+        .help = "location in chassis",
+    },
+    { /* end of list */ }
+};
+
 static void smbios_register_config(void)
 {
     qemu_add_opts(&qemu_smbios_opts);
@@ -161,6 +199,90 @@ static void smbios_validate_table(void)
     }
 }
 
+static bool smbios_skip_table(uint8_t type, bool required_table)
+{
+    if (test_bit(type, have_binfile_bitmap)) {
+        return true; /* user provided their own binary blob(s) */
+    }
+    if (test_bit(type, have_fields_bitmap)) {
+        return false; /* user provided fields via command line */
+    }
+    if (smbios_have_defaults && required_table) {
+        return false; /* we're building tables, and this one's required */
+    }
+    return true;
+}
+
+#define SMBIOS_BUILD_TABLE_PRE(tbl_type, tbl_handle, tbl_required)        \
+    struct smbios_table *w;                                               \
+    struct smbios_type_##tbl_type *t;                                     \
+    size_t w_off, t_off; /* wrapper, table offsets into smbios_entries */ \
+    int str_index = 0;                                                    \
+    do {                                                                  \
+        /* should we skip building this table ? */                        \
+        if (smbios_skip_table(tbl_type, tbl_required)) {                  \
+            return;                                                       \
+        }                                                                 \
+                                                                          \
+        /* initialize fw_cfg smbios element count */                      \
+        if (!smbios_entries) {                                            \
+            smbios_entries_len = sizeof(uint16_t);                        \
+            smbios_entries = g_malloc0(smbios_entries_len);               \
+        }                                                                 \
+                                                                          \
+        /* use offsets of wrapper w and table t within smbios_entries  */ \
+        /* (pointers must be updated after each realloc)               */ \
+        w_off = smbios_entries_len;                                       \
+        t_off = w_off + sizeof(*w);                                       \
+        smbios_entries_len = t_off + sizeof(*t);                          \
+        smbios_entries = g_realloc(smbios_entries, smbios_entries_len);   \
+        w = (struct smbios_table *)(smbios_entries + w_off);              \
+        t = (struct smbios_type_##tbl_type *)(smbios_entries + t_off);    \
+                                                                          \
+        w->header.type = SMBIOS_TABLE_ENTRY;                              \
+        w->header.length = sizeof(*w) + sizeof(*t);                       \
+                                                                          \
+        t->header.type = tbl_type;                                        \
+        t->header.length = sizeof(*t);                                    \
+        t->header.handle = tbl_handle;                                    \
+    } while (0)
+
+#define SMBIOS_TABLE_SET_STR(tbl_type, field, value)                      \
+    do {                                                                  \
+        int len = (value != NULL) ? strlen(value) + 1 : 0;                \
+        if (len > 1) {                                                    \
+            smbios_entries = g_realloc(smbios_entries,                    \
+                                       smbios_entries_len + len);         \
+            memcpy(smbios_entries + smbios_entries_len, value, len);      \
+            smbios_entries_len += len;                                    \
+            /* update pointer(s) post-realloc */                          \
+            w = (struct smbios_table *)(smbios_entries + w_off);          \
+            t = (struct smbios_type_##tbl_type *)(smbios_entries + t_off);\
+            t->field = ++str_index;                                       \
+            w->header.length += len;                                      \
+        } else {                                                          \
+            t->field = 0;                                                 \
+        }                                                                 \
+    } while (0)
+
+#define SMBIOS_BUILD_TABLE_POST                                           \
+    do {                                                                  \
+        /* add empty string if no strings defined in table */             \
+        /* (NOTE: terminating \0 currently handled by fw_cfg/seabios) */  \
+        if (str_index == 0) {                                             \
+            smbios_entries = g_realloc(smbios_entries,                    \
+                                       smbios_entries_len + 1);           \
+            *(smbios_entries + smbios_entries_len) = 0;                   \
+            smbios_entries_len += 1;                                      \
+            /* update pointer(s) post-realloc */                          \
+            w = (struct smbios_table *)(smbios_entries + w_off);          \
+            w->header.length += 1;                                        \
+        }                                                                 \
+                                                                          \
+        /* update fw_cfg smbios element count */                          \
+        *(uint16_t *)smbios_entries += 1;                                 \
+    } while (0)
+
 static void smbios_add_field(int type, int offset, const void *data, size_t len)
 {
     struct smbios_field *field;
@@ -230,6 +352,24 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
+static void smbios_build_type_2_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(2, 0x200, false); /* optional */
+
+    SMBIOS_TABLE_SET_STR(2, manufacturer_str, type2.manufacturer);
+    SMBIOS_TABLE_SET_STR(2, product_str, type2.product);
+    SMBIOS_TABLE_SET_STR(2, version_str, type2.version);
+    SMBIOS_TABLE_SET_STR(2, serial_number_str, type2.serial);
+    SMBIOS_TABLE_SET_STR(2, asset_tag_number_str, type2.asset);
+    t->feature_flags = 0x01; /* Motherboard */
+    SMBIOS_TABLE_SET_STR(2, location_str, type2.location);
+    t->chassis_handle = 0x300; /* Type 3 (System enclosure) */
+    t->board_type = 0x0A; /* Motherboard */
+    t->contained_element_count = 0;
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
 #define SMBIOS_SET_DEFAULT(field, value)                                  \
     if (!field) {                                                         \
         field = value;                                                    \
@@ -238,14 +378,19 @@ static void smbios_build_type_1_fields(void)
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
+    smbios_have_defaults = true;
     SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer);
     SMBIOS_SET_DEFAULT(type1.product, product);
     SMBIOS_SET_DEFAULT(type1.version, version);
+    SMBIOS_SET_DEFAULT(type2.manufacturer, manufacturer);
+    SMBIOS_SET_DEFAULT(type2.product, product);
+    SMBIOS_SET_DEFAULT(type2.version, version);
 }
 
 uint8_t *smbios_get_table(size_t *length)
 {
     if (!smbios_immutable) {
+        smbios_build_type_2_table();
         smbios_build_type_0_fields();
         smbios_build_type_1_fields();
         smbios_validate_table();
@@ -381,6 +526,19 @@ void smbios_entry_add(QemuOpts *opts)
                 qemu_uuid_set = true;
             }
             return;
+        case 2:
+            qemu_opts_validate(opts, qemu_smbios_type2_opts, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                exit(1);
+            }
+            save_opt(&type2.manufacturer, opts, "manufacturer");
+            save_opt(&type2.product, opts, "product");
+            save_opt(&type2.version, opts, "version");
+            save_opt(&type2.serial, opts, "serial");
+            save_opt(&type2.asset, opts, "asset");
+            save_opt(&type2.location, opts, "location");
+            return;
         default:
             error_report("Don't know how to build fields for SMBIOS type %ld",
                          type);
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 3425d40..2642e1a 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -62,6 +62,22 @@ struct smbios_type_1 {
     uint8_t family_str;
 } QEMU_PACKED;
 
+/* SMBIOS type 2 - Base Board */
+struct smbios_type_2 {
+    struct smbios_structure_header header;
+    uint8_t manufacturer_str;
+    uint8_t product_str;
+    uint8_t version_str;
+    uint8_t serial_number_str;
+    uint8_t asset_tag_number_str;
+    uint8_t feature_flags;
+    uint8_t location_str;
+    uint16_t chassis_handle;
+    uint8_t board_type;
+    uint8_t contained_element_count;
+    /* contained elements follow */
+} QEMU_PACKED;
+
 /* SMBIOS type 3 - System Enclosure (v2.3) */
 struct smbios_type_3 {
     struct smbios_structure_header header;
@@ -78,7 +94,7 @@ struct smbios_type_3 {
     uint8_t height;
     uint8_t number_of_power_cords;
     uint8_t contained_element_count;
-    // contained elements follow
+    /* contained elements follow */
 } QEMU_PACKED;
 
 /* SMBIOS type 4 - Processor Information (v2.0) */
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 05/12] SMBIOS: Build full tables for types 0 and 1

[Gabriel L. Somlo]

Build full tables for types 0 (bios information) and 1 (system
information). Type 0 is optional, and a table will only be built
if requested via the command line; the default is to leave type 0
tables up to the bios itself.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 06f572d..b00a367 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -352,6 +352,62 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
+static void smbios_build_type_0_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(0, 0x000, false); /* optional, leave up to BIOS */
+
+    SMBIOS_TABLE_SET_STR(0, vendor_str, type0.vendor);
+    SMBIOS_TABLE_SET_STR(0, bios_version_str, type0.version);
+
+    t->bios_starting_address_segment = 0xE800; /* hardcoded in SeaBIOS */
+
+    SMBIOS_TABLE_SET_STR(0, bios_release_date_str, type0.date);
+
+    t->bios_rom_size = 0; /* hardcoded in SeaBIOS with FIXME comment */
+
+    /* BIOS characteristics not supported */
+    memset(t->bios_characteristics, 0, 8);
+    t->bios_characteristics[0] = 0x08;
+
+    /* Enable targeted content distribution (needed for SVVP, per SeaBIOS) */
+    t->bios_characteristics_extension_bytes[0] = 0;
+    t->bios_characteristics_extension_bytes[1] = 4;
+
+    if (type0.have_major_minor) {
+        t->system_bios_major_release = type0.major;
+        t->system_bios_minor_release = type0.minor;
+    } else {
+        t->system_bios_major_release = 0;
+        t->system_bios_minor_release = 0;
+    }
+
+    /* hardcoded in SeaBIOS */
+    t->embedded_controller_major_release = 0xFF;
+    t->embedded_controller_minor_release = 0xFF;
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
+static void smbios_build_type_1_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(1, 0x100, true); /* required */
+
+    SMBIOS_TABLE_SET_STR(1, manufacturer_str, type1.manufacturer);
+    SMBIOS_TABLE_SET_STR(1, product_name_str, type1.product);
+    SMBIOS_TABLE_SET_STR(1, version_str, type1.version);
+    SMBIOS_TABLE_SET_STR(1, serial_number_str, type1.serial);
+    if (qemu_uuid_set) {
+        memcpy(t->uuid, qemu_uuid, 16);
+    } else {
+        memset(t->uuid, 0, 16);
+    }
+    t->wake_up_type = 0x06; /* power switch */
+    SMBIOS_TABLE_SET_STR(1, sku_number_str, type1.sku);
+    SMBIOS_TABLE_SET_STR(1, family_str, type1.family);
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
 static void smbios_build_type_2_table(void)
 {
     SMBIOS_BUILD_TABLE_PRE(2, 0x200, false); /* optional */
@@ -379,6 +435,9 @@ void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
     smbios_have_defaults = true;
+    SMBIOS_SET_DEFAULT(type0.vendor, manufacturer);
+    SMBIOS_SET_DEFAULT(type0.version, version);
+    SMBIOS_SET_DEFAULT(type0.date, "01/01/2014");
     SMBIOS_SET_DEFAULT(type1.manufacturer, manufacturer);
     SMBIOS_SET_DEFAULT(type1.product, product);
     SMBIOS_SET_DEFAULT(type1.version, version);
@@ -390,9 +449,13 @@ void smbios_set_defaults(const char *manufacturer,
 uint8_t *smbios_get_table(size_t *length)
 {
     if (!smbios_immutable) {
+        smbios_build_type_0_table();
+        smbios_build_type_1_table();
         smbios_build_type_2_table();
+if (false) { /* shut up gcc until we remove deprecated code */
         smbios_build_type_0_fields();
         smbios_build_type_1_fields();
+}
         smbios_validate_table();
         smbios_immutable = true;
     }
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 06/12] SMBIOS: Remove unused code for passing individual fields to bios

[Gabriel L. Somlo]

This patch removes smbios_add_field() and the old code to insert
individual fields for types 0 and 1 into fw_cfg.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 80 --------------------------------------------------------
 1 file changed, 80 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index b00a367..4584774 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -29,13 +29,6 @@ struct smbios_header {
     uint8_t type;
 } QEMU_PACKED;
 
-struct smbios_field {
-    struct smbios_header header;
-    uint8_t type;
-    uint16_t offset;
-    uint8_t data[];
-} QEMU_PACKED;
-
 struct smbios_table {
     struct smbios_header header;
     uint8_t data[];
@@ -283,75 +276,6 @@ static bool smbios_skip_table(uint8_t type, bool required_table)
         *(uint16_t *)smbios_entries += 1;                                 \
     } while (0)
 
-static void smbios_add_field(int type, int offset, const void *data, size_t len)
-{
-    struct smbios_field *field;
-
-    if (!smbios_entries) {
-        smbios_entries_len = sizeof(uint16_t);
-        smbios_entries = g_malloc0(smbios_entries_len);
-    }
-    smbios_entries = g_realloc(smbios_entries, smbios_entries_len +
-                                                  sizeof(*field) + len);
-    field = (struct smbios_field *)(smbios_entries + smbios_entries_len);
-    field->header.type = SMBIOS_FIELD_ENTRY;
-    field->header.length = cpu_to_le16(sizeof(*field) + len);
-
-    field->type = type;
-    field->offset = cpu_to_le16(offset);
-    memcpy(field->data, data, len);
-
-    smbios_entries_len += sizeof(*field) + len;
-    (*(uint16_t *)smbios_entries) =
-            cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
-}
-
-static void smbios_maybe_add_str(int type, int offset, const char *data)
-{
-    if (data) {
-        smbios_add_field(type, offset, data, strlen(data) + 1);
-    }
-}
-
-static void smbios_build_type_0_fields(void)
-{
-    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, vendor_str),
-                         type0.vendor);
-    smbios_maybe_add_str(0, offsetof(struct smbios_type_0, bios_version_str),
-                         type0.version);
-    smbios_maybe_add_str(0, offsetof(struct smbios_type_0,
-                                     bios_release_date_str),
-                         type0.date);
-    if (type0.have_major_minor) {
-        smbios_add_field(0, offsetof(struct smbios_type_0,
-                                     system_bios_major_release),
-                         &type0.major, 1);
-        smbios_add_field(0, offsetof(struct smbios_type_0,
-                                     system_bios_minor_release),
-                         &type0.minor, 1);
-    }
-}
-
-static void smbios_build_type_1_fields(void)
-{
-    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, manufacturer_str),
-                         type1.manufacturer);
-    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, product_name_str),
-                         type1.product);
-    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, version_str),
-                         type1.version);
-    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, serial_number_str),
-                         type1.serial);
-    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, sku_number_str),
-                         type1.sku);
-    smbios_maybe_add_str(1, offsetof(struct smbios_type_1, family_str),
-                         type1.family);
-    if (qemu_uuid_set) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
-                         qemu_uuid, 16);
-    }
-}
-
 static void smbios_build_type_0_table(void)
 {
     SMBIOS_BUILD_TABLE_PRE(0, 0x000, false); /* optional, leave up to BIOS */
@@ -452,10 +376,6 @@ uint8_t *smbios_get_table(size_t *length)
         smbios_build_type_0_table();
         smbios_build_type_1_table();
         smbios_build_type_2_table();
-if (false) { /* shut up gcc until we remove deprecated code */
-        smbios_build_type_0_fields();
-        smbios_build_type_1_fields();
-}
         smbios_validate_table();
         smbios_immutable = true;
     }
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 07/12] SMBIOS: Build full type 3 table

[Gabriel L. Somlo]

Build smbios type 3 (system enclosure) table, and make it available
to the bios via fw_cfg. For initial compatibility with SeaBIOS, use
"Bochs" as the default manufacturer string, and leave version unset.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 4584774..47f7b0d 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -61,6 +61,10 @@ static struct {
     const char *manufacturer, *product, *version, *serial, *asset, *location;
 } type2;
 
+static struct {
+    const char *manufacturer, *version, *serial, *asset;
+} type3;
+
 static QemuOptsList qemu_smbios_opts = {
     .name = "smbios",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
@@ -177,6 +181,31 @@ static const QemuOptDesc qemu_smbios_type2_opts[] = {
     { /* end of list */ }
 };
 
+static const QemuOptDesc qemu_smbios_type3_opts[] = {
+    {
+        .name = "type",
+        .type = QEMU_OPT_NUMBER,
+        .help = "SMBIOS element type",
+    },{
+        .name = "manufacturer",
+        .type = QEMU_OPT_STRING,
+        .help = "manufacturer name",
+    },{
+        .name = "version",
+        .type = QEMU_OPT_STRING,
+        .help = "version number",
+    },{
+        .name = "serial",
+        .type = QEMU_OPT_STRING,
+        .help = "serial number",
+    },{
+        .name = "asset",
+        .type = QEMU_OPT_STRING,
+        .help = "asset tag number",
+    },
+    { /* end of list */ }
+};
+
 static void smbios_register_config(void)
 {
     qemu_add_opts(&qemu_smbios_opts);
@@ -350,6 +379,27 @@ static void smbios_build_type_2_table(void)
     SMBIOS_BUILD_TABLE_POST;
 }
 
+static void smbios_build_type_3_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(3, 0x300, true); /* required */
+
+    SMBIOS_TABLE_SET_STR(3, manufacturer_str, type3.manufacturer);
+    t->type = 0x01; /* Other */
+    SMBIOS_TABLE_SET_STR(3, version_str, type3.version);
+    SMBIOS_TABLE_SET_STR(3, serial_number_str, type3.serial);
+    SMBIOS_TABLE_SET_STR(3, asset_tag_number_str, type3.asset);
+    t->boot_up_state = 0x03; /* Safe */
+    t->power_supply_state = 0x03; /* Safe */
+    t->thermal_state = 0x03; /* Safe */
+    t->security_status = 0x02; /* Unknown */
+    t->oem_defined = 0;
+    t->height = 0;
+    t->number_of_power_cords = 0;
+    t->contained_element_count = 0;
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
 #define SMBIOS_SET_DEFAULT(field, value)                                  \
     if (!field) {                                                         \
         field = value;                                                    \
@@ -358,6 +408,7 @@ static void smbios_build_type_2_table(void)
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
+    const char *manufacturer_compat = "Bochs"; /* SeaBIOS compatibility */
     smbios_have_defaults = true;
     SMBIOS_SET_DEFAULT(type0.vendor, manufacturer);
     SMBIOS_SET_DEFAULT(type0.version, version);
@@ -368,6 +419,10 @@ void smbios_set_defaults(const char *manufacturer,
     SMBIOS_SET_DEFAULT(type2.manufacturer, manufacturer);
     SMBIOS_SET_DEFAULT(type2.product, product);
     SMBIOS_SET_DEFAULT(type2.version, version);
+    SMBIOS_SET_DEFAULT(type3.manufacturer, manufacturer_compat);
+    /* not set in SeaBIOS
+    SMBIOS_SET_DEFAULT(type3.version, version);
+    */
 }
 
 uint8_t *smbios_get_table(size_t *length)
@@ -376,6 +431,7 @@ uint8_t *smbios_get_table(size_t *length)
         smbios_build_type_0_table();
         smbios_build_type_1_table();
         smbios_build_type_2_table();
+        smbios_build_type_3_table();
         smbios_validate_table();
         smbios_immutable = true;
     }
@@ -522,6 +578,17 @@ void smbios_entry_add(QemuOpts *opts)
             save_opt(&type2.asset, opts, "asset");
             save_opt(&type2.location, opts, "location");
             return;
+        case 3:
+            qemu_opts_validate(opts, qemu_smbios_type3_opts, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                exit(1);
+            }
+            save_opt(&type3.manufacturer, opts, "manufacturer");
+            save_opt(&type3.version, opts, "version");
+            save_opt(&type3.serial, opts, "serial");
+            save_opt(&type3.asset, opts, "asset");
+            return;
         default:
             error_report("Don't know how to build fields for SMBIOS type %ld",
                          type);
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 08/12] SMBIOS: Build full type 4 tables

[Gabriel L. Somlo]

Build full smbios type 4 (processor information) tables, and make
them available to the bios via fw_cfg. For initial compatibility
with SeaBIOS, use "Bochs" as the default manufacturer string, and
leave version unset.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/pc.c             |  3 ++
 hw/i386/smbios.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/i386/smbios.h |  1 +
 3 files changed, 100 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e715a33..9c4623e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1011,6 +1011,9 @@ void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
         sysbus_mmio_map_overlap(SYS_BUS_DEVICE(icc_bridge), 0,
                                 APIC_DEFAULT_ADDRESS, 0x1000);
     }
+
+    /* tell smbios about cpuid version and features */
+    smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]);
 }
 
 /* pci-info ROM file. Little endian format */
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 47f7b0d..5b80021 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -42,6 +42,7 @@ static size_t smbios_entries_len;
 static int smbios_type4_count = 0;
 static bool smbios_immutable;
 static bool smbios_have_defaults;
+static uint32_t smbios_cpuid_version, smbios_cpuid_features; /* for type 4 */
 
 static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
 static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
@@ -65,6 +66,10 @@ static struct {
     const char *manufacturer, *version, *serial, *asset;
 } type3;
 
+static struct {
+    const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
+} type4;
+
 static QemuOptsList qemu_smbios_opts = {
     .name = "smbios",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
@@ -206,6 +211,39 @@ static const QemuOptDesc qemu_smbios_type3_opts[] = {
     { /* end of list */ }
 };
 
+static const QemuOptDesc qemu_smbios_type4_opts[] = {
+    {
+        .name = "type",
+        .type = QEMU_OPT_NUMBER,
+        .help = "SMBIOS element type",
+    },{
+        .name = "sock_pfx",
+        .type = QEMU_OPT_STRING,
+        .help = "socket designation string prefix",
+    },{
+        .name = "manufacturer",
+        .type = QEMU_OPT_STRING,
+        .help = "manufacturer name",
+    },{
+        .name = "version",
+        .type = QEMU_OPT_STRING,
+        .help = "version number",
+    },{
+        .name = "serial",
+        .type = QEMU_OPT_STRING,
+        .help = "serial number",
+    },{
+        .name = "asset",
+        .type = QEMU_OPT_STRING,
+        .help = "asset tag number",
+    },{
+        .name = "part",
+        .type = QEMU_OPT_STRING,
+        .help = "part number",
+    },
+    { /* end of list */ }
+};
+
 static void smbios_register_config(void)
 {
     qemu_add_opts(&qemu_smbios_opts);
@@ -400,11 +438,45 @@ static void smbios_build_type_3_table(void)
     SMBIOS_BUILD_TABLE_POST;
 }
 
+static void smbios_build_type_4_table(unsigned instance)
+{
+    char sock_str[128];
+
+    SMBIOS_BUILD_TABLE_PRE(4, 0x400 + instance, true); /* required */
+
+    snprintf(sock_str, sizeof(sock_str), "%s%2x", type4.sock_pfx, instance);
+    SMBIOS_TABLE_SET_STR(4, socket_designation_str, sock_str);
+    t->processor_type = 0x03; /* CPU */
+    t->processor_family = 0x01; /* Other */
+    SMBIOS_TABLE_SET_STR(4, processor_manufacturer_str, type4.manufacturer);
+    t->processor_id[0] = smbios_cpuid_version;
+    t->processor_id[1] = smbios_cpuid_features;
+    SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
+    t->voltage = 0;
+    t->external_clock = 0; /* Unknown */
+    t->max_speed = 2000; /* hardcoded in SeaBIOS (use 0/Unknown instead ?) */
+    t->current_speed = 2000; /* hardcoded in SeaBIOS (use 0/Unknown ?) */
+    t->status = 0x41; /* Socket populated, CPU enabled */
+    t->processor_upgrade = 0x01; /* Other */
+    t->l1_cache_handle = 0xFFFF; /* N/A */
+    t->l2_cache_handle = 0xFFFF; /* N/A */
+    t->l3_cache_handle = 0xFFFF; /* N/A */
+
+    SMBIOS_BUILD_TABLE_POST;
+    smbios_type4_count++;
+}
+
 #define SMBIOS_SET_DEFAULT(field, value)                                  \
     if (!field) {                                                         \
         field = value;                                                    \
     }
 
+void smbios_set_cpuid(uint32_t version, uint32_t features)
+{
+    smbios_cpuid_version = version;
+    smbios_cpuid_features = features;
+}
+
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version)
 {
@@ -423,15 +495,26 @@ void smbios_set_defaults(const char *manufacturer,
     /* not set in SeaBIOS
     SMBIOS_SET_DEFAULT(type3.version, version);
     */
+    SMBIOS_SET_DEFAULT(type4.sock_pfx, "CPU");
+    SMBIOS_SET_DEFAULT(type4.manufacturer, manufacturer_compat);
+    /* not set in SeaBIOS
+    SMBIOS_SET_DEFAULT(type4.version, version);
+    */
 }
 
 uint8_t *smbios_get_table(size_t *length)
 {
+    unsigned i;
+
     if (!smbios_immutable) {
         smbios_build_type_0_table();
         smbios_build_type_1_table();
         smbios_build_type_2_table();
         smbios_build_type_3_table();
+        for (i = 0; i < smp_cpus; i++) {
+            /* count CPUs starting with 1, to minimize diff vs. SeaBIOS */
+            smbios_build_type_4_table(i + 1);
+        }
         smbios_validate_table();
         smbios_immutable = true;
     }
@@ -589,6 +672,19 @@ void smbios_entry_add(QemuOpts *opts)
             save_opt(&type3.serial, opts, "serial");
             save_opt(&type3.asset, opts, "asset");
             return;
+        case 4:
+            qemu_opts_validate(opts, qemu_smbios_type4_opts, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                exit(1);
+            }
+            save_opt(&type4.sock_pfx, opts, "sock_pfx");
+            save_opt(&type4.manufacturer, opts, "manufacturer");
+            save_opt(&type4.version, opts, "version");
+            save_opt(&type4.serial, opts, "serial");
+            save_opt(&type4.asset, opts, "asset");
+            save_opt(&type4.part, opts, "part");
+            return;
         default:
             error_report("Don't know how to build fields for SMBIOS type %ld",
                          type);
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 2642e1a..af5ee01 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -18,6 +18,7 @@
 #define SMBIOS_MAX_TYPE 127
 
 void smbios_entry_add(QemuOpts *opts);
+void smbios_set_cpuid(uint32_t version, uint32_t features);
 void smbios_set_defaults(const char *manufacturer,
                          const char *product, const char *version);
 uint8_t *smbios_get_table(size_t *length);
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 09/12] SMBIOS: Build full smbios memory tables (type 16, 17, 19, and 20)

[Gabriel L. Somlo]

Build full smbios tables representing the system RAM:
  - type 16 (physical memory array): represents the entire system RAM;
  - type 17 (memory device) tables: one per virtual DIMM;
  - type 19 (memory array mapped address): represent major RAM areas
    (currently one for below-4G memory, and, if applicable, one for
     above-4G memory);
  - type 20 (memory device mapped address): mappings between type 19
    areas and type 17 DIMMs;
These tables will be made available to the bios via fw_cfg.

This patch also thoroughly documents the current memory table layout.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/pc_piix.c        |   3 +-
 hw/i386/pc_q35.c         |   3 +-
 hw/i386/smbios.c         | 256 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/i386/smbios.h |  13 ++-
 4 files changed, 264 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 8513de0..db075eb 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -146,7 +146,8 @@ static void pc_init1(QEMUMachineInitArgs *args,
     if (smbios_defaults) {
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
-                                  args->machine->name);
+                             args->machine->name,
+                             below_4g_mem_size, above_4g_mem_size);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index eacec53..3aaac7a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -133,7 +133,8 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
     if (smbios_defaults) {
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
-                                  args->machine->name);
+                             args->machine->name,
+                             below_4g_mem_size, above_4g_mem_size);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 5b80021..6510ff3 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -43,6 +43,7 @@ static int smbios_type4_count = 0;
 static bool smbios_immutable;
 static bool smbios_have_defaults;
 static uint32_t smbios_cpuid_version, smbios_cpuid_features; /* for type 4 */
+static ram_addr_t smbios_below_4g_ram, smbios_above_4g_ram; /* for type 19 */
 
 static DECLARE_BITMAP(have_binfile_bitmap, SMBIOS_MAX_TYPE+1);
 static DECLARE_BITMAP(have_fields_bitmap, SMBIOS_MAX_TYPE+1);
@@ -70,6 +71,10 @@ static struct {
     const char *sock_pfx, *manufacturer, *version, *serial, *asset, *part;
 } type4;
 
+static struct {
+    const char *loc_pfx, *bank, *manufacturer, *serial, *asset, *part;
+} type17;
+
 static QemuOptsList qemu_smbios_opts = {
     .name = "smbios",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
@@ -244,6 +249,39 @@ static const QemuOptDesc qemu_smbios_type4_opts[] = {
     { /* end of list */ }
 };
 
+static const QemuOptDesc qemu_smbios_type17_opts[] = {
+    {
+        .name = "type",
+        .type = QEMU_OPT_NUMBER,
+        .help = "SMBIOS element type",
+    },{
+        .name = "loc_pfx",
+        .type = QEMU_OPT_STRING,
+        .help = "device locator string prefix",
+    },{
+        .name = "bank",
+        .type = QEMU_OPT_STRING,
+        .help = "bank locator string",
+    },{
+        .name = "manufacturer",
+        .type = QEMU_OPT_STRING,
+        .help = "manufacturer name",
+    },{
+        .name = "serial",
+        .type = QEMU_OPT_STRING,
+        .help = "serial number",
+    },{
+        .name = "asset",
+        .type = QEMU_OPT_STRING,
+        .help = "asset tag number",
+    },{
+        .name = "part",
+        .type = QEMU_OPT_STRING,
+        .help = "part number",
+    },
+    { /* end of list */ }
+};
+
 static void smbios_register_config(void)
 {
     qemu_add_opts(&qemu_smbios_opts);
@@ -466,6 +504,117 @@ static void smbios_build_type_4_table(unsigned instance)
     smbios_type4_count++;
 }
 
+#define ONE_KB ((ram_addr_t)1 << 10)
+#define ONE_MB ((ram_addr_t)1 << 20)
+#define ONE_GB ((ram_addr_t)1 << 30)
+
+static void smbios_build_type_16_table(unsigned dimm_cnt)
+{
+    ram_addr_t  ram_size_kb = ram_size >> 10;
+
+    SMBIOS_BUILD_TABLE_PRE(16, 0x1000, true); /* required */
+
+    t->location = 0x01; /* Other */
+    t->use = 0x03; /* System memory */
+    t->error_correction = 0x06; /* Multi-bit ECC (for Microsoft, per SeaBIOS) */
+    /* if ram_size < 2T, use value in Kilobytes; 0x80000000 == 2T and over */
+    t->maximum_capacity = (ram_size_kb < 0x80000000) ? ram_size_kb : 0x80000000;
+    if (t->maximum_capacity == 0x80000000) {
+        /* TODO: support smbios v2.7 extended capacity */
+        fprintf(stderr, "qemu: warning: SMBIOS v2.7+ required for "
+                        "ram_size >= 2T (%ld)\n", ram_size);
+    }
+    t->memory_error_information_handle = 0xFFFE; /* Not provided */
+    t->number_of_memory_devices = dimm_cnt;
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
+static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
+{
+    char loc_str[128];
+    ram_addr_t size_mb;
+
+    SMBIOS_BUILD_TABLE_PRE(17, 0x1100 + instance, true); /* required */
+
+    t->physical_memory_array_handle = 0x1000; /* Type 16 (Phys. Mem. Array) */
+    t->memory_error_information_handle = 0; /* SeaBIOS, should be 0xFFFE(N/A) */
+    t->total_width = 64; /* hardcoded in SeaBIOS */
+    t->data_width = 64; /* hardcoded in SeaBIOS */
+    size_mb = QEMU_ALIGN_UP(size, ONE_MB) / ONE_MB;
+    if (size_mb < 0x7FFF) {
+        t->size = size_mb;
+    } else {
+        t->size = 0x7FFF;
+        /* TODO: support smbios v2.7 extended capacity */
+        fprintf(stderr, "qemu: warning: SMBIOS v2.7+ required for "
+                        "DIMM size >= 0x7FFF Mbytes (0x%lx)\n", size_mb);
+    }
+    t->form_factor = 0x09; /* DIMM */
+    t->device_set = 0; /* Not in a set */
+    snprintf(loc_str, sizeof(loc_str), "%s %d", type17.loc_pfx, instance);
+    SMBIOS_TABLE_SET_STR(17, device_locator_str, loc_str);
+    SMBIOS_TABLE_SET_STR(17, bank_locator_str, type17.bank);
+    t->memory_type = 0x07; /* RAM */
+    t->type_detail = 0; /* hardcoded in SeaBIOS */
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
+static void smbios_build_type_19_table(unsigned instance,
+                                       ram_addr_t start, ram_addr_t size)
+{
+    ram_addr_t end, start_kb, end_kb;
+
+    SMBIOS_BUILD_TABLE_PRE(19, 0x1300 + instance, true); /* required */
+
+    end = start + size - 1;
+    assert(end > start);
+    start_kb = start / ONE_KB;
+    end_kb = end / ONE_KB;
+    if (start_kb >= UINT32_MAX || end_kb >= UINT32_MAX) {
+        t->starting_address = t->ending_address = UINT32_MAX;
+        fprintf(stderr, "qemu: warning: SMBIOS v2.7+ required for "
+                        "type19(start=%lx, size=%lx)\n", start, size);
+    } else {
+        t->starting_address = start_kb;
+        t->ending_address = end_kb;
+    }
+    t->memory_array_handle = 0x1000; /* Type 16 (Phys. Mem. Array) */
+    t->partition_width = 1; /* One device per row */
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
+static void smbios_build_type_20_table(unsigned instance,
+                                       unsigned dev_hndl, unsigned array_hndl,
+                                       ram_addr_t start, ram_addr_t size)
+{
+    ram_addr_t end, start_kb, end_kb;
+
+    SMBIOS_BUILD_TABLE_PRE(20, 0x1400 + instance, true); /* required */
+
+    end = start + size - 1;
+    assert(end > start);
+    start_kb = start / ONE_KB;
+    end_kb = end / ONE_KB;
+    if (start_kb >= UINT32_MAX || end_kb >= UINT32_MAX) {
+        t->starting_address = t->ending_address = UINT32_MAX;
+        fprintf(stderr, "qemu: warning: SMBIOS v2.7+ required for "
+                        "type20(start=%lx, size=%lx)\n", start, size);
+    } else {
+        t->starting_address = start_kb;
+        t->ending_address = end_kb;
+    }
+    t->memory_device_handle = 0x1100 + dev_hndl; /* Type 17 (Memory Device) */
+    t->memory_array_mapped_address_handle = 0x1300 + array_hndl; /* Type 19 */
+    t->partition_row_position = 1; /* One device per row, always first pos. */
+    t->interleave_position = 0; /* Not interleaved */
+    t->interleaved_data_depth = 0; /* Not interleaved */
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
 #define SMBIOS_SET_DEFAULT(field, value)                                  \
     if (!field) {                                                         \
         field = value;                                                    \
@@ -478,10 +627,17 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
 }
 
 void smbios_set_defaults(const char *manufacturer,
-                         const char *product, const char *version)
+                         const char *product, const char *version,
+                         ram_addr_t below_4g_mem_size,
+                         ram_addr_t above_4g_mem_size)
 {
     const char *manufacturer_compat = "Bochs"; /* SeaBIOS compatibility */
     smbios_have_defaults = true;
+
+    assert(ram_size == below_4g_mem_size + above_4g_mem_size);
+    smbios_below_4g_ram = below_4g_mem_size;
+    smbios_above_4g_ram = above_4g_mem_size;
+
     SMBIOS_SET_DEFAULT(type0.vendor, manufacturer);
     SMBIOS_SET_DEFAULT(type0.version, version);
     SMBIOS_SET_DEFAULT(type0.date, "01/01/2014");
@@ -500,11 +656,12 @@ void smbios_set_defaults(const char *manufacturer,
     /* not set in SeaBIOS
     SMBIOS_SET_DEFAULT(type4.version, version);
     */
+    SMBIOS_SET_DEFAULT(type17.loc_pfx, "DIMM");
 }
 
 uint8_t *smbios_get_table(size_t *length)
 {
-    unsigned i;
+    unsigned i, dimm_cnt;
 
     if (!smbios_immutable) {
         smbios_build_type_0_table();
@@ -515,6 +672,88 @@ uint8_t *smbios_get_table(size_t *length)
             /* count CPUs starting with 1, to minimize diff vs. SeaBIOS */
             smbios_build_type_4_table(i + 1);
         }
+
+        /* SeaBIOS expects tables compliant to smbios v2.4;
+         * As such, we currently support ram_size up to 2T
+         * (relevant to type 16), and DIMM sizes up to 16G
+         * (for type 17).
+         *
+         * One type 16 (physical memory array) table is created
+         * to represent the entire given ram_size, which is then
+         * split into type 17 (memory device) DMIMMs of 16G, with
+         * the last DIMM covering the sub-16G remainder
+         * (ram_size % 16G).
+         *
+         * Up to two type 19 (memory array mapped address) tables
+         * are created: the first one covers below-4G memory, and
+         * the second, if applicable, covers above-4g memory.
+         *
+         * Tables of type 20 (memory device mapped address) are
+         * created as necessary, to connect type 17 DIMMs to
+         * type 19 memory areas.
+         *
+         * The following figure illustrates how many instances of
+         * each type are generated:
+         *
+         *  -------             -------
+         * |  T17  |           |  T17  |
+         * | <=16G |           | <=16G | ...
+         * | 1100h |<----+     | 1101h |
+         *  -------      |      -------
+         *     ^         |         ^
+         *     |         |         |
+         *  ---+---   ---+---   ---+---
+         * |  T20  | |  T20  | |  T20  |
+         * |  <4G  | |  4G+  | | <=16G | ...
+         * | 1400h | | 1401h | | 1402h |
+         *  ---+---   ---+---   ---+---
+         *     |         |         |
+         *     v         v         v
+         *  -------   -------------------...--
+         * |  T19  | |         T19            |
+         * |  <4G  | |      4G and up         |
+         * | 1300h | |        1301h           |
+         *  -------   -------------------...--
+         *
+         * With under 4G of memory, a single DIMM and a single
+         * below-4G memory area are linked together by a single
+         * type 20 device mapped address.
+         *
+         * With over 4G (but less than 16G) of memory, we still
+         * require only one DIMM, but create two memory areas,
+         * one representing the below_4g_ram, and the other one
+         * for above_4g_ram. Two type 20 device mapped address
+         * tables link our DIMM to the below_4g and above_4g
+         * areas, respectively.
+         *
+         * With over 16G of memory, we create additional DIMMs, and
+         * additional type 20 device mapped address tables to link
+         * each such additional DIMM to the above_4g_ram area.
+         */
+
+#define MAX_DIMM_SZ (16 * ONE_GB)
+#define GET_DIMM_SZ ((i < dimm_cnt - 1) ? MAX_DIMM_SZ : ram_size % MAX_DIMM_SZ)
+
+        dimm_cnt = QEMU_ALIGN_UP(ram_size, MAX_DIMM_SZ) / MAX_DIMM_SZ;
+        smbios_build_type_16_table(dimm_cnt);
+        for (i = 0; i < dimm_cnt; i++) {
+            smbios_build_type_17_table(i, GET_DIMM_SZ);
+        }
+        smbios_build_type_19_table(0, 0, smbios_below_4g_ram);
+        smbios_build_type_20_table(0, 0, 0, 0, smbios_below_4g_ram);
+        if (smbios_above_4g_ram) {
+            ram_addr_t start = 4 * ONE_GB, size;
+            smbios_build_type_19_table(1, start, smbios_above_4g_ram);
+            for (i = 0; i < dimm_cnt; i++) {
+                size = GET_DIMM_SZ;
+                if (i == 0) { /* below-4G portion of DIMM 0 already mapped */
+                    size -= smbios_below_4g_ram;
+                }
+                smbios_build_type_20_table(i + 1, i, 1, start, size);
+                start += size;
+            }
+        }
+
         smbios_validate_table();
         smbios_immutable = true;
     }
@@ -685,6 +924,19 @@ void smbios_entry_add(QemuOpts *opts)
             save_opt(&type4.asset, opts, "asset");
             save_opt(&type4.part, opts, "part");
             return;
+        case 17:
+            qemu_opts_validate(opts, qemu_smbios_type17_opts, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                exit(1);
+            }
+            save_opt(&type17.loc_pfx, opts, "loc_pfx");
+            save_opt(&type17.bank, opts, "bank");
+            save_opt(&type17.manufacturer, opts, "manufacturer");
+            save_opt(&type17.serial, opts, "serial");
+            save_opt(&type17.asset, opts, "asset");
+            save_opt(&type17.part, opts, "part");
+            return;
         default:
             error_report("Don't know how to build fields for SMBIOS type %ld",
                          type);
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index af5ee01..2a0d384 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -20,7 +20,9 @@
 void smbios_entry_add(QemuOpts *opts);
 void smbios_set_cpuid(uint32_t version, uint32_t features);
 void smbios_set_defaults(const char *manufacturer,
-                         const char *product, const char *version);
+                         const char *product, const char *version,
+                         ram_addr_t below_4g_mem_size,
+                         ram_addr_t above_4g_mem_size);
 uint8_t *smbios_get_table(size_t *length);
 
 /*
@@ -118,9 +120,7 @@ struct smbios_type_4 {
     uint16_t l3_cache_handle;
 } QEMU_PACKED;
 
-/* SMBIOS type 16 - Physical Memory Array
- *   Associated with one type 17 (Memory Device).
- */
+/* SMBIOS type 16 - Physical Memory Array */
 struct smbios_type_16 {
     struct smbios_structure_header header;
     uint8_t location;
@@ -130,9 +130,8 @@ struct smbios_type_16 {
     uint16_t memory_error_information_handle;
     uint16_t number_of_memory_devices;
 } QEMU_PACKED;
-/* SMBIOS type 17 - Memory Device
- *   Associated with one type 19
- */
+
+/* SMBIOS type 17 - Memory Device */
 struct smbios_type_17 {
     struct smbios_structure_header header;
     uint16_t physical_memory_array_handle;
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 10/12] SMBIOS: Build full tables for type 32 and 127

[Gabriel L. Somlo]

Build full smbios type 32 (system boot info) and 127 (end-of-table)
tables, and make them available via fw_cfg.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 6510ff3..b1f1d46 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -615,6 +615,22 @@ static void smbios_build_type_20_table(unsigned instance,
     SMBIOS_BUILD_TABLE_POST;
 }
 
+static void smbios_build_type_32_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(32, 0x2000, true); /* required */
+
+    memset(t->reserved, 0, 6);
+    t->boot_status = 0; /* No errors detected */
+
+    SMBIOS_BUILD_TABLE_POST;
+}
+
+static void smbios_build_type_127_table(void)
+{
+    SMBIOS_BUILD_TABLE_PRE(127, 0x7F00, true); /* required */
+    SMBIOS_BUILD_TABLE_POST;
+}
+
 #define SMBIOS_SET_DEFAULT(field, value)                                  \
     if (!field) {                                                         \
         field = value;                                                    \
@@ -754,6 +770,9 @@ uint8_t *smbios_get_table(size_t *length)
             }
         }
 
+        smbios_build_type_32_table();
+        smbios_build_type_127_table();
+
         smbios_validate_table();
         smbios_immutable = true;
     }
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 11/12] SMBIOS: Update all table definitions to smbios spec v2.3

[Gabriel L. Somlo]

Table definitions for types 4 and 17 are only up to v2.0,
so add fields specified in smbios v2.3, as expected (and
advertised) by the SeaBIOS smbios entry point structure.

In particular, OS X guests insist on type 17 being v2.3
compliant, to avoid GUI crashes when "about this mac" is
chosen in the Finder menu.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c         |  9 +++++++++
 include/hw/i386/smbios.h | 13 +++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index b1f1d46..f7a9a92 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -499,6 +499,9 @@ static void smbios_build_type_4_table(unsigned instance)
     t->l1_cache_handle = 0xFFFF; /* N/A */
     t->l2_cache_handle = 0xFFFF; /* N/A */
     t->l3_cache_handle = 0xFFFF; /* N/A */
+    SMBIOS_TABLE_SET_STR(4, serial_number_str, type4.serial);
+    SMBIOS_TABLE_SET_STR(4, asset_tag_number_str, type4.asset);
+    SMBIOS_TABLE_SET_STR(4, part_number_str, type4.part);
 
     SMBIOS_BUILD_TABLE_POST;
     smbios_type4_count++;
@@ -557,6 +560,11 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
     SMBIOS_TABLE_SET_STR(17, bank_locator_str, type17.bank);
     t->memory_type = 0x07; /* RAM */
     t->type_detail = 0; /* hardcoded in SeaBIOS */
+    t->speed = 0; /* Unknown */
+    SMBIOS_TABLE_SET_STR(17, manufacturer_str, type17.manufacturer);
+    SMBIOS_TABLE_SET_STR(17, serial_number_str, type17.serial);
+    SMBIOS_TABLE_SET_STR(17, asset_tag_number_str, type17.asset);
+    SMBIOS_TABLE_SET_STR(17, part_number_str, type17.part);
 
     SMBIOS_BUILD_TABLE_POST;
 }
@@ -673,6 +681,7 @@ void smbios_set_defaults(const char *manufacturer,
     SMBIOS_SET_DEFAULT(type4.version, version);
     */
     SMBIOS_SET_DEFAULT(type17.loc_pfx, "DIMM");
+    SMBIOS_SET_DEFAULT(type17.manufacturer, manufacturer);
 }
 
 uint8_t *smbios_get_table(size_t *length)
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 2a0d384..6bde151 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -100,7 +100,7 @@ struct smbios_type_3 {
     /* contained elements follow */
 } QEMU_PACKED;
 
-/* SMBIOS type 4 - Processor Information (v2.0) */
+/* SMBIOS type 4 - Processor Information (v2.3) */
 struct smbios_type_4 {
     struct smbios_structure_header header;
     uint8_t socket_designation_str;
@@ -118,6 +118,10 @@ struct smbios_type_4 {
     uint16_t l1_cache_handle;
     uint16_t l2_cache_handle;
     uint16_t l3_cache_handle;
+    uint8_t serial_number_str;
+    uint8_t asset_tag_number_str;
+    uint8_t part_number_str;
+
 } QEMU_PACKED;
 
 /* SMBIOS type 16 - Physical Memory Array */
@@ -131,7 +135,7 @@ struct smbios_type_16 {
     uint16_t number_of_memory_devices;
 } QEMU_PACKED;
 
-/* SMBIOS type 17 - Memory Device */
+/* SMBIOS type 17 - Memory Device (v2.3) */
 struct smbios_type_17 {
     struct smbios_structure_header header;
     uint16_t physical_memory_array_handle;
@@ -145,6 +149,11 @@ struct smbios_type_17 {
     uint8_t bank_locator_str;
     uint8_t memory_type;
     uint16_t type_detail;
+    uint16_t speed;
+    uint8_t manufacturer_str;
+    uint8_t serial_number_str;
+    uint8_t asset_tag_number_str;
+    uint8_t part_number_str;
 } QEMU_PACKED;
 
 /* SMBIOS type 19 - Memory Array Mapped Address */
-- 
1.8.5.3

[Qemu-devel] [v4 PATCH 12/12] SMBIOS: Remove SeaBIOS compatibility quirks

[Gabriel L. Somlo]

  - Replace some arbitrarily hardcoded fields with proper
    "n/a" or "unknown" values;
  - Use QEMU-supplied default manufacturer and version strings;
  - Count CPUs starting with 0 instead of 1, to maintain uniformity
    with other multiple-instance items.

Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
---
 hw/i386/smbios.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index f7a9a92..999c400 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -492,8 +492,8 @@ static void smbios_build_type_4_table(unsigned instance)
     SMBIOS_TABLE_SET_STR(4, processor_version_str, type4.version);
     t->voltage = 0;
     t->external_clock = 0; /* Unknown */
-    t->max_speed = 2000; /* hardcoded in SeaBIOS (use 0/Unknown instead ?) */
-    t->current_speed = 2000; /* hardcoded in SeaBIOS (use 0/Unknown ?) */
+    t->max_speed = 0; /* Unknown */
+    t->current_speed = 0; /* Unknown */
     t->status = 0x41; /* Socket populated, CPU enabled */
     t->processor_upgrade = 0x01; /* Other */
     t->l1_cache_handle = 0xFFFF; /* N/A */
@@ -541,9 +541,9 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
     SMBIOS_BUILD_TABLE_PRE(17, 0x1100 + instance, true); /* required */
 
     t->physical_memory_array_handle = 0x1000; /* Type 16 (Phys. Mem. Array) */
-    t->memory_error_information_handle = 0; /* SeaBIOS, should be 0xFFFE(N/A) */
-    t->total_width = 64; /* hardcoded in SeaBIOS */
-    t->data_width = 64; /* hardcoded in SeaBIOS */
+    t->memory_error_information_handle = 0xFFFE; /* Not provided */
+    t->total_width = 0xFFFF; /* Unknown */
+    t->data_width = 0xFFFF; /* Unknown */
     size_mb = QEMU_ALIGN_UP(size, ONE_MB) / ONE_MB;
     if (size_mb < 0x7FFF) {
         t->size = size_mb;
@@ -559,7 +559,7 @@ static void smbios_build_type_17_table(unsigned instance, ram_addr_t size)
     SMBIOS_TABLE_SET_STR(17, device_locator_str, loc_str);
     SMBIOS_TABLE_SET_STR(17, bank_locator_str, type17.bank);
     t->memory_type = 0x07; /* RAM */
-    t->type_detail = 0; /* hardcoded in SeaBIOS */
+    t->type_detail = 0x02; /* Other */
     t->speed = 0; /* Unknown */
     SMBIOS_TABLE_SET_STR(17, manufacturer_str, type17.manufacturer);
     SMBIOS_TABLE_SET_STR(17, serial_number_str, type17.serial);
@@ -655,7 +655,6 @@ void smbios_set_defaults(const char *manufacturer,
                          ram_addr_t below_4g_mem_size,
                          ram_addr_t above_4g_mem_size)
 {
-    const char *manufacturer_compat = "Bochs"; /* SeaBIOS compatibility */
     smbios_have_defaults = true;
 
     assert(ram_size == below_4g_mem_size + above_4g_mem_size);
@@ -671,15 +670,11 @@ void smbios_set_defaults(const char *manufacturer,
     SMBIOS_SET_DEFAULT(type2.manufacturer, manufacturer);
     SMBIOS_SET_DEFAULT(type2.product, product);
     SMBIOS_SET_DEFAULT(type2.version, version);
-    SMBIOS_SET_DEFAULT(type3.manufacturer, manufacturer_compat);
-    /* not set in SeaBIOS
+    SMBIOS_SET_DEFAULT(type3.manufacturer, manufacturer);
     SMBIOS_SET_DEFAULT(type3.version, version);
-    */
     SMBIOS_SET_DEFAULT(type4.sock_pfx, "CPU");
-    SMBIOS_SET_DEFAULT(type4.manufacturer, manufacturer_compat);
-    /* not set in SeaBIOS
+    SMBIOS_SET_DEFAULT(type4.manufacturer, manufacturer);
     SMBIOS_SET_DEFAULT(type4.version, version);
-    */
     SMBIOS_SET_DEFAULT(type17.loc_pfx, "DIMM");
     SMBIOS_SET_DEFAULT(type17.manufacturer, manufacturer);
 }
@@ -694,8 +689,7 @@ uint8_t *smbios_get_table(size_t *length)
         smbios_build_type_2_table();
         smbios_build_type_3_table();
         for (i = 0; i < smp_cpus; i++) {
-            /* count CPUs starting with 1, to minimize diff vs. SeaBIOS */
-            smbios_build_type_4_table(i + 1);
+            smbios_build_type_4_table(i);
         }
 
         /* SeaBIOS expects tables compliant to smbios v2.4;
-- 
1.8.5.3

[Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Tue, Mar 18, 2014 at 07:23:17PM -0400, Gabriel L. Somlo wrote:
> At this point, can anyone with access to a real, physical, NUMA
> system dump the smbios tables with dmidecode and post them here?
> I think that would be very informative.

So I thrashed around a bit trying to find a real NUMA box,
and found a Dell R410 whose BIOS claims to support NUMA by
disabling the "Node Interleaving" option (haven't actually
configured it to run NUMA, but based on what else I found,
I no longer think I need to -- keep reading :)

So, to my surprise, I noticed this machine did NOT have any
Type 20 tables in SMBIOS at all !!!

Then, after a more careful reading of the SMBIOS manual, I
noticed that Type 20 was made OPTIONAL as v2.5 of the spec,
cca. 2006 !!!

In conclusion, we *could* simply scan e820 for E820_RAM type
regions and generate Type 19 tables for each one we find,
and we no longer have to care at all about generating Type 20
nodes to link 19s to 17s, which basically makes the problem
go away, as far as I am concerned.

I tested this (omitting Type 20) on OS X (10.6 through 10.9),
XPsp3 and Win7, successfully, without either of them noticing
any changes from previous executions where smbios tables
were "different" (i.e., when they used to include T20).


At this point, though, I'd like some feedback before I shoot
out another version (v5) of this patch set:


- Should I pretend we're generating v2.5 tables ?

  	- this means Type 4 now has extra fields (i.e. the
	  number of cores, etc), which should be relatively
	  easy to add, so I'd be OK doing that, if we agree
	  on everything else. Heck, how about going all out
	  and implementing v2.8 ? That would get us past the
	  2T limit in Types 16, 17, and 19 !

- SeaBIOS is still in charge of providing the smbios_entry_point
  structure, and it's unlikely we can reasonably expect it to
  bump the version to 2.5 (not that it seems to matter, if my
  tests are to be believed)

	- on that note, SeaBIOS will still cheerfully generate
	  Type 20 tables if nothing is supplied to it via fw_cfg
	  from QEMU, and, again, I'm not sure how likely it is
	  we can get it to stop doing that :)

- Does anyone still care ? ;)


Let me know what you all think!

Thanks,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Wed, Mar 26, 2014 at 03:58:50PM -0400, Gabriel L. Somlo wrote:
> - SeaBIOS is still in charge of providing the smbios_entry_point
>   structure, and it's unlikely we can reasonably expect it to
>   bump the version to 2.5 (not that it seems to matter, if my
>   tests are to be believed)

This is why ultimately we want to use the romfile_loader mechanism for
smbios - that interface will allow qemu to generate both the smbios
table and the smbios entry point.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Wed, Mar 26, 2014 at 06:36:10PM -0400, Kevin O'Connor wrote:
> On Wed, Mar 26, 2014 at 03:58:50PM -0400, Gabriel L. Somlo wrote:
> > - SeaBIOS is still in charge of providing the smbios_entry_point
> >   structure, and it's unlikely we can reasonably expect it to
> >   bump the version to 2.5 (not that it seems to matter, if my
> >   tests are to be believed)
> 
> This is why ultimately we want to use the romfile_loader mechanism for
> smbios - that interface will allow qemu to generate both the smbios
> table and the smbios entry point.

Ah, so romfile_loader is the "whole-blob" fw_cfg transfer method (so
far in smbios.c we've been dealing with individual fields, and
individual table types).

The only sticking point remaining would be who gets to generate the
Type 0 (BIOS Information) table and when, which is something QEMU
should arguably NOT be doing on behalf of SeaBIOS (or OVMF, or ...).

I guess everyone's busy with QEMU 2.0, so I'll keep playing with this
stuff on my own and bring it up again afterwards.

Obviously, more comments and feedback are welcome at any time, should
there be any interest :)

Thanks,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 03/31/14 22:18, Gabriel L. Somlo wrote:
> On Wed, Mar 26, 2014 at 06:36:10PM -0400, Kevin O'Connor wrote:
>> On Wed, Mar 26, 2014 at 03:58:50PM -0400, Gabriel L. Somlo wrote:
>>> - SeaBIOS is still in charge of providing the smbios_entry_point
>>>   structure, and it's unlikely we can reasonably expect it to
>>>   bump the version to 2.5 (not that it seems to matter, if my
>>>   tests are to be believed)
>>
>> This is why ultimately we want to use the romfile_loader mechanism for
>> smbios - that interface will allow qemu to generate both the smbios
>> table and the smbios entry point.
> 
> Ah, so romfile_loader is the "whole-blob" fw_cfg transfer method (so
> far in smbios.c we've been dealing with individual fields, and
> individual table types).
> 
> The only sticking point remaining would be who gets to generate the
> Type 0 (BIOS Information) table and when, which is something QEMU
> should arguably NOT be doing on behalf of SeaBIOS (or OVMF, or ...).
> 
> I guess everyone's busy with QEMU 2.0, so I'll keep playing with this
> stuff on my own and bring it up again afterwards.
> 
> Obviously, more comments and feedback are welcome at any time, should
> there be any interest :)

Sorry I can follow this discussion only intermittently.

- I think OVMF would be fine with the Type 0 table as-is (it has a
default table and adheres to field-level patches). Full tables for other
types would be welcome.

- In edk2 (and on platforms that conform to the platform init (PI)
spec), platform drivers install their smbios tables through
EFI_SMBIOS_PROTOCOL. (Documented in Vol5 of the PI spec.)

In edk2, the "central" driver that produces this protocol (== provides
the service) is "MdeModulePkg/Universal/SmbiosDxe".

The OVMF platform driver that installs the smbios tables through the
protocol (== consumes the service) is "OvmfPkg/SmbiosPlatformDxe".

The set and the contents of the smbios tables are the jurisdiction of
the platform driver (the service consumer) -- the driver in OvmfPkg.

The smbios entry point (including the smbios std version) is the
jurisdiction of the service producer -- the driver in MdeModulePkg.

The platform driver can query the smbios std version from
EFI_SMBIOS_PROTOCOL. The platform driver should also filter the tables
it intends to install through the protocol so that the table types
conform to the smbios std version supported by the service provider.

This is to say, OVMF can fetch tables via fw_cfg, and install them via
the EFI_SMBIOS_PROTOCOL instance that "MdeModulePkg/Universal/SmbiosDxe"
provides. However, OVMF can't change the SMBIOS version or other aspects
of the smbios entry point. At best OVMF could filter out tables (grabbed
from fw_cfg) that are above the std version that EFI_SMBIOS_PROTOCOL
supports/advertises.

Currently OVMF does no such filtering (it would require hard-coding
which table type is defined in which smbios version). It does require
support for version 2.3, or it gives up.

In any case, the SMBIOS version currently supported by
"MdeModulePkg/Universal/SmbiosDxe" is 2.7.1 (SVN r11690), which is quite
permissive.

Summary:
- type 0 should be OK as-is, other types are most welcome as full blobs,
- OvmfPkg can't influence the smbios version advertised, it belongs to
qanother (central) driver in edk2 that is built into OVMF.fd,
- OvmfPkg currently doesn't filter fw_cfg table types against the smbios
version (determined by that other, central driver in edk2),
- however that version is reasonably high (2.7.1).

(Note: most of "OvmfPkg/SmbiosPlatformDxe" that's relevant for the above
is not in upstream edk2. You can find the patches in
<http://www.kraxel.org/repos/manual/edk2/> and
<http://copr-be.cloud.fedoraproject.org/results/bonzini/ovmf/>.)

Thanks
Laszlo

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Tue, Apr 01, 2014 at 10:40:00AM +0200, Laszlo Ersek wrote:
> On 03/31/14 22:18, Gabriel L. Somlo wrote:
> > The only sticking point remaining would be who gets to generate the
> > Type 0 (BIOS Information) table and when, which is something QEMU
> > should arguably NOT be doing on behalf of SeaBIOS (or OVMF, or ...).
> > 
> > I guess everyone's busy with QEMU 2.0, so I'll keep playing with this
> > stuff on my own and bring it up again afterwards.
> > 
> > Obviously, more comments and feedback are welcome at any time, should
> > there be any interest :)
> 
> Sorry I can follow this discussion only intermittently.
> 
> - I think OVMF would be fine with the Type 0 table as-is (it has a
> default table and adheres to field-level patches). Full tables for other
> types would be welcome.

When SeaBIOS generates the smbios table, it creates a hardcoded type 0
sub-table.  (See SeaBIOS code src/fw/smbios.c:smbios_init_type_0.)  If
OVMF is okay with the same hardcodes, then I'd suggest QEMU create the
table the same way SeaBIOS currently does.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 04/01/14 16:39, Kevin O'Connor wrote:
> On Tue, Apr 01, 2014 at 10:40:00AM +0200, Laszlo Ersek wrote:
>> On 03/31/14 22:18, Gabriel L. Somlo wrote:
>>> The only sticking point remaining would be who gets to generate the
>>> Type 0 (BIOS Information) table and when, which is something QEMU
>>> should arguably NOT be doing on behalf of SeaBIOS (or OVMF, or ...).
>>>
>>> I guess everyone's busy with QEMU 2.0, so I'll keep playing with this
>>> stuff on my own and bring it up again afterwards.
>>>
>>> Obviously, more comments and feedback are welcome at any time, should
>>> there be any interest :)
>>
>> Sorry I can follow this discussion only intermittently.
>>
>> - I think OVMF would be fine with the Type 0 table as-is (it has a
>> default table and adheres to field-level patches). Full tables for other
>> types would be welcome.
> 
> When SeaBIOS generates the smbios table, it creates a hardcoded type 0
> sub-table.  (See SeaBIOS code src/fw/smbios.c:smbios_init_type_0.)  If
> OVMF is okay with the same hardcodes, then I'd suggest QEMU create the
> table the same way SeaBIOS currently does.

In SeaBIOS,

    if (!get_field(0, offsetof(struct smbios_type_0,
                               bios_characteristics_extension_bytes),
                   &p->bios_characteristics_extension_bytes)) {
        p->bios_characteristics_extension_bytes[0] = 0;
        /* Enable targeted content distribution. Needed for SVVP */
        p->bios_characteristics_extension_bytes[1] = 4;
    }

bit 2 of the BIOS Characteristics Extension Byte 2 (7.1.2.2) is set, for
"Enable Targeted Content Distribution".

In OVMF, the same byte has the following bits set:

Bit 3 -- UEFI Specification is supported.
Bit 4 -- The SMBIOS table describes a virtual machine. (If this bit is
         not set, no inference can be made about the virtuality of the
         system.)

I have nothing against bit 2 (I didn't include it because I had no clue
what it meant, but we can certainly set that bit down the road). Bit 3
would be wrong for SeaBIOS, and it would be wrong to leave clear for
OVMF. Bit 4 would be wrong for SeaBIOS (as a static default), but is
correct (and very nice, although not necessary) for OVMF.

Gerd posted a textual comparison before (with dmidecode):

  http://thread.gmane.org/gmane.comp.emulators.qemu/260804/focus=261248

But I'm including a textual diff too (RHEL-7 host, RHEL-7 guests):

> --- seabios-rhel7.txt	2014-04-01 17:39:49.148601405 +0200
> +++ ovmf-rhel7.txt	2014-04-01 17:40:01.075658204 +0200
> @@ -1,128 +1,34 @@
>  # dmidecode 2.12
> -SMBIOS 2.4 present.
> -11 structures occupying 369 bytes.
> -Table at 0x000FDD80.
> +# SMBIOS entry point at 0x3fed8000
> +SMBIOS 2.7 present.
> +3 structures occupying 185 bytes.
> +Table at 0x3FED7000.
>
>  Handle 0x0000, DMI type 0, 24 bytes
>  BIOS Information
> -	Vendor: Bochs
> -	Version: Bochs
> -	Release Date: 01/01/2011
> +	Vendor: EFI Development Kit II / OVMF
> +	Version: 0.1
> +	Release Date: 06/03/2013
>  	Address: 0xE8000
>  	Runtime Size: 96 kB
>  	ROM Size: 64 kB
>  	Characteristics:
>  		BIOS characteristics not supported
> -		Targeted content distribution is supported
> -	BIOS Revision: 1.0
> +		UEFI is supported
> +		System is a virtual machine
> +	BIOS Revision: 0.1

Of course, this is not to say that SeaBIOS and OVMF should continue
patching the Type 0 table field-wise. If we can control the Table 0
fields on this level of detail on the qemu command line, and the parent
of qemu (libvirtd or the user's shell) takes care to set them in
accordance with the guest firmware, then the full blob suffices for
Table 0 too.

(I'm retaining the rest of the diff below.)

Thanks
Laszlo

>
> -Handle 0x0100, DMI type 1, 27 bytes
> +Handle 0x0001, DMI type 1, 27 bytes
>  System Information
>  	Manufacturer: Red Hat
>  	Product Name: KVM
>  	Version: RHEL 7.0.0 PC (i440FX + PIIX, 1996)
> -	Serial Number: Not Specified
> -	UUID: C0F384E5-6AEA-46E9-8340-E5AF2F0DCC9B
> +	Serial Number: n/a
> +	UUID: 8A2A6D47-99A6-486D-AFAE-A53741464C37
>  	Wake-up Type: Power Switch
> -	SKU Number: Not Specified
> +	SKU Number: n/a
>  	Family: Red Hat Enterprise Linux
>
> -Handle 0x0300, DMI type 3, 20 bytes
> -Chassis Information
> -	Manufacturer: Bochs
> -	Type: Other
> -	Lock: Not Present
> -	Version: Not Specified
> -	Serial Number: Not Specified
> -	Asset Tag: Not Specified
> -	Boot-up State: Safe
> -	Power Supply State: Safe
> -	Thermal State: Safe
> -	Security Status: Unknown
> -	OEM Information: 0x00000000
> -	Height: Unspecified
> -	Number Of Power Cords: Unspecified
> -
> -Handle 0x0401, DMI type 4, 32 bytes
> -Processor Information
> -	Socket Designation: CPU 1
> -	Type: Central Processor
> -	Family: Other
> -	Manufacturer: Bochs
> -	ID: 63 06 00 00 FD FB 8B 07
> -	Version: Not Specified
> -	Voltage: Unknown
> -	External Clock: Unknown
> -	Max Speed: 2000 MHz
> -	Current Speed: 2000 MHz
> -	Status: Populated, Enabled
> -	Upgrade: Other
> -	L1 Cache Handle: Not Provided
> -	L2 Cache Handle: Not Provided
> -	L3 Cache Handle: Not Provided
> -
> -Handle 0x0402, DMI type 4, 32 bytes
> -Processor Information
> -	Socket Designation: CPU 2
> -	Type: Central Processor
> -	Family: Other
> -	Manufacturer: Bochs
> -	ID: 63 06 00 00 FD FB 8B 07
> -	Version: Not Specified
> -	Voltage: Unknown
> -	External Clock: Unknown
> -	Max Speed: 2000 MHz
> -	Current Speed: 2000 MHz
> -	Status: Populated, Enabled
> -	Upgrade: Other
> -	L1 Cache Handle: Not Provided
> -	L2 Cache Handle: Not Provided
> -	L3 Cache Handle: Not Provided
> -
> -Handle 0x1000, DMI type 16, 15 bytes
> -Physical Memory Array
> -	Location: Other
> -	Use: System Memory
> -	Error Correction Type: Multi-bit ECC
> -	Maximum Capacity: 1 GB
> -	Error Information Handle: Not Provided
> -	Number Of Devices: 1
> -
> -Handle 0x1100, DMI type 17, 21 bytes
> -Memory Device
> -	Array Handle: 0x1000
> -	Error Information Handle: 0x0000
> -	Total Width: 64 bits
> -	Data Width: 64 bits
> -	Size: 1024 MB
> -	Form Factor: DIMM
> -	Set: None
> -	Locator: DIMM 0
> -	Bank Locator: Not Specified
> -	Type: RAM
> -	Type Detail: None
> -
> -Handle 0x1300, DMI type 19, 15 bytes
> -Memory Array Mapped Address
> -	Starting Address: 0x00000000000
> -	Ending Address: 0x0003FFFFFFF
> -	Range Size: 1 GB
> -	Physical Array Handle: 0x1000
> -	Partition Width: 1
> -
> -Handle 0x1400, DMI type 20, 19 bytes
> -Memory Device Mapped Address
> -	Starting Address: 0x00000000000
> -	Ending Address: 0x0003FFFFFFF
> -	Range Size: 1 GB
> -	Physical Device Handle: 0x1100
> -	Memory Array Mapped Address Handle: 0x1300
> -	Partition Row Position: 1
> -
> -Handle 0x2000, DMI type 32, 11 bytes
> -System Boot Information
> -	Status: No errors detected
> -
> -Handle 0x7F00, DMI type 127, 4 bytes
> +Handle 0xFEFF, DMI type 127, 4 bytes
>  End Of Table
>

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Tue, Apr 01, 2014 at 05:47:09PM +0200, Laszlo Ersek wrote:
> On 04/01/14 16:39, Kevin O'Connor wrote:
> > On Tue, Apr 01, 2014 at 10:40:00AM +0200, Laszlo Ersek wrote:
> >> On 03/31/14 22:18, Gabriel L. Somlo wrote:
> >>> The only sticking point remaining would be who gets to generate the
> >>> Type 0 (BIOS Information) table and when, which is something QEMU
> >>> should arguably NOT be doing on behalf of SeaBIOS (or OVMF, or ...).
> >>>
> >>
> >> Sorry I can follow this discussion only intermittently.
> >>
> >> - I think OVMF would be fine with the Type 0 table as-is (it has a
> >> default table and adheres to field-level patches). Full tables for other
> >> types would be welcome.
> > 
> > When SeaBIOS generates the smbios table, it creates a hardcoded type 0
> > sub-table.  (See SeaBIOS code src/fw/smbios.c:smbios_init_type_0.)  If
> > OVMF is okay with the same hardcodes, then I'd suggest QEMU create the
> > table the same way SeaBIOS currently does.

OK, smbios_init_type_0() is called by the add_struct() macro in
smbios_setup(), but not if fw_cfg already holds a QEMU-supplied
type 0 table.

When it is called, smbios_init_type_0() will look for individual
fields supplied via fw_cfg, and only use its own defaults otherwise.

By default, QEMU will not set any type 0 fields, nor generate
a type 0 table, which means that each specific BIOS gets to
supply its own default type 0 table.

My patch set supplies various full tables via fw_cfg, but by default
it still doesn't built a type 0. That only happens if either a binary
table or default fields for type 0 are provided on the command line
(same as the current behavior, except I'm always sending a full table
rather than field-by-field).

> bit 2 of the BIOS Characteristics Extension Byte 2 (7.1.2.2) is set, for
> "Enable Targeted Content Distribution".
> 
> In OVMF, the same byte has the following bits set:
> 
> Bit 3 -- UEFI Specification is supported.
> Bit 4 -- The SMBIOS table describes a virtual machine. (If this bit is
>          not set, no inference can be made about the virtuality of the
>          system.)
> 
> I have nothing against bit 2 (I didn't include it because I had no clue
> what it meant, but we can certainly set that bit down the road). Bit 3
> would be wrong for SeaBIOS, and it would be wrong to leave clear for
> OVMF. Bit 4 would be wrong for SeaBIOS (as a static default), but is
> correct (and very nice, although not necessary) for OVMF.

I can add an extra command line option for type 0 defaults (e.g.
"char_ext" but we can pick a better name):

    "-smbios type=0,vendor='foo',version='x.y',char_ext=4"

... and make the user responsible for supplying the correct value
if/when they wish to override the defaults. I'll do that in the
v5 patch set I'm working on right now :)

As an aside, I think in the end it doesn't matter much if we supply
individual field defaults or full tables for *optional* types such
as type 0. I just like to generate full tables across the board to
keep reusing the same code, rather than leave the individual-field
logic in just for this one table type...

>From the conversation so far, it seems to me that:

	- type 0 is best left to the BIOS (user overrides via
	  command line at their own risk)

	- therefore, the maximum granularity of QEMU-generated
	  elements should be full tables of a given type, and
	  not the full SMBIOS blob at once (other mechanisms to
	  allow the BIOS to insert its own type 0 welcome, but
	  so far this seems the most straightforward one).

	- this means the smbios structure header has to be left
	  up to the BIOS

	- the BIOS is then responsible for setting the smbios
	  spec version (2.4 for SeaBIOS, 2.7.1 for OVMF).

On that last point, at least Linux seems to be OK with individual
type tables having a higher version than the structure header; i.e.,
dmidecode works fine when e.g. the structure header says 2.4 but
the type 4 cpu record is 2.6. I'll test on Windows and OS X as well,
and post my results here.

My one remaining question is: how do we get the BIOS to *not* generate
a certain table type that's being left out on purpose by QEMU ?

I'm talking here of type 20, which is no longer required as of spec
v2.5, and which would unnecessarily complicate things if/when more
than two E820_RAM memory areas are present...

Thanks much,
--Gabriel 

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Tue, Apr 01, 2014 at 02:47:27PM -0400, Gabriel L. Somlo wrote:
> On Tue, Apr 01, 2014 at 05:47:09PM +0200, Laszlo Ersek wrote:
> > bit 2 of the BIOS Characteristics Extension Byte 2 (7.1.2.2) is set, for
> > "Enable Targeted Content Distribution".
> > 
> > In OVMF, the same byte has the following bits set:
> > 
> > Bit 3 -- UEFI Specification is supported.
> > Bit 4 -- The SMBIOS table describes a virtual machine. (If this bit is
> >          not set, no inference can be made about the virtuality of the
> >          system.)
> > 
> > I have nothing against bit 2 (I didn't include it because I had no clue
> > what it meant, but we can certainly set that bit down the road). Bit 3
> > would be wrong for SeaBIOS, and it would be wrong to leave clear for
> > OVMF. Bit 4 would be wrong for SeaBIOS (as a static default), but is
> > correct (and very nice, although not necessary) for OVMF.
> 
> I can add an extra command line option for type 0 defaults (e.g.
> "char_ext" but we can pick a better name):
> 
>     "-smbios type=0,vendor='foo',version='x.y',char_ext=4"
> 
> ... and make the user responsible for supplying the correct value
> if/when they wish to override the defaults. I'll do that in the
> v5 patch set I'm working on right now :)
> 
> As an aside, I think in the end it doesn't matter much if we supply
> individual field defaults or full tables for *optional* types such
> as type 0. I just like to generate full tables across the board to
> keep reusing the same code, rather than leave the individual-field
> logic in just for this one table type...
> 
> From the conversation so far, it seems to me that:
> 
> 	- type 0 is best left to the BIOS (user overrides via
> 	  command line at their own risk)
> 
> 	- therefore, the maximum granularity of QEMU-generated
> 	  elements should be full tables of a given type, and
> 	  not the full SMBIOS blob at once (other mechanisms to
> 	  allow the BIOS to insert its own type 0 welcome, but
> 	  so far this seems the most straightforward one).

I don't agree - I think ultimately we want QEMU to generate the full
SMBIOS table and pass it to the firmware via the romfile_loader
mechanism.  The only thing that has been raised as an issue with this
is one bit in the smbios table (UEFI support).  For this one bit, I
think QEMU can just put together a sane default and the firmware can
patch up the one bit (either manually or via a new romfile_loader
command).

> 
> 	- this means the smbios structure header has to be left
> 	  up to the BIOS
> 
> 	- the BIOS is then responsible for setting the smbios
> 	  spec version (2.4 for SeaBIOS, 2.7.1 for OVMF).
> 
> On that last point, at least Linux seems to be OK with individual
> type tables having a higher version than the structure header; i.e.,
> dmidecode works fine when e.g. the structure header says 2.4 but
> the type 4 cpu record is 2.6. I'll test on Windows and OS X as well,
> and post my results here.
> 
> My one remaining question is: how do we get the BIOS to *not* generate
> a certain table type that's being left out on purpose by QEMU ?
> 
> I'm talking here of type 20, which is no longer required as of spec
> v2.5, and which would unnecessarily complicate things if/when more
> than two E820_RAM memory areas are present...

The above are good examples why I think QEMU should be the sole owner
of the SMBIOS.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Tue, Apr 01, 2014 at 04:28:32PM -0400, Kevin O'Connor wrote:
> > From the conversation so far, it seems to me that:
> > 
> > 	- type 0 is best left to the BIOS (user overrides via
> > 	  command line at their own risk)
> > 
> > 	- therefore, the maximum granularity of QEMU-generated
> > 	  elements should be full tables of a given type, and
> > 	  not the full SMBIOS blob at once (other mechanisms to
> > 	  allow the BIOS to insert its own type 0 welcome, but
> > 	  so far this seems the most straightforward one).
> 
> I don't agree - I think ultimately we want QEMU to generate the full
> SMBIOS table and pass it to the firmware via the romfile_loader
> mechanism.  The only thing that has been raised as an issue with this
> is one bit in the smbios table (UEFI support).  For this one bit, I
> think QEMU can just put together a sane default and the firmware can
> patch up the one bit (either manually or via a new romfile_loader
> command).
> 
> > 
> > 	- this means the smbios structure header has to be left
> > 	  up to the BIOS
> > 
> > 	- the BIOS is then responsible for setting the smbios
> > 	  spec version (2.4 for SeaBIOS, 2.7.1 for OVMF).
> > 
> > On that last point, at least Linux seems to be OK with individual
> > type tables having a higher version than the structure header; i.e.,
> > dmidecode works fine when e.g. the structure header says 2.4 but
> > the type 4 cpu record is 2.6. I'll test on Windows and OS X as well,
> > and post my results here.
> > 
> > My one remaining question is: how do we get the BIOS to *not* generate
> > a certain table type that's being left out on purpose by QEMU ?
> > 
> > I'm talking here of type 20, which is no longer required as of spec
> > v2.5, and which would unnecessarily complicate things if/when more
> > than two E820_RAM memory areas are present...
> 
> The above are good examples why I think QEMU should be the sole owner
> of the SMBIOS.

Assuming all relevant QEMU maintainers are OK with the idea of
creating a full SMBIOS blob (with e.g. type 0 defaulting to the
relevant SeaBIOS values, override-able to fit some different bios,
e.g. OVMF), would you take a patch to check for this blob in
smbios_setup() (in SeaBIOS src/fw/smbios.c) ? Right now, it's either
individual fields or table-at-a-time blobs only, AFAICT.

Assuming "yes", would OVMF accept a similar patch (unless it's already
set up to receive such a blob, I forget whether that came up earlier
in the thread) ?

Thanks,
  Gabriel

--
In Soviet Russia, problem divide-and-conquer YOU ;)

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 04/01/14 23:28, Gabriel L. Somlo wrote:
> On Tue, Apr 01, 2014 at 04:28:32PM -0400, Kevin O'Connor wrote:
>>> From the conversation so far, it seems to me that:
>>>
>>> 	- type 0 is best left to the BIOS (user overrides via
>>> 	  command line at their own risk)
>>>
>>> 	- therefore, the maximum granularity of QEMU-generated
>>> 	  elements should be full tables of a given type, and
>>> 	  not the full SMBIOS blob at once (other mechanisms to
>>> 	  allow the BIOS to insert its own type 0 welcome, but
>>> 	  so far this seems the most straightforward one).
>>
>> I don't agree - I think ultimately we want QEMU to generate the full
>> SMBIOS table and pass it to the firmware via the romfile_loader
>> mechanism.  The only thing that has been raised as an issue with this
>> is one bit in the smbios table (UEFI support).  For this one bit, I
>> think QEMU can just put together a sane default and the firmware can
>> patch up the one bit (either manually or via a new romfile_loader
>> command).
>>
>>>
>>> 	- this means the smbios structure header has to be left
>>> 	  up to the BIOS
>>>
>>> 	- the BIOS is then responsible for setting the smbios
>>> 	  spec version (2.4 for SeaBIOS, 2.7.1 for OVMF).
>>>
>>> On that last point, at least Linux seems to be OK with individual
>>> type tables having a higher version than the structure header; i.e.,
>>> dmidecode works fine when e.g. the structure header says 2.4 but
>>> the type 4 cpu record is 2.6. I'll test on Windows and OS X as well,
>>> and post my results here.
>>>
>>> My one remaining question is: how do we get the BIOS to *not* generate
>>> a certain table type that's being left out on purpose by QEMU ?
>>>
>>> I'm talking here of type 20, which is no longer required as of spec
>>> v2.5, and which would unnecessarily complicate things if/when more
>>> than two E820_RAM memory areas are present...
>>
>> The above are good examples why I think QEMU should be the sole owner
>> of the SMBIOS.
> 
> Assuming all relevant QEMU maintainers are OK with the idea of
> creating a full SMBIOS blob (with e.g. type 0 defaulting to the
> relevant SeaBIOS values, override-able to fit some different bios,
> e.g. OVMF), would you take a patch to check for this blob in
> smbios_setup() (in SeaBIOS src/fw/smbios.c) ? Right now, it's either
> individual fields or table-at-a-time blobs only, AFAICT.
> 
> Assuming "yes", would OVMF accept a similar patch (unless it's already
> set up to receive such a blob, I forget whether that came up earlier
> in the thread) ?

Right now, OVMF can accept individual fields, or table-at-a-time blobs,
via fw_cfg.

The internal interface (EFI_SMBIOS_PROTOCOL) expects one table at a time
(for which table-at-a-time blobs are a perfect match).

If qemu gives OVMF a complete, concatenated dump of all tables, I'll
have to split that up into individual tables, and install those one by one.

qemu --[fw_cfg]--> OVMF platform code --[EFI_SMBIOS_PROTOCOL]--> edk2
     "some" format:                       strictly per-table
     - field patch
     - per-table blob
     - complete dump?

I think that concatenating table-at-a-time blobs in SeaBIOS is easier
than parsing & splitting a complete dump into tables in OVMF.

Kevin might disagree of course :)

Thanks
Laszlo

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Tue, Apr 01, 2014 at 11:44:12PM +0200, Laszlo Ersek wrote:
> Right now, OVMF can accept individual fields, or table-at-a-time blobs,
> via fw_cfg.
> 
> The internal interface (EFI_SMBIOS_PROTOCOL) expects one table at a time
> (for which table-at-a-time blobs are a perfect match).

I wasn't aware of this.  The SMBIOS spec calls for all the sub-tables
to be concatenanted into a single linear area of memory.  Is there
something in EFI or OVMF that is dictating otherwise?  Can you provide
a link so I can further understand?  (I briefly checked through the
UEFI v2.3.1 spec and nothing popped out at me.)

> I think that concatenating table-at-a-time blobs in SeaBIOS is easier
> than parsing & splitting a complete dump into tables in OVMF.

I don't think it's very difficult either way.  It would be nice,
though, if there was just one owner for the smbios.  The current setup
where some data comes from QEMU and some from the firmware, along with
mechanisms for providing defaults and overrides is way too complex in
my opinion.

Thanks,
-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 04/02/14 00:00, Kevin O'Connor wrote:
> On Tue, Apr 01, 2014 at 11:44:12PM +0200, Laszlo Ersek wrote:
>> Right now, OVMF can accept individual fields, or table-at-a-time blobs,
>> via fw_cfg.
>>
>> The internal interface (EFI_SMBIOS_PROTOCOL) expects one table at a time
>> (for which table-at-a-time blobs are a perfect match).
> 
> I wasn't aware of this.  The SMBIOS spec calls for all the sub-tables
> to be concatenanted into a single linear area of memory.  Is there
> something in EFI or OVMF that is dictating otherwise?  Can you provide
> a link so I can further understand?  (I briefly checked through the
> UEFI v2.3.1 spec and nothing popped out at me.)

The "UEFI Specification" is not relevant here, the "UEFI Platform
Initialization (PI) Specification" is.

You can download the PI spec at <http://www.uefi.org/specs/access>. The
relevant section is (I have version 1.2.1):

VOLUME 5: Platform Initialization Specification
Standards
6 SMBIOS Protocol

The function to call is EFI_SMBIOS_PROTOCOL.Add().

>> I think that concatenating table-at-a-time blobs in SeaBIOS is easier
>> than parsing & splitting a complete dump into tables in OVMF.
> 
> I don't think it's very difficult either way.  It would be nice,
> though, if there was just one owner for the smbios.  The current setup
> where some data comes from QEMU and some from the firmware, along with
> mechanisms for providing defaults and overrides is way too complex in
> my opinion.

I certainly agree with the direction. I'm OK with the current
table-at-a-time blobs (which should leave only the SMBIOS entry point to
the firmware). I'm also OK with any new, comprehensive format that
allows me, with reasonable parsing, to identify the individual tables in
the big concat (and to throw away the passed down entry point).

I already wrote display_uuid() [src/fw/smbios.c] for SeaBIOS, so I guess
I could repeat the exercise if it's unavoidable... :)

Thanks
Laszlo

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Wed, Apr 02, 2014 at 12:35:26AM +0200, Laszlo Ersek wrote:
> On 04/02/14 00:00, Kevin O'Connor wrote:
> > On Tue, Apr 01, 2014 at 11:44:12PM +0200, Laszlo Ersek wrote:
> >> Right now, OVMF can accept individual fields, or table-at-a-time blobs,
> >> via fw_cfg.
> >>
> >> The internal interface (EFI_SMBIOS_PROTOCOL) expects one table at a time
> >> (for which table-at-a-time blobs are a perfect match).
> > 
> > I wasn't aware of this.  The SMBIOS spec calls for all the sub-tables
> > to be concatenanted into a single linear area of memory.  Is there
> > something in EFI or OVMF that is dictating otherwise?  Can you provide
> > a link so I can further understand?  (I briefly checked through the
> > UEFI v2.3.1 spec and nothing popped out at me.)
> 
> The "UEFI Specification" is not relevant here, the "UEFI Platform
> Initialization (PI) Specification" is.
> 
> You can download the PI spec at <http://www.uefi.org/specs/access>. The
> relevant section is (I have version 1.2.1):
> 
> VOLUME 5: Platform Initialization Specification
> Standards
> 6 SMBIOS Protocol
> 
> The function to call is EFI_SMBIOS_PROTOCOL.Add().
> 
> >> I think that concatenating table-at-a-time blobs in SeaBIOS is easier
> >> than parsing & splitting a complete dump into tables in OVMF.
> > 
> > I don't think it's very difficult either way.  It would be nice,
> > though, if there was just one owner for the smbios.  The current setup
> > where some data comes from QEMU and some from the firmware, along with
> > mechanisms for providing defaults and overrides is way too complex in
> > my opinion.
> 
> I certainly agree with the direction. I'm OK with the current
> table-at-a-time blobs (which should leave only the SMBIOS entry point to
> the firmware). I'm also OK with any new, comprehensive format that
> allows me, with reasonable parsing, to identify the individual tables in
> the big concat (and to throw away the passed down entry point).
> 
> I already wrote display_uuid() [src/fw/smbios.c] for SeaBIOS, so I guess
> I could repeat the exercise if it's unavoidable... :)

Kevin, Laszlo,

What if I found a way to send an entry point structure via fw_cfg, as
a signal to ${BIOS} to simply assemble all the table-at-a-time blobs,
but without generating any of its own ?

I'm still working my way through whether *I* like the idea or not, but
figured I'd throw it out there as a potential compromise ? :)

Thx,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 04/02/14 14:38, Gabriel L. Somlo wrote:
> On Wed, Apr 02, 2014 at 12:35:26AM +0200, Laszlo Ersek wrote:
>> On 04/02/14 00:00, Kevin O'Connor wrote:
>>> On Tue, Apr 01, 2014 at 11:44:12PM +0200, Laszlo Ersek wrote:
>>>> Right now, OVMF can accept individual fields, or table-at-a-time blobs,
>>>> via fw_cfg.
>>>>
>>>> The internal interface (EFI_SMBIOS_PROTOCOL) expects one table at a time
>>>> (for which table-at-a-time blobs are a perfect match).
>>>
>>> I wasn't aware of this.  The SMBIOS spec calls for all the sub-tables
>>> to be concatenanted into a single linear area of memory.  Is there
>>> something in EFI or OVMF that is dictating otherwise?  Can you provide
>>> a link so I can further understand?  (I briefly checked through the
>>> UEFI v2.3.1 spec and nothing popped out at me.)
>>
>> The "UEFI Specification" is not relevant here, the "UEFI Platform
>> Initialization (PI) Specification" is.
>>
>> You can download the PI spec at <http://www.uefi.org/specs/access>. The
>> relevant section is (I have version 1.2.1):
>>
>> VOLUME 5: Platform Initialization Specification
>> Standards
>> 6 SMBIOS Protocol
>>
>> The function to call is EFI_SMBIOS_PROTOCOL.Add().
>>
>>>> I think that concatenating table-at-a-time blobs in SeaBIOS is easier
>>>> than parsing & splitting a complete dump into tables in OVMF.
>>>
>>> I don't think it's very difficult either way.  It would be nice,
>>> though, if there was just one owner for the smbios.  The current setup
>>> where some data comes from QEMU and some from the firmware, along with
>>> mechanisms for providing defaults and overrides is way too complex in
>>> my opinion.
>>
>> I certainly agree with the direction. I'm OK with the current
>> table-at-a-time blobs (which should leave only the SMBIOS entry point to
>> the firmware). I'm also OK with any new, comprehensive format that
>> allows me, with reasonable parsing, to identify the individual tables in
>> the big concat (and to throw away the passed down entry point).
>>
>> I already wrote display_uuid() [src/fw/smbios.c] for SeaBIOS, so I guess
>> I could repeat the exercise if it's unavoidable... :)
> 
> Kevin, Laszlo,
> 
> What if I found a way to send an entry point structure via fw_cfg, as
> a signal to ${BIOS} to simply assemble all the table-at-a-time blobs,
> but without generating any of its own ?
> 
> I'm still working my way through whether *I* like the idea or not, but
> figured I'd throw it out there as a potential compromise ? :)

If you send the entry point structure in a new fw_cfg file, then (I
believe) I could ignore it easily, and just go ahead with the
table-at-a-time blobs. OVMF has defaults (fallbacks) only for Type 0 and
Type 1 tables now, which you are (almost) guaranteed to send down
anyway, so the above approach might work for OVMF without even changing
the OVMF code. (Regarding individual fields, you'd simply not send such,
if I understand correctly.)

Thanks
Laszlo

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Wed, Apr 02, 2014 at 12:35:26AM +0200, Laszlo Ersek wrote:
> On 04/02/14 00:00, Kevin O'Connor wrote:
> > On Tue, Apr 01, 2014 at 11:44:12PM +0200, Laszlo Ersek wrote:
> >> Right now, OVMF can accept individual fields, or table-at-a-time blobs,
> >> via fw_cfg.
> >>
> >> The internal interface (EFI_SMBIOS_PROTOCOL) expects one table at a time
> >> (for which table-at-a-time blobs are a perfect match).
> > 
> > I wasn't aware of this.  The SMBIOS spec calls for all the sub-tables
> > to be concatenanted into a single linear area of memory.  Is there
> > something in EFI or OVMF that is dictating otherwise?  Can you provide
> > a link so I can further understand?  (I briefly checked through the
> > UEFI v2.3.1 spec and nothing popped out at me.)
> 
> The "UEFI Specification" is not relevant here, the "UEFI Platform
> Initialization (PI) Specification" is.
> 
> You can download the PI spec at <http://www.uefi.org/specs/access>. The
> relevant section is (I have version 1.2.1):

Oh, those crazy UEFI guys!

The internal edk2 code appears to use these individual calls to
ultimately build the concatenated smbios table from them.  So, nothing
special here - just standard UEFI over-design.

> >> I think that concatenating table-at-a-time blobs in SeaBIOS is easier
> >> than parsing & splitting a complete dump into tables in OVMF.
> > 
> > I don't think it's very difficult either way.  It would be nice,
> > though, if there was just one owner for the smbios.  The current setup
> > where some data comes from QEMU and some from the firmware, along with
> > mechanisms for providing defaults and overrides is way too complex in
> > my opinion.
> 
> I certainly agree with the direction. I'm OK with the current
> table-at-a-time blobs (which should leave only the SMBIOS entry point to
> the firmware). I'm also OK with any new, comprehensive format that
> allows me, with reasonable parsing, to identify the individual tables in
> the big concat (and to throw away the passed down entry point).
> 
> I already wrote display_uuid() [src/fw/smbios.c] for SeaBIOS, so I guess
> I could repeat the exercise if it's unavoidable... :)

Makes sense.

Thanks.
-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gerd Hoffmann]

  Hi,

> If qemu gives OVMF a complete, concatenated dump of all tables, I'll
> have to split that up into individual tables, and install those one by one.

I feel like I should have a look at how coreboot handles this for an
additional data point ...

cheers,
  Gerd

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Wed, Apr 02, 2014 at 05:07:05PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > If qemu gives OVMF a complete, concatenated dump of all tables, I'll
> > have to split that up into individual tables, and install those one by one.
> 
> I feel like I should have a look at how coreboot handles this for an
> additional data point ...

Sounds like maybe I also need to take a step back and try this stuff
out (at least with Linux and Windows), for some first hand experience
with how smbios tables are handled outside SeaBIOS... :)

Speaking of, I *thought* I had a vague idea of how all this stuff fits
together, but it turns out I don't... There's

	- OVMF
	  http://sourceforge.net/apps/mediawiki/tianocore/index.php?title=OVMF

	- TianoCore
	  http://www.coreboot.org/TianoCore

	- coreboot
	  http://www.coreboot.org/Download_coreboot

Apparently, TianoCore is a "coreboot payload", which in my mind is
somewhat analogous to bootloader "stages" chaining off each other,
but then what's OVMF (the only thing I actually tried, which only
works on piix) ? Is it a packaged bundle of coreboot+tianocore ?
or something else entirely ?

What if I want to send a patch against this whole "thing" to
facilitate integration with the new smbios table generator in qemu ?

Which git repos do I need to have around, and how to stitch them
together to obtain "the thing you use as an argument to -bios in lieu
of SeaBIOS", when it comes time to test ? :)

I'm guessing this is a FAQ, so if there's one place that explains it
all, please point me at it. Otherwise I'd be happy to write it up once
I get my head wrapped around it :)

Thanks much,
--Gabriel

PS. Coreboot says it supports q35 as well
    (http://www.coreboot.org/Supported_Motherboards)
    so if my guess that "coreboot + tianocore == ovmf" is true, then
    it must be tianocore that doesn't yet support q35 ?

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Wed, Apr 02, 2014 at 01:01:28PM -0400, Gabriel L. Somlo wrote:
> Speaking of, I *thought* I had a vague idea of how all this stuff fits
> together, but it turns out I don't... There's
> 
> 	- OVMF
> 	  http://sourceforge.net/apps/mediawiki/tianocore/index.php?title=OVMF
> 
> 	- TianoCore
> 	  http://www.coreboot.org/TianoCore
> 
> 	- coreboot
> 	  http://www.coreboot.org/Download_coreboot
> 
> Apparently, TianoCore is a "coreboot payload", which in my mind is
> somewhat analogous to bootloader "stages" chaining off each other,
> but then what's OVMF (the only thing I actually tried, which only
> works on piix) ? Is it a packaged bundle of coreboot+tianocore ?
> or something else entirely ?
> 
> What if I want to send a patch against this whole "thing" to
> facilitate integration with the new smbios table generator in qemu ?
> 
> Which git repos do I need to have around, and how to stitch them
> together to obtain "the thing you use as an argument to -bios in lieu
> of SeaBIOS", when it comes time to test ? :)
> 
> I'm guessing this is a FAQ, so if there's one place that explains it
> all, please point me at it. Otherwise I'd be happy to write it up once
> I get my head wrapped around it :)

Nevermind, it seems it's all under git://git.code.sf.net/p/tianocore/edk2 :)

Although the nomenclature is still a bit fuzzy to me, the "thing to build"
within edk2 appears to be OvmfPkg (ACTIVE_PLATFORM = OvmfPkg/OvmfPkgX64.dsc,
with TOOL_CHAIN_TAG = GCC48, in Conf/target.txt, at least on F20).

I now have the latest and greatest "upstream" OVMF.fd, and I can use it
(piix only) to boot Fedora 20 live x86_64. Guess I'm on my way :)

I get "missing smbios entry point" when I do a dmidecode, BTW. QEMU is
sending type 1, 3, 4, etc. blobs in fw_cfg, not sure yet what I need
to do to get OVMF to add the entry point... Maybe I should try without
my smbios-patched qemu ?

Thanks,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 04/03/14 03:57, Gabriel L. Somlo wrote:
> On Wed, Apr 02, 2014 at 01:01:28PM -0400, Gabriel L. Somlo wrote:
>> Speaking of, I *thought* I had a vague idea of how all this stuff fits
>> together, but it turns out I don't... There's
>>
>> 	- OVMF
>> 	  http://sourceforge.net/apps/mediawiki/tianocore/index.php?title=OVMF
>>
>> 	- TianoCore
>> 	  http://www.coreboot.org/TianoCore
>>
>> 	- coreboot
>> 	  http://www.coreboot.org/Download_coreboot
>>
>> Apparently, TianoCore is a "coreboot payload", which in my mind is
>> somewhat analogous to bootloader "stages" chaining off each other,
>> but then what's OVMF (the only thing I actually tried, which only
>> works on piix) ? Is it a packaged bundle of coreboot+tianocore ?
>> or something else entirely ?
>>
>> What if I want to send a patch against this whole "thing" to
>> facilitate integration with the new smbios table generator in qemu ?
>>
>> Which git repos do I need to have around, and how to stitch them
>> together to obtain "the thing you use as an argument to -bios in lieu
>> of SeaBIOS", when it comes time to test ? :)

Unless you want to do OVMF development yourself (ie. as long as you'd
like to test only), you're best off with

(a) Gerd's packages:

http://www.kraxel.org/repos/

(b) If you use a Fedora host, you can also try a (recently refreshed)
Copr build, thanks to Paolo:

http://copr-be.cloud.fedoraproject.org/results/bonzini/ovmf/

Under (a) you find some short instructions, and a set of RPMs that is
automatically rebuilt twice a day (IIRC).

Both (a) and (b) include the downstream-only SMBIOS patches.

>> I'm guessing this is a FAQ, so if there's one place that explains it
>> all, please point me at it. Otherwise I'd be happy to write it up once
>> I get my head wrapped around it :)
> 
> Nevermind, it seems it's all under git://git.code.sf.net/p/tianocore/edk2 :)
> 
> Although the nomenclature is still a bit fuzzy to me, the "thing to build"
> within edk2 appears to be OvmfPkg (ACTIVE_PLATFORM = OvmfPkg/OvmfPkgX64.dsc,
> with TOOL_CHAIN_TAG = GCC48, in Conf/target.txt, at least on F20).
> 
> I now have the latest and greatest "upstream" OVMF.fd, and I can use it
> (piix only) to boot Fedora 20 live x86_64. Guess I'm on my way :)

You can most certainly build OVMF yourself, yes; the OvmfPkg/build.sh
script is a convenience wrapper. See also OvmfPkg/README.

> I get "missing smbios entry point" when I do a dmidecode, BTW. QEMU is
> sending type 1, 3, 4, etc. blobs in fw_cfg, not sure yet what I need
> to do to get OVMF to add the entry point... Maybe I should try without
> my smbios-patched qemu ?

You don't see SMBIOS tables in the guest because you've built upstream
OVMF. As I said before, upstream OvmfPkg doesn't include my SMBIOS
patches. Both (a) and (b) do however.

One further note (also mentioned in OvmfPkg/README): don't use OVMF.fd
with -bios; use it with -pflash (you need a Linux 3.7+ host for this).
This will give your guest real runtime variable services -- non-volatile
variable data will be written back to the OVMF.fd file.

Thanks
Laszlo

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Thu, Apr 03, 2014 at 11:42:31AM +0200, Laszlo Ersek wrote:
> Unless you want to do OVMF development yourself (ie. as long as you'd
> like to test only), you're best off with
> 
> (a) Gerd's packages:
> 
> http://www.kraxel.org/repos/
> 
> (b) If you use a Fedora host, you can also try a (recently refreshed)
> Copr build, thanks to Paolo:
> 
> http://copr-be.cloud.fedoraproject.org/results/bonzini/ovmf/
> 
> Under (a) you find some short instructions, and a set of RPMs that is
> automatically rebuilt twice a day (IIRC).
> 
> Both (a) and (b) include the downstream-only SMBIOS patches.
>
> [...]
>
> You don't see SMBIOS tables in the guest because you've built upstream
> OVMF. As I said before, upstream OvmfPkg doesn't include my SMBIOS
> patches. Both (a) and (b) do however.

Oh, OK. Basically, I'm interested in looking at the sources
(specifically, the SMBIOS code :) ) to try and develop my own
sense of what might be the most agreeable way forward for my
QEMU smbios patches... That way, I'd actually have a (hopefully
somewhat intelligent) position of my own to support :) :)

For that, ideally, I'd like to clone a git repo (and pull once
every few days to stay on top of things). Looks like the top-level
upstream one doesn't have your smbios code, so would there be a
"mid-stream" (for the lack of a better term) git repo you have
that I can clone and hack against for the smbios stuff ?

I ask because yesterday was the first time I started really paying
attention to edk2 and ovmf, and I don't yet have a sense of how fast
the "state of the universe" would run away from me while my back is
turned, i.e. how fast a point-in-time snapshot of e.g. Gerd or Paolo's
RPMs would become stale... Extracting and patching source code from
RPMs is OK once or twice, but I imagine will be much less fun than
"git pull" once it gets repetitive :D

> One further note (also mentioned in OvmfPkg/README): don't use OVMF.fd
> with -bios; use it with -pflash (you need a Linux 3.7+ host for this).
> This will give your guest real runtime variable services -- non-volatile
> variable data will be written back to the OVMF.fd file.

Good to know (-bios == read-only, -pflash == writable) :)

I'm really only in it for testing smbios interactions with qemu though,
at least at this stage... :)

Thanks a ton,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 04/03/14 15:32, Gabriel L. Somlo wrote:
> On Thu, Apr 03, 2014 at 11:42:31AM +0200, Laszlo Ersek wrote:

>> You don't see SMBIOS tables in the guest because you've built upstream
>> OVMF. As I said before, upstream OvmfPkg doesn't include my SMBIOS
>> patches. Both (a) and (b) do however.
> 
> Oh, OK. Basically, I'm interested in looking at the sources
> (specifically, the SMBIOS code :) ) to try and develop my own
> sense of what might be the most agreeable way forward for my
> QEMU smbios patches... That way, I'd actually have a (hopefully
> somewhat intelligent) position of my own to support :) :)
> 
> For that, ideally, I'd like to clone a git repo (and pull once
> every few days to stay on top of things). Looks like the top-level
> upstream one doesn't have your smbios code, so would there be a
> "mid-stream" (for the lack of a better term) git repo you have
> that I can clone and hack against for the smbios stuff ?

https://github.com/lersek/edk2/commits/smbios

(I don't push to this repo normally, so you should continue fetching
from tianocore/edk2.)

I've thrown in two convenience patches for more detailed debug messages.
(See also the debug console hints in the README file.)

> I ask because yesterday was the first time I started really paying
> attention to edk2 and ovmf, and I don't yet have a sense of how fast
> the "state of the universe" would run away from me while my back is
> turned, i.e. how fast a point-in-time snapshot of e.g. Gerd or Paolo's
> RPMs would become stale... Extracting and patching source code from
> RPMs is OK once or twice, but I imagine will be much less fun than
> "git pull" once it gets repetitive :D

Taking the patches from the SRPM and applying them manually would bring
you to the same spot as fetching the patches from the above link (except
the debug patches). Afterwards you should continue fetching form
tianocore/edk2, either merging those commits into your (now new) branch,
or rebasing your (now new) branch.

(In general I don't push my WIP publicly.)

>> One further note (also mentioned in OvmfPkg/README): don't use OVMF.fd
>> with -bios; use it with -pflash (you need a Linux 3.7+ host for this).
>> This will give your guest real runtime variable services -- non-volatile
>> variable data will be written back to the OVMF.fd file.
> 
> Good to know (-bios == read-only, -pflash == writable) :)
> 
> I'm really only in it for testing smbios interactions with qemu though,
> at least at this stage... :)
> 
> Thanks a ton,

No; thank you for doing this.

Laszlo

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gerd Hoffmann]

On Do, 2014-04-03 at 09:32 -0400, Gabriel L. Somlo wrote:
> For that, ideally, I'd like to clone a git repo (and pull once
> every few days to stay on top of things). Looks like the top-level
> upstream one doesn't have your smbios code, so would there be a
> "mid-stream" (for the lack of a better term) git repo you have
> that I can clone and hack against for the smbios stuff ?

http://www.kraxel.org/cgit/jenkins/edk2/tree/

spec file and patches.  specfile is patched by the jenkins build
scripts, so a manual rpm build will need some fiddeling, but I guess you
are more interested in the patches ;)

cheers,
  Gerd

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gerd Hoffmann]

On Do, 2014-04-03 at 11:42 +0200, Laszlo Ersek wrote:
> (a) Gerd's packages:
> 
> http://www.kraxel.org/repos/

> Under (a) you find some short instructions, and a set of RPMs that is
> automatically rebuilt twice a day (IIRC).

It polls the git repos once per hour and kicks a build on new commits.
So usually it should not take more than two hours from commit to updated
packages.

cheers,
  Gerd

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Tue, Apr 01, 2014 at 05:28:10PM -0400, Gabriel L. Somlo wrote:
> Assuming all relevant QEMU maintainers are OK with the idea of
> creating a full SMBIOS blob (with e.g. type 0 defaulting to the
> relevant SeaBIOS values, override-able to fit some different bios,
> e.g. OVMF), would you take a patch to check for this blob in
> smbios_setup() (in SeaBIOS src/fw/smbios.c) ? Right now, it's either
> individual fields or table-at-a-time blobs only, AFAICT.

Yes.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gerd Hoffmann]

  Hi,

> > From the conversation so far, it seems to me that:
> > 
> > 	- type 0 is best left to the BIOS (user overrides via
> > 	  command line at their own risk)

I think it was a bad idea to allow overriding type0 fields in the first
place.  It also isn't used in practice.  I don't think it is a big
problem to loose that capability.

> > 	- therefore, the maximum granularity of QEMU-generated
> > 	  elements should be full tables of a given type, and
> > 	  not the full SMBIOS blob at once (other mechanisms to
> > 	  allow the BIOS to insert its own type 0 welcome, but
> > 	  so far this seems the most straightforward one).

Just an idea:  Is the table ordering important?  I don't think so.  If
qemu supplies a blob with all tables except type0, can the firmware
simply append a type0 record to the blob supplied by qemu?

> I don't agree - I think ultimately we want QEMU to generate the full
> SMBIOS table and pass it to the firmware via the romfile_loader
> mechanism.

I still think the firmware (and *only* the firmware) should supply the
type0 table.  I also think seabios should fill in something more
sensible in there, such as "Vendor: SeaBIOS" and "Version:
rel-1.7.4-0-g96917a8-...".

> The only thing that has been raised as an issue with this
> is one bit in the smbios table (UEFI support).

IMO 'dmidecode -t0' should show what firmware you are running
(seabios/ovmf/coreboot/whatever), not something made up by qemu.

cheers,
  Gerd

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Wed, Apr 02, 2014 at 05:04:57PM +0200, Gerd Hoffmann wrote:
> > > 	- therefore, the maximum granularity of QEMU-generated
> > > 	  elements should be full tables of a given type, and
> > > 	  not the full SMBIOS blob at once (other mechanisms to
> > > 	  allow the BIOS to insert its own type 0 welcome, but
> > > 	  so far this seems the most straightforward one).
> 
> Just an idea:  Is the table ordering important?  I don't think so.  If
> qemu supplies a blob with all tables except type0, can the firmware
> simply append a type0 record to the blob supplied by qemu?

I don't see anything in the spec that would prohibit it, but I don't
think it's done in practice.  Given how many different OS parsers
there are and their dubious quality, I think we'd want to be as simple
as possible and continue to put table 0 at the start.

> > I don't agree - I think ultimately we want QEMU to generate the full
> > SMBIOS table and pass it to the firmware via the romfile_loader
> > mechanism.
> 
> I still think the firmware (and *only* the firmware) should supply the
> type0 table.  I also think seabios should fill in something more
> sensible in there, such as "Vendor: SeaBIOS" and "Version:
> rel-1.7.4-0-g96917a8-...".
> 
> > The only thing that has been raised as an issue with this
> > is one bit in the smbios table (UEFI support).
> 
> IMO 'dmidecode -t0' should show what firmware you are running
> (seabios/ovmf/coreboot/whatever), not something made up by qemu.

Ultimately my preference would be to make a clean break from the
existing smbios fw_cfg system as it is too complex, too confusing, and
too inflexible.  We could implement the above as you suggest, but I
fear it would require keeping much of the complexity of the current
fw_cfg interface.  (It's also a new feature as SeaBIOS doesn't
currently put any useful info in type 0.)

The already existing romfile_loader interface seems to provide a
nearly complete solution to replace the smbios fw_cfg system.  If
there is really demand for more firmware info in the type 0 table, why
don't we use romfile_loader, have qemu put together a dummy type 0
table, and then have seabios update it in place?  Sure, that's ugly,
but I'm pretty sure it would be less ugly then keeping the existing
smbios fw_cfg system around.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Fri, Apr 04, 2014 at 08:34:11PM -0400, Kevin O'Connor wrote:
> > 
> > IMO 'dmidecode -t0' should show what firmware you are running
> > (seabios/ovmf/coreboot/whatever), not something made up by qemu.
> 
> Ultimately my preference would be to make a clean break from the
> existing smbios fw_cfg system as it is too complex, too confusing, and
> too inflexible.  We could implement the above as you suggest, but I
> fear it would require keeping much of the complexity of the current
> fw_cfg interface.  (It's also a new feature as SeaBIOS doesn't
> currently put any useful info in type 0.)

I like this idea too: older/current versions of qemu just do what they
do, and work with SeaBIOS the way they always have. Future versions
just send a completely new fw_cfg message type, which, when
encountered by SeaBIOS, causes it to only add in (or edit) type 0, and
leave everything else as is.

> I don't see anything in the spec that would prohibit it, but I don't
> think it's done in practice.  Given how many different OS parsers
> there are and their dubious quality, I think we'd want to be as simple
> as possible and continue to put table 0 at the start.
>...
> The already existing romfile_loader interface seems to provide a
> nearly complete solution to replace the smbios fw_cfg system.  If
> there is really demand for more firmware info in the type 0 table, why
> don't we use romfile_loader, have qemu put together a dummy type 0
> table, and then have seabios update it in place?  Sure, that's ugly,
> but I'm pretty sure it would be less ugly then keeping the existing
> smbios fw_cfg system around.

The only fly in this ointment may be that type 0 doesn't have a fixed
length that could be edited in place, if you consider the various
strings that get tacked on to the end of it. So you'd still have to
slide the rest of the smbios payload left or right to shrink or
enlarge the type 0 blob, depending on how you modify the various
strings it contains...

--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Fri, Apr 04, 2014 at 09:15:14PM -0400, Gabriel L. Somlo wrote:
> On Fri, Apr 04, 2014 at 08:34:11PM -0400, Kevin O'Connor wrote:
> > > 
> > > IMO 'dmidecode -t0' should show what firmware you are running
> > > (seabios/ovmf/coreboot/whatever), not something made up by qemu.
> > 
> > Ultimately my preference would be to make a clean break from the
> > existing smbios fw_cfg system as it is too complex, too confusing, and
> > too inflexible.  We could implement the above as you suggest, but I
> > fear it would require keeping much of the complexity of the current
> > fw_cfg interface.  (It's also a new feature as SeaBIOS doesn't
> > currently put any useful info in type 0.)
> 
> I like this idea too: older/current versions of qemu just do what they
> do, and work with SeaBIOS the way they always have. Future versions
> just send a completely new fw_cfg message type, which, when
> encountered by SeaBIOS, causes it to only add in (or edit) type 0, and
> leave everything else as is.

Right.

> > I don't see anything in the spec that would prohibit it, but I don't
> > think it's done in practice.  Given how many different OS parsers
> > there are and their dubious quality, I think we'd want to be as simple
> > as possible and continue to put table 0 at the start.
> >...
> > The already existing romfile_loader interface seems to provide a
> > nearly complete solution to replace the smbios fw_cfg system.  If
> > there is really demand for more firmware info in the type 0 table, why
> > don't we use romfile_loader, have qemu put together a dummy type 0
> > table, and then have seabios update it in place?  Sure, that's ugly,
> > but I'm pretty sure it would be less ugly then keeping the existing
> > smbios fw_cfg system around.
> 
> The only fly in this ointment may be that type 0 doesn't have a fixed
> length that could be edited in place, if you consider the various
> strings that get tacked on to the end of it. So you'd still have to
> slide the rest of the smbios payload left or right to shrink or
> enlarge the type 0 blob, depending on how you modify the various
> strings it contains...

The dummy type 0 subtable that QEMU generates can have dummy space
padded strings that the firmware can overwrite.  Until recently, the
max size smbios string was 64 bytes, so that size could be used.  (As
above, I admit that this is ugly, but the alternatives also seem
ugly.)  Another option would be to just leave the strings at a QEMU
default as that's no different from what SeaBIOS does today.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gerd Hoffmann]

  Hi,

> > The only fly in this ointment may be that type 0 doesn't have a fixed
> > length that could be edited in place, if you consider the various
> > strings that get tacked on to the end of it. So you'd still have to
> > slide the rest of the smbios payload left or right to shrink or
> > enlarge the type 0 blob, depending on how you modify the various
> > strings it contains...
> 
> The dummy type 0 subtable that QEMU generates can have dummy space
> padded strings that the firmware can overwrite.  Until recently, the
> max size smbios string was 64 bytes, so that size could be used.  (As
> above, I admit that this is ugly, but the alternatives also seem
> ugly.)  Another option would be to just leave the strings at a QEMU
> default as that's no different from what SeaBIOS does today.

I don't think we need to make it that complicated.  smbios tables don't
have any references, right?  I mean any references which would need a
fixup (such as table pointers in RSDP in acpi) and therefore would need
the romfile_loader.  The string references within a table are relative
don't need special care.

Gabriel has code to generate all tables needed in qemu meanwhile, so I
think we can simply have a blob in fw_cfg with all tables (except
type0).  firmware generates type0 table like it does today, then simply
appends the fw_cfg blob as-is, then appends a end-of-tables marker.
Done.

OVMF probably would have to parse the blob, split it into tables, then
install them one by one.  But I suspect that will be less code than
dealing with the complex smbios fw_cfg interface we have today ...

cheers,
  Gerd

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Mon, Apr 07, 2014 at 09:09:56AM +0200, Gerd Hoffmann wrote:
> > > The only fly in this ointment may be that type 0 doesn't have a fixed
> > > length that could be edited in place, if you consider the various
> > > strings that get tacked on to the end of it. So you'd still have to
> > > slide the rest of the smbios payload left or right to shrink or
> > > enlarge the type 0 blob, depending on how you modify the various
> > > strings it contains...
> > 
> > The dummy type 0 subtable that QEMU generates can have dummy space
> > padded strings that the firmware can overwrite.  Until recently, the
> > max size smbios string was 64 bytes, so that size could be used.  (As
> > above, I admit that this is ugly, but the alternatives also seem
> > ugly.)  Another option would be to just leave the strings at a QEMU
> > default as that's no different from what SeaBIOS does today.
> 
> I don't think we need to make it that complicated.  smbios tables don't
> have any references, right?  I mean any references which would need a
> fixup (such as table pointers in RSDP in acpi) and therefore would need
> the romfile_loader.  The string references within a table are relative
> don't need special care.

The smbios anchor table needs to have the address of the main smbios
table.  It would be preferable to get the anchor table from qemu as
the anchor table has the smbios version info.

But, anchor table aside, you are correct.

> Gabriel has code to generate all tables needed in qemu meanwhile, so I
> think we can simply have a blob in fw_cfg with all tables (except
> type0).  firmware generates type0 table like it does today, then simply
> appends the fw_cfg blob as-is, then appends a end-of-tables marker.
> Done.
> 
> OVMF probably would have to parse the blob, split it into tables, then
> install them one by one.  But I suspect that will be less code than
> dealing with the complex smbios fw_cfg interface we have today ...

How about having QEMU produce the smbios table with a dummy type0
table and then both seabios and ovmf can replace the type0 table if
desired.  After all, if OVMF is splitting the blob into tables, it can
just as easily replace type0 as append it.

This way, the QEMU output is technically complete.  And if someone
wishes to code up SeaBIOS to do the type0 replace (I'm not convinced
it's even necessary) then at least that SeaBIOS code could be used on
coreboot as well.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Laszlo Ersek]

On 04/07/14 16:14, Kevin O'Connor wrote:
> On Mon, Apr 07, 2014 at 09:09:56AM +0200, Gerd Hoffmann wrote:
>>>> The only fly in this ointment may be that type 0 doesn't have a fixed
>>>> length that could be edited in place, if you consider the various
>>>> strings that get tacked on to the end of it. So you'd still have to
>>>> slide the rest of the smbios payload left or right to shrink or
>>>> enlarge the type 0 blob, depending on how you modify the various
>>>> strings it contains...
>>>
>>> The dummy type 0 subtable that QEMU generates can have dummy space
>>> padded strings that the firmware can overwrite.  Until recently, the
>>> max size smbios string was 64 bytes, so that size could be used.  (As
>>> above, I admit that this is ugly, but the alternatives also seem
>>> ugly.)  Another option would be to just leave the strings at a QEMU
>>> default as that's no different from what SeaBIOS does today.
>>
>> I don't think we need to make it that complicated.  smbios tables don't
>> have any references, right?  I mean any references which would need a
>> fixup (such as table pointers in RSDP in acpi) and therefore would need
>> the romfile_loader.  The string references within a table are relative
>> don't need special care.
> 
> The smbios anchor table needs to have the address of the main smbios
> table.  It would be preferable to get the anchor table from qemu as
> the anchor table has the smbios version info.
> 
> But, anchor table aside, you are correct.
> 
>> Gabriel has code to generate all tables needed in qemu meanwhile, so I
>> think we can simply have a blob in fw_cfg with all tables (except
>> type0).  firmware generates type0 table like it does today, then simply
>> appends the fw_cfg blob as-is, then appends a end-of-tables marker.
>> Done.
>>
>> OVMF probably would have to parse the blob, split it into tables, then
>> install them one by one.  But I suspect that will be less code than
>> dealing with the complex smbios fw_cfg interface we have today ...
> 
> How about having QEMU produce the smbios table with a dummy type0
> table and then both seabios and ovmf can replace the type0 table if
> desired.  After all, if OVMF is splitting the blob into tables, it can
> just as easily replace type0 as append it.
> 
> This way, the QEMU output is technically complete.  And if someone
> wishes to code up SeaBIOS to do the type0 replace (I'm not convinced
> it's even necessary) then at least that SeaBIOS code could be used on
> coreboot as well.

Works for me.

Thanks
Laszlo

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Mon, Apr 07, 2014 at 10:14:36AM -0400, Kevin O'Connor wrote:
> How about having QEMU produce the smbios table with a dummy type0
> table and then both seabios and ovmf can replace the type0 table if
> desired.  After all, if OVMF is splitting the blob into tables, it can
> just as easily replace type0 as append it.
> 
> This way, the QEMU output is technically complete.  And if someone
> wishes to code up SeaBIOS to do the type0 replace (I'm not convinced
> it's even necessary) then at least that SeaBIOS code could be used on
> coreboot as well.

OK, so as far as I'm concerned, it's down to two alternatives:

1. I create a full blob, starting with the anchor/entrypoint, and
followed by a (dummy) type 0, type 1, etc, etc. all the way to
the type 127 end marker. Pass that in via fw_cfg, and each BIOS
is then responsible for editing type 0, overwriting it with
appropriate values.

I like this very much :) except for a serious discomfort with the idea
of imposing an additional "convention" on the size (and choice of)
strings included with the type0 table, beyond the smbios spec.


2. I create a third fw_cfg smbios "type" for the entry point
structure.

The BIOS (e.g. smbios_setup() in SeaBIOS) checks for the presence of
this blob *first*. If present, it simply grabs all table blobs from
fw_cfg and assembles them (e.g. via get_external()). Create its own
type 0 if not already included in fw_cfg, and recompute the checksum for
the entry point.

If the entry point blob is not found, smbios_setup() proceeds with its
current logic (looking for table or field blobs in fw_cfg, and
creating its own entry point structure).


Now, 2 would be uglier, hands down, but has the advantage of not
adding arbitrary restrictions on what the type0 structure can look
like.



If we go with 1 (all I need is you all to say "go for it, we're OK
with arbitrary restrictions on type0" :) ), I'll add prominent
comments in the qemu smbios source about what the restrictions are.

We have vendor, version, and date, currently. Date can be "YYYY-MM-DD"
or somesuch (for a total strlen of 10, not including \0).

Any idea what the longest "vendor" and "version" string might look
like ?

What do we do if we have *shorter* strings than that, fill with
spaces to keep the hardcoded strlen intact across the qemu/bios
demarc ?


Thanks,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Mon, Apr 07, 2014 at 10:49:54AM -0400, Gabriel L. Somlo wrote:
> On Mon, Apr 07, 2014 at 10:14:36AM -0400, Kevin O'Connor wrote:
> > How about having QEMU produce the smbios table with a dummy type0
> > table and then both seabios and ovmf can replace the type0 table if
> > desired.  After all, if OVMF is splitting the blob into tables, it can
> > just as easily replace type0 as append it.
> > 
> > This way, the QEMU output is technically complete.  And if someone
> > wishes to code up SeaBIOS to do the type0 replace (I'm not convinced
> > it's even necessary) then at least that SeaBIOS code could be used on
> > coreboot as well.
> 
> OK, so as far as I'm concerned, it's down to two alternatives:
> 
> 1. I create a full blob, starting with the anchor/entrypoint, and
> followed by a (dummy) type 0, type 1, etc, etc. all the way to
> the type 127 end marker. Pass that in via fw_cfg, and each BIOS
> is then responsible for editing type 0, overwriting it with
> appropriate values.
> 
> I like this very much :) except for a serious discomfort with the idea
> of imposing an additional "convention" on the size (and choice of)
> strings included with the type0 table, beyond the smbios spec.

I agree that was too ugly.  What I'm proposing now is that QEMU
produce a valid type0 table (with strings populated, but no special
padding) along with all the other tables (up to and including type
127).

Then SeaBIOS and OVMF can either pass this on exactly as is, or they
can modify the table to replace type0.

This is a few more lines of code for SeaBIOS (to replace type0 instead
of just patching it), but it has the advantage that QEMU developers
know they must produce a valid smbios table according to the specs and
the firmware developers know they will get a valid smbios table
(according to the specs) and know they must ultimately produce a valid
smbios table.

So, I'm suggesting QEMU produce two new fw_cfg files: an anchor file
with the valid anchor table (the address pointer can be just set to
zero), and an smbios table file with the complete set of smbios tables
formatted according to the smbios spec.  SeaBIOS can then use the
existence of one of these new files to determine if it should deploy
(and optionally modify) them or if it should use the old smbios
generation code.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Mon, Apr 07, 2014 at 11:23:44AM -0400, Kevin O'Connor wrote:
> So, I'm suggesting QEMU produce two new fw_cfg files: an anchor file
> with the valid anchor table (the address pointer can be just set to
> zero), and an smbios table file with the complete set of smbios tables
> formatted according to the smbios spec.  SeaBIOS can then use the
> existence of one of these new files to determine if it should deploy
> (and optionally modify) them or if it should use the old smbios
> generation code.

Oh, OK. Right now we have (in qemu):

#define SMBIOS_FIELD_ENTRY 0
#define SMBIOS_TABLE_ENTRY 1

I will be adding (actually, migrating to):

#define SMBIOS_ANCHOR_ENTRY 2 /* for the smbios entry point table */
#define SMBIOS_FULLTABLE_ENTRY 3 /* for the blob containing all types */

The cool thing here is that, along with the payload for each type, I
can create a wrapper structure, like there already exists for fields
and individual table types:

struct smbios_field {
    struct smbios_header header;
    uint8_t type;
    uint16_t offset;
    uint8_t data[];
} QEMU_PACKED;

struct smbios_table {
    struct smbios_header header;
    uint8_t data[];
} QEMU_PACKED;

I can add such a structure for the anchor/entrypoint table and for the
full blob-of-tables payload, in which I can tell you how big type 0 is,
so the BIOS (SeaBIOS/TianoCore) side surgery can be made that much
easier...

Thanks,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Kevin O'Connor]

On Mon, Apr 07, 2014 at 02:05:21PM -0400, Gabriel L. Somlo wrote:
> On Mon, Apr 07, 2014 at 11:23:44AM -0400, Kevin O'Connor wrote:
> > So, I'm suggesting QEMU produce two new fw_cfg files: an anchor file
> > with the valid anchor table (the address pointer can be just set to
> > zero), and an smbios table file with the complete set of smbios tables
> > formatted according to the smbios spec.  SeaBIOS can then use the
> > existence of one of these new files to determine if it should deploy
> > (and optionally modify) them or if it should use the old smbios
> > generation code.
> 
> Oh, OK. Right now we have (in qemu):
> 
> #define SMBIOS_FIELD_ENTRY 0
> #define SMBIOS_TABLE_ENTRY 1
> 
> I will be adding (actually, migrating to):
> 
> #define SMBIOS_ANCHOR_ENTRY 2 /* for the smbios entry point table */
> #define SMBIOS_FULLTABLE_ENTRY 3 /* for the blob containing all types */

No - don't do that.  Lets leave the existing smbios fw_cfg entry
(0x8001) unchanged.  Instead, introduce two new fw_cfg files using the
fw_cfg_add_file() interface (eg, "etc/smbios/smbios-anchor" and
"etc/smbios/smbios-tables").

[...]
> I can add such a structure for the anchor/entrypoint table and for the
> full blob-of-tables payload, in which I can tell you how big type 0 is,
> so the BIOS (SeaBIOS/TianoCore) side surgery can be made that much
> easier...

It's trivial for the firmware to calculate this on its own, so I
recommend just putting the anchor table and main tables unmodified in
their respective fw_cfg files.

-Kevin

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Mon, Apr 07, 2014 at 02:57:08PM -0400, Kevin O'Connor wrote:
> On Mon, Apr 07, 2014 at 02:05:21PM -0400, Gabriel L. Somlo wrote:
> > On Mon, Apr 07, 2014 at 11:23:44AM -0400, Kevin O'Connor wrote:
> > > So, I'm suggesting QEMU produce two new fw_cfg files: an anchor file
> > > with the valid anchor table (the address pointer can be just set to
> > > zero), and an smbios table file with the complete set of smbios tables
> > > formatted according to the smbios spec.  SeaBIOS can then use the
> > > existence of one of these new files to determine if it should deploy
> > > (and optionally modify) them or if it should use the old smbios
> > > generation code.
> > 
> > Oh, OK. Right now we have (in qemu):
> > 
> > #define SMBIOS_FIELD_ENTRY 0
> > #define SMBIOS_TABLE_ENTRY 1
> > 
> > I will be adding (actually, migrating to):
> > 
> > #define SMBIOS_ANCHOR_ENTRY 2 /* for the smbios entry point table */
> > #define SMBIOS_FULLTABLE_ENTRY 3 /* for the blob containing all types */
> 
> No - don't do that.  Lets leave the existing smbios fw_cfg entry
> (0x8001) unchanged.  Instead, introduce two new fw_cfg files using the
> fw_cfg_add_file() interface (eg, "etc/smbios/smbios-anchor" and
> "etc/smbios/smbios-tables").

OK.

> > I can add such a structure for the anchor/entrypoint table and for the
> > full blob-of-tables payload, in which I can tell you how big type 0 is,
> > so the BIOS (SeaBIOS/TianoCore) side surgery can be made that much
> > easier...
> 
> It's trivial for the firmware to calculate this on its own, so I
> recommend just putting the anchor table and main tables unmodified in
> their respective fw_cfg files.

Not sure why it never occurred to me before (lack of caffeine, or various
day-job related distractions :) ) but if the QEMU default dummy type 0
table is just that -- a dummy table -- there's *nothing* preventing me
from leaving all (three) of its strings *empty* :)

Then we'll know *exactly* what the size of type 0 is, when it's time
to surgically transplant a new one within the BIOS...

I remember neither Windows, Linux (F20 live) or OS X objecting to a
type 0 smbios table with all-undefined strings, during some previous
tests.

I'll try to send out an updated patch set later in the week.

Thanks again,
--Gabriel

Re: [Qemu-devel] E820 (Re: [v4 PATCH 00/12] SMBIOS: build full tables in QEMU)

[Gabriel L. Somlo]

On Wed, Mar 26, 2014 at 03:58:50PM -0400, Gabriel L. Somlo wrote:
> On Tue, Mar 18, 2014 at 07:23:17PM -0400, Gabriel L. Somlo wrote:
> > At this point, can anyone with access to a real, physical, NUMA
> > system dump the smbios tables with dmidecode and post them here?
> > I think that would be very informative.
> 
> So I thrashed around a bit trying to find a real NUMA box,
> and found a Dell R410 whose BIOS claims to support NUMA by
> disabling the "Node Interleaving" option

So, flipping that option on and off does indeed switch the system
between NUMA and UMA modes. Running "numactl -H" in UMA mode gets us
this output:

    available: 1 nodes (0)
    node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21
    22 23
    node 0 size: 131059 MB
    node 0 free: 127898 MB
    node distances:
    node   0 
      0:  10 

In NUMA mode, we get this:

    available: 2 nodes (0-1)
    node 0 cpus: 0 2 4 6 8 10 12 14 16 18 20 22
    node 0 size: 65536 MB
    node 0 free: 64047 MB
    node 1 cpus: 1 3 5 7 9 11 13 15 17 19 21 23
    node 1 size: 65523 MB
    node 1 free: 63841 MB
    node distances:
    node   0   1 
      0:  10  20 
      1:  20  10 

The "dmidecode" output stays unchanged between UMA and NUMA
modes (showing only memory tables, types 16-19, and there is NO
type 20):

    Handle 0x1000, DMI type 16, 15 bytes
    Physical Memory Array
            Location: System Board Or Motherboard
            Use: System Memory
            Error Correction Type: Multi-bit ECC
            Maximum Capacity: 128 GB
            Error Information Handle: Not Provided
            Number Of Devices: 8

    Handle 0x1100, DMI type 17, 28 bytes
    Memory Device
            Array Handle: 0x1000
            Error Information Handle: Not Provided
            Total Width: 72 bits
            Data Width: 64 bits
            Size: 16384 MB
            Form Factor: DIMM
            Set: 1
            Locator: DIMM_A1 
            Bank Locator: Not Specified
            Type: DDR3
            Type Detail: Synchronous Registered (Buffered)
            Speed: 1066 MHz
            Manufacturer: 00CE00B380CE
            Serial Number: 44056D5E
            Asset Tag: 01105061
            Part Number: M393B2K70CM0-YF8  
            Rank: 4

    Handle 0x1101, DMI type 17, 28 bytes
    Handle 0x1102, DMI type 17, 28 bytes
    Handle 0x1103, DMI type 17, 28 bytes
    Handle 0x1109, DMI type 17, 28 bytes
    Handle 0x110A, DMI type 17, 28 bytes
    Handle 0x110B, DMI type 17, 28 bytes
    Handle 0x110C, DMI type 17, 28 bytes
            /* all of them similar to 0x1100 above [GLS] */

    Handle 0x1300, DMI type 19, 15 bytes
    Memory Array Mapped Address
            Starting Address: 0x00000000000
            Ending Address: 0x000BFFFFFFF
            Range Size: 3 GB
            Physical Array Handle: 0x1000
            Partition Width: 2

    Handle 0x1301, DMI type 19, 15 bytes
    Memory Array Mapped Address
            Starting Address: 0x00100000000
            Ending Address: 0x0203FFFFFFF
            Range Size: 125 GB
            Physical Array Handle: 0x1000
            Partition Width: 2

The output of "dmesg | grep -i e820" remains unchanged between UMA and
NUMA modes:

    e820: BIOS-provided physical RAM map:
    BIOS-e820: [mem 0x0000000000000000-0x000000000009ffff] usable
    BIOS-e820: [mem 0x0000000000100000-0x00000000bf378fff] usable
    BIOS-e820: [mem 0x00000000bf379000-0x00000000bf38efff] reserved
    BIOS-e820: [mem 0x00000000bf38f000-0x00000000bf3cdfff] ACPI data
    BIOS-e820: [mem 0x00000000bf3ce000-0x00000000bfffffff] reserved
    BIOS-e820: [mem 0x00000000e0000000-0x00000000efffffff] reserved
    BIOS-e820: [mem 0x00000000fe000000-0x00000000ffffffff] reserved
    BIOS-e820: [mem 0x0000000100000000-0x000000203fffffff] usable
    e820: update [mem 0x00000000-0x0000ffff] usable ==> reserved
    e820: remove [mem 0x000a0000-0x000fffff] usable
    e820: last_pfn = 0x2040000 max_arch_pfn = 0x400000000
    e820: update [mem 0xc0000000-0xffffffff] usable ==> reserved
    e820: last_pfn = 0xbf379 max_arch_pfn = 0x400000000
    e820: [mem 0xc0000000-0xdfffffff] available for PCI devices
    PCI: MMCONFIG at [mem 0xe0000000-0xefffffff] reserved in E820
    e820: reserve RAM buffer [mem 0xbf379000-0xbfffffff]

So, even in NUMA mode, there still appear to be only two contiguous
E820_RAM type entries in the "sanitized" e820 table (the regions
marked "usable" appear to be adjacent). And the E820_RAM contiguous
regions are not per-node or anything like that. Not that this is to
be taken as the "One True Way", but still, it's a data point.

Anyhow, my original question/worry ("Oonce you create Type17 DIMMs
from all the RAM, and Type19 regions for each E820_RAM type entry,
how do you tie them together with Type20s ?") has been answered
("You just don't" :) )

Just figured I'd share this, maybe it can be useful to someone else
besides me...

Cheers,
--Gabriel