[PATCH v2 0/6] hw/m68k: Strengthen QOM/SysBus API uses

[PATCH v2 0/6] hw/m68k: Strengthen QOM/SysBus API uses

[Philippe Mathieu-Daudé]

All series reviewed.

v2:
- Addressed Thomas comments in patches 1-2
- Added R-b tags

Avoid QOM objects poking at each other internals:
- Pass "link" properties
- Access MMIO via SysBus API
- Simplify using sysbus_create_simple()

Philippe Mathieu-Daudé (6):
  hw/m68k/irqc: Pass CPU using QOM link property
  hw/m68k/mcf5206: Pass CPU using QOM link property
  hw/m68k/mcf_intc: Expose MMIO region via SysBus API
  hw/m68k/mcf_intc: Pass CPU using QOM link property
  hw/m68k/next-cube: Do not open-code sysbus_create_simple()
  hw/m68k/virt: Do not open-code sysbus_create_simple()

 include/hw/intc/m68k_irqc.h |  1 +
 hw/intc/m68k_irqc.c         | 10 +++++++++-
 hw/m68k/an5206.c            |  6 ++++--
 hw/m68k/mcf5206.c           |  9 ++++++++-
 hw/m68k/mcf_intc.c          | 21 ++++++++++++++-------
 hw/m68k/next-cube.c         |  9 ++-------
 hw/m68k/virt.c              |  9 ++++-----
 7 files changed, 42 insertions(+), 23 deletions(-)

-- 
2.41.0

[PATCH v2 1/6] hw/m68k/irqc: Pass CPU using QOM link property

[Philippe Mathieu-Daudé]

Avoid the interrupt controller directly access the 'first_cpu'
global. Pass 'cpu' from the board code.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/intc/m68k_irqc.h |  1 +
 hw/intc/m68k_irqc.c         | 10 +++++++++-
 hw/m68k/virt.c              |  2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/hw/intc/m68k_irqc.h b/include/hw/intc/m68k_irqc.h
index ef91f21812..693e33b0aa 100644
--- a/include/hw/intc/m68k_irqc.h
+++ b/include/hw/intc/m68k_irqc.h
@@ -33,6 +33,7 @@ typedef struct M68KIRQCState {
     SysBusDevice parent_obj;
 
     uint8_t ipr;
+    ArchCPU *cpu;
 
     /* statistics */
     uint64_t stats_irq_count[M68K_IRQC_LEVEL_NUM];
diff --git a/hw/intc/m68k_irqc.c b/hw/intc/m68k_irqc.c
index 0c515e4ecb..e09705eeaf 100644
--- a/hw/intc/m68k_irqc.c
+++ b/hw/intc/m68k_irqc.c
@@ -11,6 +11,7 @@
 #include "cpu.h"
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
+#include "hw/qdev-properties.h"
 #include "hw/nmi.h"
 #include "hw/intc/intc.h"
 #include "hw/intc/m68k_irqc.h"
@@ -35,7 +36,7 @@ static void m68k_irqc_print_info(InterruptStatsProvider *obj, Monitor *mon)
 static void m68k_set_irq(void *opaque, int irq, int level)
 {
     M68KIRQCState *s = opaque;
-    M68kCPU *cpu = M68K_CPU(first_cpu);
+    M68kCPU *cpu = M68K_CPU(s->cpu);
     int i;
 
     if (level) {
@@ -85,12 +86,19 @@ static const VMStateDescription vmstate_m68k_irqc = {
     }
 };
 
+static Property m68k_irqc_properties[] = {
+    DEFINE_PROP_LINK("m68k-cpu", M68KIRQCState, cpu,
+                     TYPE_M68K_CPU, ArchCPU *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void m68k_irqc_class_init(ObjectClass *oc, void *data)
  {
     DeviceClass *dc = DEVICE_CLASS(oc);
     NMIClass *nc = NMI_CLASS(oc);
     InterruptStatsProviderClass *ic = INTERRUPT_STATS_PROVIDER_CLASS(oc);
 
+    device_class_set_props(dc, m68k_irqc_properties);
     nc->nmi_monitor_handler = m68k_nmi;
     dc->reset = m68k_irqc_reset;
     dc->vmsd = &vmstate_m68k_irqc;
diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
index 2dd3c99894..e7dc188855 100644
--- a/hw/m68k/virt.c
+++ b/hw/m68k/virt.c
@@ -155,6 +155,8 @@ static void virt_init(MachineState *machine)
     /* IRQ Controller */
 
     irqc_dev = qdev_new(TYPE_M68K_IRQC);
+    object_property_set_link(OBJECT(irqc_dev), "m68k-cpu",
+                             OBJECT(cpu), &error_abort);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(irqc_dev), &error_fatal);
 
     /*
-- 
2.41.0

[PATCH v2 2/6] hw/m68k/mcf5206: Pass CPU using QOM link property

[Philippe Mathieu-Daudé]

Avoid the interrupt controller directly access the first cpu via
the qemu_get_cpu() call. Pass it as argument to mcf5206_init()
from the board code.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/m68k/an5206.c  | 6 ++++--
 hw/m68k/mcf5206.c | 9 ++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/m68k/an5206.c b/hw/m68k/an5206.c
index 11ae4c9795..1e8e64f8bd 100644
--- a/hw/m68k/an5206.c
+++ b/hw/m68k/an5206.c
@@ -20,12 +20,14 @@
 #define AN5206_MBAR_ADDR 0x10000000
 #define AN5206_RAMBAR_ADDR 0x20000000
 
-static void mcf5206_init(MemoryRegion *sysmem, uint32_t base)
+static void mcf5206_init(M68kCPU *cpu, MemoryRegion *sysmem, uint32_t base)
 {
     DeviceState *dev;
     SysBusDevice *s;
 
     dev = qdev_new(TYPE_MCF5206_MBAR);
+    object_property_set_link(OBJECT(dev), "m68k-cpu",
+                             OBJECT(cpu), &error_abort);
     s = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(s, &error_fatal);
 
@@ -60,7 +62,7 @@ static void an5206_init(MachineState *machine)
     memory_region_init_ram(sram, NULL, "an5206.sram", 512, &error_fatal);
     memory_region_add_subregion(address_space_mem, AN5206_RAMBAR_ADDR, sram);
 
-    mcf5206_init(address_space_mem, AN5206_MBAR_ADDR);
+    mcf5206_init(cpu, address_space_mem, AN5206_MBAR_ADDR);
 
     /* Load kernel.  */
     if (!kernel_filename) {
diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
index 2ab1b4f059..f920ca2ceb 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -10,6 +10,7 @@
 #include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "cpu.h"
+#include "hw/qdev-properties.h"
 #include "hw/boards.h"
 #include "hw/irq.h"
 #include "hw/m68k/mcf.h"
@@ -601,13 +602,19 @@ static void mcf5206_mbar_realize(DeviceState *dev, Error **errp)
     s->timer[1] = m5206_timer_init(s->pic[10]);
     s->uart[0] = mcf_uart_init(s->pic[12], serial_hd(0));
     s->uart[1] = mcf_uart_init(s->pic[13], serial_hd(1));
-    s->cpu = M68K_CPU(qemu_get_cpu(0));
 }
 
+static Property mcf5206_mbar_properties[] = {
+    DEFINE_PROP_LINK("m68k-cpu", m5206_mbar_state, cpu,
+                     TYPE_M68K_CPU, M68kCPU *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void mcf5206_mbar_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
+    device_class_set_props(dc, mcf5206_mbar_properties);
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->desc = "MCF5206 system integration module";
     dc->realize = mcf5206_mbar_realize;
-- 
2.41.0

[PATCH v2 3/6] hw/m68k/mcf_intc: Expose MMIO region via SysBus API

[Philippe Mathieu-Daudé]

QOM objects shouldn't access each other internals fields
except using the QOM API.

Here the caller of mcf_intc_init() access the MMIO region from
the MCF_INTC state. Avoid that by exposing that region via
sysbus_init_mmio(), then get it with sysbus_mmio_get_region().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Thomas Huth <huth@tuxfamily.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/mcf_intc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
index 4cd30188c0..1f74ea0e14 100644
--- a/hw/m68k/mcf_intc.c
+++ b/hw/m68k/mcf_intc.c
@@ -173,6 +173,7 @@ static void mcf_intc_instance_init(Object *obj)
     mcf_intc_state *s = MCF_INTC(obj);
 
     memory_region_init_io(&s->iomem, obj, &mcf_intc_ops, s, "mcf", 0x100);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
 }
 
 static void mcf_intc_class_init(ObjectClass *oc, void *data)
@@ -211,7 +212,8 @@ qemu_irq *mcf_intc_init(MemoryRegion *sysmem,
     s = MCF_INTC(dev);
     s->cpu = cpu;
 
-    memory_region_add_subregion(sysmem, base, &s->iomem);
+    memory_region_add_subregion(sysmem, base,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
 
     return qemu_allocate_irqs(mcf_intc_set_irq, s, 64);
 }
-- 
2.41.0

[PATCH v2 4/6] hw/m68k/mcf_intc: Pass CPU using QOM link property

[Philippe Mathieu-Daudé]

QOM objects shouldn't access each other internals fields
except using the QOM API.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Thomas Huth <huth@tuxfamily.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/mcf_intc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/m68k/mcf_intc.c b/hw/m68k/mcf_intc.c
index 1f74ea0e14..1d3b34e18c 100644
--- a/hw/m68k/mcf_intc.c
+++ b/hw/m68k/mcf_intc.c
@@ -14,6 +14,7 @@
 #include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "hw/m68k/mcf.h"
+#include "hw/qdev-properties.h"
 #include "qom/object.h"
 
 #define TYPE_MCF_INTC "mcf-intc"
@@ -176,10 +177,17 @@ static void mcf_intc_instance_init(Object *obj)
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
 }
 
+static Property mcf_intc_properties[] = {
+    DEFINE_PROP_LINK("m68k-cpu", mcf_intc_state, cpu,
+                     TYPE_M68K_CPU, M68kCPU *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void mcf_intc_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
+    device_class_set_props(dc, mcf_intc_properties);
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->reset = mcf_intc_reset;
 }
@@ -204,16 +212,13 @@ qemu_irq *mcf_intc_init(MemoryRegion *sysmem,
                         M68kCPU *cpu)
 {
     DeviceState  *dev;
-    mcf_intc_state *s;
 
     dev = qdev_new(TYPE_MCF_INTC);
+    object_property_set_link(OBJECT(dev), "m68k-cpu",
+                             OBJECT(cpu), &error_abort);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-
-    s = MCF_INTC(dev);
-    s->cpu = cpu;
-
     memory_region_add_subregion(sysmem, base,
                                 sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
 
-    return qemu_allocate_irqs(mcf_intc_set_irq, s, 64);
+    return qemu_allocate_irqs(mcf_intc_set_irq, dev, 64);
 }
-- 
2.41.0

[PATCH v2 5/6] hw/m68k/next-cube: Do not open-code sysbus_create_simple()

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

  @@
  identifier dev;
  identifier sbd;
  expression qom_type;
  expression addr;
  @@
  -    dev = qdev_new(qom_type);
  -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
  -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
  +    dev = sysbus_create_simple(qom_type, addr, NULL);

then manually removing the 'dev' variable to avoid:

  error: variable 'dev' set but not used [-Werror,-Wunused-but-set-variable]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/next-cube.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 5d244b3b95..d17e6be8e1 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -950,7 +950,6 @@ static void next_cube_init(MachineState *machine)
     MemoryRegion *bmapm2 = g_new(MemoryRegion, 1);
     MemoryRegion *sysmem = get_system_memory();
     const char *bios_name = machine->firmware ?: ROM_FILE;
-    DeviceState *dev;
     DeviceState *pcdev;
 
     /* Initialize the cpu core */
@@ -974,9 +973,7 @@ static void next_cube_init(MachineState *machine)
     memory_region_add_subregion(sysmem, 0x04000000, machine->ram);
 
     /* Framebuffer */
-    dev = qdev_new(TYPE_NEXTFB);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x0B000000);
+    sysbus_create_simple(TYPE_NEXTFB, 0x0B000000, NULL);
 
     /* MMIO */
     sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 0, 0x02000000);
@@ -993,9 +990,7 @@ static void next_cube_init(MachineState *machine)
     memory_region_add_subregion(sysmem, 0x820c0000, bmapm2);
 
     /* KBD */
-    dev = qdev_new(TYPE_NEXTKBD);
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x0200e000);
+    sysbus_create_simple(TYPE_NEXTKBD, 0x0200e000, NULL);
 
     /* Load ROM here */
     /* still not sure if the rom should also be mapped at 0x0*/
-- 
2.41.0

[PATCH v2 6/6] hw/m68k/virt: Do not open-code sysbus_create_simple()

[Philippe Mathieu-Daudé]

Mechanical change using the following coccinelle script:

  @@
  identifier dev;
  expression qom_type;
  expression addr;
  expression irq;
  @@
  -    dev = qdev_new(qom_type);
  -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
  -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
  -    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, irq);
  +    dev = sysbus_create_simple(qom_type, addr, irq);

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Thomas Huth <huth@tuxfamily.org>
---
 hw/m68k/virt.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
index e7dc188855..2e49e262ee 100644
--- a/hw/m68k/virt.c
+++ b/hw/m68k/virt.c
@@ -201,11 +201,8 @@ static void virt_init(MachineState *machine)
     sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_GF_TTY_IRQ_BASE));
 
     /* virt controller */
-    dev = qdev_new(TYPE_VIRT_CTRL);
-    sysbus = SYS_BUS_DEVICE(dev);
-    sysbus_realize_and_unref(sysbus, &error_fatal);
-    sysbus_mmio_map(sysbus, 0, VIRT_CTRL_MMIO_BASE);
-    sysbus_connect_irq(sysbus, 0, PIC_GPIO(VIRT_CTRL_IRQ_BASE));
+    dev = sysbus_create_simple(TYPE_VIRT_CTRL, VIRT_CTRL_MMIO_BASE,
+                               PIC_GPIO(VIRT_CTRL_IRQ_BASE));
 
     /* virtio-mmio */
     io_base = VIRT_VIRTIO_MMIO_BASE;
-- 
2.41.0