[Qemu-devel] [PATCH 0/7] smbios cleanup & nicer defaults for type 1

[Qemu-devel] [PATCH 0/7] smbios cleanup & nicer defaults for type 1

[Markus Armbruster]

This gets rid of one of the last get_param_value() users, makes
multiple -smbios work sanely, cleans up the gross side effect in
qemu_uuid_parse(), and more.  Topped off with a little feature in the
last patch.

Markus Armbruster (7):
  smbios: Normalize smbios_entry_add()'s error handling to exit(1)
  smbios: Convert to QemuOpts
  smbios: Improve diagnostics for conflicting entries
  smbios: Make multiple -smbios type= accumulate sanely
  smbios: Factor out smbios_maybe_add_str()
  vl: Set current_machine early
  smbios: Set system manufacturer, product & version by default

 arch_init.c                |   9 +-
 hw/i386/pc.c               |   6 +-
 hw/i386/pc_piix.c          |   5 +
 hw/i386/pc_q35.c           |   3 +
 hw/i386/smbios.c           | 349 ++++++++++++++++++++++++++++++++-------------
 include/hw/i386/pc.h       |   1 +
 include/hw/i386/smbios.h   |   7 +-
 include/sysemu/arch_init.h |   2 +-
 include/sysemu/sysemu.h    |   1 +
 vl.c                       |   8 +-
 10 files changed, 276 insertions(+), 115 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1)

[Markus Armbruster]

It exits on all error conditions but one, where it returns -1.
Normalize, and return void.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 arch_init.c              |  4 +---
 hw/i386/smbios.c         | 10 +++++-----
 include/hw/i386/smbios.h |  2 +-
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index e9dd96f..e0ac143 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1096,9 +1096,7 @@ void do_acpitable_option(const QemuOpts *opts)
 void do_smbios_option(const char *optarg)
 {
 #ifdef TARGET_I386
-    if (smbios_entry_add(optarg) < 0) {
-        exit(1);
-    }
+    smbios_entry_add(optarg);
 #endif
 }
 
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index e708cb8..0608aee 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -183,7 +183,7 @@ static void smbios_build_type_1_fields(const char *t)
                          buf, strlen(buf) + 1);
 }
 
-int smbios_entry_add(const char *t)
+void smbios_entry_add(const char *t)
 {
     char buf[1024];
 
@@ -222,7 +222,7 @@ int smbios_entry_add(const char *t)
         smbios_entries_len += sizeof(*table) + size;
         (*(uint16_t *)smbios_entries) =
                 cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
-        return 0;
+        return;
     }
 
     if (get_param_value(buf, sizeof(buf), "type", t)) {
@@ -230,10 +230,10 @@ int smbios_entry_add(const char *t)
         switch (type) {
         case 0:
             smbios_build_type_0_fields(t);
-            return 0;
+            return;
         case 1:
             smbios_build_type_1_fields(t);
-            return 0;
+            return;
         default:
             error_report("Don't know how to build fields for SMBIOS type %ld",
                          type);
@@ -242,5 +242,5 @@ int smbios_entry_add(const char *t)
     }
 
     error_report("Must specify type= or file=");
-    return -1;
+    exit(1);
 }
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 9babeaf..56c6108 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -13,7 +13,7 @@
  *
  */
 
-int smbios_entry_add(const char *t);
+void smbios_entry_add(const char *t);
 void smbios_add_field(int type, int offset, const void *data, size_t len);
 uint8_t *smbios_get_table(size_t *length);
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/7] smbios: Convert to QemuOpts

[Markus Armbruster]

So that it can be set in config file for -readconfig.

This tightens parsing of -smbios, and makes it more consistent with
other options: unknown parameters are rejected, numbers with trailing
junk are rejected, when a parameter is given multiple times, last
rather than first wins, ...

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 arch_init.c                |   4 +-
 hw/i386/smbios.c           | 211 ++++++++++++++++++++++++++++++++++++---------
 include/hw/i386/smbios.h   |   4 +-
 include/sysemu/arch_init.h |   2 +-
 vl.c                       |   3 +-
 5 files changed, 180 insertions(+), 44 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index e0ac143..5930935 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1093,10 +1093,10 @@ void do_acpitable_option(const QemuOpts *opts)
 #endif
 }
 
-void do_smbios_option(const char *optarg)
+void do_smbios_option(QemuOpts *opts)
 {
 #ifdef TARGET_I386
-    smbios_entry_add(optarg);
+    smbios_entry_add(opts);
 #endif
 }
 
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 0608aee..a113f8b 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -2,9 +2,11 @@
  * SMBIOS Support
  *
  * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
+ * Copyright (C) 2013 Red Hat, Inc.
  *
  * Authors:
  *  Alex Williamson <alex.williamson@hp.com>
+ *  Markus Armbruster <armbru@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2.  See
  * the COPYING file in the top-level directory.
@@ -13,6 +15,7 @@
  * GNU GPL, version 2 or (at your option) any later version.
  */
 
+#include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "hw/i386/smbios.h"
@@ -41,11 +44,100 @@ struct smbios_table {
 #define SMBIOS_FIELD_ENTRY 0
 #define SMBIOS_TABLE_ENTRY 1
 
-
 static uint8_t *smbios_entries;
 static size_t smbios_entries_len;
 static int smbios_type4_count = 0;
 
+static QemuOptsList qemu_smbios_opts = {
+    .name = "smbios",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
+    .desc = {
+        /*
+         * no elements => accept any params
+         * validation will happen later
+         */
+        { /* end of list */ }
+    }
+};
+
+static const QemuOptDesc qemu_smbios_file_opts[] = {
+    {
+        .name = "file",
+        .type = QEMU_OPT_STRING,
+        .help = "binary file containing an SMBIOS element",
+    },
+    { /* end of list */ }
+};
+
+static const QemuOptDesc qemu_smbios_type0_opts[] = {
+    {
+        .name = "type",
+        .type = QEMU_OPT_NUMBER,
+        .help = "SMBIOS element type",
+    },{
+        .name = "vendor",
+        .type = QEMU_OPT_STRING,
+        .help = "vendor name",
+    },{
+        .name = "version",
+        .type = QEMU_OPT_STRING,
+        .help = "version number",
+    },{
+        .name = "date",
+        .type = QEMU_OPT_STRING,
+        .help = "release date",
+    },{
+        .name = "release",
+        .type = QEMU_OPT_STRING,
+        .help = "revision number",
+    },
+    { /* end of list */ }
+};
+
+static const QemuOptDesc qemu_smbios_type1_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 = "uuid",
+        .type = QEMU_OPT_STRING,
+        .help = "UUID",
+    },{
+        .name = "sku",
+        .type = QEMU_OPT_STRING,
+        .help = "SKU number",
+    },{
+        .name = "family",
+        .type = QEMU_OPT_STRING,
+        .help = "family name",
+    },
+    { /* end of list */ }
+};
+
+static void smbios_register_config(void)
+{
+    qemu_add_opts(&qemu_smbios_opts);
+}
+
+machine_init(smbios_register_config);
+
 static void smbios_validate_table(void)
 {
     if (smbios_type4_count && smbios_type4_count != smp_cpus) {
@@ -124,23 +216,30 @@ void smbios_add_field(int type, int offset, const void *data, size_t len)
             cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
 }
 
-static void smbios_build_type_0_fields(const char *t)
+static void smbios_build_type_0_fields(QemuOpts *opts)
 {
-    char buf[1024];
+    const char *val;
     unsigned char major, minor;
 
-    if (get_param_value(buf, sizeof(buf), "vendor", t))
+    val = qemu_opt_get(opts, "vendor");
+    if (val) {
         smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "version", t))
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "version");
+    if (val) {
         smbios_add_field(0, offsetof(struct smbios_type_0, bios_version_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "date", t))
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "date");
+    if (val) {
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      bios_release_date_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "release", t)) {
-        if (sscanf(buf, "%hhu.%hhu", &major, &minor) != 2) {
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "release");
+    if (val) {
+        if (sscanf(val, "%hhu.%hhu", &major, &minor) != 2) {
             error_report("Invalid release");
             exit(1);
         }
@@ -153,47 +252,69 @@ static void smbios_build_type_0_fields(const char *t)
     }
 }
 
-static void smbios_build_type_1_fields(const char *t)
+static void smbios_build_type_1_fields(QemuOpts *opts)
 {
-    char buf[1024];
+    const char *val;
 
-    if (get_param_value(buf, sizeof(buf), "manufacturer", t))
+    val = qemu_opt_get(opts, "manufacturer");
+    if (val) {
         smbios_add_field(1, offsetof(struct smbios_type_1, manufacturer_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "product", t))
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "product");
+    if (val) {
         smbios_add_field(1, offsetof(struct smbios_type_1, product_name_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "version", t))
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "version");
+    if (val) {
         smbios_add_field(1, offsetof(struct smbios_type_1, version_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "serial", t))
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "serial");
+    if (val) {
         smbios_add_field(1, offsetof(struct smbios_type_1, serial_number_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "uuid", t)) {
-        if (qemu_uuid_parse(buf, qemu_uuid) != 0) {
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "uuid");
+    if (val) {
+        if (qemu_uuid_parse(val, qemu_uuid) != 0) {
             error_report("Invalid UUID");
             exit(1);
         }
     }
-    if (get_param_value(buf, sizeof(buf), "sku", t))
+    val = qemu_opt_get(opts, "sku");
+    if (val) {
         smbios_add_field(1, offsetof(struct smbios_type_1, sku_number_str),
-                         buf, strlen(buf) + 1);
-    if (get_param_value(buf, sizeof(buf), "family", t))
+                         val, strlen(val) + 1);
+    }
+    val = qemu_opt_get(opts, "family");
+    if (val) {
         smbios_add_field(1, offsetof(struct smbios_type_1, family_str),
-                         buf, strlen(buf) + 1);
+                         val, strlen(val) + 1);
+    }
 }
 
-void smbios_entry_add(const char *t)
+void smbios_entry_add(QemuOpts *opts)
 {
-    char buf[1024];
+    Error *local_err = NULL;
+    const char *val;
 
-    if (get_param_value(buf, sizeof(buf), "file", t)) {
+    val = qemu_opt_get(opts, "file");
+    if (val) {
         struct smbios_structure_header *header;
         struct smbios_table *table;
-        int size = get_image_size(buf);
+        int size;
+
+        qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            exit(1);
+        }
 
+        size = get_image_size(val);
         if (size == -1 || size < sizeof(struct smbios_structure_header)) {
-            error_report("Cannot read SMBIOS file %s", buf);
+            error_report("Cannot read SMBIOS file %s", val);
             exit(1);
         }
 
@@ -208,8 +329,8 @@ void smbios_entry_add(const char *t)
         table->header.type = SMBIOS_TABLE_ENTRY;
         table->header.length = cpu_to_le16(sizeof(*table) + size);
 
-        if (load_image(buf, table->data) != size) {
-            error_report("Failed to load SMBIOS file %s", buf);
+        if (load_image(val, table->data) != size) {
+            error_report("Failed to load SMBIOS file %s", val);
             exit(1);
         }
 
@@ -225,17 +346,29 @@ void smbios_entry_add(const char *t)
         return;
     }
 
-    if (get_param_value(buf, sizeof(buf), "type", t)) {
-        unsigned long type = strtoul(buf, NULL, 0);
+    val = qemu_opt_get(opts, "type");
+    if (val) {
+        unsigned long type = strtoul(val, NULL, 0);
+
         switch (type) {
         case 0:
-            smbios_build_type_0_fields(t);
+            qemu_opts_validate(opts, qemu_smbios_type0_opts, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                exit(1);
+            }
+            smbios_build_type_0_fields(opts);
             return;
         case 1:
-            smbios_build_type_1_fields(t);
+            qemu_opts_validate(opts, qemu_smbios_type1_opts, &local_err);
+            if (local_err) {
+                error_report("%s", error_get_pretty(local_err));
+                exit(1);
+            }
+            smbios_build_type_1_fields(opts);
             return;
         default:
-            error_report("Don't know how to build fields for SMBIOS type %ld",
+            error_report("Don't know how to build fields for SMBIOS type %" PRIu64,
                          type);
             exit(1);
         }
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 56c6108..d9f43b7 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -13,7 +13,9 @@
  *
  */
 
-void smbios_entry_add(const char *t);
+#include "qemu/option.h"
+
+void smbios_entry_add(QemuOpts *opts);
 void smbios_add_field(int type, int offset, const void *data, size_t len);
 uint8_t *smbios_get_table(size_t *length);
 
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index dece913..be71bca 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -28,7 +28,7 @@ extern const uint32_t arch_type;
 
 void select_soundhw(const char *optarg);
 void do_acpitable_option(const QemuOpts *opts);
-void do_smbios_option(const char *optarg);
+void do_smbios_option(QemuOpts *opts);
 void cpudef_init(void);
 void audio_init(void);
 int tcg_available(void);
diff --git a/vl.c b/vl.c
index 25b8f2f..0deadc4 100644
--- a/vl.c
+++ b/vl.c
@@ -3565,7 +3565,8 @@ int main(int argc, char **argv, char **envp)
                 do_acpitable_option(opts);
                 break;
             case QEMU_OPTION_smbios:
-                do_smbios_option(optarg);
+                opts = qemu_opts_parse(qemu_find_opts("smbios"), optarg, 0);
+                do_smbios_option(opts);
                 break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
-- 
1.7.11.7

[Qemu-devel] [PATCH 3/7] smbios: Improve diagnostics for conflicting entries

[Markus Armbruster]

We allow either tables or fields for the same type.  Makes sense,
because SeaBIOS uses fields only when no tables are present.

We do this by searching the SMBIOS blob for a previously added table
or field.  Error messages look like this:

    qemu-system-x86_64: -smbios type=1,serial=42: SMBIOS type 1 table already defined, cannot add field

User needs to know that "table" is defined by -smbios file=..., and
"field" by -smbios type=...

Instead of searching the blob, record additions of interest, and check
that.  Simpler, and makes better error messages possible:

    qemu-system-x86_64: -smbios file=smbios_type_1.bin: Can't mix file= and type= for same type
    qemu-system-x86_64: -smbios type=1,serial=42,serial=99: This is the conflicting setting

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/smbios.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index a113f8b..7908c47 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -48,6 +48,12 @@ static uint8_t *smbios_entries;
 static size_t smbios_entries_len;
 static int smbios_type4_count = 0;
 
+static struct {
+    bool seen;
+    int headertype;
+    Location loc;
+} first_opt[2];
+
 static QemuOptsList qemu_smbios_opts = {
     .name = "smbios",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
@@ -159,35 +165,20 @@ uint8_t *smbios_get_table(size_t *length)
  */
 static void smbios_check_collision(int type, int entry)
 {
-    uint16_t *num_entries = (uint16_t *)smbios_entries;
-    struct smbios_header *header;
-    char *p;
-    int i;
-
-    if (!num_entries)
-        return;
-
-    p = (char *)(num_entries + 1);
-
-    for (i = 0; i < *num_entries; i++) {
-        header = (struct smbios_header *)p;
-        if (entry == SMBIOS_TABLE_ENTRY && header->type == SMBIOS_FIELD_ENTRY) {
-            struct smbios_field *field = (void *)header;
-            if (type == field->type) {
-                error_report("SMBIOS type %d field already defined, "
-                             "cannot add table", type);
-                exit(1);
-            }
-        } else if (entry == SMBIOS_FIELD_ENTRY &&
-                   header->type == SMBIOS_TABLE_ENTRY) {
-            struct smbios_structure_header *table = (void *)(header + 1);
-            if (type == table->type) {
-                error_report("SMBIOS type %d table already defined, "
-                             "cannot add field", type);
+    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);
         }
-        p += le16_to_cpu(header->length);
     }
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 4/7] smbios: Make multiple -smbios type= accumulate sanely

[Markus Armbruster]

Currently, -smbios type=T,NAME=VAL,... adds one field (T,NAME) with
value VAL to fw_cfg for each unique NAME.  If NAME occurs multiple
times, the last one's VAL is used (before the QemuOpts conversion, the
first one was used).

Multiple -smbios can add multiple fields with the same (T, NAME).
SeaBIOS reads all of them from fw_cfg, but uses only the first field
(T, NAME).  The others are ignored.

"First one wins, subsequent ones get ignored silently" isn't nice.  We
commonly let the last option win.  Useful, because it lets you
-readconfig first, then selectively override with command line
options.

Clean up -smbios to work the common way.  Accumulate the settings,
with later ones overwriting earlier ones.  Put the result into fw_cfg
(no more useless duplicates).

Bonus cleanup: qemu_uuid_parse() no longer sets SMBIOS system uuid by
side effect.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 arch_init.c              |   3 -
 hw/i386/smbios.c         | 152 ++++++++++++++++++++++++++++-------------------
 include/hw/i386/smbios.h |   1 -
 include/sysemu/sysemu.h  |   1 +
 vl.c                     |   2 +
 5 files changed, 94 insertions(+), 65 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 5930935..8ea6fba 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1072,9 +1072,6 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
     if (ret != 16) {
         return -1;
     }
-#ifdef TARGET_I386
-    smbios_add_field(1, offsetof(struct smbios_type_1, uuid), uuid, 16);
-#endif
     return 0;
 }
 
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 7908c47..fb1b27b 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -47,6 +47,7 @@ struct smbios_table {
 static uint8_t *smbios_entries;
 static size_t smbios_entries_len;
 static int smbios_type4_count = 0;
+static bool smbios_immutable;
 
 static struct {
     bool seen;
@@ -54,6 +55,17 @@ static struct {
     Location loc;
 } first_opt[2];
 
+static struct {
+    const char *vendor, *version, *date;
+    bool have_major_minor;
+    uint8_t major, minor;
+} type0;
+
+static struct {
+    const char *manufacturer, *product, *version, *serial, *sku, *family;
+    /* uuid is in qemu_uuid[] */
+} type1;
+
 static QemuOptsList qemu_smbios_opts = {
     .name = "smbios",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
@@ -152,13 +164,6 @@ static void smbios_validate_table(void)
     }
 }
 
-uint8_t *smbios_get_table(size_t *length)
-{
-    smbios_validate_table();
-    *length = smbios_entries_len;
-    return smbios_entries;
-}
-
 /*
  * To avoid unresolvable overlaps in data, don't allow both
  * tables and fields for the same smbios type.
@@ -182,12 +187,10 @@ static void smbios_check_collision(int type, int entry)
     }
 }
 
-void smbios_add_field(int type, int offset, const void *data, size_t len)
+static void smbios_add_field(int type, int offset, const void *data, size_t len)
 {
     struct smbios_field *field;
 
-    smbios_check_collision(type, SMBIOS_FIELD_ENTRY);
-
     if (!smbios_entries) {
         smbios_entries_len = sizeof(uint16_t);
         smbios_entries = g_malloc0(smbios_entries_len);
@@ -207,82 +210,81 @@ void smbios_add_field(int type, int offset, const void *data, size_t len)
             cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
 }
 
-static void smbios_build_type_0_fields(QemuOpts *opts)
+static void smbios_build_type_0_fields(void)
 {
-    const char *val;
-    unsigned char major, minor;
-
-    val = qemu_opt_get(opts, "vendor");
-    if (val) {
+    if (type0.vendor) {
         smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
-                         val, strlen(val) + 1);
+                         type0.vendor, strlen(type0.vendor) + 1);
     }
-    val = qemu_opt_get(opts, "version");
-    if (val) {
+    if (type0.version) {
         smbios_add_field(0, offsetof(struct smbios_type_0, bios_version_str),
-                         val, strlen(val) + 1);
+                         type0.version, strlen(type0.version) + 1);
     }
-    val = qemu_opt_get(opts, "date");
-    if (val) {
+    if (type0.date) {
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      bios_release_date_str),
-                         val, strlen(val) + 1);
+                         type0.date, strlen(type0.date) + 1);
     }
-    val = qemu_opt_get(opts, "release");
-    if (val) {
-        if (sscanf(val, "%hhu.%hhu", &major, &minor) != 2) {
-            error_report("Invalid release");
-            exit(1);
-        }
+    if (type0.have_major_minor) {
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      system_bios_major_release),
-                         &major, 1);
+                         &type0.major, 1);
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      system_bios_minor_release),
-                         &minor, 1);
+                         &type0.minor, 1);
     }
 }
 
-static void smbios_build_type_1_fields(QemuOpts *opts)
+static void smbios_build_type_1_fields(void)
 {
-    const char *val;
-
-    val = qemu_opt_get(opts, "manufacturer");
-    if (val) {
+    if (type1.manufacturer) {
         smbios_add_field(1, offsetof(struct smbios_type_1, manufacturer_str),
-                         val, strlen(val) + 1);
+                         type1.manufacturer, strlen(type1.manufacturer) + 1);
     }
-    val = qemu_opt_get(opts, "product");
-    if (val) {
+    if (type1.product) {
         smbios_add_field(1, offsetof(struct smbios_type_1, product_name_str),
-                         val, strlen(val) + 1);
+                         type1.product, strlen(type1.product) + 1);
     }
-    val = qemu_opt_get(opts, "version");
-    if (val) {
+    if (type1.version) {
         smbios_add_field(1, offsetof(struct smbios_type_1, version_str),
-                         val, strlen(val) + 1);
+                         type1.version, strlen(type1.version) + 1);
     }
-    val = qemu_opt_get(opts, "serial");
-    if (val) {
+    if (type1.serial) {
         smbios_add_field(1, offsetof(struct smbios_type_1, serial_number_str),
-                         val, strlen(val) + 1);
-    }
-    val = qemu_opt_get(opts, "uuid");
-    if (val) {
-        if (qemu_uuid_parse(val, qemu_uuid) != 0) {
-            error_report("Invalid UUID");
-            exit(1);
-        }
+                         type1.serial, strlen(type1.serial) + 1);
     }
-    val = qemu_opt_get(opts, "sku");
-    if (val) {
+    if (type1.sku) {
         smbios_add_field(1, offsetof(struct smbios_type_1, sku_number_str),
-                         val, strlen(val) + 1);
+                         type1.sku, strlen(type1.sku) + 1);
     }
-    val = qemu_opt_get(opts, "family");
-    if (val) {
+    if (type1.family) {
         smbios_add_field(1, offsetof(struct smbios_type_1, family_str),
-                         val, strlen(val) + 1);
+                         type1.family, strlen(type1.family) + 1);
+    }
+    if (qemu_uuid_set) {
+        smbios_add_field(1, offsetof(struct smbios_type_1, uuid),
+                         qemu_uuid, 16);
+    }
+}
+
+uint8_t *smbios_get_table(size_t *length)
+{
+    if (!smbios_immutable) {
+        smbios_build_type_0_fields();
+        smbios_build_type_1_fields();
+        smbios_validate_table();
+        smbios_immutable = true;
+    }
+    *length = smbios_entries_len;
+    return smbios_entries;
+}
+
+static void save_opt(const char **dest, QemuOpts *opts, const char *name)
+{
+    const char *val = qemu_opt_get(opts, name);
+
+    if (val) {
+        *dest = val;
     }
 }
 
@@ -291,6 +293,7 @@ void smbios_entry_add(QemuOpts *opts)
     Error *local_err = NULL;
     const char *val;
 
+    assert(!smbios_immutable);
     val = qemu_opt_get(opts, "file");
     if (val) {
         struct smbios_structure_header *header;
@@ -341,6 +344,8 @@ void smbios_entry_add(QemuOpts *opts)
     if (val) {
         unsigned long type = strtoul(val, NULL, 0);
 
+        smbios_check_collision(type, SMBIOS_FIELD_ENTRY);
+
         switch (type) {
         case 0:
             qemu_opts_validate(opts, qemu_smbios_type0_opts, &local_err);
@@ -348,7 +353,18 @@ void smbios_entry_add(QemuOpts *opts)
                 error_report("%s", error_get_pretty(local_err));
                 exit(1);
             }
-            smbios_build_type_0_fields(opts);
+            save_opt(&type0.vendor, opts, "vendor");
+            save_opt(&type0.version, opts, "version");
+            save_opt(&type0.date, opts, "date");
+
+            val = qemu_opt_get(opts, "release");
+            if (val) {
+                if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
+                    error_report("Invalid release");
+                    exit(1);
+                }
+                type0.have_major_minor = true;
+            }
             return;
         case 1:
             qemu_opts_validate(opts, qemu_smbios_type1_opts, &local_err);
@@ -356,7 +372,21 @@ void smbios_entry_add(QemuOpts *opts)
                 error_report("%s", error_get_pretty(local_err));
                 exit(1);
             }
-            smbios_build_type_1_fields(opts);
+            save_opt(&type1.manufacturer, opts, "manufacturer");
+            save_opt(&type1.product, opts, "product");
+            save_opt(&type1.version, opts, "version");
+            save_opt(&type1.serial, opts, "serial");
+            save_opt(&type1.sku, opts, "sku");
+            save_opt(&type1.family, opts, "family");
+
+            val = qemu_opt_get(opts, "uuid");
+            if (val) {
+                if (qemu_uuid_parse(val, qemu_uuid) != 0) {
+                    error_report("Invalid UUID");
+                    exit(1);
+                }
+                qemu_uuid_set = true;
+            }
             return;
         default:
             error_report("Don't know how to build fields for SMBIOS type %" PRIu64,
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index d9f43b7..b08ec71 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -16,7 +16,6 @@
 #include "qemu/option.h"
 
 void smbios_entry_add(QemuOpts *opts);
-void smbios_add_field(int type, int offset, const void *data, size_t len);
 uint8_t *smbios_get_table(size_t *length);
 
 /*
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 3caeb66..d490c99 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -16,6 +16,7 @@ extern const char *bios_name;
 
 extern const char *qemu_name;
 extern uint8_t qemu_uuid[];
+extern bool qemu_uuid_set;
 int qemu_uuid_parse(const char *str, uint8_t *uuid);
 #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
 
diff --git a/vl.c b/vl.c
index 0deadc4..45b4c52 100644
--- a/vl.c
+++ b/vl.c
@@ -254,6 +254,7 @@ uint64_t node_mem[MAX_NODES];
 unsigned long *node_cpumask[MAX_NODES];
 
 uint8_t qemu_uuid[16];
+bool qemu_uuid_set;
 
 static QEMUBootSetHandler *boot_set_handler;
 static void *boot_set_opaque;
@@ -3662,6 +3663,7 @@ int main(int argc, char **argv, char **envp)
                             " Wrong format.\n");
                     exit(1);
                 }
+                qemu_uuid_set = true;
                 break;
 	    case QEMU_OPTION_option_rom:
 		if (nb_option_roms >= MAX_OPTION_ROMS) {
-- 
1.7.11.7

[Qemu-devel] [PATCH 5/7] smbios: Factor out smbios_maybe_add_str()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/smbios.c | 61 +++++++++++++++++++++++---------------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index fb1b27b..a2eb9bf 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -210,21 +210,22 @@ static void smbios_add_field(int type, int offset, const void *data, size_t len)
             cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1);
 }
 
-static void smbios_build_type_0_fields(void)
+static void smbios_maybe_add_str(int type, int offset, const char *data)
 {
-    if (type0.vendor) {
-        smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str),
-                         type0.vendor, strlen(type0.vendor) + 1);
-    }
-    if (type0.version) {
-        smbios_add_field(0, offsetof(struct smbios_type_0, bios_version_str),
-                         type0.version, strlen(type0.version) + 1);
+    if (data) {
+        smbios_add_field(type, offset, data, strlen(data) + 1);
     }
-    if (type0.date) {
-        smbios_add_field(0, offsetof(struct smbios_type_0,
+}
+
+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, strlen(type0.date) + 1);
-    }
+                         type0.date);
     if (type0.have_major_minor) {
         smbios_add_field(0, offsetof(struct smbios_type_0,
                                      system_bios_major_release),
@@ -237,30 +238,18 @@ static void smbios_build_type_0_fields(void)
 
 static void smbios_build_type_1_fields(void)
 {
-    if (type1.manufacturer) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, manufacturer_str),
-                         type1.manufacturer, strlen(type1.manufacturer) + 1);
-    }
-    if (type1.product) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, product_name_str),
-                         type1.product, strlen(type1.product) + 1);
-    }
-    if (type1.version) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, version_str),
-                         type1.version, strlen(type1.version) + 1);
-    }
-    if (type1.serial) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, serial_number_str),
-                         type1.serial, strlen(type1.serial) + 1);
-    }
-    if (type1.sku) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, sku_number_str),
-                         type1.sku, strlen(type1.sku) + 1);
-    }
-    if (type1.family) {
-        smbios_add_field(1, offsetof(struct smbios_type_1, family_str),
-                         type1.family, strlen(type1.family) + 1);
-    }
+    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);
-- 
1.7.11.7

[Qemu-devel] [PATCH 6/7] vl: Set current_machine early

[Markus Armbruster]

I'd like to access QEMUMachine from a QEMUMachine init() method, which
is currently not possible.  Instead of passing it as an argument, I
simply set current_machine earlier.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 45b4c52..9387662 100644
--- a/vl.c
+++ b/vl.c
@@ -3897,6 +3897,7 @@ int main(int argc, char **argv, char **envp)
         fprintf(stderr, "No machine found.\n");
         exit(1);
     }
+    current_machine = machine;
 
     if (machine->hw_version) {
         qemu_set_version(machine->hw_version);
@@ -4325,8 +4326,6 @@ int main(int argc, char **argv, char **envp)
 
     set_numa_modes();
 
-    current_machine = machine;
-
     /* init USB devices */
     if (usb_enabled(false)) {
         if (foreach_device_config(DEV_USB, usb_parse) < 0)
-- 
1.7.11.7

[Qemu-devel] [PATCH 7/7] smbios: Set system manufacturer, product & version by default

[Markus Armbruster]

Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs,
no version.  Best SeaBIOS can do, but we can provide better defaults:
manufacturer QEMU, product & version taken from QEMUMachine desc and
name.

Take care to do this only for new machine types, of course.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/pc.c             |  6 +++---
 hw/i386/pc_piix.c        |  5 +++++
 hw/i386/pc_q35.c         |  3 +++
 hw/i386/smbios.c         | 12 +++++++++++-
 include/hw/i386/pc.h     |  1 +
 include/hw/i386/smbios.h |  2 +-
 6 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c5d8570..c427f9f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -605,7 +605,7 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
     return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
 }
 
-static FWCfgState *bochs_bios_init(void)
+static FWCfgState *bochs_bios_init(bool smbios_type1_defaults)
 {
     FWCfgState *fw_cfg;
     uint8_t *smbios_table;
@@ -636,7 +636,7 @@ static FWCfgState *bochs_bios_init(void)
                      acpi_tables, acpi_tables_len);
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
 
-    smbios_table = smbios_get_table(&smbios_len);
+    smbios_table = smbios_get_table(&smbios_len, smbios_type1_defaults);
     if (smbios_table)
         fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
                          smbios_table, smbios_len);
@@ -1146,7 +1146,7 @@ FWCfgState *pc_memory_init(MemoryRegion *system_memory,
                                         option_rom_mr,
                                         1);
 
-    fw_cfg = bochs_bios_init();
+    fw_cfg = bochs_bios_init(guest_info->smbios_type1_defaults);
     rom_set_fw(fw_cfg);
 
     if (linux_boot) {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b58c255..60bf752 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -58,6 +58,7 @@ static const int ide_irq[MAX_IDE_BUS] = { 14, 15 };
 
 static bool has_pvpanic = true;
 static bool has_pci_info = true;
+static bool smbios_type1_defaults = true;
 
 /* PC hardware initialisation */
 static void pc_init1(MemoryRegion *system_memory,
@@ -128,6 +129,7 @@ static void pc_init1(MemoryRegion *system_memory,
 
     guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
     guest_info->has_pci_info = has_pci_info;
+    guest_info->smbios_type1_defaults = smbios_type1_defaults;
 
     /* Set PCI window size the way seabios has always done it. */
     /* Power of 2 so bios can cover it with a single MTRR */
@@ -264,6 +266,7 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
 static void pc_init_pci_1_5(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    smbios_type1_defaults = false;
     pc_init_pci(args);
 }
 
@@ -304,6 +307,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
     const char *boot_device = args->boot_device;
     has_pvpanic = false;
     has_pci_info = false;
+    smbios_type1_defaults = false;
     disable_kvm_pv_eoi();
     enable_compat_apic_id_mode();
     pc_init1(get_system_memory(),
@@ -323,6 +327,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
     const char *boot_device = args->boot_device;
     has_pvpanic = false;
     has_pci_info = false;
+    smbios_type1_defaults = false;
     if (cpu_model == NULL)
         cpu_model = "486";
     disable_kvm_pv_eoi();
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6f10246..c756474 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -48,6 +48,7 @@
 
 static bool has_pvpanic = true;
 static bool has_pci_info = true;
+static bool smbios_type1_defaults = true;
 
 /* PC hardware initialisation */
 static void pc_q35_init(QEMUMachineInitArgs *args)
@@ -109,6 +110,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 
     guest_info = pc_guest_info_init(below_4g_mem_size, above_4g_mem_size);
     guest_info->has_pci_info = has_pci_info;
+    guest_info->smbios_type1_defaults = smbios_type1_defaults;
 
     /* allocate ram and load rom/bios */
     if (!xen_enabled()) {
@@ -217,6 +219,7 @@ static void pc_q35_init(QEMUMachineInitArgs *args)
 static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
 {
     has_pci_info = false;
+    smbios_type1_defaults = false;
     pc_q35_init(args);
 }
 
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index a2eb9bf..e6413a5 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -18,6 +18,7 @@
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "hw/boards.h"
 #include "hw/i386/smbios.h"
 #include "hw/loader.h"
 
@@ -256,9 +257,18 @@ static void smbios_build_type_1_fields(void)
     }
 }
 
-uint8_t *smbios_get_table(size_t *length)
+uint8_t *smbios_get_table(size_t *length, bool type1_defaults)
 {
     if (!smbios_immutable) {
+        if (type1_defaults && !type1.manufacturer) {
+            type1.manufacturer = "QEMU";
+        }
+        if (type1_defaults && !type1.product) {
+            type1.product = current_machine->desc;
+        }
+        if (type1_defaults && !type1.version) {
+            type1.version = current_machine->name;
+        }
         smbios_build_type_0_fields();
         smbios_build_type_1_fields();
         smbios_validate_table();
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 61ff154..860f2bb 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -20,6 +20,7 @@ typedef struct PcPciInfo {
 struct PcGuestInfo {
     PcPciInfo pci_info;
     bool has_pci_info;
+    bool smbios_type1_defaults;
     FWCfgState *fw_cfg;
 };
 
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index b08ec71..e258d9a 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -16,7 +16,7 @@
 #include "qemu/option.h"
 
 void smbios_entry_add(QemuOpts *opts);
-uint8_t *smbios_get_table(size_t *length);
+uint8_t *smbios_get_table(size_t *length, bool type1_defaults);
 
 /*
  * SMBIOS spec defined tables
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> It exits on all error conditions but one, where it returns -1.
> Normalize, and return void.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  arch_init.c              |  4 +---
>  hw/i386/smbios.c         | 10 +++++-----
>  include/hw/i386/smbios.h |  2 +-
>  3 files changed, 7 insertions(+), 9 deletions(-)
> 

> @@ -242,5 +242,5 @@ int smbios_entry_add(const char *t)
>      }
>  
>      error_report("Must specify type= or file=");
> -    return -1;
> +    exit(1);

I hate magic numbers; wouldn't exit(EXIT_FAILURE) be more explicit at
your intent?  At any rate, you're not the first person to use exit(1), so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 2/7] smbios: Convert to QemuOpts

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> So that it can be set in config file for -readconfig.
> 
> This tightens parsing of -smbios, and makes it more consistent with
> other options: unknown parameters are rejected, numbers with trailing
> junk are rejected, when a parameter is given multiple times, last
> rather than first wins, ...
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  arch_init.c                |   4 +-
>  hw/i386/smbios.c           | 211 ++++++++++++++++++++++++++++++++++++---------
>  include/hw/i386/smbios.h   |   4 +-
>  include/sysemu/arch_init.h |   2 +-
>  vl.c                       |   3 +-
>  5 files changed, 180 insertions(+), 44 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 3/7] smbios: Improve diagnostics for conflicting entries

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> We allow either tables or fields for the same type.  Makes sense,
> because SeaBIOS uses fields only when no tables are present.
> 
> We do this by searching the SMBIOS blob for a previously added table
> or field.  Error messages look like this:
> 
>     qemu-system-x86_64: -smbios type=1,serial=42: SMBIOS type 1 table already defined, cannot add field
> 
> User needs to know that "table" is defined by -smbios file=..., and
> "field" by -smbios type=...
> 
> Instead of searching the blob, record additions of interest, and check
> that.  Simpler, and makes better error messages possible:
> 
>     qemu-system-x86_64: -smbios file=smbios_type_1.bin: Can't mix file= and type= for same type
>     qemu-system-x86_64: -smbios type=1,serial=42,serial=99: This is the conflicting setting
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/smbios.c | 43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)

Nicer message in fewer lines of code - what's not to like about it?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 4/7] smbios: Make multiple -smbios type= accumulate sanely

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> Currently, -smbios type=T,NAME=VAL,... adds one field (T,NAME) with
> value VAL to fw_cfg for each unique NAME.  If NAME occurs multiple
> times, the last one's VAL is used (before the QemuOpts conversion, the
> first one was used).
> 
> Multiple -smbios can add multiple fields with the same (T, NAME).
> SeaBIOS reads all of them from fw_cfg, but uses only the first field
> (T, NAME).  The others are ignored.
> 
> "First one wins, subsequent ones get ignored silently" isn't nice.  We
> commonly let the last option win.  Useful, because it lets you
> -readconfig first, then selectively override with command line
> options.
> 
> Clean up -smbios to work the common way.  Accumulate the settings,
> with later ones overwriting earlier ones.  Put the result into fw_cfg
> (no more useless duplicates).
> 
> Bonus cleanup: qemu_uuid_parse() no longer sets SMBIOS system uuid by
> side effect.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

>      }
> -    val = qemu_opt_get(opts, "release");
> -    if (val) {
> -        if (sscanf(val, "%hhu.%hhu", &major, &minor) != 2) {
> -            error_report("Invalid release");
> -            exit(1);

> @@ -348,7 +353,18 @@ void smbios_entry_add(QemuOpts *opts)
>                  error_report("%s", error_get_pretty(local_err));
>                  exit(1);
>              }
> -            smbios_build_type_0_fields(opts);
> +            save_opt(&type0.vendor, opts, "vendor");
> +            save_opt(&type0.version, opts, "version");
> +            save_opt(&type0.date, opts, "date");
> +
> +            val = qemu_opt_get(opts, "release");
> +            if (val) {
> +                if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {

sscanf() is lousy at detecting overflow.  Since this is just code
motion, you can get away without changing it now, but it would be nice
if you at least considered switching this to use [a sane wrapper around]
strtol() for proper overflow detection of user input at some point in
your refactoring.


> +            val = qemu_opt_get(opts, "uuid");
> +            if (val) {
> +                if (qemu_uuid_parse(val, qemu_uuid) != 0) {
> +                    error_report("Invalid UUID");
> +                    exit(1);

My comments from earlier in the series about preferring
exit(EXIT_FAILURE) apply here as well.

Neither issue is a show-stopper, so

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 5/7] smbios: Factor out smbios_maybe_add_str()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/smbios.c | 61 +++++++++++++++++++++++---------------------------------
>  1 file changed, 25 insertions(+), 36 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 6/7] vl: Set current_machine early

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> I'd like to access QEMUMachine from a QEMUMachine init() method, which
> is currently not possible.  Instead of passing it as an argument, I
> simply set current_machine earlier.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  vl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 7/7] smbios: Set system manufacturer, product & version by default

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> Currently, we get SeaBIOS defaults: manufacturer Bochs, product Bochs,
> no version.  Best SeaBIOS can do, but we can provide better defaults:
> manufacturer QEMU, product & version taken from QEMUMachine desc and
> name.
> 
> Take care to do this only for new machine types, of course.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/pc.c             |  6 +++---
>  hw/i386/pc_piix.c        |  5 +++++
>  hw/i386/pc_q35.c         |  3 +++
>  hw/i386/smbios.c         | 12 +++++++++++-
>  include/hw/i386/pc.h     |  1 +
>  include/hw/i386/smbios.h |  2 +-
>  6 files changed, 24 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 4/7] smbios: Make multiple -smbios type= accumulate sanely

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 07/17/2013 11:16 AM, Markus Armbruster wrote:
>> Currently, -smbios type=T,NAME=VAL,... adds one field (T,NAME) with
>> value VAL to fw_cfg for each unique NAME.  If NAME occurs multiple
>> times, the last one's VAL is used (before the QemuOpts conversion, the
>> first one was used).
>> 
>> Multiple -smbios can add multiple fields with the same (T, NAME).
>> SeaBIOS reads all of them from fw_cfg, but uses only the first field
>> (T, NAME).  The others are ignored.
>> 
>> "First one wins, subsequent ones get ignored silently" isn't nice.  We
>> commonly let the last option win.  Useful, because it lets you
>> -readconfig first, then selectively override with command line
>> options.
>> 
>> Clean up -smbios to work the common way.  Accumulate the settings,
>> with later ones overwriting earlier ones.  Put the result into fw_cfg
>> (no more useless duplicates).
>> 
>> Bonus cleanup: qemu_uuid_parse() no longer sets SMBIOS system uuid by
>> side effect.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>>      }
>> -    val = qemu_opt_get(opts, "release");
>> -    if (val) {
>> -        if (sscanf(val, "%hhu.%hhu", &major, &minor) != 2) {
>> -            error_report("Invalid release");
>> -            exit(1);
>
>> @@ -348,7 +353,18 @@ void smbios_entry_add(QemuOpts *opts)
>>                  error_report("%s", error_get_pretty(local_err));
>>                  exit(1);
>>              }
>> -            smbios_build_type_0_fields(opts);
>> +            save_opt(&type0.vendor, opts, "vendor");
>> +            save_opt(&type0.version, opts, "version");
>> +            save_opt(&type0.date, opts, "date");
>> +
>> +            val = qemu_opt_get(opts, "release");
>> +            if (val) {
>> +                if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
>
> sscanf() is lousy at detecting overflow.  Since this is just code
> motion, you can get away without changing it now, but it would be nice
> if you at least considered switching this to use [a sane wrapper around]
> strtol() for proper overflow detection of user input at some point in
> your refactoring.

Correct use of strol() & friends is surprisingly involved, and a bit
more than I bargained for when moving this code around :)

>> +            val = qemu_opt_get(opts, "uuid");
>> +            if (val) {
>> +                if (qemu_uuid_parse(val, qemu_uuid) != 0) {
>> +                    error_report("Invalid UUID");
>> +                    exit(1);
>
> My comments from earlier in the series about preferring
> exit(EXIT_FAILURE) apply here as well.

I very much prefer all unsuccessful exits to look the same.  There are
eight instances of exit(1) in this file before my series, I'm removing
three, and adding six.

While I'm sympathetic to the general "symbols are nicer than magic
numbers" sentiment, exit(1) is as harmless as it gets.

> Neither issue is a show-stopper, so
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks for the review!

Re: [Qemu-devel] [PATCH 0/7] smbios cleanup & nicer defaults for type 1

[Markus Armbruster]

Ping for 1.6

Markus Armbruster <armbru@redhat.com> writes:

> This gets rid of one of the last get_param_value() users, makes
> multiple -smbios work sanely, cleans up the gross side effect in
> qemu_uuid_parse(), and more.  Topped off with a little feature in the
> last patch.
>
> Markus Armbruster (7):
>   smbios: Normalize smbios_entry_add()'s error handling to exit(1)
>   smbios: Convert to QemuOpts
>   smbios: Improve diagnostics for conflicting entries
>   smbios: Make multiple -smbios type= accumulate sanely
>   smbios: Factor out smbios_maybe_add_str()
>   vl: Set current_machine early
>   smbios: Set system manufacturer, product & version by default
>
>  arch_init.c                |   9 +-
>  hw/i386/pc.c               |   6 +-
>  hw/i386/pc_piix.c          |   5 +
>  hw/i386/pc_q35.c           |   3 +
>  hw/i386/smbios.c           | 349 ++++++++++++++++++++++++++++++++-------------
>  include/hw/i386/pc.h       |   1 +
>  include/hw/i386/smbios.h   |   7 +-
>  include/sysemu/arch_init.h |   2 +-
>  include/sysemu/sysemu.h    |   1 +
>  vl.c                       |   8 +-
>  10 files changed, 276 insertions(+), 115 deletions(-)