[PATCH 00/10] usb cleanups: remove usb_bus_find(), extract sysbus-ohci to a separate file

[PATCH 00/10] usb cleanups: remove usb_bus_find(), extract sysbus-ohci to a separate file

[Paolo Bonzini]

Patches 1 to 3 start the removal of usb_bus_find() by replacing it
with object_resolve_type_unambiguous(), a function that is included
in the ACPI builder but should arguably be part of the core QOM API.
Because the only USB host controller must be the one created by the
machine init function itself, there is no need for a special functions
that looks for the "first" bus.

Patches 4 to 7 allow machines that have builtin USB devices to make
the USB system merely the default, and not require it.  At this point
usb_bus_find() is not used anymore outside usb.c, and the only remaining
caller can inline it in patch 8.

Patches 9 and 10 do other build system cleanups to the USB subsystem,
extracting sysbus-ohci to a separate file (as is already the case for
EHCI and XHCI sysbus variants) and removing a duplicate file in the
Meson sourcesets.

The diffstat looks unfavorable, but it's mostly due to
improved documentation and to the duplicated header comment
in hw/usb/hcd-ohci-sysbus.c.

Paolo

Paolo Bonzini (10):
  acpi, qom: move object_resolve_type_unambiguous to core QOM
  ppc: sam460ex: do not use usb_bus_find()
  sh4: r2d: do not use usb_bus_find()
  mips/loongson3_virt: do not require CONFIG_USB
  hppa: do not require CONFIG_USB
  mac_newworld: do not require CONFIG_USB
  pseries: do not require CONFIG_USB
  usb: remove usb_bus_find
  usb: extract sysbus-ohci to a separate file
  usb: remove duplicate file in system_ss

 include/hw/usb.h         |  1 -
 include/qom/object.h     | 13 ++++++
 hw/hppa/machine.c        |  7 +--
 hw/i386/acpi-build.c     | 19 ++------
 hw/mips/loongson3_virt.c |  5 +-
 hw/ppc/mac_newworld.c    |  7 ++-
 hw/ppc/sam460ex.c        |  6 ++-
 hw/ppc/spapr.c           |  7 ++-
 hw/sh4/r2d.c             |  4 +-
 hw/usb/bus.c             | 15 +-----
 hw/usb/hcd-ohci-sysbus.c | 91 ++++++++++++++++++++++++++++++++++++++++
 hw/usb/hcd-ohci.c        | 58 ------------------------
 qom/object.c             | 16 +++++++
 hw/arm/Kconfig           | 12 +++--
 hw/display/Kconfig       |  1 +
 hw/hppa/Kconfig          |  2 +-
 hw/ppc/Kconfig           |  5 +-
 hw/sh4/Kconfig           |  1 -
 hw/usb/Kconfig           |  4 ++
 hw/usb/meson.build       |  3 +-
 20 files changed, 163 insertions(+), 114 deletions(-)
 create mode 100644 hw/usb/hcd-ohci-sysbus.c

-- 
2.43.0

[PATCH 01/10] acpi, qom: move object_resolve_type_unambiguous to core QOM

[Paolo Bonzini]

object_resolve_type_unambiguous provides a useful functionality, that
is currently emulated for example by usb_bus_find().  Move it to core
code and add error reporting for increased generality.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h | 13 +++++++++++++
 hw/i386/acpi-build.c | 19 ++++---------------
 qom/object.c         | 16 ++++++++++++++++
 3 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index afccd24ca7a..e9ed9550f05 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1550,6 +1550,19 @@ Object *object_resolve_path(const char *path, bool *ambiguous);
 Object *object_resolve_path_type(const char *path, const char *typename,
                                  bool *ambiguous);
 
+/**
+ * object_resolve_type_unambiguous:
+ * @typename: the type to look for
+ * @errp: pointer to error object
+ *
+ * Return the only object in the QOM tree of type @typename.
+ * If no match or more than one match is found, an error is
+ * returned.
+ *
+ * Returns: The matched object or NULL on path lookup failure.
+ */
+Object *object_resolve_type_unambiguous(const char *typename, Error **errp);
+
 /**
  * object_resolve_path_at:
  * @parent: the object in which to resolve the path
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index d3ce96dd9f9..4b47dbfd71c 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -192,21 +192,10 @@ static void init_common_fadt_data(MachineState *ms, Object *o,
     *data = fadt;
 }
 
-static Object *object_resolve_type_unambiguous(const char *typename)
-{
-    bool ambig;
-    Object *o = object_resolve_path_type("", typename, &ambig);
-
-    if (ambig || !o) {
-        return NULL;
-    }
-    return o;
-}
-
 static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
 {
-    Object *piix = object_resolve_type_unambiguous(TYPE_PIIX4_PM);
-    Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE);
+    Object *piix = object_resolve_type_unambiguous(TYPE_PIIX4_PM, NULL);
+    Object *lpc = object_resolve_type_unambiguous(TYPE_ICH9_LPC_DEVICE, NULL);
     Object *obj = piix ? piix : lpc;
     QObject *o;
     pm->cpu_hp_io_base = 0;
@@ -1428,8 +1417,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
            AcpiPmInfo *pm, AcpiMiscInfo *misc,
            Range *pci_hole, Range *pci_hole64, MachineState *machine)
 {
-    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE);
-    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE);
+    Object *i440fx = object_resolve_type_unambiguous(TYPE_I440FX_PCI_HOST_BRIDGE, NULL);
+    Object *q35 = object_resolve_type_unambiguous(TYPE_Q35_HOST_DEVICE, NULL);
     CrsRangeEntry *entry;
     Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
     CrsRangeSet crs_range_set;
diff --git a/qom/object.c b/qom/object.c
index 2c4c64d2b63..d4a001cf411 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -2229,6 +2229,22 @@ Object *object_resolve_path_at(Object *parent, const char *path)
     return object_resolve_abs_path(parent, parts, TYPE_OBJECT);
 }
 
+Object *object_resolve_type_unambiguous(const char *typename, Error **errp)
+{
+    bool ambig;
+    Object *o = object_resolve_path_type("", typename, &ambig);
+
+    if (ambig) {
+        error_setg(errp, "More than one object of type %s", typename);
+        return NULL;
+    }
+    if (!o) {
+        error_setg(errp, "No object found of type %s", typename);
+        return NULL;
+    }
+    return o;
+}
+
 typedef struct StringProperty
 {
     char *(*get)(Object *, Error **);
-- 
2.43.0

[PATCH 02/10] ppc: sam460ex: do not use usb_bus_find()

[Paolo Bonzini]

usb_bus_find() is always used with argument -1; it can be replaced with
a search of the single USB bus on the machine.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ppc/sam460ex.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 1e615b8d355..4d5655ab6b4 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -273,6 +273,7 @@ static void sam460ex_init(MachineState *machine)
     DeviceState *uic[4];
     int i;
     PCIBus *pci_bus;
+    USBBus *usb_bus;
     PowerPCCPU *cpu;
     CPUPPCState *env;
     I2CBus *i2c;
@@ -420,8 +421,9 @@ static void sam460ex_init(MachineState *machine)
     sysbus_realize_and_unref(sbdev, &error_fatal);
     sysbus_mmio_map(sbdev, 0, 0x4bffd0000);
     sysbus_connect_irq(sbdev, 0, qdev_get_gpio_in(uic[2], 30));
-    usb_create_simple(usb_bus_find(-1), "usb-kbd");
-    usb_create_simple(usb_bus_find(-1), "usb-mouse");
+    usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort));
+    usb_create_simple(usb_bus, "usb-kbd");
+    usb_create_simple(usb_bus, "usb-mouse");
 
     /* PCIe buses */
     dev = qdev_new(TYPE_PPC460EX_PCIE_HOST);
-- 
2.43.0

[PATCH 03/10] sh4: r2d: do not use usb_bus_find()

[Paolo Bonzini]

usb_bus_find() is always used with argument -1; it can be replaced with
a search of the single USB bus on the machine.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/sh4/r2d.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index c73e8f49b8a..4d34ad00d93 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -244,6 +244,7 @@ static void r2d_init(MachineState *machine)
     SysBusDevice *busdev;
     MemoryRegion *address_space_mem = get_system_memory();
     PCIBus *pci_bus;
+    USBBus *usb_bus;
 
     cpu = SUPERH_CPU(cpu_create(machine->cpu_type));
     env = &cpu->env;
@@ -312,7 +313,8 @@ static void r2d_init(MachineState *machine)
     pci_init_nic_devices(pci_bus, mc->default_nic);
 
     /* USB keyboard */
-    usb_create_simple(usb_bus_find(-1), "usb-kbd");
+    usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort));
+    usb_create_simple(usb_bus, "usb-kbd");
 
     /* Todo: register on board registers */
     memset(&boot_params, 0, sizeof(boot_params));
-- 
2.43.0

[PATCH 04/10] mips/loongson3_virt: do not require CONFIG_USB

[Paolo Bonzini]

Once the Kconfig for hw/mips is cleaned up, it will be possible to build a
binary that does not include any USB host controller and therefore that
does not include the code guarded by CONFIG_USB.  While the simpler
creation functions such as usb_create_simple can be inlined, this is not
true of usb_bus_find().  Remove it, replacing it with a search of the
single USB bus created by loongson3_virt_devices_init().

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/mips/loongson3_virt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index caedde2df00..b2a8b22b4ea 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -447,8 +447,9 @@ static inline void loongson3_virt_devices_init(MachineState *machine,
 
     if (defaults_enabled() && object_class_by_name("pci-ohci")) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
-        usb_create_simple(usb_bus_find(-1), "usb-kbd");
-        usb_create_simple(usb_bus_find(-1), "usb-tablet");
+        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
+        usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
+        usb_create_simple(USB_BUS(usb_bus), "usb-tablet");
     }
 
     pci_init_nic_devices(pci_bus, mc->default_nic);
-- 
2.43.0

[PATCH 05/10] hppa: do not require CONFIG_USB

[Paolo Bonzini]

With --without-default-devices it is possible to build a binary that
does not include any USB host controller and therefore that does not
include the code guarded by CONFIG_USB.  While the simpler creation
functions such as usb_create_simple can be inlined, this is not true
of usb_bus_find().  Remove it, replacing it with a search of the single
USB bus on the machine.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/hppa/machine.c | 7 ++++---
 hw/hppa/Kconfig   | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 5fcaf5884be..11982d5776c 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -396,10 +396,11 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
     }
 
     /* create USB OHCI controller for USB keyboard & mouse on Astro machines */
-    if (!lasi_dev && machine->enable_graphics) {
+    if (!lasi_dev && machine->enable_graphics && defaults_enabled()) {
         pci_create_simple(pci_bus, -1, "pci-ohci");
-        usb_create_simple(usb_bus_find(-1), "usb-kbd");
-        usb_create_simple(usb_bus_find(-1), "usb-mouse");
+        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
+        usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
+        usb_create_simple(USB_BUS(usb_bus), "usb-mouse");
     }
 
     /* register power switch emulation */
diff --git a/hw/hppa/Kconfig b/hw/hppa/Kconfig
index dff5df7f725..ee7ffd2bfb5 100644
--- a/hw/hppa/Kconfig
+++ b/hw/hppa/Kconfig
@@ -2,6 +2,7 @@ config HPPA_B160L
     bool
     imply PCI_DEVICES
     imply E1000_PCI
+    imply USB_OHCI_PCI
     imply VIRTIO_VGA
     select ASTRO
     select DINO
@@ -17,4 +18,3 @@ config HPPA_B160L
     select LASIPS2
     select PARALLEL
     select ARTIST
-    select USB_OHCI_PCI
-- 
2.43.0

[PATCH 06/10] mac_newworld: do not require CONFIG_USB

[Paolo Bonzini]

With --without-default-devices it should not be required to have
devices in the binary that are removed by -nodefaults.  It should be
therefore possible to build a binary that does not include any USB
host controller or any of the code guarded by CONFIG_USB.  While the
simpler creation functions such as usb_create_simple can be inlined,
this is not true of usb_bus_find().  Remove it, replacing it with a
search of the single USB bus on the machine.

With this change, it is possible to change "select USB_OHCI_PCI" into
an "imply" directive.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ppc/mac_newworld.c | 7 +++----
 hw/ppc/Kconfig        | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index b36dbaf2b68..4efebab66b4 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -431,10 +431,9 @@ static void ppc_core99_init(MachineState *machine)
         /* U3 needs to use USB for input because Linux doesn't support via-cuda
         on PPC64 */
         if (!has_adb || machine_arch == ARCH_MAC99_U3) {
-            USBBus *usb_bus = usb_bus_find(-1);
-
-            usb_create_simple(usb_bus, "usb-kbd");
-            usb_create_simple(usb_bus, "usb-mouse");
+            Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
+            usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
+            usb_create_simple(USB_BUS(usb_bus), "usb-mouse");
         }
     }
 
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 44263a58c4d..9841c2c9690 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -116,13 +116,13 @@ config MAC_NEWWORLD
     imply PCI_DEVICES
     imply SUNGEM
     imply TEST_DEVICES
+    imply USB_OHCI_PCI
     select ADB
     select MACIO
     select MACIO_GPIO
     select MAC_PMU
     select UNIN_PCI
     select FW_CFG_PPC
-    select USB_OHCI_PCI
 
 config E500
     bool
-- 
2.43.0

[PATCH 07/10] pseries: do not require CONFIG_USB

[Paolo Bonzini]

With --without-default-devices it is possible to build a binary that
does not include any USB host controller and therefore that does not
include the code guarded by CONFIG_USB.  While the simpler creation
functions such as usb_create_simple can be inlined, this is not true
of usb_bus_find().  Remove it, replacing it with a search of the single
USB bus on the machine.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ppc/spapr.c | 7 +++----
 hw/ppc/Kconfig | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0d72d286d80..44d339982da 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3024,10 +3024,9 @@ static void spapr_machine_init(MachineState *machine)
         }
 
         if (has_vga) {
-            USBBus *usb_bus = usb_bus_find(-1);
-
-            usb_create_simple(usb_bus, "usb-kbd");
-            usb_create_simple(usb_bus, "usb-mouse");
+            Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
+            usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
+            usb_create_simple(USB_BUS(usb_bus), "usb-mouse");
         }
     }
 
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 9841c2c9690..d497fa2b825 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -1,5 +1,6 @@
 config PSERIES
     bool
+    imply USB_OHCI_PCI
     imply PCI_DEVICES
     imply TEST_DEVICES
     imply VIRTIO_VGA
-- 
2.43.0

[PATCH 08/10] usb: remove usb_bus_find

[Paolo Bonzini]

Inline the sole remaining use, which is for the -usbdevice command line.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/hw/usb.h |  1 -
 hw/usb/bus.c     | 15 +--------------
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/include/hw/usb.h b/include/hw/usb.h
index cfeead28403..d46d96779ad 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -498,7 +498,6 @@ struct USBBusOps {
 void usb_bus_new(USBBus *bus, size_t bus_size,
                  USBBusOps *ops, DeviceState *host);
 void usb_bus_release(USBBus *bus);
-USBBus *usb_bus_find(int busnr);
 void usb_legacy_register(const char *typename, const char *usbdevice_name,
                          USBDevice *(*usbdevice_init)(void));
 USBDevice *usbdevice_create(const char *cmdline);
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 76fda41b7ec..796769fadb4 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -100,19 +100,6 @@ void usb_bus_release(USBBus *bus)
     QTAILQ_REMOVE(&busses, bus, next);
 }
 
-USBBus *usb_bus_find(int busnr)
-{
-    USBBus *bus;
-
-    if (-1 == busnr)
-        return QTAILQ_FIRST(&busses);
-    QTAILQ_FOREACH(bus, &busses, next) {
-        if (bus->busnr == busnr)
-            return bus;
-    }
-    return NULL;
-}
-
 static void usb_device_realize(USBDevice *dev, Error **errp)
 {
     USBDeviceClass *klass = USB_DEVICE_GET_CLASS(dev);
@@ -643,7 +630,7 @@ HumanReadableText *qmp_x_query_usb(Error **errp)
 /* handle legacy -usbdevice cmd line option */
 USBDevice *usbdevice_create(const char *driver)
 {
-    USBBus *bus = usb_bus_find(-1 /* any */);
+    USBBus *bus = QTAILQ_FIRST(&busses);
     LegacyUSBFactory *f = NULL;
     Error *err = NULL;
     GSList *i;
-- 
2.43.0

[PATCH 09/10] usb: extract sysbus-ohci to a separate file

[Paolo Bonzini]

Split the sysbus version to a separate file so that it is not
included in PCI-only machines, and adjust Kconfig for machines
that do need sysbus-ohci.  The copyrights are based on the
time and employer of balrog and Paul Brook's contributions.

While adjusting the SM501 dependency, move it to the right place
instead of keeping it in the R4D machine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/usb/hcd-ohci-sysbus.c | 91 ++++++++++++++++++++++++++++++++++++++++
 hw/usb/hcd-ohci.c        | 58 ------------------------
 hw/arm/Kconfig           | 12 +++--
 hw/display/Kconfig       |  1 +
 hw/ppc/Kconfig           |  2 +-
 hw/sh4/Kconfig           |  1 -
 hw/usb/Kconfig           |  4 ++
 hw/usb/meson.build       |  1 +
 8 files changed, 105 insertions(+), 65 deletions(-)
 create mode 100644 hw/usb/hcd-ohci-sysbus.c

diff --git a/hw/usb/hcd-ohci-sysbus.c b/hw/usb/hcd-ohci-sysbus.c
new file mode 100644
index 00000000000..4e4481232b6
--- /dev/null
+++ b/hw/usb/hcd-ohci-sysbus.c
@@ -0,0 +1,91 @@
+/*
+ * QEMU USB OHCI Emulation
+ * Copyright (c) 2006 Openedhand Ltd.
+ * Copyright (c) 2010 CodeSourcery
+ * Copyright (c) 2024 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "qemu/timer.h"
+#include "hw/usb.h"
+#include "migration/vmstate.h"
+#include "hw/sysbus.h"
+#include "hw/qdev-dma.h"
+#include "hw/qdev-properties.h"
+#include "trace.h"
+#include "hcd-ohci.h"
+
+
+static void ohci_realize_pxa(DeviceState *dev, Error **errp)
+{
+    OHCISysBusState *s = SYSBUS_OHCI(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    Error *err = NULL;
+
+    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset,
+                  s->masterbus, s->firstport,
+                  &address_space_memory, ohci_sysbus_die, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_init_irq(sbd, &s->ohci.irq);
+    sysbus_init_mmio(sbd, &s->ohci.mem);
+}
+
+static void usb_ohci_reset_sysbus(DeviceState *dev)
+{
+    OHCISysBusState *s = SYSBUS_OHCI(dev);
+    OHCIState *ohci = &s->ohci;
+
+    ohci_hard_reset(ohci);
+}
+
+static Property ohci_sysbus_properties[] = {
+    DEFINE_PROP_STRING("masterbus", OHCISysBusState, masterbus),
+    DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3),
+    DEFINE_PROP_UINT32("firstport", OHCISysBusState, firstport, 0),
+    DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = ohci_realize_pxa;
+    set_bit(DEVICE_CATEGORY_USB, dc->categories);
+    dc->desc = "OHCI USB Controller";
+    device_class_set_props(dc, ohci_sysbus_properties);
+    dc->reset = usb_ohci_reset_sysbus;
+}
+
+static const TypeInfo ohci_sysbus_info = {
+    .name          = TYPE_SYSBUS_OHCI,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(OHCISysBusState),
+    .class_init    = ohci_sysbus_class_init,
+};
+
+static void ohci_register_types(void)
+{
+    type_register_static(&ohci_sysbus_info);
+}
+
+type_init(ohci_register_types)
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index d73b53f33c8..fc8fc91a1d1 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1955,31 +1955,6 @@ void ohci_sysbus_die(struct OHCIState *ohci)
     ohci_bus_stop(ohci);
 }
 
-static void ohci_realize_pxa(DeviceState *dev, Error **errp)
-{
-    OHCISysBusState *s = SYSBUS_OHCI(dev);
-    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-    Error *err = NULL;
-
-    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset,
-                  s->masterbus, s->firstport,
-                  &address_space_memory, ohci_sysbus_die, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    sysbus_init_irq(sbd, &s->ohci.irq);
-    sysbus_init_mmio(sbd, &s->ohci.mem);
-}
-
-static void usb_ohci_reset_sysbus(DeviceState *dev)
-{
-    OHCISysBusState *s = SYSBUS_OHCI(dev);
-    OHCIState *ohci = &s->ohci;
-
-    ohci_hard_reset(ohci);
-}
-
 static const VMStateDescription vmstate_ohci_state_port = {
     .name = "ohci-core/port",
     .version_id = 1,
@@ -2054,36 +2029,3 @@ const VMStateDescription vmstate_ohci_state = {
         NULL
     }
 };
-
-static Property ohci_sysbus_properties[] = {
-    DEFINE_PROP_STRING("masterbus", OHCISysBusState, masterbus),
-    DEFINE_PROP_UINT32("num-ports", OHCISysBusState, num_ports, 3),
-    DEFINE_PROP_UINT32("firstport", OHCISysBusState, firstport, 0),
-    DEFINE_PROP_DMAADDR("dma-offset", OHCISysBusState, dma_offset, 0),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void ohci_sysbus_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-
-    dc->realize = ohci_realize_pxa;
-    set_bit(DEVICE_CATEGORY_USB, dc->categories);
-    dc->desc = "OHCI USB Controller";
-    device_class_set_props(dc, ohci_sysbus_properties);
-    dc->reset = usb_ohci_reset_sysbus;
-}
-
-static const TypeInfo ohci_sysbus_info = {
-    .name          = TYPE_SYSBUS_OHCI,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(OHCISysBusState),
-    .class_init    = ohci_sysbus_class_init,
-};
-
-static void ohci_register_types(void)
-{
-    type_register_static(&ohci_sysbus_info);
-}
-
-type_init(ohci_register_types)
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 29abe1da29c..ffb9041b07f 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -185,7 +185,7 @@ config PXA2XX
     select SERIAL
     select SD
     select SSI
-    select USB_OHCI
+    select USB_OHCI_SYSBUS
     select PCMCIA
 
 config GUMSTIX
@@ -256,7 +256,7 @@ config REALVIEW
     select PL310  # cache controller
     select ARM_SBCON_I2C
     select DS1338 # I2C RTC+NVRAM
-    select USB_OHCI
+    select USB_OHCI_SYSBUS
 
 config SBSA_REF
     bool
@@ -337,7 +337,7 @@ config VERSATILE
     select PL080  # DMA controller
     select PL190  # Vector PIC
     select REALVIEW
-    select USB_OHCI
+    select USB_OHCI_SYSBUS
 
 config VEXPRESS
     bool
@@ -393,6 +393,7 @@ config ALLWINNER_A10
     select AXP2XX_PMU
     select SERIAL
     select UNIMP
+    select USB_OHCI_SYSBUS
 
 config ALLWINNER_H3
     bool
@@ -406,7 +407,7 @@ config ALLWINNER_H3
     select ARM_TIMER
     select ARM_GIC
     select UNIMP
-    select USB_OHCI
+    select USB_OHCI_SYSBUS
     select USB_EHCI_SYSBUS
     select SD
 
@@ -422,7 +423,7 @@ config ALLWINNER_R40
     select ARM_TIMER
     select ARM_GIC
     select UNIMP
-    select USB_OHCI
+    select USB_OHCI_SYSBUS
     select USB_EHCI_SYSBUS
     select SD
 
@@ -528,6 +529,7 @@ config NPCM7XX
     select SSI
     select UNIMP
     select PCA954X
+    select USB_OHCI_SYSBUS
 
 config FSL_IMX25
     bool
diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 1aafe1923d2..07acb37dc66 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -77,6 +77,7 @@ config SM501
     select I2C
     select DDC
     select SERIAL
+    select USB_OHCI_SYSBUS
 
 config TCX
     bool
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index d497fa2b825..d49e7847909 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -67,7 +67,7 @@ config SAM460EX
     select SM501
     select SMBUS_EEPROM
     select USB_EHCI_SYSBUS
-    select USB_OHCI
+    select USB_OHCI_SYSBUS
     select FDT_PPC
 
 config AMIGAONE
diff --git a/hw/sh4/Kconfig b/hw/sh4/Kconfig
index ab733a3f760..e0c4ecd1a53 100644
--- a/hw/sh4/Kconfig
+++ b/hw/sh4/Kconfig
@@ -6,7 +6,6 @@ config R2D
     select I82378 if TEST_DEVICES
     select IDE_MMIO
     select PFLASH_CFI02
-    select USB_OHCI_PCI
     select PCI
     select SM501
     select SH7750
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 0f486764ed6..f569ed7eeaa 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -11,6 +11,10 @@ config USB_OHCI
     bool
     select USB
 
+config USB_OHCI_SYSBUS
+    bool
+    select USB_OHCI
+
 config USB_OHCI_PCI
     bool
     default y if PCI_DEVICES
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 2c13c528785..94f0e3b034d 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -15,6 +15,7 @@ system_ss.add(when: 'CONFIG_USB', if_true: files(
 system_ss.add(when: 'CONFIG_USB_UHCI', if_true: files('hcd-uhci.c'))
 system_ss.add(when: 'CONFIG_USB_OHCI', if_true: files('hcd-ohci.c'))
 system_ss.add(when: 'CONFIG_USB_OHCI_PCI', if_true: files('hcd-ohci-pci.c'))
+system_ss.add(when: 'CONFIG_USB_OHCI_SYSBUS', if_true: files('hcd-ohci-sysbus.c'))
 system_ss.add(when: 'CONFIG_USB_EHCI', if_true: files('hcd-ehci.c'))
 system_ss.add(when: 'CONFIG_USB_EHCI_PCI', if_true: files('hcd-ehci-pci.c'))
 system_ss.add(when: 'CONFIG_USB_EHCI_SYSBUS', if_true: files('hcd-ehci.c', 'hcd-ehci-sysbus.c'))
-- 
2.43.0

[PATCH 10/10] usb: remove duplicate file in system_ss

[Paolo Bonzini]

Because USB_EHCI_SYSBUS selects USB_EHCI, there is no need to include
hcd-ehci.c explicitly.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/usb/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 94f0e3b034d..aac3bb35f27 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -18,7 +18,7 @@ system_ss.add(when: 'CONFIG_USB_OHCI_PCI', if_true: files('hcd-ohci-pci.c'))
 system_ss.add(when: 'CONFIG_USB_OHCI_SYSBUS', if_true: files('hcd-ohci-sysbus.c'))
 system_ss.add(when: 'CONFIG_USB_EHCI', if_true: files('hcd-ehci.c'))
 system_ss.add(when: 'CONFIG_USB_EHCI_PCI', if_true: files('hcd-ehci-pci.c'))
-system_ss.add(when: 'CONFIG_USB_EHCI_SYSBUS', if_true: files('hcd-ehci.c', 'hcd-ehci-sysbus.c'))
+system_ss.add(when: 'CONFIG_USB_EHCI_SYSBUS', if_true: files('hcd-ehci-sysbus.c'))
 system_ss.add(when: 'CONFIG_USB_XHCI', if_true: files('hcd-xhci.c'))
 system_ss.add(when: 'CONFIG_USB_XHCI_PCI', if_true: files('hcd-xhci-pci.c'))
 system_ss.add(when: 'CONFIG_USB_XHCI_SYSBUS', if_true: files('hcd-xhci-sysbus.c'))
-- 
2.43.0

Re: [PATCH 01/10] acpi, qom: move object_resolve_type_unambiguous to core QOM

[Philippe Mathieu-Daudé]

On 23/2/24 13:43, Paolo Bonzini wrote:
> object_resolve_type_unambiguous provides a useful functionality, that
> is currently emulated for example by usb_bus_find().  Move it to core
> code and add error reporting for increased generality.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/qom/object.h | 13 +++++++++++++
>   hw/i386/acpi-build.c | 19 ++++---------------
>   qom/object.c         | 16 ++++++++++++++++
>   3 files changed, 33 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 08/10] usb: remove usb_bus_find

[Philippe Mathieu-Daudé]

On 23/2/24 13:44, Paolo Bonzini wrote:
> Inline the sole remaining use, which is for the -usbdevice command line.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/hw/usb.h |  1 -
>   hw/usb/bus.c     | 15 +--------------
>   2 files changed, 1 insertion(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 09/10] usb: extract sysbus-ohci to a separate file

[Philippe Mathieu-Daudé]

On 23/2/24 13:44, Paolo Bonzini wrote:
> Split the sysbus version to a separate file so that it is not
> included in PCI-only machines, and adjust Kconfig for machines
> that do need sysbus-ohci.  The copyrights are based on the
> time and employer of balrog and Paul Brook's contributions.
> 
> While adjusting the SM501 dependency, move it to the right place
> instead of keeping it in the R4D machine.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/usb/hcd-ohci-sysbus.c | 91 ++++++++++++++++++++++++++++++++++++++++
>   hw/usb/hcd-ohci.c        | 58 ------------------------
>   hw/arm/Kconfig           | 12 +++--
>   hw/display/Kconfig       |  1 +
>   hw/ppc/Kconfig           |  2 +-
>   hw/sh4/Kconfig           |  1 -
>   hw/usb/Kconfig           |  4 ++
>   hw/usb/meson.build       |  1 +
>   8 files changed, 105 insertions(+), 65 deletions(-)
>   create mode 100644 hw/usb/hcd-ohci-sysbus.c


> +static void ohci_realize_pxa(DeviceState *dev, Error **errp)

s/ohci_realize_pxa/ohci_sysbus_realize/

> +{
> +    OHCISysBusState *s = SYSBUS_OHCI(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    Error *err = NULL;
> +
> +    usb_ohci_init(&s->ohci, dev, s->num_ports, s->dma_offset,
> +                  s->masterbus, s->firstport,
> +                  &address_space_memory, ohci_sysbus_die, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    sysbus_init_irq(sbd, &s->ohci.irq);
> +    sysbus_init_mmio(sbd, &s->ohci.mem);
> +}
> +
> +static void usb_ohci_reset_sysbus(DeviceState *dev)

s/usb_ohci_reset_sysbus/ohci_sysbus/reset/.

To be converted to Resettable API.

> +{
> +    OHCISysBusState *s = SYSBUS_OHCI(dev);
> +    OHCIState *ohci = &s->ohci;
> +
> +    ohci_hard_reset(ohci);
> +}


> +static void ohci_register_types(void)
> +{
> +    type_register_static(&ohci_sysbus_info);
> +}
> +
> +type_init(ohci_register_types)

Better directly use DEFINE_TYPES() in new uses, even for
a single type.


> diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
> index 0f486764ed6..f569ed7eeaa 100644
> --- a/hw/usb/Kconfig
> +++ b/hw/usb/Kconfig
> @@ -11,6 +11,10 @@ config USB_OHCI
>       bool
>       select USB
>   
> +config USB_OHCI_SYSBUS
> +    bool
> +    select USB_OHCI

I start to think USB_OHCI_MMIO would be clearer. I can
clean that up later, so:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   config USB_OHCI_PCI
>       bool
>       default y if PCI_DEVICES

Re: [PATCH 05/10] hppa: do not require CONFIG_USB

[Philippe Mathieu-Daudé]

On 23/2/24 13:44, Paolo Bonzini wrote:
> With --without-default-devices it is possible to build a binary that
> does not include any USB host controller and therefore that does not
> include the code guarded by CONFIG_USB.  While the simpler creation
> functions such as usb_create_simple can be inlined, this is not true
> of usb_bus_find().  Remove it, replacing it with a search of the single
> USB bus on the machine.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/hppa/machine.c | 7 ++++---
>   hw/hppa/Kconfig   | 2 +-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 5fcaf5884be..11982d5776c 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -396,10 +396,11 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
>       }
>   
>       /* create USB OHCI controller for USB keyboard & mouse on Astro machines */
> -    if (!lasi_dev && machine->enable_graphics) {
> +    if (!lasi_dev && machine->enable_graphics && defaults_enabled()) {
>           pci_create_simple(pci_bus, -1, "pci-ohci");
> -        usb_create_simple(usb_bus_find(-1), "usb-kbd");
> -        usb_create_simple(usb_bus_find(-1), "usb-mouse");
> +        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);

Declare variable at begin of function; can be casted to USB_BUS once.
Otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +        usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
> +        usb_create_simple(USB_BUS(usb_bus), "usb-mouse");
>       }

Re: [PATCH 06/10] mac_newworld: do not require CONFIG_USB

[Philippe Mathieu-Daudé]

On 23/2/24 13:44, Paolo Bonzini wrote:
> With --without-default-devices it should not be required to have
> devices in the binary that are removed by -nodefaults.  It should be
> therefore possible to build a binary that does not include any USB
> host controller or any of the code guarded by CONFIG_USB.  While the
> simpler creation functions such as usb_create_simple can be inlined,
> this is not true of usb_bus_find().  Remove it, replacing it with a
> search of the single USB bus on the machine.
> 
> With this change, it is possible to change "select USB_OHCI_PCI" into
> an "imply" directive.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/ppc/mac_newworld.c | 7 +++----
>   hw/ppc/Kconfig        | 2 +-
>   2 files changed, 4 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/10] pseries: do not require CONFIG_USB

[Philippe Mathieu-Daudé]

On 23/2/24 13:44, Paolo Bonzini wrote:
> With --without-default-devices it is possible to build a binary that
> does not include any USB host controller and therefore that does not
> include the code guarded by CONFIG_USB.  While the simpler creation
> functions such as usb_create_simple can be inlined, this is not true
> of usb_bus_find().  Remove it, replacing it with a search of the single
> USB bus on the machine.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/ppc/spapr.c | 7 +++----
>   hw/ppc/Kconfig | 1 +
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0d72d286d80..44d339982da 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3024,10 +3024,9 @@ static void spapr_machine_init(MachineState *machine)
>           }
>   
>           if (has_vga) {

Pre-existing, don't we want defaults_enabled() instead of has_vga here?

> -            USBBus *usb_bus = usb_bus_find(-1);
> -
> -            usb_create_simple(usb_bus, "usb-kbd");
> -            usb_create_simple(usb_bus, "usb-mouse");
> +            Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
> +            usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
> +            usb_create_simple(USB_BUS(usb_bus), "usb-mouse");
>           }
>       }
>   
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index 9841c2c9690..d497fa2b825 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -1,5 +1,6 @@
>   config PSERIES
>       bool
> +    imply USB_OHCI_PCI
>       imply PCI_DEVICES
>       imply TEST_DEVICES
>       imply VIRTIO_VGA

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 10/10] usb: remove duplicate file in system_ss

[Philippe Mathieu-Daudé]

On 23/2/24 13:44, Paolo Bonzini wrote:
> Because USB_EHCI_SYSBUS selects USB_EHCI, there is no need to include
> hcd-ehci.c explicitly.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/usb/meson.build | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 07/10] pseries: do not require CONFIG_USB

[Paolo Bonzini]

On Fri, Feb 23, 2024 at 6:33 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 23/2/24 13:44, Paolo Bonzini wrote:
> > With --without-default-devices it is possible to build a binary that
> > does not include any USB host controller and therefore that does not
> > include the code guarded by CONFIG_USB.  While the simpler creation
> > functions such as usb_create_simple can be inlined, this is not true
> > of usb_bus_find().  Remove it, replacing it with a search of the single
> > USB bus on the machine.
> >
> > Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   hw/ppc/spapr.c | 7 +++----
> >   hw/ppc/Kconfig | 1 +
> >   2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0d72d286d80..44d339982da 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3024,10 +3024,9 @@ static void spapr_machine_init(MachineState *machine)
> >           }
> >
> >           if (has_vga) {
>
> Pre-existing, don't we want defaults_enabled() instead of has_vga here?

Yeah, but I am not sure it can be changed since pseries machine types
are versioned.

Paolo

Re: [PATCH 05/10] hppa: do not require CONFIG_USB

[Thomas Huth]

On 23/02/2024 13.44, Paolo Bonzini wrote:
> With --without-default-devices it is possible to build a binary that
> does not include any USB host controller and therefore that does not
> include the code guarded by CONFIG_USB.  While the simpler creation
> functions such as usb_create_simple can be inlined, this is not true
> of usb_bus_find().  Remove it, replacing it with a search of the single
> USB bus on the machine.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/hppa/machine.c | 7 ++++---
>   hw/hppa/Kconfig   | 2 +-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 5fcaf5884be..11982d5776c 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -396,10 +396,11 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
>       }
>   
>       /* create USB OHCI controller for USB keyboard & mouse on Astro machines */
> -    if (!lasi_dev && machine->enable_graphics) {
> +    if (!lasi_dev && machine->enable_graphics && defaults_enabled()) {

Do we need the defaults_enabled() here? Isn't enable_graphics already 
disabled if defaults_enabled() is not set?

  Thomas


>           pci_create_simple(pci_bus, -1, "pci-ohci");
> -        usb_create_simple(usb_bus_find(-1), "usb-kbd");
> -        usb_create_simple(usb_bus_find(-1), "usb-mouse");
> +        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
> +        usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
> +        usb_create_simple(USB_BUS(usb_bus), "usb-mouse");
>       }

Re: [PATCH 05/10] hppa: do not require CONFIG_USB

[BALATON Zoltan]

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

On Fri, 23 Feb 2024, Thomas Huth wrote:
> On 23/02/2024 13.44, Paolo Bonzini wrote:
>> With --without-default-devices it is possible to build a binary that
>> does not include any USB host controller and therefore that does not
>> include the code guarded by CONFIG_USB.  While the simpler creation
>> functions such as usb_create_simple can be inlined, this is not true
>> of usb_bus_find().  Remove it, replacing it with a search of the single
>> USB bus on the machine.
>> 
>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   hw/hppa/machine.c | 7 ++++---
>>   hw/hppa/Kconfig   | 2 +-
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>> index 5fcaf5884be..11982d5776c 100644
>> --- a/hw/hppa/machine.c
>> +++ b/hw/hppa/machine.c
>> @@ -396,10 +396,11 @@ static void machine_HP_common_init_tail(MachineState 
>> *machine, PCIBus *pci_bus,
>>       }
>>         /* create USB OHCI controller for USB keyboard & mouse on Astro 
>> machines */
>> -    if (!lasi_dev && machine->enable_graphics) {
>> +    if (!lasi_dev && machine->enable_graphics && defaults_enabled()) {
>
> Do we need the defaults_enabled() here? Isn't enable_graphics already 
> disabled if defaults_enabled() is not set?

Isn't enable_graphics controlled by -nographic and defaults_enabled 
controlled by -nodefaults? I think they are independent but maybe that 
does not answer the question if it's needed or not.

Regards,
BALATON Zoltan

> Thomas
>
>
>>           pci_create_simple(pci_bus, -1, "pci-ohci");
>> -        usb_create_simple(usb_bus_find(-1), "usb-kbd");
>> -        usb_create_simple(usb_bus_find(-1), "usb-mouse");
>> +        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, 
>> &error_abort);
>> +        usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
>> +        usb_create_simple(USB_BUS(usb_bus), "usb-mouse");
>>       }
>
>
>

Re: [PATCH 05/10] hppa: do not require CONFIG_USB

[Paolo Bonzini]

On Fri, Feb 23, 2024 at 8:56 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >> -    if (!lasi_dev && machine->enable_graphics) {
> >> +    if (!lasi_dev && machine->enable_graphics && defaults_enabled()) {
> >
> > Do we need the defaults_enabled() here? Isn't enable_graphics already
> > disabled if defaults_enabled() is not set?
>
> Isn't enable_graphics controlled by -nographic and defaults_enabled
> controlled by -nodefaults? I think they are independent but maybe that
> does not answer the question if it's needed or not.

Indeed they are different. The idea is that if graphics are disabled
you interact with a serial console so you don't need keyboard or
mouse; and if default devices are disabled of course you create as
little as possible and leave everything to the user.

Paolo

Re: [PATCH 05/10] hppa: do not require CONFIG_USB

[Thomas Huth]

On 24/02/2024 23.38, Paolo Bonzini wrote:
> On Fri, Feb 23, 2024 at 8:56 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> -    if (!lasi_dev && machine->enable_graphics) {
>>>> +    if (!lasi_dev && machine->enable_graphics && defaults_enabled()) {
>>>
>>> Do we need the defaults_enabled() here? Isn't enable_graphics already
>>> disabled if defaults_enabled() is not set?
>>
>> Isn't enable_graphics controlled by -nographic and defaults_enabled
>> controlled by -nodefaults? I think they are independent but maybe that
>> does not answer the question if it's needed or not.
> 
> Indeed they are different. The idea is that if graphics are disabled
> you interact with a serial console so you don't need keyboard or
> mouse; and if default devices are disabled of course you create as
> little as possible and leave everything to the user.

Indeed, I just double-checked and they are independent, sorry for 
mis-remembering that.

  Thomas

Re: [PATCH 01/10] acpi, qom: move object_resolve_type_unambiguous to core QOM

[Thomas Huth]

On 23/02/2024 13.43, Paolo Bonzini wrote:
> object_resolve_type_unambiguous provides a useful functionality, that
> is currently emulated for example by usb_bus_find().  Move it to core
> code and add error reporting for increased generality.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   include/qom/object.h | 13 +++++++++++++
>   hw/i386/acpi-build.c | 19 ++++---------------
>   qom/object.c         | 16 ++++++++++++++++
>   3 files changed, 33 insertions(+), 15 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 02/10] ppc: sam460ex: do not use usb_bus_find()

[Thomas Huth]

On 23/02/2024 13.43, Paolo Bonzini wrote:
> usb_bus_find() is always used with argument -1; it can be replaced with
> a search of the single USB bus on the machine.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/ppc/sam460ex.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 1e615b8d355..4d5655ab6b4 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -273,6 +273,7 @@ static void sam460ex_init(MachineState *machine)
>       DeviceState *uic[4];
>       int i;
>       PCIBus *pci_bus;
> +    USBBus *usb_bus;
>       PowerPCCPU *cpu;
>       CPUPPCState *env;
>       I2CBus *i2c;
> @@ -420,8 +421,9 @@ static void sam460ex_init(MachineState *machine)
>       sysbus_realize_and_unref(sbdev, &error_fatal);
>       sysbus_mmio_map(sbdev, 0, 0x4bffd0000);
>       sysbus_connect_irq(sbdev, 0, qdev_get_gpio_in(uic[2], 30));
> -    usb_create_simple(usb_bus_find(-1), "usb-kbd");
> -    usb_create_simple(usb_bus_find(-1), "usb-mouse");
> +    usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort));
> +    usb_create_simple(usb_bus, "usb-kbd");
> +    usb_create_simple(usb_bus, "usb-mouse");
>   
>       /* PCIe buses */
>       dev = qdev_new(TYPE_PPC460EX_PCIE_HOST);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 03/10] sh4: r2d: do not use usb_bus_find()

[Thomas Huth]

On 23/02/2024 13.43, Paolo Bonzini wrote:
> usb_bus_find() is always used with argument -1; it can be replaced with
> a search of the single USB bus on the machine.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/sh4/r2d.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index c73e8f49b8a..4d34ad00d93 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -244,6 +244,7 @@ static void r2d_init(MachineState *machine)
>       SysBusDevice *busdev;
>       MemoryRegion *address_space_mem = get_system_memory();
>       PCIBus *pci_bus;
> +    USBBus *usb_bus;
>   
>       cpu = SUPERH_CPU(cpu_create(machine->cpu_type));
>       env = &cpu->env;
> @@ -312,7 +313,8 @@ static void r2d_init(MachineState *machine)
>       pci_init_nic_devices(pci_bus, mc->default_nic);
>   
>       /* USB keyboard */
> -    usb_create_simple(usb_bus_find(-1), "usb-kbd");
> +    usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort));
> +    usb_create_simple(usb_bus, "usb-kbd");
>   
>       /* Todo: register on board registers */
>       memset(&boot_params, 0, sizeof(boot_params));

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 04/10] mips/loongson3_virt: do not require CONFIG_USB

[Thomas Huth]

On 23/02/2024 13.44, Paolo Bonzini wrote:
> Once the Kconfig for hw/mips is cleaned up, it will be possible to build a
> binary that does not include any USB host controller and therefore that
> does not include the code guarded by CONFIG_USB.  While the simpler
> creation functions such as usb_create_simple can be inlined, this is not
> true of usb_bus_find().  Remove it, replacing it with a search of the
> single USB bus created by loongson3_virt_devices_init().
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/mips/loongson3_virt.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index caedde2df00..b2a8b22b4ea 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -447,8 +447,9 @@ static inline void loongson3_virt_devices_init(MachineState *machine,
>   
>       if (defaults_enabled() && object_class_by_name("pci-ohci")) {
>           pci_create_simple(pci_bus, -1, "pci-ohci");
> -        usb_create_simple(usb_bus_find(-1), "usb-kbd");
> -        usb_create_simple(usb_bus_find(-1), "usb-tablet");
> +        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);

Please swap the "Object *usb_bus" and the "pci_create_simple" lines, so that 
the declaration is at the beginning of the block.

  Thomas

Re: [PATCH 09/10] usb: extract sysbus-ohci to a separate file

[Thomas Huth]

On 23/02/2024 13.44, Paolo Bonzini wrote:
> Split the sysbus version to a separate file so that it is not
> included in PCI-only machines, and adjust Kconfig for machines
> that do need sysbus-ohci.  The copyrights are based on the
> time and employer of balrog and Paul Brook's contributions.
> 
> While adjusting the SM501 dependency, move it to the right place
> instead of keeping it in the R4D machine.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   hw/usb/hcd-ohci-sysbus.c | 91 ++++++++++++++++++++++++++++++++++++++++
>   hw/usb/hcd-ohci.c        | 58 ------------------------
>   hw/arm/Kconfig           | 12 +++--
>   hw/display/Kconfig       |  1 +
>   hw/ppc/Kconfig           |  2 +-
>   hw/sh4/Kconfig           |  1 -
>   hw/usb/Kconfig           |  4 ++
>   hw/usb/meson.build       |  1 +
>   8 files changed, 105 insertions(+), 65 deletions(-)
>   create mode 100644 hw/usb/hcd-ohci-sysbus.c
> 
> diff --git a/hw/usb/hcd-ohci-sysbus.c b/hw/usb/hcd-ohci-sysbus.c
> new file mode 100644
> index 00000000000..4e4481232b6
> --- /dev/null
> +++ b/hw/usb/hcd-ohci-sysbus.c
> @@ -0,0 +1,91 @@
> +/*
> + * QEMU USB OHCI Emulation
> + * Copyright (c) 2006 Openedhand Ltd.
> + * Copyright (c) 2010 CodeSourcery
> + * Copyright (c) 2024 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/irq.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "qemu/timer.h"
> +#include "hw/usb.h"
> +#include "migration/vmstate.h"
> +#include "hw/sysbus.h"
> +#include "hw/qdev-dma.h"
> +#include "hw/qdev-properties.h"
> +#include "trace.h"
> +#include "hcd-ohci.h"
> +
> +
> +static void ohci_realize_pxa(DeviceState *dev, Error **errp)

Maybe this could be renamed to ohci_sysbs_realize() now since the code is 
used in non-pxa devices nowadays, too?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 02/10] ppc: sam460ex: do not use usb_bus_find()

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Once the Kconfig for hw/mips is cleaned up, it will be possible to build a
> binary that does not include any USB host controller and therefore that
> does not include the code guarded by CONFIG_USB.  While the simpler
> creation functions such as usb_create_simple can be inlined, this is not
> true of usb_bus_find().  Remove it, replacing it with a search of the
> single USB bus created by loongson3_virt_devices_init().
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mips/loongson3_virt.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index caedde2df00..b2a8b22b4ea 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -447,8 +447,9 @@ static inline void loongson3_virt_devices_init(MachineState *machine,
>  
>      if (defaults_enabled() && object_class_by_name("pci-ohci")) {
>          pci_create_simple(pci_bus, -1, "pci-ohci");
> -        usb_create_simple(usb_bus_find(-1), "usb-kbd");
> -        usb_create_simple(usb_bus_find(-1), "usb-tablet");
> +        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
> +        usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
> +        usb_create_simple(USB_BUS(usb_bus), "usb-tablet");

In the previous patches, you cast just once, like this:

           USBBus *usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort));
           usb_create_simple(usb_bus, "usb-kbd");
           usb_create_simple(usb_bus, "usb-tablet");

Could you do that here, too?

Same for the next few patches.

>      }
>  
>      pci_init_nic_devices(pci_bus, mc->default_nic);

Re: [PATCH 02/10] ppc: sam460ex: do not use usb_bus_find()

[Markus Armbruster]

Please ignore this one; I replied to the wrong patch by accident.

Re: [PATCH 04/10] mips/loongson3_virt: do not require CONFIG_USB

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Once the Kconfig for hw/mips is cleaned up, it will be possible to build a
> binary that does not include any USB host controller and therefore that
> does not include the code guarded by CONFIG_USB.  While the simpler
> creation functions such as usb_create_simple can be inlined, this is not
> true of usb_bus_find().  Remove it, replacing it with a search of the
> single USB bus created by loongson3_virt_devices_init().
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/mips/loongson3_virt.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index caedde2df00..b2a8b22b4ea 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -447,8 +447,9 @@ static inline void loongson3_virt_devices_init(MachineState *machine,
>  
>      if (defaults_enabled() && object_class_by_name("pci-ohci")) {
>          pci_create_simple(pci_bus, -1, "pci-ohci");
> -        usb_create_simple(usb_bus_find(-1), "usb-kbd");
> -        usb_create_simple(usb_bus_find(-1), "usb-tablet");
> +        Object *usb_bus = object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort);
> +        usb_create_simple(USB_BUS(usb_bus), "usb-kbd");
> +        usb_create_simple(USB_BUS(usb_bus), "usb-tablet");

In the previous patches, you cast just once, like this:

           USBBus *usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort));
           usb_create_simple(usb_bus, "usb-kbd");
           usb_create_simple(usb_bus, "usb-tablet");

Could you do that here, too?

Same for the next few patches.

>      }
>  
>      pci_init_nic_devices(pci_bus, mc->default_nic);

Re: [PATCH 02/10] ppc: sam460ex: do not use usb_bus_find()

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> usb_bus_find() is always used with argument -1; it can be replaced with
> a search of the single USB bus on the machine.
>
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ppc/sam460ex.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 1e615b8d355..4d5655ab6b4 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -273,6 +273,7 @@ static void sam460ex_init(MachineState *machine)
>      DeviceState *uic[4];
>      int i;
>      PCIBus *pci_bus;
> +    USBBus *usb_bus;
>      PowerPCCPU *cpu;
>      CPUPPCState *env;
>      I2CBus *i2c;
> @@ -420,8 +421,9 @@ static void sam460ex_init(MachineState *machine)
>      sysbus_realize_and_unref(sbdev, &error_fatal);
>      sysbus_mmio_map(sbdev, 0, 0x4bffd0000);
>      sysbus_connect_irq(sbdev, 0, qdev_get_gpio_in(uic[2], 30));
> -    usb_create_simple(usb_bus_find(-1), "usb-kbd");
> -    usb_create_simple(usb_bus_find(-1), "usb-mouse");
> +    usb_bus = USB_BUS(object_resolve_type_unambiguous(TYPE_USB_BUS, &error_abort));

This long line is really easy to break.

> +    usb_create_simple(usb_bus, "usb-kbd");
> +    usb_create_simple(usb_bus, "usb-mouse");
>  
>      /* PCIe buses */
>      dev = qdev_new(TYPE_PPC460EX_PCIE_HOST);

Re: [PATCH 00/10] usb cleanups: remove usb_bus_find(), extract sysbus-ohci to a separate file

[Philippe Mathieu-Daudé]

On 23/2/24 13:43, Paolo Bonzini wrote:

> Paolo Bonzini (10):
>    acpi, qom: move object_resolve_type_unambiguous to core QOM
>    ppc: sam460ex: do not use usb_bus_find()
>    sh4: r2d: do not use usb_bus_find()
>    mips/loongson3_virt: do not require CONFIG_USB
>    hppa: do not require CONFIG_USB
>    mac_newworld: do not require CONFIG_USB
>    pseries: do not require CONFIG_USB
>    usb: remove usb_bus_find
>    usb: extract sysbus-ohci to a separate file
>    usb: remove duplicate file in system_ss

Thanks, queued addressing Thomas & Markus comments.

Re: [PATCH 02/10] ppc: sam460ex: do not use usb_bus_find()

[BALATON Zoltan]

On Mon, 26 Feb 2024, Markus Armbruster wrote:
> Please ignore this one; I replied to the wrong patch by accident.

Your comment is still valid for that patch so no need to ignore it.

Regards,
BALATON Zoltan