* [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1
@ 2013-08-16 13:18 armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1) armbru
` (9 more replies)
0 siblings, 10 replies; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
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.
v2: Rebase, only last patch had conflicts
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.8.1.4
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1)
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
@ 2013-08-16 13:18 ` armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts armbru
` (8 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
It exits on all error conditions but one, where it returns -1.
Normalize, and return void.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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 68a7ab7..1ddead0 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1136,9 +1136,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.8.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1) armbru
@ 2013-08-16 13:18 ` armbru
2013-09-28 20:47 ` Michael S. Tsirkin
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 3/7] smbios: Improve diagnostics for conflicting entries armbru
` (7 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
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>
Reviewed-by: Eric Blake <eblake@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 1ddead0..96477fd 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1133,10 +1133,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 f422a1c..f63ffd2 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.8.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2 3/7] smbios: Improve diagnostics for conflicting entries
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1) armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts armbru
@ 2013-08-16 13:18 ` armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 4/7] smbios: Make multiple -smbios type= accumulate sanely armbru
` (6 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
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>
Reviewed-by: Eric Blake <eblake@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.8.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2 4/7] smbios: Make multiple -smbios type= accumulate sanely
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
` (2 preceding siblings ...)
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 3/7] smbios: Improve diagnostics for conflicting entries armbru
@ 2013-08-16 13:18 ` armbru
2013-08-17 12:48 ` Eric Blake
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 5/7] smbios: Factor out smbios_maybe_add_str() armbru
` (5 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
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>
Reviewed-by: Eric Blake <eblake@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 96477fd..3cb1caf 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1112,9 +1112,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 d7a77b6..290ade7 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 f63ffd2..ba3084c 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.8.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2 5/7] smbios: Factor out smbios_maybe_add_str()
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
` (3 preceding siblings ...)
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 4/7] smbios: Make multiple -smbios type= accumulate sanely armbru
@ 2013-08-16 13:18 ` armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early armbru
` (4 subsequent siblings)
9 siblings, 0 replies; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@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.8.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
` (4 preceding siblings ...)
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 5/7] smbios: Factor out smbios_maybe_add_str() armbru
@ 2013-08-16 13:18 ` armbru
2013-08-17 13:07 ` Andreas Färber
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default armbru
` (3 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
vl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/vl.c b/vl.c
index ba3084c..258e164 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.8.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
` (5 preceding siblings ...)
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early armbru
@ 2013-08-16 13:18 ` armbru
2013-08-27 17:04 ` Michael S. Tsirkin
2013-08-17 12:08 ` [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 Laszlo Ersek
` (2 subsequent siblings)
9 siblings, 1 reply; 24+ messages in thread
From: armbru @ 2013-08-16 13:18 UTC (permalink / raw)
To: qemu-devel; +Cc: lersek, aliguori, ehabkost
From: Markus Armbruster <armbru@redhat.com>
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>
Reviewed-by: Eric Blake <eblake@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 e8bc8ce..eb7ffc4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -604,7 +604,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;
@@ -635,7 +635,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);
@@ -1155,7 +1155,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 6e1e654..2a621ef 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;
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->isapc_ram_fw = !pci_enabled;
+ guest_info->smbios_type1_defaults = smbios_type1_defaults;
/* allocate ram and load rom/bios */
if (!xen_enabled()) {
@@ -258,6 +260,7 @@ static void pc_init_pci_1_6(QEMUMachineInitArgs *args)
static void pc_init_pci_1_5(QEMUMachineInitArgs *args)
{
has_pvpanic = true;
+ smbios_type1_defaults = false;
pc_init_pci_1_6(args);
}
@@ -298,6 +301,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
const char *initrd_filename = args->initrd_filename;
const char *boot_device = args->boot_device;
has_pci_info = false;
+ smbios_type1_defaults = false;
disable_kvm_pv_eoi();
enable_compat_apic_id_mode();
pc_init1(get_system_memory(),
@@ -316,6 +320,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
const char *initrd_filename = args->initrd_filename;
const char *boot_device = args->boot_device;
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 10e770e..05bce55 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -48,6 +48,7 @@
static bool has_pvpanic;
static bool has_pci_info = true;
+static bool smbios_type1_defaults = true;
/* PC hardware initialisation */
static void pc_q35_init(QEMUMachineInitArgs *args)
@@ -111,6 +112,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->isapc_ram_fw = false;
+ guest_info->smbios_type1_defaults = smbios_type1_defaults;
/* allocate ram and load rom/bios */
if (!xen_enabled()) {
@@ -227,6 +229,7 @@ static void pc_q35_init_1_6(QEMUMachineInitArgs *args)
static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
{
has_pvpanic = true;
+ smbios_type1_defaults = false;
pc_q35_init_1_6(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 f79d478..5797b97 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -20,6 +20,7 @@ typedef struct PcPciInfo {
struct PcGuestInfo {
bool has_pci_info;
bool isapc_ram_fw;
+ 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.8.1.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
` (6 preceding siblings ...)
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default armbru
@ 2013-08-17 12:08 ` Laszlo Ersek
2013-08-17 12:50 ` Eric Blake
2013-09-17 15:27 ` Michael S. Tsirkin
2013-09-28 19:41 ` Michael S. Tsirkin
9 siblings, 1 reply; 24+ messages in thread
From: Laszlo Ersek @ 2013-08-17 12:08 UTC (permalink / raw)
To: Eric Blake; +Cc: aliguori, armbru, ehabkost, qemu-devel
Eric,
On 08/16/13 15:18, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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.
>
> v2: Rebase, only last patch had conflicts
can you please re-ack this? You acked v2:
http://thread.gmane.org/gmane.comp.emulators.qemu/222747
(And I don't have the slightest clue why it hasn't been merged after
your Reviewed-by; it was posted, and reviewed, and pinged, before the
hard freeze for 1.6)
If you don't have the time, I can review it from scratch.
Sometime.
Hopefully.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/7] smbios: Make multiple -smbios type= accumulate sanely
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 4/7] smbios: Make multiple -smbios type= accumulate sanely armbru
@ 2013-08-17 12:48 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2013-08-17 12:48 UTC (permalink / raw)
To: armbru; +Cc: aliguori, lersek, qemu-devel, ehabkost
[-- Attachment #1: Type: text/plain, Size: 906 bytes --]
On 08/16/2013 07:18 AM, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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).
>
> +
> + val = qemu_opt_get(opts, "release");
> + if (val) {
> + if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
> + error_report("Invalid release");
As in v1, I'm not a fan of using sscanf for integer parsing (it has
undefined behavior if the user provides a value that overflows the
destination type); but as this is code motion, it does not invalidate my
review.
--
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1
2013-08-17 12:08 ` [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 Laszlo Ersek
@ 2013-08-17 12:50 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2013-08-17 12:50 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: aliguori, armbru, ehabkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 936 bytes --]
On 08/17/2013 06:08 AM, Laszlo Ersek wrote:
> Eric,
>
> On 08/16/13 15:18, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> 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.
>>
>> v2: Rebase, only last patch had conflicts
>
> can you please re-ack this? You acked v2:
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/222747
My prior review still stands. The series looks good to me.
>
> (And I don't have the slightest clue why it hasn't been merged after
> your Reviewed-by; it was posted, and reviewed, and pinged, before the
> hard freeze for 1.6)
I have no idea what happened either.
--
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early armbru
@ 2013-08-17 13:07 ` Andreas Färber
2013-08-19 9:35 ` Markus Armbruster
0 siblings, 1 reply; 24+ messages in thread
From: Andreas Färber @ 2013-08-17 13:07 UTC (permalink / raw)
To: armbru; +Cc: aliguori, lersek, qemu-devel, ehabkost
Am 16.08.2013 15:18, schrieb armbru@redhat.com:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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.
We had such a patch for CPU hot-add and decided against doing this.
Currently current_machine != signals that it has been initialized. And
generally we have been trying to get away from accessing globals from
random parts of code.
Can't you pass either QEMUMachine or the specific fields needed from PC
code to those SMBIOS functions? You did add a bool argument.
Andreas
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> vl.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index ba3084c..258e164 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)
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early
2013-08-17 13:07 ` Andreas Färber
@ 2013-08-19 9:35 ` Markus Armbruster
2013-08-19 16:37 ` Andreas Färber
0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2013-08-19 9:35 UTC (permalink / raw)
To: Andreas Färber; +Cc: aliguori, lersek, qemu-devel, ehabkost
Andreas Färber <afaerber@suse.de> writes:
> Am 16.08.2013 15:18, schrieb armbru@redhat.com:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> 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.
>
> We had such a patch for CPU hot-add and decided against doing this.
> Currently current_machine != signals that it has been initialized. And
Does any code actually depend on this undocumented condition? I found
none.
> generally we have been trying to get away from accessing globals from
> random parts of code.
Global state need to be managed with care. Global variables have their
place in that.
In my experience, the most common kind of carelessness involving global
variables is indisciplined *updating* of global state via global
variables.
Unchanging global state is relatively harmless, and referring to it via
global variables is often the easiest and most obvious way to manage
such state.
> Can't you pass either QEMUMachine or the specific fields needed from PC
> code to those SMBIOS functions? You did add a bool argument.
Can't see how to do that without passing the machine to QEMUMachine
method init(), which involves touching all boards. I doubt that's a
good idea, but if you insist, I can do it.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early
2013-08-19 9:35 ` Markus Armbruster
@ 2013-08-19 16:37 ` Andreas Färber
2013-08-20 9:09 ` Markus Armbruster
0 siblings, 1 reply; 24+ messages in thread
From: Andreas Färber @ 2013-08-19 16:37 UTC (permalink / raw)
To: Markus Armbruster; +Cc: aliguori, lersek, qemu-devel, ehabkost
Am 19.08.2013 11:35, schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
>
>> Am 16.08.2013 15:18, schrieb armbru@redhat.com:
>>> From: Markus Armbruster <armbru@redhat.com>
>>>
>>> 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.
>>
>> We had such a patch for CPU hot-add and decided against doing this.
>> Currently current_machine != signals that it has been initialized. And
>
> Does any code actually depend on this undocumented condition? I found
> none.
I didn't audit. Currently the users are limited to vl.c itself,
device-hotplug.c for block_default_type and qmp.c for hot_add_cpu. pc.c
feels odd in that mix.
[...]
>> Can't you pass either QEMUMachine or the specific fields needed from PC
>> code to those SMBIOS functions? You did add a bool argument.
>
> Can't see how to do that without passing the machine to QEMUMachine
> method init(), which involves touching all boards. I doubt that's a
> good idea, but if you insist, I can do it.
Isn't that exactly what QEMUMachineArgs was meant to address? :)
Had a look at your don't-explode patches and they looked good.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early
2013-08-19 16:37 ` Andreas Färber
@ 2013-08-20 9:09 ` Markus Armbruster
0 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2013-08-20 9:09 UTC (permalink / raw)
To: Andreas Färber; +Cc: aliguori, lersek, qemu-devel, ehabkost
Andreas Färber <afaerber@suse.de> writes:
> Am 19.08.2013 11:35, schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>>
>>> Am 16.08.2013 15:18, schrieb armbru@redhat.com:
>>>> From: Markus Armbruster <armbru@redhat.com>
>>>>
>>>> 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.
>>>
>>> We had such a patch for CPU hot-add and decided against doing this.
>>> Currently current_machine != signals that it has been initialized. And
>>
>> Does any code actually depend on this undocumented condition? I found
>> none.
>
> I didn't audit. Currently the users are limited to vl.c itself,
> device-hotplug.c for block_default_type and qmp.c for hot_add_cpu. pc.c
> feels odd in that mix.
>
> [...]
>>> Can't you pass either QEMUMachine or the specific fields needed from PC
>>> code to those SMBIOS functions? You did add a bool argument.
>>
>> Can't see how to do that without passing the machine to QEMUMachine
>> method init(), which involves touching all boards. I doubt that's a
>> good idea, but if you insist, I can do it.
>
> Isn't that exactly what QEMUMachineArgs was meant to address? :)
> Had a look at your don't-explode patches and they looked good.
I could give it a try, but to be honest, I'm reluctant to base new work
on a series that has been ignored by all committers for more than two
months.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default armbru
@ 2013-08-27 17:04 ` Michael S. Tsirkin
2013-08-27 23:22 ` Eric Blake
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2013-08-27 17:04 UTC (permalink / raw)
To: armbru; +Cc: aliguori, lersek, qemu-devel, ehabkost
On Fri, Aug 16, 2013 at 03:18:34PM +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
We can do this of course, but why is this better?
It seems to expose new information to the guest
for no reason, we just might come to regret it
later if guests start implementing hacks keying
off the version number.
> ---
> 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 e8bc8ce..eb7ffc4 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -604,7 +604,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;
> @@ -635,7 +635,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);
> @@ -1155,7 +1155,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 6e1e654..2a621ef 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;
> 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->isapc_ram_fw = !pci_enabled;
> + guest_info->smbios_type1_defaults = smbios_type1_defaults;
>
> /* allocate ram and load rom/bios */
> if (!xen_enabled()) {
> @@ -258,6 +260,7 @@ static void pc_init_pci_1_6(QEMUMachineInitArgs *args)
> static void pc_init_pci_1_5(QEMUMachineInitArgs *args)
> {
> has_pvpanic = true;
> + smbios_type1_defaults = false;
> pc_init_pci_1_6(args);
> }
>
> @@ -298,6 +301,7 @@ static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args)
> const char *initrd_filename = args->initrd_filename;
> const char *boot_device = args->boot_device;
> has_pci_info = false;
> + smbios_type1_defaults = false;
> disable_kvm_pv_eoi();
> enable_compat_apic_id_mode();
> pc_init1(get_system_memory(),
> @@ -316,6 +320,7 @@ static void pc_init_isa(QEMUMachineInitArgs *args)
> const char *initrd_filename = args->initrd_filename;
> const char *boot_device = args->boot_device;
> 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 10e770e..05bce55 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -48,6 +48,7 @@
>
> static bool has_pvpanic;
> static bool has_pci_info = true;
> +static bool smbios_type1_defaults = true;
>
> /* PC hardware initialisation */
> static void pc_q35_init(QEMUMachineInitArgs *args)
> @@ -111,6 +112,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->isapc_ram_fw = false;
> + guest_info->smbios_type1_defaults = smbios_type1_defaults;
>
> /* allocate ram and load rom/bios */
> if (!xen_enabled()) {
> @@ -227,6 +229,7 @@ static void pc_q35_init_1_6(QEMUMachineInitArgs *args)
> static void pc_q35_init_1_5(QEMUMachineInitArgs *args)
> {
> has_pvpanic = true;
> + smbios_type1_defaults = false;
> pc_q35_init_1_6(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 f79d478..5797b97 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -20,6 +20,7 @@ typedef struct PcPciInfo {
> struct PcGuestInfo {
> bool has_pci_info;
> bool isapc_ram_fw;
> + 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.8.1.4
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default
2013-08-27 17:04 ` Michael S. Tsirkin
@ 2013-08-27 23:22 ` Eric Blake
2013-08-28 6:05 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Eric Blake @ 2013-08-27 23:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: aliguori, lersek, armbru, ehabkost, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]
On 08/27/2013 11:04 AM, Michael S. Tsirkin wrote:
> On Fri, Aug 16, 2013 at 03:18:34PM +0200, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> We can do this of course, but why is this better?
> It seems to expose new information to the guest
> for no reason, we just might come to regret it
> later if guests start implementing hacks keying
> off the version number.
Some guests (cough: some OEM builds of windows) already DO key off of
SMBIOS information to determine if they are running in a valid
environment. Furthermore, making this change to qemu makes it easier to
decouple the strings being presented to guests by default; it's always
better to have a situation where changing just qemu works, instead of
having to patch both qemu and BIOS in tandem.
The choice of whether to present the different information MUST be tied
to machine types (we cannot change SMBIOS data without an explicit
change to a newer machine type, precisely _because_ there are guests
that base their licensing decisions on constancy of BIOS information).
But for a new machine type, presenting qemu as the machine type rather
than being stuck to a particular SeaBIOS build seems nicer.
--
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default
2013-08-27 23:22 ` Eric Blake
@ 2013-08-28 6:05 ` Michael S. Tsirkin
0 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2013-08-28 6:05 UTC (permalink / raw)
To: Eric Blake; +Cc: aliguori, lersek, armbru, ehabkost, qemu-devel
On Tue, Aug 27, 2013 at 05:22:19PM -0600, Eric Blake wrote:
> On 08/27/2013 11:04 AM, Michael S. Tsirkin wrote:
> > On Fri, Aug 16, 2013 at 03:18:34PM +0200, armbru@redhat.com wrote:
> >> From: Markus Armbruster <armbru@redhat.com>
> >>
> >> 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>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> > We can do this of course, but why is this better?
> > It seems to expose new information to the guest
> > for no reason, we just might come to regret it
> > later if guests start implementing hacks keying
> > off the version number.
>
> Some guests (cough: some OEM builds of windows) already DO key off of
> SMBIOS information to determine if they are running in a valid
> environment.
Well they are unlikely to like either QEMU or Bochs.
> Furthermore, making this change to qemu makes it easier to
> decouple the strings being presented to guests by default; it's always
> better to have a situation where changing just qemu works, instead of
> having to patch both qemu and BIOS in tandem.
>
> The choice of whether to present the different information MUST be tied
> to machine types (we cannot change SMBIOS data without an explicit
> change to a newer machine type, precisely _because_ there are guests
> that base their licensing decisions on constancy of BIOS information).
> But for a new machine type, presenting qemu as the machine type rather
> than being stuck to a particular SeaBIOS build seems nicer.
Yes but why change anything at all?
We have command line flags to set these fields
for whatever you want to boot OEM windows -
which is highly unlikely to match any QEMU version.
Why isn't this enough?
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
` (7 preceding siblings ...)
2013-08-17 12:08 ` [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 Laszlo Ersek
@ 2013-09-17 15:27 ` Michael S. Tsirkin
2013-09-18 11:42 ` Markus Armbruster
2013-09-28 19:41 ` Michael S. Tsirkin
9 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2013-09-17 15:27 UTC (permalink / raw)
To: armbru; +Cc: aliguori, lersek, qemu-devel, ehabkost
On Fri, Aug 16, 2013 at 03:18:27PM +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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.
>
> v2: Rebase, only last patch had conflicts
OK my thinking at this point is:
patches 1-6 are ready
Any objections?
patch 7 - I would prefer some way to explicitly set
default smbios manufacturer/version in machine type
and set these from machine type, instead of
the smbios_type1_defaults boolean.
Would you like
- me to apply 1-6 and keep working on 7?
- wait for you to repost v3?
- look for another maintainer to take patchset as is (if someone
cares to, I won't object)?
> 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.8.1.4
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1
2013-09-17 15:27 ` Michael S. Tsirkin
@ 2013-09-18 11:42 ` Markus Armbruster
2013-09-24 6:07 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Markus Armbruster @ 2013-09-18 11:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aliguori, lersek, qemu-devel, Andreas Färber, ehabkost
[Note cc: Andreas]
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Aug 16, 2013 at 03:18:27PM +0200, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> 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.
>>
>> v2: Rebase, only last patch had conflicts
>
> OK my thinking at this point is:
> patches 1-6 are ready
> Any objections?
> patch 7 - I would prefer some way to explicitly set
> default smbios manufacturer/version in machine type
> and set these from machine type, instead of
> the smbios_type1_defaults boolean.
Are you asking for a new QEMUMachine member holding manufacturer (either
"Bochs", "QEMU" or null), and new members holding product and version
(either null or same value as existing members desc and version, at
least now)? Or just for moving smbios_type1_defaults from init function
into QEMUMachine?
> Would you like
> - me to apply 1-6 and keep working on 7?
> - wait for you to repost v3?
> - look for another maintainer to take patchset as is (if someone
> cares to, I won't object)?
Waiting for another maintainer after waiting >2 months for *any*
maintainer doesn't strike me as a good idea %-}
I'm totally fine with you taking just PATCH 1-5. PATCH 6, however,
should not be applied without PATCH 7. Andreas doesn't like PATCH 6,
and overruling his dislike without an actual use for it (which comes
only in PATCH 7) isn't nice.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1
2013-09-18 11:42 ` Markus Armbruster
@ 2013-09-24 6:07 ` Michael S. Tsirkin
0 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2013-09-24 6:07 UTC (permalink / raw)
To: Markus Armbruster
Cc: Andreas Färber, lersek, qemu-devel, Anthony Liguori, ehabkost
On Wed, Sep 18, 2013 at 01:42:28PM +0200, Markus Armbruster wrote:
> [Note cc: Andreas]
>
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > On Fri, Aug 16, 2013 at 03:18:27PM +0200, armbru@redhat.com wrote:
> >> From: Markus Armbruster <armbru@redhat.com>
> >>
> >> 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.
> >>
> >> v2: Rebase, only last patch had conflicts
> >
> > OK my thinking at this point is:
> > patches 1-6 are ready
> > Any objections?
> > patch 7 - I would prefer some way to explicitly set
> > default smbios manufacturer/version in machine type
> > and set these from machine type, instead of
> > the smbios_type1_defaults boolean.
>
> Are you asking for a new QEMUMachine member holding manufacturer (either
> "Bochs", "QEMU" or null), and new members holding product and version
> (either null or same value as existing members desc and version, at
> least now)?
Yes, I think we can live with this one.
Though I would really prefer a property of some device.
> Or just for moving smbios_type1_defaults from init function
> into QEMUMachine?
>
> > Would you like
> > - me to apply 1-6 and keep working on 7?
> > - wait for you to repost v3?
> > - look for another maintainer to take patchset as is (if someone
> > cares to, I won't object)?
>
> Waiting for another maintainer after waiting >2 months for *any*
> maintainer doesn't strike me as a good idea %-}
>
> I'm totally fine with you taking just PATCH 1-5. PATCH 6, however,
> should not be applied without PATCH 7. Andreas doesn't like PATCH 6,
> and overruling his dislike without an actual use for it (which comes
> only in PATCH 7) isn't nice.
I missed this fact, thanks for pointing it out.
So please arrive at consensus with Andreas re PATCH 6.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
` (8 preceding siblings ...)
2013-09-17 15:27 ` Michael S. Tsirkin
@ 2013-09-28 19:41 ` Michael S. Tsirkin
9 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2013-09-28 19:41 UTC (permalink / raw)
To: armbru; +Cc: aliguori, lersek, qemu-devel, ehabkost
On Fri, Aug 16, 2013 at 03:18:27PM +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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.
>
> v2: Rebase, only last patch had conflicts
I applied patch 1-5 on my tree, thanks everyone.
> 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.8.1.4
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts armbru
@ 2013-09-28 20:47 ` Michael S. Tsirkin
2013-09-30 8:48 ` Markus Armbruster
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2013-09-28 20:47 UTC (permalink / raw)
To: armbru; +Cc: lersek, qemu-devel, Anthony Liguori, ehabkost
On Fri, Aug 16, 2013 at 03:18:29PM +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> 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>
> Reviewed-by: Eric Blake <eblake@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 1ddead0..96477fd 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1133,10 +1133,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);
> }
This triggers a build failure:
/scm/qemu/hw/i386/smbios.c: In function ‘smbios_entry_add’:
/scm/qemu/hw/i386/smbios.c:382:26: error: format ‘%llu’ expects argument
of type ‘long long unsigned int’, but argument 2 has type ‘long unsigned
int’ [-Werror=format=]
type);
^
It's a long value, why are you printing it with PRIu64?
%ld seems right.
I reverted just this chunk.
> 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 f422a1c..f63ffd2 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.8.1.4
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts
2013-09-28 20:47 ` Michael S. Tsirkin
@ 2013-09-30 8:48 ` Markus Armbruster
0 siblings, 0 replies; 24+ messages in thread
From: Markus Armbruster @ 2013-09-30 8:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: lersek, qemu-devel, Anthony Liguori, ehabkost
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Aug 16, 2013 at 03:18:29PM +0200, armbru@redhat.com wrote:
>> From: Markus Armbruster <armbru@redhat.com>
>>
>> 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
[...]
>> 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
[...]
>> @@ -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);
>> }
>
> This triggers a build failure:
>
> /scm/qemu/hw/i386/smbios.c: In function ‘smbios_entry_add’:
> /scm/qemu/hw/i386/smbios.c:382:26: error: format ‘%llu’ expects argument
> of type ‘long long unsigned int’, but argument 2 has type ‘long unsigned
> int’ [-Werror=format=]
> type);
> ^
>
> It's a long value, why are you printing it with PRIu64?
> %ld seems right.
Yup.
> I reverted just this chunk.
Thanks for catching this. My compiler doesn't :(
[...]
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2013-09-30 8:48 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1) armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts armbru
2013-09-28 20:47 ` Michael S. Tsirkin
2013-09-30 8:48 ` Markus Armbruster
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 3/7] smbios: Improve diagnostics for conflicting entries armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 4/7] smbios: Make multiple -smbios type= accumulate sanely armbru
2013-08-17 12:48 ` Eric Blake
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 5/7] smbios: Factor out smbios_maybe_add_str() armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early armbru
2013-08-17 13:07 ` Andreas Färber
2013-08-19 9:35 ` Markus Armbruster
2013-08-19 16:37 ` Andreas Färber
2013-08-20 9:09 ` Markus Armbruster
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default armbru
2013-08-27 17:04 ` Michael S. Tsirkin
2013-08-27 23:22 ` Eric Blake
2013-08-28 6:05 ` Michael S. Tsirkin
2013-08-17 12:08 ` [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 Laszlo Ersek
2013-08-17 12:50 ` Eric Blake
2013-09-17 15:27 ` Michael S. Tsirkin
2013-09-18 11:42 ` Markus Armbruster
2013-09-24 6:07 ` Michael S. Tsirkin
2013-09-28 19:41 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.