[Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Marcel Apfelbaum]

Patch e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.

This resulted in a Qemu crash:
$ qemu-system-x86_64 -usb
qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper:
Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
Aborted (core dumped)

Fixed it by simplifying usb_enabled usage:
 - removed the usb_enabled argument
 - call directly to machine's QOM property.
 - expose defaults_enabled to calling sites

As part of this series the semantics for some ppc machines to
create the default usb controller were changed, but I still think
is correct, please let me know if you see a problem there.

Based on the comments I will receive, I will continue to fix the options
properties in the same way.

Marcel Apfelbaum (6):
  hw/ppc: modified the condition for usb controllers to be created for
    some ppc machines
  hw/machine: added machine_usb wrapper
  hw/usb: simplified usb_enabled
  hw/ppc/mac_newworld: QOMified mac99 machines
  hw/ppc/spapr: simplify usb controller creation logic
  hw/ppc/mac_newworld: simplify usb controller creation logic

 hw/arm/nseries.c        |  2 +-
 hw/arm/pxa2xx.c         |  4 ++--
 hw/arm/realview.c       |  2 +-
 hw/arm/versatilepb.c    |  2 +-
 hw/core/machine.c       |  5 +++++
 hw/i386/pc_piix.c       |  2 +-
 hw/i386/pc_q35.c        |  2 +-
 hw/ppc/mac_newworld.c   | 32 +++++++++++++++++++++-----------
 hw/ppc/mac_oldworld.c   |  2 +-
 hw/ppc/prep.c           |  2 +-
 hw/ppc/spapr.c          |  3 ++-
 include/hw/boards.h     |  2 ++
 include/sysemu/sysemu.h |  3 ++-
 vl.c                    | 16 ++++++++++------
 14 files changed, 51 insertions(+), 28 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/6] hw/ppc: modified the condition for usb controllers to be created for some ppc machines

[Marcel Apfelbaum]

Some ppc machines create a default usb controller based on a 'machine condition'.
Until now the logic was: create the usb controller if:
 -  the usb option was supplied in cli and value is true or
 -  the usb option was absent and both set_defaults and the machine
    condition were true.

Modified the logic to:
Create the usb controller if:
 - the machine condition is true and defaults are enabled or
 - the usb option is supplied and true.

The main for this is to simplify the usb_enabled method.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/ppc/mac_newworld.c   | 3 ++-
 hw/ppc/spapr.c          | 2 +-
 include/sysemu/sysemu.h | 1 +
 vl.c                    | 7 ++++++-
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index b60a832..8ba9499 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -417,7 +417,8 @@ static void ppc_core99_init(MachineState *machine)
     dev = qdev_create(adb_bus, TYPE_ADB_MOUSE);
     qdev_init_nofail(dev);
 
-    if (usb_enabled(machine_arch == ARCH_MAC99_U3)) {
+    if ((machine_arch == ARCH_MAC99_U3 && defaults_enabled()) ||
+        usb_enabled(false)) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
         /* U3 needs to use USB for input because Linux doesn't support via-cuda
         on PPC64 */
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 08401e0..d5de301 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1486,7 +1486,7 @@ static void ppc_spapr_init(MachineState *machine)
         spapr->has_graphics = true;
     }
 
-    if (usb_enabled(spapr->has_graphics)) {
+    if ((spapr->has_graphics && defaults_enabled()) || usb_enabled(false)) {
         pci_create_simple(phb->bus, -1, "pci-ohci");
         if (spapr->has_graphics) {
             usbdevice_create("keyboard");
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 503e5a4..a31044c 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -227,6 +227,7 @@ void qemu_boot_set(const char *boot_order, Error **errp);
 
 QemuOpts *qemu_get_machine_opts(void);
 
+bool defaults_enabled(void);
 bool usb_enabled(bool default_usb);
 
 extern QemuOptsList qemu_legacy_drive_opts;
diff --git a/vl.c b/vl.c
index bea9656..415535f 100644
--- a/vl.c
+++ b/vl.c
@@ -997,10 +997,15 @@ static int parse_name(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+bool defaults_enabled(void)
+{
+    return has_defaults;
+}
+
 bool usb_enabled(bool default_usb)
 {
     return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
-                             has_defaults && default_usb);
+                             default_usb);
 }
 
 #ifndef _WIN32
-- 
2.1.0

[Qemu-devel] [PATCH 2/6] hw/machine: added machine_usb wrapper

[Marcel Apfelbaum]

Following QOM convention, object properties should
not be accessed directly.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/core/machine.c   | 5 +++++
 include/hw/boards.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index a0ae5f9..fbd91be 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -398,6 +398,11 @@ static void machine_finalize(Object *obj)
     g_free(ms->firmware);
 }
 
+bool machine_usb(MachineState *machine)
+{
+    return machine->usb;
+}
+
 static const TypeInfo machine_info = {
     .name = TYPE_MACHINE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/boards.h b/include/hw/boards.h
index e0a6790..3ddc449 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -65,6 +65,8 @@ int qemu_register_machine(QEMUMachine *m);
 MachineClass *find_default_machine(void);
 extern MachineState *current_machine;
 
+bool machine_usb(MachineState *machine);
+
 /**
  * MachineClass:
  * @qemu_machine: #QEMUMachine
-- 
2.1.0

[Qemu-devel] [PATCH 3/6] hw/usb: simplified usb_enabled

[Marcel Apfelbaum]

The argument is not longer used and the implementation
uses now QOM instead of QemuOpts.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/arm/nseries.c        |  2 +-
 hw/arm/pxa2xx.c         |  4 ++--
 hw/arm/realview.c       |  2 +-
 hw/arm/versatilepb.c    |  2 +-
 hw/i386/pc_piix.c       |  2 +-
 hw/i386/pc_q35.c        |  2 +-
 hw/ppc/mac_newworld.c   |  2 +-
 hw/ppc/mac_oldworld.c   |  2 +-
 hw/ppc/prep.c           |  2 +-
 hw/ppc/spapr.c          |  2 +-
 include/sysemu/sysemu.h |  2 +-
 vl.c                    | 11 +++++------
 12 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index c7ebaa6..4d7be5e 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -1344,7 +1344,7 @@ static void n8x0_init(MachineState *machine,
     n8x0_dss_setup(s);
     n8x0_cbus_setup(s);
     n8x0_uart_setup(s);
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         n8x0_usb_setup(s);
     }
 
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index 8967cc4..165ba2a 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -2143,7 +2143,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
     }
 
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         sysbus_create_simple("sysbus-ohci", 0x4c000000,
                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
     }
@@ -2276,7 +2276,7 @@ PXA2xxState *pxa255_init(MemoryRegion *address_space, unsigned int sdram_size)
         s->ssp[i] = (SSIBus *)qdev_get_child_bus(dev, "ssi");
     }
 
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         sysbus_create_simple("sysbus-ohci", 0x4c000000,
                         qdev_get_gpio_in(s->pic, PXA2XX_PIC_USBH1));
     }
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 66e51ef..50cb93d 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -261,7 +261,7 @@ static void realview_init(MachineState *machine,
         sysbus_connect_irq(busdev, 2, pic[50]);
         sysbus_connect_irq(busdev, 3, pic[51]);
         pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
-        if (usb_enabled(false)) {
+        if (usb_enabled()) {
             pci_create_simple(pci_bus, -1, "pci-ohci");
         }
         n = drive_get_max_bus(IF_SCSI);
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 6c4c2e7..b1dae77 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -281,7 +281,7 @@ static void versatile_init(MachineState *machine, int board_id)
             pci_nic_init_nofail(nd, pci_bus, "rtl8139", NULL);
         }
     }
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
     }
     n = drive_get_max_bus(IF_SCSI);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 220f741..f0a3201 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -274,7 +274,7 @@ static void pc_init1(MachineState *machine,
     pc_cmos_init(below_4g_mem_size, above_4g_mem_size, machine->boot_order,
                  machine, floppy, idebus[0], idebus[1], rtc_state);
 
-    if (pci_enabled && usb_enabled(false)) {
+    if (pci_enabled && usb_enabled()) {
         pci_create_simple(pci_bus, piix3_devfn + 2, "piix3-usb-uhci");
     }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 7ba0535..a432944 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -265,7 +265,7 @@ static void pc_q35_init(MachineState *machine)
     ide_drive_get(hd, ICH_AHCI(ahci)->ahci.ports);
     ahci_ide_create_devs(ahci, hd);
 
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         /* Should we create 6 UHCI according to ich9 spec? */
         ehci_create_ich9_with_companions(host_bus, 0x1d);
     }
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 8ba9499..ed37d6b 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -418,7 +418,7 @@ static void ppc_core99_init(MachineState *machine)
     qdev_init_nofail(dev);
 
     if ((machine_arch == ARCH_MAC99_U3 && defaults_enabled()) ||
-        usb_enabled(false)) {
+        usb_enabled()) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
         /* U3 needs to use USB for input because Linux doesn't support via-cuda
         on PPC64 */
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index c7224d7..3079510 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -304,7 +304,7 @@ static void ppc_heathrow_init(MachineState *machine)
     dev = qdev_create(adb_bus, TYPE_ADB_MOUSE);
     qdev_init_nofail(dev);
 
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
     }
 
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index dd8433d..15df7f3 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -539,7 +539,7 @@ static void ppc_prep_init(MachineState *machine)
     memory_region_add_subregion(sysmem, 0xFEFF0000, xcsr);
 #endif
 
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d5de301..72c3102 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1486,7 +1486,7 @@ static void ppc_spapr_init(MachineState *machine)
         spapr->has_graphics = true;
     }
 
-    if ((spapr->has_graphics && defaults_enabled()) || usb_enabled(false)) {
+    if ((spapr->has_graphics && defaults_enabled()) || usb_enabled()) {
         pci_create_simple(phb->bus, -1, "pci-ohci");
         if (spapr->has_graphics) {
             usbdevice_create("keyboard");
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index a31044c..748d059 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -228,7 +228,7 @@ void qemu_boot_set(const char *boot_order, Error **errp);
 QemuOpts *qemu_get_machine_opts(void);
 
 bool defaults_enabled(void);
-bool usb_enabled(bool default_usb);
+bool usb_enabled(void);
 
 extern QemuOptsList qemu_legacy_drive_opts;
 extern QemuOptsList qemu_common_drive_opts;
diff --git a/vl.c b/vl.c
index 415535f..7786b2f 100644
--- a/vl.c
+++ b/vl.c
@@ -1002,10 +1002,9 @@ bool defaults_enabled(void)
     return has_defaults;
 }
 
-bool usb_enabled(bool default_usb)
+bool usb_enabled(void)
 {
-    return qemu_opt_get_bool(qemu_get_machine_opts(), "usb",
-                             default_usb);
+    return machine_usb(current_machine);
 }
 
 #ifndef _WIN32
@@ -1229,7 +1228,7 @@ static int usb_device_add(const char *devname)
     const char *p;
 #endif
 
-    if (!usb_enabled(false)) {
+    if (!usb_enabled()) {
         return -1;
     }
 
@@ -1261,7 +1260,7 @@ static int usb_device_del(const char *devname)
         return -1;
     }
 
-    if (!usb_enabled(false)) {
+    if (!usb_enabled()) {
         return -1;
     }
 
@@ -4230,7 +4229,7 @@ int main(int argc, char **argv, char **envp)
     set_numa_modes();
 
     /* init USB devices */
-    if (usb_enabled(false)) {
+    if (usb_enabled()) {
         if (foreach_device_config(DEV_USB, usb_parse) < 0)
             exit(1);
     }
-- 
2.1.0

[Qemu-devel] [PATCH 4/6] hw/ppc/mac_newworld: QOMified mac99 machines

[Marcel Apfelbaum]

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/ppc/mac_newworld.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index ed37d6b..b54f94a 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -502,18 +502,27 @@ static int core99_kvm_type(const char *arg)
     return 2;
 }
 
-static QEMUMachine core99_machine = {
-    .name = "mac99",
-    .desc = "Mac99 based PowerMAC",
-    .init = ppc_core99_init,
-    .max_cpus = MAX_CPUS,
-    .default_boot_order = "cd",
-    .kvm_type = core99_kvm_type,
+static void core99_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->name = "mac99";
+    mc->desc = "Mac99 based PowerMAC";
+    mc->init = ppc_core99_init;
+    mc->max_cpus = MAX_CPUS;
+    mc->default_boot_order = "cd";
+    mc->kvm_type = core99_kvm_type;
+}
+
+static const TypeInfo core99_machine_info = {
+    .name          = "mac99-machine",
+    .parent        = TYPE_MACHINE,
+    .class_init    = core99_machine_class_init,
 };
 
-static void core99_machine_init(void)
+static void mac_machine_register_types(void)
 {
-    qemu_register_machine(&core99_machine);
+    type_register_static(&core99_machine_info);
 }
 
-machine_init(core99_machine_init);
+type_init(mac_machine_register_types)
-- 
2.1.0

[Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Marcel Apfelbaum]

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/ppc/spapr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 72c3102..53c4116 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1484,9 +1484,10 @@ static void ppc_spapr_init(MachineState *machine)
     /* Graphics */
     if (spapr_vga_init(phb->bus)) {
         spapr->has_graphics = true;
+        machine->usb |= defaults_enabled();
     }
 
-    if ((spapr->has_graphics && defaults_enabled()) || usb_enabled()) {
+    if (machine->usb) {
         pci_create_simple(phb->bus, -1, "pci-ohci");
         if (spapr->has_graphics) {
             usbdevice_create("keyboard");
-- 
2.1.0

[Qemu-devel] [PATCH 6/6] hw/ppc/mac_newworld: simplify usb controller creation logic

[Marcel Apfelbaum]

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/ppc/mac_newworld.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index b54f94a..c377012 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -371,6 +371,7 @@ static void ppc_core99_init(MachineState *machine)
         /* 970 gets a U3 bus */
         pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io());
         machine_arch = ARCH_MAC99_U3;
+        machine->usb |= defaults_enabled();
     } else {
         pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io());
         machine_arch = ARCH_MAC99;
@@ -417,8 +418,7 @@ static void ppc_core99_init(MachineState *machine)
     dev = qdev_create(adb_bus, TYPE_ADB_MOUSE);
     qdev_init_nofail(dev);
 
-    if ((machine_arch == ARCH_MAC99_U3 && defaults_enabled()) ||
-        usb_enabled()) {
+    if (machine->usb) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
         /* U3 needs to use USB for input because Linux doesn't support via-cuda
         on PPC64 */
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Paolo Bonzini]

On 06/01/2015 14:29, Marcel Apfelbaum wrote:
> Patch e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
> 
> This resulted in a Qemu crash:
> $ qemu-system-x86_64 -usb
> qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper:
> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
> Aborted (core dumped)
> 
> Fixed it by simplifying usb_enabled usage:
>  - removed the usb_enabled argument
>  - call directly to machine's QOM property.
>  - expose defaults_enabled to calling sites
> 
> As part of this series the semantics for some ppc machines to
> create the default usb controller were changed, but I still think
> is correct, please let me know if you see a problem there.
> 
> Based on the comments I will receive, I will continue to fix the options
> properties in the same way.
> 
> Marcel Apfelbaum (6):
>   hw/ppc: modified the condition for usb controllers to be created for
>     some ppc machines
>   hw/machine: added machine_usb wrapper
>   hw/usb: simplified usb_enabled
>   hw/ppc/mac_newworld: QOMified mac99 machines
>   hw/ppc/spapr: simplify usb controller creation logic
>   hw/ppc/mac_newworld: simplify usb controller creation logic
> 
>  hw/arm/nseries.c        |  2 +-
>  hw/arm/pxa2xx.c         |  4 ++--
>  hw/arm/realview.c       |  2 +-
>  hw/arm/versatilepb.c    |  2 +-
>  hw/core/machine.c       |  5 +++++
>  hw/i386/pc_piix.c       |  2 +-
>  hw/i386/pc_q35.c        |  2 +-
>  hw/ppc/mac_newworld.c   | 32 +++++++++++++++++++++-----------
>  hw/ppc/mac_oldworld.c   |  2 +-
>  hw/ppc/prep.c           |  2 +-
>  hw/ppc/spapr.c          |  3 ++-
>  include/hw/boards.h     |  2 ++
>  include/sysemu/sysemu.h |  3 ++-
>  vl.c                    | 16 ++++++++++------
>  14 files changed, 51 insertions(+), 28 deletions(-)
> 

I like the way you structured the series!

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Paolo Bonzini]

On 06/01/2015 20:48, Paolo Bonzini wrote:
> I like the way you structured the series!
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Hmm, actually doesn't this break -machine usb=no?

Paolo

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Paolo Bonzini]

On 06/01/2015 14:29, Marcel Apfelbaum wrote:
> @@ -1484,9 +1484,10 @@ static void ppc_spapr_init(MachineState *machine)
>      /* Graphics */
>      if (spapr_vga_init(phb->bus)) {
>          spapr->has_graphics = true;
> +        machine->usb |= defaults_enabled();
>      }

Could the solution be to do this in instance_init?  Then you would have
patches 2, 4, 5, 6, 3 (patch 1 would not be needed anymore).

Paolo

Re: [Qemu-devel] [PATCH 1/6] hw/ppc: modified the condition for usb controllers to be created for some ppc machines

[Alexander Graf]

On 06.01.15 14:29, Marcel Apfelbaum wrote:
> Some ppc machines create a default usb controller based on a 'machine condition'.
> Until now the logic was: create the usb controller if:
>  -  the usb option was supplied in cli and value is true or
>  -  the usb option was absent and both set_defaults and the machine
>     condition were true.
> 
> Modified the logic to:
> Create the usb controller if:
>  - the machine condition is true and defaults are enabled or
>  - the usb option is supplied and true.
> 
> The main for this is to simplify the usb_enabled method.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

I think this preserves the logic from a user's perspective quite well.

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


Alex

Re: [Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Alexander Graf]

On 06.01.15 21:41, Paolo Bonzini wrote:
> 
> 
> On 06/01/2015 20:48, Paolo Bonzini wrote:
>> I like the way you structured the series!
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Hmm, actually doesn't this break -machine usb=no?

I think it does, but I don't think we really need to care. We can just
assume that the 64bit mac99 always has USB and it's impossible to turn off.

For sPAPR, we only ever enable USB as convenience option when VGA is
enabled. I think it's a pretty fair thing to do there and simply ignore
usb=no. If you really need it, just don't use -vga.

At the end of the day, I don't think anyone will notice. I'd definitely
prefer to keep the code simple.

However, I think looking forward we'll want to spawn an XHCI adapter
rather than OHCI. It's just a lot cheaper to emulate. But that's out of
scope of this patch set.


Alex

Re: [Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Paolo Bonzini]

On 06/01/2015 22:54, Alexander Graf wrote:
> 
> 
> On 06.01.15 21:41, Paolo Bonzini wrote:
>>
>>
>> On 06/01/2015 20:48, Paolo Bonzini wrote:
>>> I like the way you structured the series!
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Hmm, actually doesn't this break -machine usb=no?
> 
> I think it does, but I don't think we really need to care. We can just
> assume that the 64bit mac99 always has USB and it's impossible to turn off.
> 
> For sPAPR, we only ever enable USB as convenience option when VGA is
> enabled. I think it's a pretty fair thing to do there and simply ignore
> usb=no. If you really need it, just don't use -vga.
> 
> At the end of the day, I don't think anyone will notice. I'd definitely
> prefer to keep the code simple.

Yeah, I agree.  If it's hard to fix, this patchset is okay.  But if it's
not too hard (and I don't think it is, especially if machine_parse is
centralized as in my reply to "[PATCH] vl.c: fix regression when reading
machine type from config file") the old semantics made more sense.

Paolo

> However, I think looking forward we'll want to spawn an XHCI adapter
> rather than OHCI. It's just a lot cheaper to emulate. But that's out of
> scope of this patch set.

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Marcel Apfelbaum]

On 01/06/2015 10:45 PM, Paolo Bonzini wrote:
>
>
> On 06/01/2015 14:29, Marcel Apfelbaum wrote:
>> @@ -1484,9 +1484,10 @@ static void ppc_spapr_init(MachineState *machine)
>>       /* Graphics */
>>       if (spapr_vga_init(phb->bus)) {
>>           spapr->has_graphics = true;
>> +        machine->usb |= defaults_enabled();
>>       }
>
> Could the solution be to do this in instance_init?  Then you would have
> patches 2, 4, 5, 6, 3 (patch 1 would not be needed anymore).
Hi Paolo,
Thanks for the review.

While I agree it will be better if we place this in instance_init,
setting the machine_usb to defaults_enabled() there would be problematic
since it depends on
  - papr_vga_init(phb->bus) for sparpr and
  - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99.
    (The env itself is set in machine_init)

Both of those conditions may be available only at machine_init time,
and I am not sure how it would affect those machines.

This is why I prefer it this way,
Thanks,
Marcel

>
> Paolo
>

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Paolo Bonzini]

On 07/01/2015 12:03, Marcel Apfelbaum wrote:
> 
> While I agree it will be better if we place this in instance_init,
> setting the machine_usb to defaults_enabled() there would be problematic
> since it depends on
>  - papr_vga_init(phb->bus) for sparpr and

That's effectively vga_interface_type == VGA_DEVICE ||
vga_interface_type == VGA_STD.

>  - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99.
>    (The env itself is set in machine_init)

Alex, why is auto-USB disabled for 6xx?  Can it use vga_interface_type
like spapr does?

Paolo

> Both of those conditions may be available only at machine_init time,
> and I am not sure how it would affect those machines.

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Alexander Graf]

On 07.01.15 12:07, Paolo Bonzini wrote:
> 
> 
> On 07/01/2015 12:03, Marcel Apfelbaum wrote:
>>
>> While I agree it will be better if we place this in instance_init,
>> setting the machine_usb to defaults_enabled() there would be problematic
>> since it depends on
>>  - papr_vga_init(phb->bus) for sparpr and
> 
> That's effectively vga_interface_type == VGA_DEVICE ||
> vga_interface_type == VGA_STD.
> 
>>  - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99.
>>    (The env itself is set in machine_init)
> 
> Alex, why is auto-USB disabled for 6xx?  Can it use vga_interface_type
> like spapr does?

That one's a nasty hack. We basically have 2 different machine types
that we expose as a single type to the user: mac99. In reality there's a
64bit mac99 and a 32bit mac99.

32bit mac99 can expose keyboard and mouse via a special apple bus. That
driver doesn't work with 64bit Linux guests though, so there we need USB.

Thinking about it, maybe the best way forward would be to create 2
machine types out of these. Have a mac99 (32bit) and a mac99-g5 target
where the g5 target defaults to -cpu G5 and USB enabled.

All of this is pretty frankenstein btw. What we would really want for a
G5 guest is something built around U3 or U4, not the U1 that -M mac99
exposes.


Alex

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Paolo Bonzini]

On 07/01/2015 12:15, Alexander Graf wrote:
> 
> 
> On 07.01.15 12:07, Paolo Bonzini wrote:
>>
>>
>> On 07/01/2015 12:03, Marcel Apfelbaum wrote:
>>>
>>> While I agree it will be better if we place this in instance_init,
>>> setting the machine_usb to defaults_enabled() there would be problematic
>>> since it depends on
>>>  - papr_vga_init(phb->bus) for sparpr and
>>
>> That's effectively vga_interface_type == VGA_DEVICE ||
>> vga_interface_type == VGA_STD.
>>
>>>  - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99.
>>>    (The env itself is set in machine_init)
>>
>> Alex, why is auto-USB disabled for 6xx?  Can it use vga_interface_type
>> like spapr does?
> 
> That one's a nasty hack. We basically have 2 different machine types
> that we expose as a single type to the user: mac99. In reality there's a
> 64bit mac99 and a 32bit mac99.
> 
> 32bit mac99 can expose keyboard and mouse via a special apple bus. That
> driver doesn't work with 64bit Linux guests though, so there we need USB.
> 
> Thinking about it, maybe the best way forward would be to create 2
> machine types out of these. Have a mac99 (32bit) and a mac99-g5 target
> where the g5 target defaults to -cpu G5 and USB enabled.
> 
> All of this is pretty frankenstein btw. What we would really want for a
> G5 guest is something built around U3 or U4, not the U1 that -M mac99
> exposes.

Hmm, then I guess let's apply these patches and fix things up later?

Paolo

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Alexander Graf]

On 07.01.15 12:22, Paolo Bonzini wrote:
> 
> 
> On 07/01/2015 12:15, Alexander Graf wrote:
>>
>>
>> On 07.01.15 12:07, Paolo Bonzini wrote:
>>>
>>>
>>> On 07/01/2015 12:03, Marcel Apfelbaum wrote:
>>>>
>>>> While I agree it will be better if we place this in instance_init,
>>>> setting the machine_usb to defaults_enabled() there would be problematic
>>>> since it depends on
>>>>  - papr_vga_init(phb->bus) for sparpr and
>>>
>>> That's effectively vga_interface_type == VGA_DEVICE ||
>>> vga_interface_type == VGA_STD.
>>>
>>>>  - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99.
>>>>    (The env itself is set in machine_init)
>>>
>>> Alex, why is auto-USB disabled for 6xx?  Can it use vga_interface_type
>>> like spapr does?
>>
>> That one's a nasty hack. We basically have 2 different machine types
>> that we expose as a single type to the user: mac99. In reality there's a
>> 64bit mac99 and a 32bit mac99.
>>
>> 32bit mac99 can expose keyboard and mouse via a special apple bus. That
>> driver doesn't work with 64bit Linux guests though, so there we need USB.
>>
>> Thinking about it, maybe the best way forward would be to create 2
>> machine types out of these. Have a mac99 (32bit) and a mac99-g5 target
>> where the g5 target defaults to -cpu G5 and USB enabled.
>>
>> All of this is pretty frankenstein btw. What we would really want for a
>> G5 guest is something built around U3 or U4, not the U1 that -M mac99
>> exposes.
> 
> Hmm, then I guess let's apply these patches and fix things up later?

Certainly works for me ;).


Alex

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Paolo Bonzini]

On 07/01/2015 12:27, Alexander Graf wrote:
> > Hmm, then I guess let's apply these patches and fix things up later?
>
> Certainly works for me ;).

Send a pull request then. :)

Paolo

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Alexander Graf]

On 07.01.15 12:32, Paolo Bonzini wrote:
> 
> 
> On 07/01/2015 12:27, Alexander Graf wrote:
>>> Hmm, then I guess let's apply these patches and fix things up later?
>>
>> Certainly works for me ;).
> 
> Send a pull request then. :)

Ok, applied the patch set to ppc-next. Now let's see how much of my
autotest setup fell apart over the holidays ;).


Alex

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Marcel Apfelbaum]

On 01/07/2015 01:15 PM, Alexander Graf wrote:
>
>
> On 07.01.15 12:07, Paolo Bonzini wrote:
>>
>>
>> On 07/01/2015 12:03, Marcel Apfelbaum wrote:
>>>
>>> While I agree it will be better if we place this in instance_init,
>>> setting the machine_usb to defaults_enabled() there would be problematic
>>> since it depends on
>>>   - papr_vga_init(phb->bus) for sparpr and
>>
>> That's effectively vga_interface_type == VGA_DEVICE ||
>> vga_interface_type == VGA_STD.
That means moving select_vgahw (vl.c) that sets vga_interface_type
much much earlier in main, before the current machine is created.
And it depends itself on other stuff...

>>
>>>   - (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) for mac99.
>>>     (The env itself is set in machine_init)
>>
>> Alex, why is auto-USB disabled for 6xx?  Can it use vga_interface_type
>> like spapr does?
>
> That one's a nasty hack. We basically have 2 different machine types
> that we expose as a single type to the user: mac99. In reality there's a
> 64bit mac99 and a 32bit mac99.
>
> 32bit mac99 can expose keyboard and mouse via a special apple bus. That
> driver doesn't work with 64bit Linux guests though, so there we need USB.
>
> Thinking about it, maybe the best way forward would be to create 2
> machine types out of these. Have a mac99 (32bit) and a mac99-g5 target
> where the g5 target defaults to -cpu G5 and USB enabled.
>
> All of this is pretty frankenstein btw. What we would really want for a
> G5 guest is something built around U3 or U4, not the U1 that -M mac99
> exposes.
Given my (lack of) expertise on ppc, I shouldn't throw myself yet in
the above adventure.

Since I will not be able (soon) to get in the stuff Alex mentioned and
the implications moving the setting of vga_interface_type earlier in main
fall far beyond this series target (fix a bug/simpligu -usb on the way),
I suggest putting the above on todo list.

I plan to do the same (access machine's properties instead of quering QemuOpts)
for all other machine properties because I am sure we have other hidden bugs there.


Thanks,
Marcel

>
>
> Alex
>

Re: [Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Stefan Hajnoczi]

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

On Tue, Jan 06, 2015 at 03:29:11PM +0200, Marcel Apfelbaum wrote:
> Patch e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
> 
> This resulted in a Qemu crash:
> $ qemu-system-x86_64 -usb
> qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper:
> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
> Aborted (core dumped)
> 
> Fixed it by simplifying usb_enabled usage:
>  - removed the usb_enabled argument
>  - call directly to machine's QOM property.
>  - expose defaults_enabled to calling sites
> 
> As part of this series the semantics for some ppc machines to
> create the default usb controller were changed, but I still think
> is correct, please let me know if you see a problem there.
> 
> Based on the comments I will receive, I will continue to fix the options
> properties in the same way.
> 
> Marcel Apfelbaum (6):
>   hw/ppc: modified the condition for usb controllers to be created for
>     some ppc machines
>   hw/machine: added machine_usb wrapper
>   hw/usb: simplified usb_enabled
>   hw/ppc/mac_newworld: QOMified mac99 machines
>   hw/ppc/spapr: simplify usb controller creation logic
>   hw/ppc/mac_newworld: simplify usb controller creation logic
> 
>  hw/arm/nseries.c        |  2 +-
>  hw/arm/pxa2xx.c         |  4 ++--
>  hw/arm/realview.c       |  2 +-
>  hw/arm/versatilepb.c    |  2 +-
>  hw/core/machine.c       |  5 +++++
>  hw/i386/pc_piix.c       |  2 +-
>  hw/i386/pc_q35.c        |  2 +-
>  hw/ppc/mac_newworld.c   | 32 +++++++++++++++++++++-----------
>  hw/ppc/mac_oldworld.c   |  2 +-
>  hw/ppc/prep.c           |  2 +-
>  hw/ppc/spapr.c          |  3 ++-
>  include/hw/boards.h     |  2 ++
>  include/sysemu/sysemu.h |  3 ++-
>  vl.c                    | 16 ++++++++++------
>  14 files changed, 51 insertions(+), 28 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 5/6] hw/ppc/spapr: simplify usb controller creation logic

[Marcel Apfelbaum]

On 01/07/2015 01:37 PM, Alexander Graf wrote:
>
>
> On 07.01.15 12:32, Paolo Bonzini wrote:
>>
>>
>> On 07/01/2015 12:27, Alexander Graf wrote:
>>>> Hmm, then I guess let's apply these patches and fix things up later?
>>>
>>> Certainly works for me ;).
>>
>> Send a pull request then. :)
>
> Ok, applied the patch set to ppc-next. Now let's see how much of my
> autotest setup fell apart over the holidays ;).
>
>
> Alex
>

Thanks a lot for taking the time to discuss this!
Marcel

Re: [Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Peter Maydell]

On 6 January 2015 at 13:29, Marcel Apfelbaum <marcel@redhat.com> wrote:
> Patch e79d5a6 ("machine: remove qemu_machine_opts global list") removed option
> descriptions from the -machine QemuOptsList to avoid repeating MachineState's QOM properties.
>
> This resulted in a Qemu crash:
> $ qemu-system-x86_64 -usb
> qemu-system-x86_64: util/qemu-option.c:387: qemu_opt_get_bool_helper:
> Assertion `opt->desc && opt->desc->type == QEMU_OPT_BOOL' failed.
> Aborted (core dumped)

> Based on the comments I will receive, I will continue to fix the options
> properties in the same way.

So does this mean that we still have other broken options? Do we
have a timescale for fixing them? I was hoping to be able to commit
a patchset fixing these regressions this week; if we can't do that
then we should probably revert the original changes instead :-(

-- PMM

Re: [Qemu-devel] [PATCH 0/6] simplify usb enabling logic and fix a Qemu crash

[Paolo Bonzini]

On 08/01/2015 14:27, Peter Maydell wrote:
> So does this mean that we still have other broken options? Do we
> have a timescale for fixing them? I was hoping to be able to commit
> a patchset fixing these regressions this week; if we can't do that
> then we should probably revert the original changes instead :-(

Based on a grep of machine_opts, the following suboptions are broken:

- mem-merge
- dump-guest-core
- phandle_start
- iommu
- kernel_irqchip
- kvm_shadow_mem

None is really as important as -usb, so I would have no problem with
unfreezing the tree, but we definitely should have a timeframe for
fixing it.

Paolo