Patchwork [4/7] Fix -machine options accel, kernel_irqchip, kvm_shadow_mem

login
register
mail settings
Submitter Markus Armbruster
Date July 4, 2013, 1:09 p.m.
Message ID <1372943363-24081-5-git-send-email-armbru@redhat.com>
Download mbox | patch
Permalink /patch/256897/
State New
Headers show

Comments

Markus Armbruster - July 4, 2013, 1:09 p.m.
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(-)
Peter Maydell - July 4, 2013, 2:42 p.m.
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
Markus Armbruster - July 4, 2013, 3:58 p.m.
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.
Peter Maydell - July 4, 2013, 4:03 p.m.
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
Markus Armbruster - July 4, 2013, 4:50 p.m.
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.

Patch

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";