[Qemu-devel] [PATCH 0/7] Fixes around -machine

[Qemu-devel] [PATCH 0/7] Fixes around -machine

[Markus Armbruster]

QemuOpts is fertile ground for odd bugs.

Note: PATCH 4/7 is stable branch material for downstreams that enable
KVM by default.

Markus Armbruster (7):
  qemu-option: Fix qemu_opts_find() for null id arguments
  qemu-option: Fix qemu_opts_set_defaults() for corner cases
  vl: New qemu_get_machine_opts()
  Fix -machine options accel, kernel_irqchip, kvm_shadow_mem
  microblaze: Fix latent bug with default DTB lookup
  Simplify -machine option queries with qemu_get_machine_opts()
  vl: Tighten parsing of -machine option phandle_start

 device_tree.c           | 27 ++++++++------------------
 exec.c                  | 12 ++++--------
 hw/arm/boot.c           |  8 +-------
 hw/microblaze/boot.c    | 27 +++++++++++++-------------
 hw/ppc/e500.c           | 25 +++++++-----------------
 hw/ppc/spapr.c          | 28 ++++++++++-----------------
 include/sysemu/sysemu.h |  2 ++
 kvm-all.c               |  5 +----
 target-i386/kvm.c       | 17 +++++++----------
 util/qemu-option.c      | 22 +++++----------------
 vl.c                    | 51 +++++++++++++++++++++++++++----------------------
 11 files changed, 86 insertions(+), 138 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/7] qemu-option: Fix qemu_opts_find() for null id arguments

[Markus Armbruster]

Crashes when the first list member has an ID.  Admittedly nonsensical
reproducer:

$ qemu-system-x86_64 -nodefaults -machine id=foo -machine ""

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 412c425..2715f27 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -706,16 +706,12 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
     QemuOpts *opts;
 
     QTAILQ_FOREACH(opts, &list->head, next) {
-        if (!opts->id) {
-            if (!id) {
-                return opts;
-            }
-            continue;
+        if (!opts->id && !id) {
+            return opts;
         }
-        if (strcmp(opts->id, id) != 0) {
-            continue;
+        if (opts->id && id && !strcmp(opts->id, id)) {
+            return opts;
         }
-        return opts;
     }
     return NULL;
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/7] qemu-option: Fix qemu_opts_set_defaults() for corner cases

[Markus Armbruster]

Commit 4f6dd9a changed the initialization of opts in opts_parse() to
this:

    if (defaults) {
        if (!id && !QTAILQ_EMPTY(&list->head)) {
            opts = qemu_opts_find(list, NULL);
        } else {
            opts = qemu_opts_create(list, id, 0);
        }
    } else {
        opts = qemu_opts_create(list, id, 1);
    }

Same as before for !defaults.

If defaults is true, and params has no ID, and options exist, we use
the first assignment.  It sets opts to null if all options have an ID.
opts_parse() then returns null.  qemu_opts_set_defaults() asserts the
value is non-null.  It's the only caller that passes true for
defaults.

To reproduce, try "-M xenpv -machine id=foo" (yes, "id=foo" is silly,
but it shouldn't crash).

I believe the function attempts to do the following:

    If options don't yet exist, create new options
    Else, if defaults, modify the existing options
    Else, if list->merge_lists, modify the existing options
    Else, fail

A straightforward call of qemu_opts_create() does exactly that.

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 2715f27..e0ef426 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -914,15 +914,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         get_opt_value(value, sizeof(value), p+4);
         id = value;
     }
-    if (defaults) {
-        if (!id && !QTAILQ_EMPTY(&list->head)) {
-            opts = qemu_opts_find(list, NULL);
-        } else {
-            opts = qemu_opts_create(list, id, 0, &local_err);
-        }
-    } else {
-        opts = qemu_opts_create(list, id, 1, &local_err);
-    }
+    opts = qemu_opts_create(list, id, !defaults, &local_err);
     if (opts == NULL) {
         if (error_is_set(&local_err)) {
             qerror_report_err(local_err);
-- 
1.7.11.7

[Qemu-devel] [PATCH 3/7] vl: New qemu_get_machine_opts()

[Markus Armbruster]

To be used in the next few commits to fix or clean up queries of
"machine" options (-machine and its sugared forms).

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/sysemu/sysemu.h |  2 ++
 vl.c                    | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 2fb71af..d85bdc0 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -185,6 +185,8 @@ char *get_boot_devices_list(size_t *size);
 
 DeviceState *get_boot_device(uint32_t position);
 
+QemuOpts *qemu_get_machine_opts(void);
+
 bool usb_enabled(bool default_usb);
 
 extern QemuOptsList qemu_drive_opts;
diff --git a/vl.c b/vl.c
index 6d9fd7d..e68d19c 100644
--- a/vl.c
+++ b/vl.c
@@ -516,6 +516,25 @@ static QemuOptsList qemu_realtime_opts = {
     },
 };
 
+/**
+ * Get machine options
+ *
+ * Returns: machine options (never null).
+ */
+QemuOpts *qemu_get_machine_opts(void)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts("machine");
+    assert(list);
+    opts = qemu_opts_find(list, NULL);
+    if (!opts) {
+        opts = qemu_opts_create_nofail(list);
+    }
+    return opts;
+}
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
-- 
1.7.11.7

[Qemu-devel] [PATCH 4/7] Fix -machine options accel, kernel_irqchip, kvm_shadow_mem

[Markus Armbruster]

Multiple -machine options with the same ID are merged.  All but the
one without an ID are to be silently ignored.

In most places, we query these options with a null ID.  This is
correct.

In some places, we instead query whatever options come first in the
list.  This is wrong.  When the -machine processed first happens to
have an ID, options are taken from that ID, and the ones specified
without ID are silently ignored.

Example:

    $ upstream-qemu -nodefaults -S -display none -monitor stdio -machine id=foo -machine accel=kvm,usb=on
    $ upstream-qemu -nodefaults -S -display none -monitor stdio -machine id=foo,accel=kvm,usb=on -machine accel=xen
    $ upstream-qemu -nodefaults -S -display none -monitor stdio -machine accel=xen -machine id=foo,accel=kvm,usb=on

    $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -machine accel=kvm,usb=on
    QEMU 1.5.50 monitor - type 'help' for more information
    (qemu) info kvm
    kvm support: enabled
    (qemu) info usb
    (qemu) q
    $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -machine id=foo -machine accel=kvm,usb=on
    QEMU 1.5.50 monitor - type 'help' for more information
    (qemu) info kvm
    kvm support: disabled
    (qemu) info usb
    (qemu) q
    $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -machine id=foo,accel=kvm,usb=on -machine accel=xen
    QEMU 1.5.50 monitor - type 'help' for more information
    (qemu) info kvm
    kvm support: enabled
    (qemu) info usb
    USB support not enabled
    (qemu) q
    $ qemu-system-x86_64 -nodefaults -S -display none -monitor stdio -machine accel=xen -machine id=foo,accel=kvm,usb=on
    xc: error: Could not obtain handle on privileged command interface (2 = No such file or directory): Internal error
    xen be core: can't open xen interface
    failed to initialize Xen: Operation not permitted

Option usb is queried correctly, and the one without an ID wins,
regardless of option order.

Option accel is queried incorrectly, and which one wins depends on
option order and ID.

Affected options are accel (and its sugared forms -enable-kvm and
-no-kvm), kernel_irqchip, kvm_shadow_mem.

Additionally, option kernel_irqchip is normally on by default, except
it's off when no -machine options are given.  Bug can't bite, because
kernel_irqchip is used only when KVM is enabled, KVM is off by
default, and enabling always creates -machine options.  Downstreams
that enable KVM by default do get bitten, though.

Use qemu_get_machine_opts() to fix these bugs.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/e500.c     | 13 ++++---------
 kvm-all.c         |  5 +----
 target-i386/kvm.c | 17 +++++++----------
 vl.c              |  8 ++------
 4 files changed, 14 insertions(+), 29 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 38f7990..5c02713 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -528,7 +528,6 @@ static DeviceState *ppce500_init_mpic_kvm(PPCE500Params *params,
 static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
                                    qemu_irq **irqs)
 {
-    QemuOptsList *list;
     qemu_irq *mpic;
     DeviceState *dev = NULL;
     SysBusDevice *s;
@@ -537,15 +536,11 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr,
     mpic = g_new(qemu_irq, 256);
 
     if (kvm_enabled()) {
-        bool irqchip_allowed = true, irqchip_required = false;
-
-        list = qemu_find_opts("machine");
-        if (!QTAILQ_EMPTY(&list->head)) {
-            irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+        QemuOpts *machine_opts = qemu_get_machine_opts();
+        bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
                                                 "kernel_irqchip", true);
-            irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
-                                                 "kernel_irqchip", false);
-        }
+        bool irqchip_required = qemu_opt_get_bool(machine_opts,
+                                                  "kernel_irqchip", false);
 
         if (irqchip_allowed) {
             dev = ppce500_init_mpic_kvm(params, irqs);
diff --git a/kvm-all.c b/kvm-all.c
index c757dd2..526b3c0 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1283,12 +1283,9 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, EventNotifier *n, int virq)
 
 static int kvm_irqchip_create(KVMState *s)
 {
-    QemuOptsList *list = qemu_find_opts("machine");
     int ret;
 
-    if (QTAILQ_EMPTY(&list->head) ||
-        !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
-                           "kernel_irqchip", true) ||
+    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip", true) ||
         !kvm_check_extension(s, KVM_CAP_IRQCHIP)) {
         return 0;
     }
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 39f4fbb..0a2310d 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -741,7 +741,6 @@ static int kvm_get_supported_msrs(KVMState *s)
 
 int kvm_arch_init(KVMState *s)
 {
-    QemuOptsList *list = qemu_find_opts("machine");
     uint64_t identity_base = 0xfffbc000;
     uint64_t shadow_mem;
     int ret;
@@ -790,15 +789,13 @@ int kvm_arch_init(KVMState *s)
     }
     qemu_register_reset(kvm_unpoison_all, NULL);
 
-    if (!QTAILQ_EMPTY(&list->head)) {
-        shadow_mem = qemu_opt_get_size(QTAILQ_FIRST(&list->head),
-                                       "kvm_shadow_mem", -1);
-        if (shadow_mem != -1) {
-            shadow_mem /= 4096;
-            ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
-            if (ret < 0) {
-                return ret;
-            }
+    shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
+                                   "kvm_shadow_mem", -1);
+    if (shadow_mem != -1) {
+        shadow_mem /= 4096;
+        ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
+        if (ret < 0) {
+            return ret;
         }
     }
     return 0;
diff --git a/vl.c b/vl.c
index e68d19c..6678765 100644
--- a/vl.c
+++ b/vl.c
@@ -2691,17 +2691,13 @@ static struct {
 
 static int configure_accelerator(void)
 {
-    const char *p = NULL;
+    const char *p;
     char buf[10];
     int i, ret;
     bool accel_initialised = false;
     bool init_failed = false;
 
-    QemuOptsList *list = qemu_find_opts("machine");
-    if (!QTAILQ_EMPTY(&list->head)) {
-        p = qemu_opt_get(QTAILQ_FIRST(&list->head), "accel");
-    }
-
+    p = qemu_opt_get(qemu_get_machine_opts(), "accel");
     if (p == NULL) {
         /* Use the default "accelerator", tcg */
         p = "tcg";
-- 
1.7.11.7

[Qemu-devel] [PATCH 5/7] microblaze: Fix latent bug with default DTB lookup

[Markus Armbruster]

microblaze_load_kernel() fails to call
qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename) when no -machine
options are given.  This can't normally happen, because -machine
option kernel is mandatory for this target.  Fix it anyway, by using
qemu_get_machine_opts().

Cc: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/microblaze/boot.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 3f1d70e..5b057f7 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -28,6 +28,7 @@
 #include "qemu/config-file.h"
 #include "qemu-common.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/sysemu.h"
 #include "hw/loader.h"
 #include "elf.h"
 
@@ -93,20 +94,18 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
                             void (*machine_cpu_reset)(MicroBlazeCPU *))
 {
     QemuOpts *machine_opts;
-    const char *kernel_filename = NULL;
-    const char *kernel_cmdline = NULL;
-
-    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (machine_opts) {
-        const char *dtb_arg;
-        kernel_filename = qemu_opt_get(machine_opts, "kernel");
-        kernel_cmdline = qemu_opt_get(machine_opts, "append");
-        dtb_arg = qemu_opt_get(machine_opts, "dtb");
-        if (dtb_arg) { /* Preference a -dtb argument */
-            dtb_filename = dtb_arg;
-        } else { /* default to pcbios dtb as passed by machine_init */
-            dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
-        }
+    const char *kernel_filename;
+    const char *kernel_cmdline;
+    const char *dtb_arg;
+
+    machine_opts = qemu_get_machine_opts();
+    kernel_filename = qemu_opt_get(machine_opts, "kernel");
+    kernel_cmdline = qemu_opt_get(machine_opts, "append");
+    dtb_arg = qemu_opt_get(machine_opts, "dtb");
+    if (dtb_arg) { /* Preference a -dtb argument */
+        dtb_filename = dtb_arg;
+    } else { /* default to pcbios dtb as passed by machine_init */
+        dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
     }
 
     boot_info.machine_cpu_reset = machine_cpu_reset;
-- 
1.7.11.7

[Qemu-devel] [PATCH 6/7] Simplify -machine option queries with qemu_get_machine_opts()

[Markus Armbruster]

The previous two commits fixed bugs in -machine option queries.  I
can't find fault with the remaining queries, but let's use
qemu_get_machine_opts() everywhere, for consistency, simplicity and
robustness.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 device_tree.c  | 28 ++++++++++------------------
 exec.c         | 12 ++++--------
 hw/arm/boot.c  |  8 +-------
 hw/ppc/e500.c  | 12 +++---------
 hw/ppc/spapr.c | 28 ++++++++++------------------
 vl.c           | 22 ++++++----------------
 6 files changed, 34 insertions(+), 76 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 69be9da..0e7fe2d 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -21,6 +21,7 @@
 #include "config.h"
 #include "qemu-common.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/sysemu.h"
 #include "hw/loader.h"
 #include "qemu/option.h"
 #include "qemu/config-file.h"
@@ -239,14 +240,10 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
      * which phandle id to start allocting phandles.
      */
     if (!phandle) {
-        QemuOpts *machine_opts;
-        machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-        if (machine_opts) {
-            const char *phandle_start;
-            phandle_start = qemu_opt_get(machine_opts, "phandle_start");
-            if (phandle_start) {
-                phandle = strtoul(phandle_start, NULL, 0);
-            }
+        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
+                                                 "phandle_start");
+        if (phandle_start) {
+            phandle = strtoul(phandle_start, NULL, 0);
         }
     }
 
@@ -307,15 +304,10 @@ int qemu_devtree_add_subnode(void *fdt, const char *name)
 
 void qemu_devtree_dumpdtb(void *fdt, int size)
 {
-    QemuOpts *machine_opts;
-
-    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (machine_opts) {
-        const char *dumpdtb = qemu_opt_get(machine_opts, "dumpdtb");
-        if (dumpdtb) {
-            /* Dump the dtb to a file and quit */
-            exit(g_file_set_contents(dumpdtb, fdt, size, NULL) ? 0 : 1);
-        }
-    }
+    const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
 
+    if (dumpdtb) {
+        /* Dump the dtb to a file and quit */
+        exit(g_file_set_contents(dumpdtb, fdt, size, NULL) ? 0 : 1);
+    }
 }
diff --git a/exec.c b/exec.c
index c49806c..48d2612 100644
--- a/exec.c
+++ b/exec.c
@@ -31,6 +31,7 @@
 #include "hw/qdev.h"
 #include "qemu/osdep.h"
 #include "sysemu/kvm.h"
+#include "sysemu/sysemu.h"
 #include "hw/xen/xen.h"
 #include "qemu/timer.h"
 #include "qemu/config-file.h"
@@ -1057,12 +1058,10 @@ ram_addr_t last_ram_offset(void)
 static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
 {
     int ret;
-    QemuOpts *machine_opts;
 
     /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
-    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (machine_opts &&
-        !qemu_opt_get_bool(machine_opts, "dump-guest-core", true)) {
+    if (!qemu_opt_get_bool(qemu_get_machine_opts(),
+                           "dump-guest-core", true)) {
         ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
         if (ret) {
             perror("qemu_madvise");
@@ -1109,10 +1108,7 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev)
 
 static int memory_try_enable_merging(void *addr, size_t len)
 {
-    QemuOpts *opts;
-
-    opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (opts && !qemu_opt_get_bool(opts, "mem-merge", true)) {
+    if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {
         /* disabled by the user */
         return 0;
     }
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 7c0090f..3e8741e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -359,7 +359,6 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     uint64_t elf_entry;
     hwaddr entry;
     int big_endian;
-    QemuOpts *machine_opts;
 
     /* Load the kernel.  */
     if (!info->kernel_filename) {
@@ -367,12 +366,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         exit(1);
     }
 
-    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (machine_opts) {
-        info->dtb_filename = qemu_opt_get(machine_opts, "dtb");
-    } else {
-        info->dtb_filename = NULL;
-    }
+    info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
 
     if (!info->secondary_cpu_reset_hook) {
         info->secondary_cpu_reset_hook = default_reset_secondary;
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 5c02713..e8c7139 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -137,7 +137,6 @@ static int ppce500_load_device_tree(CPUPPCState *env,
     uint32_t clock_freq = 400000000;
     uint32_t tb_freq = 400000000;
     int i;
-    const char *toplevel_compat = NULL; /* user override */
     char compatible_sb[] = "fsl,mpc8544-immr\0simple-bus";
     char soc[128];
     char mpic[128];
@@ -158,14 +157,9 @@ static int ppce500_load_device_tree(CPUPPCState *env,
             0x0, 0xe1000000,
             0x0, 0x10000,
         };
-    QemuOpts *machine_opts;
-    const char *dtb_file = NULL;
-
-    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (machine_opts) {
-        dtb_file = qemu_opt_get(machine_opts, "dtb");
-        toplevel_compat = qemu_opt_get(machine_opts, "dt_compatible");
-    }
+    QemuOpts *machine_opts = qemu_get_machine_opts();
+    const char *dtb_file = qemu_opt_get(machine_opts, "dtb");
+    const char *toplevel_compat = qemu_opt_get(machine_opts, "dt_compatible");
 
     if (dtb_file) {
         char *filename;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fe34291..642d438 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -676,27 +676,19 @@ static void spapr_cpu_reset(void *opaque)
 
 static void spapr_create_nvram(sPAPREnvironment *spapr)
 {
-    QemuOpts *machine_opts;
-    DeviceState *dev;
+    DeviceState *dev = qdev_create(&spapr->vio_bus->bus, "spapr-nvram");
+    const char *drivename = qemu_opt_get(qemu_get_machine_opts(), "nvram");
 
-    dev = qdev_create(&spapr->vio_bus->bus, "spapr-nvram");
+    if (drivename) {
+        BlockDriverState *bs;
 
-    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (machine_opts) {
-        const char *drivename;
-
-        drivename = qemu_opt_get(machine_opts, "nvram");
-        if (drivename) {
-            BlockDriverState *bs;
-
-            bs = bdrv_find(drivename);
-            if (!bs) {
-                fprintf(stderr, "No such block device \"%s\" for nvram\n",
-                        drivename);
-                exit(1);
-            }
-            qdev_prop_set_drive_nofail(dev, "drive", bs);
+        bs = bdrv_find(drivename);
+        if (!bs) {
+            fprintf(stderr, "No such block device \"%s\" for nvram\n",
+                    drivename);
+            exit(1);
         }
+        qdev_prop_set_drive_nofail(dev, "drive", bs);
     }
 
     qdev_init_nofail(dev);
diff --git a/vl.c b/vl.c
index 6678765..fb69f22 100644
--- a/vl.c
+++ b/vl.c
@@ -1036,15 +1036,9 @@ static int parse_sandbox(QemuOpts *opts, void *opaque)
     return 0;
 }
 
-/*********QEMU USB setting******/
 bool usb_enabled(bool default_usb)
 {
-    QemuOpts *mach_opts;
-    mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (mach_opts) {
-        return qemu_opt_get_bool(mach_opts, "usb", default_usb);
-    }
-    return default_usb;
+    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb", default_usb);
 }
 
 #ifndef _WIN32
@@ -4095,14 +4089,10 @@ int main(int argc, char **argv, char **envp)
         qtest_init();
     }
 
-    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
-    if (machine_opts) {
-        kernel_filename = qemu_opt_get(machine_opts, "kernel");
-        initrd_filename = qemu_opt_get(machine_opts, "initrd");
-        kernel_cmdline = qemu_opt_get(machine_opts, "append");
-    } else {
-        kernel_filename = initrd_filename = kernel_cmdline = NULL;
-    }
+    machine_opts = qemu_get_machine_opts();
+    kernel_filename = qemu_opt_get(machine_opts, "kernel");
+    initrd_filename = qemu_opt_get(machine_opts, "initrd");
+    kernel_cmdline = qemu_opt_get(machine_opts, "append");
 
     if (!boot_order) {
         boot_order = machine->boot_order;
@@ -4145,7 +4135,7 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    if (!linux_boot && machine_opts && qemu_opt_get(machine_opts, "dtb")) {
+    if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
         fprintf(stderr, "-dtb only allowed with -kernel option\n");
         exit(1);
     }
-- 
1.7.11.7

[Qemu-devel] [PATCH 7/7] vl: Tighten parsing of -machine option phandle_start

[Markus Armbruster]

Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
actually bothers to check for errors, rather than its user, which
doesn't.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 device_tree.c | 7 ++-----
 vl.c          | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index 0e7fe2d..10cf3d0 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -240,11 +240,8 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
      * which phandle id to start allocting phandles.
      */
     if (!phandle) {
-        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
-                                                 "phandle_start");
-        if (phandle_start) {
-            phandle = strtoul(phandle_start, NULL, 0);
-        }
+        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
+                                      "phandle_start", 0);
     }
 
     if (!phandle) {
diff --git a/vl.c b/vl.c
index fb69f22..bea1a10 100644
--- a/vl.c
+++ b/vl.c
@@ -409,7 +409,7 @@ static QemuOptsList qemu_machine_opts = {
             .help = "Dump current dtb to a file and quit",
         }, {
             .name = "phandle_start",
-            .type = QEMU_OPT_STRING,
+            .type = QEMU_OPT_NUMBER,
             .help = "The first phandle ID we may generate dynamically",
         }, {
             .name = "dt_compatible",
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 7/7] vl: Tighten parsing of -machine option phandle_start

[Alexander Graf]

On 04.07.2013, at 15:09, Markus Armbruster wrote:

> Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
> actually bothers to check for errors, rather than its user, which
> doesn't.
> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> device_tree.c | 7 ++-----
> vl.c          | 2 +-
> 2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 0e7fe2d..10cf3d0 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -240,11 +240,8 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
>      * which phandle id to start allocting phandles.
>      */
>     if (!phandle) {
> -        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
> -                                                 "phandle_start");
> -        if (phandle_start) {
> -            phandle = strtoul(phandle_start, NULL, 0);
> -        }
> +        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
> +                                      "phandle_start", 0);

Zero is a valid phandle to start from. It shouldn't mean "default".


Alex

>     }
> 
>     if (!phandle) {
> diff --git a/vl.c b/vl.c
> index fb69f22..bea1a10 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -409,7 +409,7 @@ static QemuOptsList qemu_machine_opts = {
>             .help = "Dump current dtb to a file and quit",
>         }, {
>             .name = "phandle_start",
> -            .type = QEMU_OPT_STRING,
> +            .type = QEMU_OPT_NUMBER,
>             .help = "The first phandle ID we may generate dynamically",
>         }, {
>             .name = "dt_compatible",
> -- 
> 1.7.11.7
> 

Re: [Qemu-devel] [PATCH 2/7] qemu-option: Fix qemu_opts_set_defaults() for corner cases

[Peter Maydell]

On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
> Commit 4f6dd9a changed the initialization of opts in opts_parse() to
> this:
>
>     if (defaults) {
>         if (!id && !QTAILQ_EMPTY(&list->head)) {
>             opts = qemu_opts_find(list, NULL);
>         } else {
>             opts = qemu_opts_create(list, id, 0);
>         }
>     } else {
>         opts = qemu_opts_create(list, id, 1);
>     }
>
> Same as before for !defaults.
>
> If defaults is true, and params has no ID, and options exist, we use
> the first assignment.  It sets opts to null if all options have an ID.
> opts_parse() then returns null.  qemu_opts_set_defaults() asserts the
> value is non-null.  It's the only caller that passes true for
> defaults.
>
> To reproduce, try "-M xenpv -machine id=foo" (yes, "id=foo" is silly,
> but it shouldn't crash).
>
> I believe the function attempts to do the following:
>
>     If options don't yet exist, create new options
>     Else, if defaults, modify the existing options
>     Else, if list->merge_lists, modify the existing options
>     Else, fail
>
> A straightforward call of qemu_opts_create() does exactly that.

I'm not sure this is right. In particular I don't think
that your change will do the right thing if list->merge_list
isn't true (it happens to be true for the only case we
have at the moment that uses qemu_opts_set_defaults()).
If merge_list is false then the old code would prepend
the options to the first entry in the list; with your
change we will instead insert the options as a completely
new entry in the list, which doesn't seem like a sensible
thing to do.

On the other hand I don't think the old code's behaviour
was really right either. I think part of the problem here
is that it really makes no sense to specify id= for a
QemuOptsList with merge_lists=true -- id= is for distinguishing
which of multiple "-whatever id=foo,a=b -whatever id=bar,a=c"
sets you want, whereas merge_lists=true is specifying that
there should only ever be one set, because they're merged.
So I think we should just catch this early and make it
an error. This then means the rest of the code can be
simpler (and prevents the user using id= as a backdoor
for weirdly splitting a single set of options into two).

Next up, does it make sense to use qemu_opts_set_defaults()
on a list without merge_lists set to true? I think the
most sensible semantics here would be that that should mean
"use these defaults for every '-whatever'. So if you set
the defaults for '-whatever' to be 'x=y', then
"-whatever id=foo,a=b -whatever id=bar,a=c" would work
like "-whatever x=y,id=foo,a=b -whatever x=y,id=bar,a=c".
This isn't what the code currently does (what it does do
is I think a historical artefact of the fact that
qemu_opts_set_defaults() predates merge_lists). To implement
it, instead of a single qemu_opts_find() you'd need to
iterate through the list applying the defaults to every
entry.

Or we could just assert() that merge_lists==true for the
moment, with a comment about what the right semantics
would be if anybody actually needed them.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/7] vl: New qemu_get_machine_opts()

[Peter Maydell]

On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
>
> +/**
> + * Get machine options
> + *
> + * Returns: machine options (never null).
> + */
> +QemuOpts *qemu_get_machine_opts(void)
> +{
> +    QemuOptsList *list;
> +    QemuOpts *opts;
> +
> +    list = qemu_find_opts("machine");
> +    assert(list);
> +    opts = qemu_opts_find(list, NULL);
> +    if (!opts) {
> +        opts = qemu_opts_create_nofail(list);
> +    }
> +    return opts;
> +}

This looks a bit odd -- why are we creating new
options in a function that claims to only be querying
them?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/7] qemu-option: Fix qemu_opts_find() for null id arguments

[Peter Maydell]

On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
> Crashes when the first list member has an ID.  Admittedly nonsensical
> reproducer:
>
> $ qemu-system-x86_64 -nodefaults -machine id=foo -machine ""
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH 4/7] Fix -machine options accel, kernel_irqchip, kvm_shadow_mem

[Peter Maydell]

On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
> Multiple -machine options with the same ID are merged.  All but the
> one without an ID are to be silently ignored.

I think it would make more sense just to say that specifying
id= for -machine (or any other merge_lists=true option type)
is not permitted. Or do you have a reason for wanting to
have more than one -machine?

-- PMM

Re: [Qemu-devel] [PATCH 7/7] vl: Tighten parsing of -machine option phandle_start

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 04.07.2013, at 15:09, Markus Armbruster wrote:
>
>> Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
>> actually bothers to check for errors, rather than its user, which
>> doesn't.
>> 
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> device_tree.c | 7 ++-----
>> vl.c          | 2 +-
>> 2 files changed, 3 insertions(+), 6 deletions(-)
>> 
>> diff --git a/device_tree.c b/device_tree.c
>> index 0e7fe2d..10cf3d0 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -240,11 +240,8 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
   uint32_t qemu_devtree_alloc_phandle(void *fdt)
   {
       static int phandle = 0x0;

       /*
>>      * which phandle id to start allocting phandles.
>>      */
>>     if (!phandle) {
>> -        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
>> -                                                 "phandle_start");
>> -        if (phandle_start) {
>> -            phandle = strtoul(phandle_start, NULL, 0);
>> -        }
>> +        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
>> +                                      "phandle_start", 0);
>
> Zero is a valid phandle to start from. It shouldn't mean "default".

We get here only when phandle is zero (which it initially is).

If opts don't contain a value for "phandle_start", we set phandle to
zero, i.e. do nothing.  Exactly the same as before.

If that's wrong, it should be fixed in a separate patch.

Re: [Qemu-devel] [PATCH 3/7] vl: New qemu_get_machine_opts()

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> +/**
>> + * Get machine options
>> + *
>> + * Returns: machine options (never null).
>> + */
>> +QemuOpts *qemu_get_machine_opts(void)
>> +{
>> +    QemuOptsList *list;
>> +    QemuOpts *opts;
>> +
>> +    list = qemu_find_opts("machine");
>> +    assert(list);
>> +    opts = qemu_opts_find(list, NULL);
>> +    if (!opts) {
>> +        opts = qemu_opts_create_nofail(list);
>> +    }
>> +    return opts;
>> +}
>
> This looks a bit odd -- why are we creating new
> options in a function that claims to only be querying
> them?

So we never return null.  If it bothers you, I can initialize the
options to empty somewhere else, and assert they exist here.

Re: [Qemu-devel] [PATCH 3/7] vl: New qemu_get_machine_opts()

[Peter Maydell]

On 4 July 2013 16:03, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> This looks a bit odd -- why are we creating new
>> options in a function that claims to only be querying
>> them?
>
> So we never return null.  If it bothers you, I can initialize the
> options to empty somewhere else, and assert they exist here.

The other option would be to modify qemu_opt_get and
friends to accept a NULL QemuOpts* as meaning "return
the default". That seems cleaner to me than having
"machine" opts be a special case.

-- PMM

Re: [Qemu-devel] [PATCH 3/7] vl: New qemu_get_machine_opts()

[Andreas Färber]

Am 04.07.2013 16:38, schrieb Peter Maydell:
> On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> +/**
>> + * Get machine options
>> + *
>> + * Returns: machine options (never null).
>> + */
>> +QemuOpts *qemu_get_machine_opts(void)
>> +{
>> +    QemuOptsList *list;
>> +    QemuOpts *opts;
>> +
>> +    list = qemu_find_opts("machine");
>> +    assert(list);
>> +    opts = qemu_opts_find(list, NULL);
>> +    if (!opts) {
>> +        opts = qemu_opts_create_nofail(list);
>> +    }
>> +    return opts;
>> +}
> 
> This looks a bit odd -- why are we creating new
> options in a function that claims to only be querying
> them?

It's called the Singleton pattern. :P
We use it for the /machine Object, for instance.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/7] qemu-option: Fix qemu_opts_set_defaults() for corner cases

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
>> Commit 4f6dd9a changed the initialization of opts in opts_parse() to
>> this:
>>
>>     if (defaults) {
>>         if (!id && !QTAILQ_EMPTY(&list->head)) {
>>             opts = qemu_opts_find(list, NULL);
>>         } else {
>>             opts = qemu_opts_create(list, id, 0);
>>         }
>>     } else {
>>         opts = qemu_opts_create(list, id, 1);
>>     }
>>
>> Same as before for !defaults.
>>
>> If defaults is true, and params has no ID, and options exist, we use
>> the first assignment.  It sets opts to null if all options have an ID.
>> opts_parse() then returns null.  qemu_opts_set_defaults() asserts the
>> value is non-null.  It's the only caller that passes true for
>> defaults.
>>
>> To reproduce, try "-M xenpv -machine id=foo" (yes, "id=foo" is silly,
>> but it shouldn't crash).
>>
>> I believe the function attempts to do the following:
>>
>>     If options don't yet exist, create new options
>>     Else, if defaults, modify the existing options
>>     Else, if list->merge_lists, modify the existing options
>>     Else, fail
>>
>> A straightforward call of qemu_opts_create() does exactly that.
>
> I'm not sure this is right. In particular I don't think
> that your change will do the right thing if list->merge_list
> isn't true (it happens to be true for the only case we
> have at the moment that uses qemu_opts_set_defaults()).
> If merge_list is false then the old code would prepend
> the options to the first entry in the list; with your
> change we will instead insert the options as a completely
> new entry in the list, which doesn't seem like a sensible
> thing to do.

So my code fails to adhere to my own spec.

> On the other hand I don't think the old code's behaviour
> was really right either. I think part of the problem here
> is that it really makes no sense to specify id= for a
> QemuOptsList with merge_lists=true -- id= is for distinguishing
> which of multiple "-whatever id=foo,a=b -whatever id=bar,a=c"
> sets you want, whereas merge_lists=true is specifying that
> there should only ever be one set, because they're merged.

Isn't interpreting merge_lists as "there can only be one" stretching it
a bit?  All it clearly says to me is "merge multiple options with the
same ID", and that's all the code does.

Merging is merely a syntactic convenience.  Why is that convenience only
justified for "there can only be one" options, such as -machine?

> So I think we should just catch this early and make it
> an error. This then means the rest of the code can be
> simpler (and prevents the user using id= as a backdoor
> for weirdly splitting a single set of options into two).
>
> Next up, does it make sense to use qemu_opts_set_defaults()
> on a list without merge_lists set to true? I think the
> most sensible semantics here would be that that should mean
> "use these defaults for every '-whatever'. So if you set
> the defaults for '-whatever' to be 'x=y', then
> "-whatever id=foo,a=b -whatever id=bar,a=c" would work
> like "-whatever x=y,id=foo,a=b -whatever x=y,id=bar,a=c".
> This isn't what the code currently does (what it does do
> is I think a historical artefact of the fact that
> qemu_opts_set_defaults() predates merge_lists). To implement
> it, instead of a single qemu_opts_find() you'd need to
> iterate through the list applying the defaults to every
> entry.

I think applying defaults to exactly the options with the same ID as
given in the defaults is defensible semantics.  But debating this would
be a waste of time as long as nobody wants to set defaults with
merge_lists false.

> Or we could just assert() that merge_lists==true for the
> moment, with a comment about what the right semantics
> would be if anybody actually needed them.

QemuOpts has become unmanagably baroque.

My first impulse was to rip out all this default business, and
reimplement its only user completely outside of QemuOpts: dumb down
QEMUMachine default_machine_opts to default_accel, pass it to
configure_accelerator(), and call it a day.  I resisted the temptation.

I doubt fixing qemu_opts_set_defaults() to cover all the bells, whistles
and warts of QemuOpts would be a productive use of our time.  Unless
somebody objects, I'll respin with assert(list->merge_lists).

Re: [Qemu-devel] [PATCH 2/7] qemu-option: Fix qemu_opts_set_defaults() for corner cases

[Peter Maydell]

On 4 July 2013 16:28, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On the other hand I don't think the old code's behaviour
>> was really right either. I think part of the problem here
>> is that it really makes no sense to specify id= for a
>> QemuOptsList with merge_lists=true -- id= is for distinguishing
>> which of multiple "-whatever id=foo,a=b -whatever id=bar,a=c"
>> sets you want, whereas merge_lists=true is specifying that
>> there should only ever be one set, because they're merged.
>
> Isn't interpreting merge_lists as "there can only be one" stretching it
> a bit?  All it clearly says to me is "merge multiple options with the
> same ID", and that's all the code does.
>
> Merging is merely a syntactic convenience.  Why is that convenience only
> justified for "there can only be one" options, such as -machine?

Well, I think if you have a "can be only one" option then
you might as well turn on merging (as you say it's a syntactic
convenience). If you have an option where id= is mandatory
then you could have merging enabled there; but we don't have
any of those. But for options where id= is allowed but not
mandatory then merging doesn't really work. You don't want
 -device e1000 -device megasas
to merge those two, for instance.

So it just seems like it cuts down the set of combinations
to divide it into:
 * can-be-only-one, merges
 * multiple-allowed, no merging

and I guess it's less confusing for users if there aren't
too many different combinations of behaviour.

> QemuOpts has become unmanagably baroque.

Agreed. This is partly why I'm suggesting cutting down
the possible random combinations (especially where we don't
actually use them).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/7] Fix -machine options accel, kernel_irqchip, kvm_shadow_mem

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
>> Multiple -machine options with the same ID are merged.  All but the
>> one without an ID are to be silently ignored.
>
> I think it would make more sense just to say that specifying
> id= for -machine (or any other merge_lists=true option type)
> is not permitted. Or do you have a reason for wanting to
> have more than one -machine?

Adding even more options to QemuOpts is what I'd rather avoid; I find it
ridiculous enough already.

But if y'all want one to outlaw -machine id=..., I can add it.

Re: [Qemu-devel] [PATCH 4/7] Fix -machine options accel, kernel_irqchip, kvm_shadow_mem

[Peter Maydell]

On 4 July 2013 16:58, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
>>> Multiple -machine options with the same ID are merged.  All but the
>>> one without an ID are to be silently ignored.
>>
>> I think it would make more sense just to say that specifying
>> id= for -machine (or any other merge_lists=true option type)
>> is not permitted. Or do you have a reason for wanting to
>> have more than one -machine?
>
> Adding even more options to QemuOpts is what I'd rather avoid; I find it
> ridiculous enough already.

That's why I suggested that we should use the existing
merge_lists=true rather than adding another option.

> But if y'all want one to outlaw -machine id=..., I can add it.

Given that the latter half of this patchset seems to be dealing
with the fallout of letting the user specify -machine id=
it seems simpler just to say "don't do that".

-- PMM

Re: [Qemu-devel] [PATCH 3/7] vl: New qemu_get_machine_opts()

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 4 July 2013 16:03, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> This looks a bit odd -- why are we creating new
>>> options in a function that claims to only be querying
>>> them?
>>
>> So we never return null.  If it bothers you, I can initialize the
>> options to empty somewhere else, and assert they exist here.
>
> The other option would be to modify qemu_opt_get and
> friends to accept a NULL QemuOpts* as meaning "return
> the default".

I considered it, but it's more involved, and it'll sweep accidental null
opts arguments under the carpet (not sure that's worth worrying about).

>               That seems cleaner to me than having
> "machine" opts be a special case.

"machine" opts are a special case, because unlike most options, they're
a singleton.

Anyway, what do you guys want me to do?

(1) Create empty machine options on the fly (this is what the current
patch does)

(2) Initialize machine options elsewhere

(3) Make QemuOpts consistently treat NULL like empty options (possibly
quite some work)

I don't mind (1) or (2), but (3) feels like a bit more than I bargained
for.  I just want to fix the bug that bit me, not rework QemuOpts.

Re: [Qemu-devel] [PATCH 3/7] vl: New qemu_get_machine_opts()

[Peter Maydell]

On 4 July 2013 17:11, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 4 July 2013 16:03, Markus Armbruster <armbru@redhat.com> wrote:
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>> This looks a bit odd -- why are we creating new
>>>> options in a function that claims to only be querying
>>>> them?
>>>
>>> So we never return null.  If it bothers you, I can initialize the
>>> options to empty somewhere else, and assert they exist here.
>>
>> The other option would be to modify qemu_opt_get and
>> friends to accept a NULL QemuOpts* as meaning "return
>> the default".
>
> I considered it, but it's more involved, and it'll sweep accidental null
> opts arguments under the carpet (not sure that's worth worrying about).
>
>>               That seems cleaner to me than having
>> "machine" opts be a special case.
>
> "machine" opts are a special case, because unlike most options, they're
> a singleton.

So are boot-opts and smp-opts, right? If you deal with "machine"
opts as a special case you'll miss those (and any other singleton
options we add later).

> (3) Make QemuOpts consistently treat NULL like empty options (possibly
> quite some work)

I think we could reasonably change just qemu_opt_find(),
qemu_opt_has_help_opt() and qemu_opt_foreach() to allow NULL;
the rationale being that these are the "query" end of the
options API rather than the "set" end. The 'set' end needs
to provide a non-NULL opts because we can't just create one
on-demand.

If you don't want to do that then we could have a point in
vl.c where we force all our singleton opts to exist.

-- PMM

Re: [Qemu-devel] [PATCH 4/7] Fix -machine options accel, kernel_irqchip, kvm_shadow_mem

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 4 July 2013 16:58, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On 4 July 2013 14:09, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Multiple -machine options with the same ID are merged.  All but the
>>>> one without an ID are to be silently ignored.
>>>
>>> I think it would make more sense just to say that specifying
>>> id= for -machine (or any other merge_lists=true option type)
>>> is not permitted. Or do you have a reason for wanting to
>>> have more than one -machine?
>>
>> Adding even more options to QemuOpts is what I'd rather avoid; I find it
>> ridiculous enough already.
>
> That's why I suggested that we should use the existing
> merge_lists=true rather than adding another option.
>
>> But if y'all want one to outlaw -machine id=..., I can add it.
>
> Given that the latter half of this patchset seems to be dealing
> with the fallout of letting the user specify -machine id=
> it seems simpler just to say "don't do that".

Outlawing -machine id=... turns those patches from fixes of exotic bugs
into cleanup, of pretty much unchanged value.

Re: [Qemu-devel] [PATCH 7/7] vl: Tighten parsing of -machine option phandle_start

[Alexander Graf]

On 04.07.2013, at 17:01, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 04.07.2013, at 15:09, Markus Armbruster wrote:
>> 
>>> Make it QEMU_OPT_NUMBER, so it gets parsed by generic code, which
>>> actually bothers to check for errors, rather than its user, which
>>> doesn't.
>>> 
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> device_tree.c | 7 ++-----
>>> vl.c          | 2 +-
>>> 2 files changed, 3 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/device_tree.c b/device_tree.c
>>> index 0e7fe2d..10cf3d0 100644
>>> --- a/device_tree.c
>>> +++ b/device_tree.c
>>> @@ -240,11 +240,8 @@ uint32_t qemu_devtree_alloc_phandle(void *fdt)
>   uint32_t qemu_devtree_alloc_phandle(void *fdt)
>   {
>       static int phandle = 0x0;
> 
>       /*
>>>     * which phandle id to start allocting phandles.
>>>     */
>>>    if (!phandle) {
>>> -        const char *phandle_start = qemu_opt_get(qemu_get_machine_opts(),
>>> -                                                 "phandle_start");
>>> -        if (phandle_start) {
>>> -            phandle = strtoul(phandle_start, NULL, 0);
>>> -        }
>>> +        phandle = qemu_opt_get_number(qemu_get_machine_opts(),
>>> +                                      "phandle_start", 0);
>> 
>> Zero is a valid phandle to start from. It shouldn't mean "default".
> 
> We get here only when phandle is zero (which it initially is).
> 
> If opts don't contain a value for "phandle_start", we set phandle to
> zero, i.e. do nothing.  Exactly the same as before.

True. Sorry for the fuss.

Acked-by: Alexander Graf <agraf@suse.de>


Alex

> 
> If that's wrong, it should be fixed in a separate patch.

Re: [Qemu-devel] [PATCH 5/7] microblaze: Fix latent bug with default DTB lookup

[Peter Crosthwaite]

On Thu, Jul 4, 2013 at 11:09 PM, Markus Armbruster <armbru@redhat.com> wrote:
> microblaze_load_kernel() fails to call
> qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename) when no -machine
> options are given.  This can't normally happen, because -machine
> option kernel is mandatory for this target.  Fix it anyway, by using
> qemu_get_machine_opts().
>
> Cc: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Acked-by: Peter Croshwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/microblaze/boot.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index 3f1d70e..5b057f7 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -28,6 +28,7 @@
>  #include "qemu/config-file.h"
>  #include "qemu-common.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/sysemu.h"
>  #include "hw/loader.h"
>  #include "elf.h"
>
> @@ -93,20 +94,18 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>                              void (*machine_cpu_reset)(MicroBlazeCPU *))
>  {
>      QemuOpts *machine_opts;
> -    const char *kernel_filename = NULL;
> -    const char *kernel_cmdline = NULL;
> -
> -    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> -    if (machine_opts) {
> -        const char *dtb_arg;
> -        kernel_filename = qemu_opt_get(machine_opts, "kernel");
> -        kernel_cmdline = qemu_opt_get(machine_opts, "append");
> -        dtb_arg = qemu_opt_get(machine_opts, "dtb");
> -        if (dtb_arg) { /* Preference a -dtb argument */
> -            dtb_filename = dtb_arg;
> -        } else { /* default to pcbios dtb as passed by machine_init */
> -            dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
> -        }
> +    const char *kernel_filename;
> +    const char *kernel_cmdline;
> +    const char *dtb_arg;
> +
> +    machine_opts = qemu_get_machine_opts();
> +    kernel_filename = qemu_opt_get(machine_opts, "kernel");
> +    kernel_cmdline = qemu_opt_get(machine_opts, "append");
> +    dtb_arg = qemu_opt_get(machine_opts, "dtb");
> +    if (dtb_arg) { /* Preference a -dtb argument */
> +        dtb_filename = dtb_arg;
> +    } else { /* default to pcbios dtb as passed by machine_init */
> +        dtb_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, dtb_filename);
>      }
>
>      boot_info.machine_cpu_reset = machine_cpu_reset;
> --
> 1.7.11.7
>

Re: [Qemu-devel] [PATCH 0/7] Fixes around -machine

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/7] Fixes around -machine

[Markus Armbruster]

Anthony Liguori <aliguori@us.ibm.com> writes:

> Applied.  Thanks.

Peter pointed out that my code doesn't work as advertized when
!list->merge_list (never true in current usage).  I'll try to address
that on top when I get a breather I can spend on non-critical upstream
work.  Peter, do feel free to go ahead and change it the way you like
it.  If you do that, I'll spend the breather on reviewing your change :)