[PATCH 00/11] hw/m68k/next-cube: refactor to fix Coverity issue

[PATCH 00/11] hw/m68k/next-cube: refactor to fix Coverity issue

[Peter Maydell]

Currently the next-cube board code open-codes a lot of handling of
interrupts and some miscellaneous registers. This series refactors
that to move most of it into a proper QOM device. (The main motivation
here is to fix the Coverity issue CID 1421962 which is a trivial
memory leak of the memory returned by qemu_allocate_irqs().)
    
In the real hardware this functionality seems to be the responsibility
of the Peripheral Controller (PC) chip, so we name the device that.

To ease review, I structured the change as a sequence of commits:
the first commit creates the skeleton of the NeXTPC device with no
content, but with a backdoor pointer to the NeXTState machine's state
struct. That allows subsequent commits to move parts of the code and
still have refactored and non-refactored code using the same struct
data fields.  Eventually all the code and data fields can be
transferred to the new device, and we remove the backdoor pointer.

Tested with make check and make check-acceptance (which does have
a boot test for the board.)

thanks
-- PMM

Peter Maydell (11):
  hw/m68k/next-cube: Make next_irq() function static
  hw/m68k/next-cube: Move register/interrupt functionality into a device
  hw/m68k/next-cube: Move mmio_ops into NeXTPC device
  hw/m68k/next-cube: Move scr_ops into NeXTPC device
  hw/m68k/next-cube: Make next_irq take NeXTPC* as its opaque
  hw/m68k/next-cube: Move int_status and int_mask to NeXTPC struct
  hw/m68k/next-cube: Make next_irq GPIO inputs to NEXT_PC device
  hw/m68k/next-cube: Move rtc into NeXTPC struct
  hw/m68k/next-cube: Remove unused fields from NeXTState
  hw/m68k/next-cube: Add vmstate for NeXTPC device
  hw/m68k/next-cube: Add missing header comment to next-cube.h

 include/hw/m68k/next-cube.h |  15 ++-
 hw/m68k/next-cube.c         | 238 +++++++++++++++++++++++-------------
 2 files changed, 168 insertions(+), 85 deletions(-)

-- 
2.20.1

[PATCH 01/11] hw/m68k/next-cube: Make next_irq() function static

[Peter Maydell]

The next_irq() function is global, but isn't actually used anywhere
outside next-cube.c. Make it static.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/m68k/next-cube.h | 2 --
 hw/m68k/next-cube.c         | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/hw/m68k/next-cube.h b/include/hw/m68k/next-cube.h
index a3be2b32abb..5a56c354b8e 100644
--- a/include/hw/m68k/next-cube.h
+++ b/include/hw/m68k/next-cube.h
@@ -42,6 +42,4 @@ enum next_irqs {
     NEXT_SND_I
 };
 
-void next_irq(void *opaque, int number, int level);
-
 #endif /* NEXT_CUBE_H */
diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 37bc35dfa43..f622d6589c8 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -724,7 +724,7 @@ static const MemoryRegionOps dma_ops = {
  * TODO: set the shift numbers as values in the enum, so the first switch
  * will not be needed
  */
-void next_irq(void *opaque, int number, int level)
+static void next_irq(void *opaque, int number, int level)
 {
     M68kCPU *cpu = opaque;
     int shift = 0;
-- 
2.20.1

[PATCH 02/11] hw/m68k/next-cube: Move register/interrupt functionality into a device

[Peter Maydell]

Currently the next-cube board code open-codes a lot of handling of
interrupts and some miscellaneous registers.  Move this into a proper
QOM device.

In the real hardware this functionality seems to be the
responsibility of the Peripheral Controller (PC) chip, so name the
device that.

There are several different things that will need to be moved into
this device:
 * the mmio_iops register set
 * the scr_ops register set
 * the next_irq IRQ handling

To ease review, we structure the change as a sequence of commits: in
this first commit we create the skeleton of the NeXTPC device with no
content, but with a backdoor pointer to the NeXTState machine's state
struct so we can move parts of the code and still have refactored and
non-refactored code using the same struct data fields.  Further
commits will move functionality into the new device piece by piece.
At the end we will be able to remove the backdoor pointer because all
the data fields will be in the NeXTPC struct and not the NeXTState
struct.

We'll add the VMState for the new device at the end of all that; this
is in theory a migration compatibility break but this machine does
not currently support migration at all anyway.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index f622d6589c8..dccf3eb4313 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -90,6 +90,16 @@ struct NeXTState {
     NextRtc rtc;
 };
 
+#define TYPE_NEXT_PC "next-pc"
+OBJECT_DECLARE_SIMPLE_TYPE(NeXTPC, NEXT_PC)
+
+struct NeXTPC {
+    SysBusDevice parent_obj;
+
+    /* Temporary until all functionality has been moved into this device */
+    NeXTState *ns;
+};
+
 /* Thanks to NeXT forums for this */
 /*
 static const uint8_t rtc_ram3[32] = {
@@ -857,6 +867,31 @@ static void next_escc_init(M68kCPU *cpu)
     sysbus_mmio_map(s, 0, 0x2118000);
 }
 
+static void next_pc_reset(DeviceState *dev)
+{
+}
+
+static void next_pc_realize(DeviceState *dev, Error **errp)
+{
+}
+
+static void next_pc_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->desc = "NeXT Peripheral Controller";
+    dc->realize = next_pc_realize;
+    dc->reset = next_pc_reset;
+    /* We will add the VMState in a later commit */
+}
+
+static const TypeInfo next_pc_info = {
+    .name = TYPE_NEXT_PC,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(NeXTPC),
+    .class_init = next_pc_class_init,
+};
+
 static void next_cube_init(MachineState *machine)
 {
     M68kCPU *cpu;
@@ -871,6 +906,7 @@ static void next_cube_init(MachineState *machine)
     const char *bios_name = machine->firmware ?: ROM_FILE;
     NeXTState *ns = NEXT_MACHINE(machine);
     DeviceState *dev;
+    DeviceState *pcdev;
 
     /* Initialize the cpu core */
     cpu = M68K_CPU(cpu_create(machine->cpu_type));
@@ -884,6 +920,12 @@ static void next_cube_init(MachineState *machine)
     env->vbr = 0;
     env->sr  = 0x2700;
 
+    /* Peripheral Controller */
+    pcdev = qdev_new(TYPE_NEXT_PC);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(pcdev), &error_fatal);
+    /* Temporary while we refactor this code */
+    NEXT_PC(pcdev)->ns = ns;
+
     /* Set internal registers to initial values */
     /*     0x0000XX00 << vital bits */
     ns->scr1 = 0x00011102;
@@ -978,6 +1020,7 @@ static const TypeInfo next_typeinfo = {
 static void next_register_type(void)
 {
     type_register_static(&next_typeinfo);
+    type_register_static(&next_pc_info);
 }
 
 type_init(next_register_type)
-- 
2.20.1

[PATCH 03/11] hw/m68k/next-cube: Move mmio_ops into NeXTPC device

[Peter Maydell]

Move the registers handled by the mmio_ops struct into the NeXTPC
device.  This allows us to also move the scr1 and scr2 data fields.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 80 +++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 36 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index dccf3eb4313..ff121143e92 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -84,9 +84,6 @@ struct NeXTState {
     qemu_irq scsi_reset;
     qemu_irq *fd_irq;
 
-    uint32_t scr1;
-    uint32_t scr2;
-
     NextRtc rtc;
 };
 
@@ -98,6 +95,11 @@ struct NeXTPC {
 
     /* Temporary until all functionality has been moved into this device */
     NeXTState *ns;
+
+    MemoryRegion mmiomem;
+
+    uint32_t scr1;
+    uint32_t scr2;
 };
 
 /* Thanks to NeXT forums for this */
@@ -120,13 +122,13 @@ static const uint8_t rtc_ram2[32] = {
 #define SCR2_RTDATA 0x4
 #define SCR2_TOBCD(x) (((x / 10) << 4) + (x % 10))
 
-static void nextscr2_write(NeXTState *s, uint32_t val, int size)
+static void nextscr2_write(NeXTPC *s, uint32_t val, int size)
 {
     static int led;
     static int phase;
     static uint8_t old_scr2;
     uint8_t scr2_2;
-    NextRtc *rtc = &s->rtc;
+    NextRtc *rtc = &s->ns->rtc;
 
     if (size == 4) {
         scr2_2 = (val >> 8) & 0xFF;
@@ -238,7 +240,7 @@ static void nextscr2_write(NeXTState *s, uint32_t val, int size)
                     /* clear FTU */
                     if (rtc->value & 0x04) {
                         rtc->status = rtc->status & (~0x18);
-                        s->int_status = s->int_status & (~0x04);
+                        s->ns->int_status = s->ns->int_status & (~0x04);
                     }
                 }
             }
@@ -254,7 +256,7 @@ static void nextscr2_write(NeXTState *s, uint32_t val, int size)
     old_scr2 = scr2_2;
 }
 
-static uint32_t mmio_readb(NeXTState *s, hwaddr addr)
+static uint32_t mmio_readb(NeXTPC *s, hwaddr addr)
 {
     switch (addr) {
     case 0xc000:
@@ -284,7 +286,7 @@ static uint32_t mmio_readb(NeXTState *s, hwaddr addr)
     }
 }
 
-static uint32_t mmio_readw(NeXTState *s, hwaddr addr)
+static uint32_t mmio_readw(NeXTPC *s, hwaddr addr)
 {
     switch (addr) {
     default:
@@ -293,16 +295,16 @@ static uint32_t mmio_readw(NeXTState *s, hwaddr addr)
     }
 }
 
-static uint32_t mmio_readl(NeXTState *s, hwaddr addr)
+static uint32_t mmio_readl(NeXTPC *s, hwaddr addr)
 {
     switch (addr) {
     case 0x7000:
-        /* DPRINTF("Read INT status: %x\n", s->int_status); */
-        return s->int_status;
+        /* DPRINTF("Read INT status: %x\n", s->ns->int_status); */
+        return s->ns->int_status;
 
     case 0x7800:
-        DPRINTF("MMIO Read INT mask: %x\n", s->int_mask);
-        return s->int_mask;
+        DPRINTF("MMIO Read INT mask: %x\n", s->ns->int_mask);
+        return s->ns->int_mask;
 
     case 0xc000:
         return s->scr1;
@@ -316,7 +318,7 @@ static uint32_t mmio_readl(NeXTState *s, hwaddr addr)
     }
 }
 
-static void mmio_writeb(NeXTState *s, hwaddr addr, uint32_t val)
+static void mmio_writeb(NeXTPC *s, hwaddr addr, uint32_t val)
 {
     switch (addr) {
     case 0xd003:
@@ -328,21 +330,21 @@ static void mmio_writeb(NeXTState *s, hwaddr addr, uint32_t val)
 
 }
 
-static void mmio_writew(NeXTState *s, hwaddr addr, uint32_t val)
+static void mmio_writew(NeXTPC *s, hwaddr addr, uint32_t val)
 {
     DPRINTF("MMIO Write W\n");
 }
 
-static void mmio_writel(NeXTState *s, hwaddr addr, uint32_t val)
+static void mmio_writel(NeXTPC *s, hwaddr addr, uint32_t val)
 {
     switch (addr) {
     case 0x7000:
-        DPRINTF("INT Status old: %x new: %x\n", s->int_status, val);
-        s->int_status = val;
+        DPRINTF("INT Status old: %x new: %x\n", s->ns->int_status, val);
+        s->ns->int_status = val;
         break;
     case 0x7800:
-        DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, val);
-        s->int_mask  = val;
+        DPRINTF("INT Mask old: %x new: %x\n", s->ns->int_mask, val);
+        s->ns->int_mask  = val;
         break;
     case 0xc000:
         DPRINTF("SCR1 Write: %x\n", val);
@@ -358,15 +360,15 @@ static void mmio_writel(NeXTState *s, hwaddr addr, uint32_t val)
 
 static uint64_t mmio_readfn(void *opaque, hwaddr addr, unsigned size)
 {
-    NeXTState *ns = NEXT_MACHINE(opaque);
+    NeXTPC *s = NEXT_PC(opaque);
 
     switch (size) {
     case 1:
-        return mmio_readb(ns, addr);
+        return mmio_readb(s, addr);
     case 2:
-        return mmio_readw(ns, addr);
+        return mmio_readw(s, addr);
     case 4:
-        return mmio_readl(ns, addr);
+        return mmio_readl(s, addr);
     default:
         g_assert_not_reached();
     }
@@ -375,17 +377,17 @@ static uint64_t mmio_readfn(void *opaque, hwaddr addr, unsigned size)
 static void mmio_writefn(void *opaque, hwaddr addr, uint64_t value,
                          unsigned size)
 {
-    NeXTState *ns = NEXT_MACHINE(opaque);
+    NeXTPC *s = NEXT_PC(opaque);
 
     switch (size) {
     case 1:
-        mmio_writeb(ns, addr, value);
+        mmio_writeb(s, addr, value);
         break;
     case 2:
-        mmio_writew(ns, addr, value);
+        mmio_writew(s, addr, value);
         break;
     case 4:
-        mmio_writel(ns, addr, value);
+        mmio_writel(s, addr, value);
         break;
     default:
         g_assert_not_reached();
@@ -869,10 +871,23 @@ static void next_escc_init(M68kCPU *cpu)
 
 static void next_pc_reset(DeviceState *dev)
 {
+    NeXTPC *s = NEXT_PC(dev);
+
+    /* Set internal registers to initial values */
+    /*     0x0000XX00 << vital bits */
+    s->scr1 = 0x00011102;
+    s->scr2 = 0x00ff0c80;
 }
 
 static void next_pc_realize(DeviceState *dev, Error **errp)
 {
+    NeXTPC *s = NEXT_PC(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    memory_region_init_io(&s->mmiomem, OBJECT(s), &mmio_ops, s,
+                          "next.mmio", 0xD0000);
+
+    sysbus_init_mmio(sbd, &s->mmiomem);
 }
 
 static void next_pc_class_init(ObjectClass *klass, void *data)
@@ -897,7 +912,6 @@ static void next_cube_init(MachineState *machine)
     M68kCPU *cpu;
     CPUM68KState *env;
     MemoryRegion *rom = g_new(MemoryRegion, 1);
-    MemoryRegion *mmiomem = g_new(MemoryRegion, 1);
     MemoryRegion *scrmem = g_new(MemoryRegion, 1);
     MemoryRegion *dmamem = g_new(MemoryRegion, 1);
     MemoryRegion *bmapm1 = g_new(MemoryRegion, 1);
@@ -926,10 +940,6 @@ static void next_cube_init(MachineState *machine)
     /* Temporary while we refactor this code */
     NEXT_PC(pcdev)->ns = ns;
 
-    /* Set internal registers to initial values */
-    /*     0x0000XX00 << vital bits */
-    ns->scr1 = 0x00011102;
-    ns->scr2 = 0x00ff0c80;
     ns->rtc.status = 0x90;
 
     /* Load RTC RAM - TODO: provide possibility to load contents from file */
@@ -944,9 +954,7 @@ static void next_cube_init(MachineState *machine)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0x0B000000);
 
     /* MMIO */
-    memory_region_init_io(mmiomem, NULL, &mmio_ops, machine, "next.mmio",
-                          0xD0000);
-    memory_region_add_subregion(sysmem, 0x02000000, mmiomem);
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 0, 0x02000000);
 
     /* BMAP memory */
     memory_region_init_ram_shared_nomigrate(bmapm1, NULL, "next.bmapmem", 64,
-- 
2.20.1

[PATCH 04/11] hw/m68k/next-cube: Move scr_ops into NeXTPC device

[Peter Maydell]

Move the registers handled by the scr_ops struct into the NeXTPC
device.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 50 ++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index ff121143e92..f5575cb43b8 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -76,8 +76,6 @@ struct NeXTState {
     uint32_t int_mask;
     uint32_t int_status;
 
-    uint8_t scsi_csr_1;
-    uint8_t scsi_csr_2;
     next_dma dma[10];
     qemu_irq *scsi_irq;
     qemu_irq scsi_dma;
@@ -97,9 +95,12 @@ struct NeXTPC {
     NeXTState *ns;
 
     MemoryRegion mmiomem;
+    MemoryRegion scrmem;
 
     uint32_t scr1;
     uint32_t scr2;
+    uint8_t scsi_csr_1;
+    uint8_t scsi_csr_2;
 };
 
 /* Thanks to NeXT forums for this */
@@ -402,7 +403,7 @@ static const MemoryRegionOps mmio_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static uint32_t scr_readb(NeXTState *s, hwaddr addr)
+static uint32_t scr_readb(NeXTPC *s, hwaddr addr)
 {
     switch (addr) {
     case 0x14108:
@@ -436,13 +437,13 @@ static uint32_t scr_readb(NeXTState *s, hwaddr addr)
     }
 }
 
-static uint32_t scr_readw(NeXTState *s, hwaddr addr)
+static uint32_t scr_readw(NeXTPC *s, hwaddr addr)
 {
     DPRINTF("BMAP Read W @ %x\n", (unsigned int)addr);
     return 0;
 }
 
-static uint32_t scr_readl(NeXTState *s, hwaddr addr)
+static uint32_t scr_readl(NeXTPC *s, hwaddr addr)
 {
     DPRINTF("BMAP Read L @ %x\n", (unsigned int)addr);
     return 0;
@@ -455,7 +456,7 @@ static uint32_t scr_readl(NeXTState *s, hwaddr addr)
 #define SCSICSR_CPUDMA  0x10  /* if set, dma enabled */
 #define SCSICSR_INTMASK 0x20  /* if set, interrupt enabled */
 
-static void scr_writeb(NeXTState *s, hwaddr addr, uint32_t value)
+static void scr_writeb(NeXTPC *s, hwaddr addr, uint32_t value)
 {
     switch (addr) {
     case 0x14108:
@@ -501,9 +502,9 @@ static void scr_writeb(NeXTState *s, hwaddr addr, uint32_t value)
             DPRINTF("SCSICSR CPUDMA\n");
             /* qemu_irq_raise(s->scsi_dma); */
 
-            s->int_status |= 0x4000000;
+            s->ns->int_status |= 0x4000000;
         } else {
-            s->int_status &= ~(0x4000000);
+            s->ns->int_status &= ~(0x4000000);
         }
         if (value & SCSICSR_INTMASK) {
             DPRINTF("SCSICSR INTMASK\n");
@@ -533,27 +534,27 @@ static void scr_writeb(NeXTState *s, hwaddr addr, uint32_t value)
     }
 }
 
-static void scr_writew(NeXTState *s, hwaddr addr, uint32_t value)
+static void scr_writew(NeXTPC *s, hwaddr addr, uint32_t value)
 {
     DPRINTF("BMAP Write W @ %x with %x\n", (unsigned int)addr, value);
 }
 
-static void scr_writel(NeXTState *s, hwaddr addr, uint32_t value)
+static void scr_writel(NeXTPC *s, hwaddr addr, uint32_t value)
 {
     DPRINTF("BMAP Write L @ %x with %x\n", (unsigned int)addr, value);
 }
 
 static uint64_t scr_readfn(void *opaque, hwaddr addr, unsigned size)
 {
-    NeXTState *ns = NEXT_MACHINE(opaque);
+    NeXTPC *s = NEXT_PC(opaque);
 
     switch (size) {
     case 1:
-        return scr_readb(ns, addr);
+        return scr_readb(s, addr);
     case 2:
-        return scr_readw(ns, addr);
+        return scr_readw(s, addr);
     case 4:
-        return scr_readl(ns, addr);
+        return scr_readl(s, addr);
     default:
         g_assert_not_reached();
     }
@@ -562,17 +563,17 @@ static uint64_t scr_readfn(void *opaque, hwaddr addr, unsigned size)
 static void scr_writefn(void *opaque, hwaddr addr, uint64_t value,
                         unsigned size)
 {
-    NeXTState *ns = NEXT_MACHINE(opaque);
+    NeXTPC *s = NEXT_PC(opaque);
 
     switch (size) {
     case 1:
-        scr_writeb(ns, addr, value);
+        scr_writeb(s, addr, value);
         break;
     case 2:
-        scr_writew(ns, addr, value);
+        scr_writew(s, addr, value);
         break;
     case 4:
-        scr_writel(ns, addr, value);
+        scr_writel(s, addr, value);
         break;
     default:
         g_assert_not_reached();
@@ -886,8 +887,10 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
 
     memory_region_init_io(&s->mmiomem, OBJECT(s), &mmio_ops, s,
                           "next.mmio", 0xD0000);
-
+    memory_region_init_io(&s->scrmem, OBJECT(s), &scr_ops, s,
+                          "next.scr", 0x20000);
     sysbus_init_mmio(sbd, &s->mmiomem);
+    sysbus_init_mmio(sbd, &s->scrmem);
 }
 
 static void next_pc_class_init(ObjectClass *klass, void *data)
@@ -912,7 +915,6 @@ static void next_cube_init(MachineState *machine)
     M68kCPU *cpu;
     CPUM68KState *env;
     MemoryRegion *rom = g_new(MemoryRegion, 1);
-    MemoryRegion *scrmem = g_new(MemoryRegion, 1);
     MemoryRegion *dmamem = g_new(MemoryRegion, 1);
     MemoryRegion *bmapm1 = g_new(MemoryRegion, 1);
     MemoryRegion *bmapm2 = g_new(MemoryRegion, 1);
@@ -956,6 +958,9 @@ static void next_cube_init(MachineState *machine)
     /* MMIO */
     sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 0, 0x02000000);
 
+    /* BMAP IO - acts as a catch-all for now */
+    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02100000);
+
     /* BMAP memory */
     memory_region_init_ram_shared_nomigrate(bmapm1, NULL, "next.bmapmem", 64,
                                             true, &error_fatal);
@@ -964,11 +969,6 @@ static void next_cube_init(MachineState *machine)
     memory_region_init_alias(bmapm2, NULL, "next.bmapmem2", bmapm1, 0x0, 64);
     memory_region_add_subregion(sysmem, 0x820c0000, bmapm2);
 
-    /* BMAP IO - acts as a catch-all for now */
-    memory_region_init_io(scrmem, NULL, &scr_ops, machine, "next.scr",
-                          0x20000);
-    memory_region_add_subregion(sysmem, 0x02100000, scrmem);
-
     /* KBD */
     dev = qdev_new(TYPE_NEXTKBD);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-- 
2.20.1

[PATCH 05/11] hw/m68k/next-cube: Make next_irq take NeXTPC* as its opaque

[Peter Maydell]

Make the next_irq function take a NeXTPC* as its opaque rather than
the M68kCPU*.  This will make it simpler to turn the next_irq
function into a gpio input line of the NeXTPC device in the next
commit.

For this to work we have to pass the CPU to the NeXTPC device via a
link property, in the same way we do in q800.c (and for the same
reason).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index f5575cb43b8..a9e57304e04 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -94,6 +94,8 @@ struct NeXTPC {
     /* Temporary until all functionality has been moved into this device */
     NeXTState *ns;
 
+    M68kCPU *cpu;
+
     MemoryRegion mmiomem;
     MemoryRegion scrmem;
 
@@ -739,9 +741,9 @@ static const MemoryRegionOps dma_ops = {
  */
 static void next_irq(void *opaque, int number, int level)
 {
-    M68kCPU *cpu = opaque;
+    NeXTPC *s = NEXT_PC(opaque);
+    M68kCPU *cpu = s->cpu;
     int shift = 0;
-    NeXTState *ns = NEXT_MACHINE(qdev_get_machine());
 
     /* first switch sets interupt status */
     /* DPRINTF("IRQ %i\n",number); */
@@ -796,14 +798,14 @@ static void next_irq(void *opaque, int number, int level)
      * this HAS to be wrong, the interrupt handlers in mach and together
      * int_status and int_mask and return if there is a hit
      */
-    if (ns->int_mask & (1 << shift)) {
+    if (s->ns->int_mask & (1 << shift)) {
         DPRINTF("%x interrupt masked @ %x\n", 1 << shift, cpu->env.pc);
         /* return; */
     }
 
     /* second switch triggers the correct interrupt */
     if (level) {
-        ns->int_status |= 1 << shift;
+        s->ns->int_status |= 1 << shift;
 
         switch (number) {
         /* level 3 - floppy, kbd/mouse, power, ether rx/tx, scsi, clock */
@@ -832,7 +834,7 @@ static void next_irq(void *opaque, int number, int level)
             break;
         }
     } else {
-        ns->int_status &= ~(1 << shift);
+        s->ns->int_status &= ~(1 << shift);
         cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
     }
 }
@@ -847,9 +849,9 @@ static void next_serial_irq(void *opaque, int n, int level)
     }
 }
 
-static void next_escc_init(M68kCPU *cpu)
+static void next_escc_init(DeviceState *pcdev)
 {
-    qemu_irq *ser_irq = qemu_allocate_irqs(next_serial_irq, cpu, 2);
+    qemu_irq *ser_irq = qemu_allocate_irqs(next_serial_irq, pcdev, 2);
     DeviceState *dev;
     SysBusDevice *s;
 
@@ -893,6 +895,17 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->scrmem);
 }
 
+/*
+ * If the m68k CPU implemented its inbound irq lines as GPIO lines
+ * rather than via the m68k_set_irq_level() function we would not need
+ * this cpu link property and could instead provide outbound IRQ lines
+ * that the board could wire up to the CPU.
+ */
+static Property next_pc_properties[] = {
+    DEFINE_PROP_LINK("cpu", NeXTPC, cpu, TYPE_M68K_CPU, M68kCPU *),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void next_pc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -900,6 +913,7 @@ static void next_pc_class_init(ObjectClass *klass, void *data)
     dc->desc = "NeXT Peripheral Controller";
     dc->realize = next_pc_realize;
     dc->reset = next_pc_reset;
+    device_class_set_props(dc, next_pc_properties);
     /* We will add the VMState in a later commit */
 }
 
@@ -938,6 +952,7 @@ static void next_cube_init(MachineState *machine)
 
     /* Peripheral Controller */
     pcdev = qdev_new(TYPE_NEXT_PC);
+    object_property_set_link(OBJECT(pcdev), "cpu", OBJECT(cpu), &error_abort);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(pcdev), &error_fatal);
     /* Temporary while we refactor this code */
     NEXT_PC(pcdev)->ns = ns;
@@ -996,7 +1011,7 @@ static void next_cube_init(MachineState *machine)
     }
 
     /* Serial */
-    next_escc_init(cpu);
+    next_escc_init(pcdev);
 
     /* TODO: */
     /* Network */
-- 
2.20.1

[PATCH 06/11] hw/m68k/next-cube: Move int_status and int_mask to NeXTPC struct

[Peter Maydell]

All the code which accesses int_status and int_mask is now doing
so via the NeXTPC->NeXTState indirection, so we can move these
fields into the NeXTPC struct where they belong.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index a9e57304e04..6b4bcfd4b9b 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -73,9 +73,6 @@ typedef struct NextRtc {
 struct NeXTState {
     MachineState parent;
 
-    uint32_t int_mask;
-    uint32_t int_status;
-
     next_dma dma[10];
     qemu_irq *scsi_irq;
     qemu_irq scsi_dma;
@@ -103,6 +100,8 @@ struct NeXTPC {
     uint32_t scr2;
     uint8_t scsi_csr_1;
     uint8_t scsi_csr_2;
+    uint32_t int_mask;
+    uint32_t int_status;
 };
 
 /* Thanks to NeXT forums for this */
@@ -243,7 +242,7 @@ static void nextscr2_write(NeXTPC *s, uint32_t val, int size)
                     /* clear FTU */
                     if (rtc->value & 0x04) {
                         rtc->status = rtc->status & (~0x18);
-                        s->ns->int_status = s->ns->int_status & (~0x04);
+                        s->int_status = s->int_status & (~0x04);
                     }
                 }
             }
@@ -302,12 +301,12 @@ static uint32_t mmio_readl(NeXTPC *s, hwaddr addr)
 {
     switch (addr) {
     case 0x7000:
-        /* DPRINTF("Read INT status: %x\n", s->ns->int_status); */
-        return s->ns->int_status;
+        /* DPRINTF("Read INT status: %x\n", s->int_status); */
+        return s->int_status;
 
     case 0x7800:
-        DPRINTF("MMIO Read INT mask: %x\n", s->ns->int_mask);
-        return s->ns->int_mask;
+        DPRINTF("MMIO Read INT mask: %x\n", s->int_mask);
+        return s->int_mask;
 
     case 0xc000:
         return s->scr1;
@@ -342,12 +341,12 @@ static void mmio_writel(NeXTPC *s, hwaddr addr, uint32_t val)
 {
     switch (addr) {
     case 0x7000:
-        DPRINTF("INT Status old: %x new: %x\n", s->ns->int_status, val);
-        s->ns->int_status = val;
+        DPRINTF("INT Status old: %x new: %x\n", s->int_status, val);
+        s->int_status = val;
         break;
     case 0x7800:
-        DPRINTF("INT Mask old: %x new: %x\n", s->ns->int_mask, val);
-        s->ns->int_mask  = val;
+        DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, val);
+        s->int_mask  = val;
         break;
     case 0xc000:
         DPRINTF("SCR1 Write: %x\n", val);
@@ -504,9 +503,9 @@ static void scr_writeb(NeXTPC *s, hwaddr addr, uint32_t value)
             DPRINTF("SCSICSR CPUDMA\n");
             /* qemu_irq_raise(s->scsi_dma); */
 
-            s->ns->int_status |= 0x4000000;
+            s->int_status |= 0x4000000;
         } else {
-            s->ns->int_status &= ~(0x4000000);
+            s->int_status &= ~(0x4000000);
         }
         if (value & SCSICSR_INTMASK) {
             DPRINTF("SCSICSR INTMASK\n");
@@ -798,14 +797,14 @@ static void next_irq(void *opaque, int number, int level)
      * this HAS to be wrong, the interrupt handlers in mach and together
      * int_status and int_mask and return if there is a hit
      */
-    if (s->ns->int_mask & (1 << shift)) {
+    if (s->int_mask & (1 << shift)) {
         DPRINTF("%x interrupt masked @ %x\n", 1 << shift, cpu->env.pc);
         /* return; */
     }
 
     /* second switch triggers the correct interrupt */
     if (level) {
-        s->ns->int_status |= 1 << shift;
+        s->int_status |= 1 << shift;
 
         switch (number) {
         /* level 3 - floppy, kbd/mouse, power, ether rx/tx, scsi, clock */
@@ -834,7 +833,7 @@ static void next_irq(void *opaque, int number, int level)
             break;
         }
     } else {
-        s->ns->int_status &= ~(1 << shift);
+        s->int_status &= ~(1 << shift);
         cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
     }
 }
-- 
2.20.1

[PATCH 07/11] hw/m68k/next-cube: Make next_irq GPIO inputs to NEXT_PC device

[Peter Maydell]

Make the next_irq function be GPIO inputs to the NEXT_PC
device, rather than a freestanding set of qemu_irq lines.

This fixes a minor Coverity issue where it correctly points
out the trivial memory leak of the memory allocated in the
call to qemu_allocate_irqs().

Fixes: CID 1421962
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/m68k/next-cube.h |  3 ++-
 hw/m68k/next-cube.c         | 21 ++++-----------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/include/hw/m68k/next-cube.h b/include/hw/m68k/next-cube.h
index 5a56c354b8e..d38c52d540d 100644
--- a/include/hw/m68k/next-cube.h
+++ b/include/hw/m68k/next-cube.h
@@ -39,7 +39,8 @@ enum next_irqs {
     NEXT_ENRX_DMA_I,
     NEXT_SCSI_DMA_I,
     NEXT_SCC_DMA_I,
-    NEXT_SND_I
+    NEXT_SND_I,
+    NEXT_NUM_IRQS
 };
 
 #endif /* NEXT_CUBE_H */
diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 6b4bcfd4b9b..5a8fc24ed35 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -734,10 +734,6 @@ static const MemoryRegionOps dma_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-/*
- * TODO: set the shift numbers as values in the enum, so the first switch
- * will not be needed
- */
 static void next_irq(void *opaque, int number, int level)
 {
     NeXTPC *s = NEXT_PC(opaque);
@@ -838,19 +834,8 @@ static void next_irq(void *opaque, int number, int level)
     }
 }
 
-static void next_serial_irq(void *opaque, int n, int level)
-{
-    /* DPRINTF("SCC IRQ NUM %i\n",n); */
-    if (n) {
-        next_irq(opaque, NEXT_SCC_DMA_I, level);
-    } else {
-        next_irq(opaque, NEXT_SCC_I, level);
-    }
-}
-
 static void next_escc_init(DeviceState *pcdev)
 {
-    qemu_irq *ser_irq = qemu_allocate_irqs(next_serial_irq, pcdev, 2);
     DeviceState *dev;
     SysBusDevice *s;
 
@@ -866,8 +851,8 @@ static void next_escc_init(DeviceState *pcdev)
 
     s = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(s, &error_fatal);
-    sysbus_connect_irq(s, 0, ser_irq[0]);
-    sysbus_connect_irq(s, 1,  ser_irq[1]);
+    sysbus_connect_irq(s, 0, qdev_get_gpio_in(pcdev, NEXT_SCC_I));
+    sysbus_connect_irq(s, 1, qdev_get_gpio_in(pcdev, NEXT_SCC_DMA_I));
     sysbus_mmio_map(s, 0, 0x2118000);
 }
 
@@ -886,6 +871,8 @@ static void next_pc_realize(DeviceState *dev, Error **errp)
     NeXTPC *s = NEXT_PC(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
+    qdev_init_gpio_in(dev, next_irq, NEXT_NUM_IRQS);
+
     memory_region_init_io(&s->mmiomem, OBJECT(s), &mmio_ops, s,
                           "next.mmio", 0xD0000);
     memory_region_init_io(&s->scrmem, OBJECT(s), &scr_ops, s,
-- 
2.20.1

[PATCH 08/11] hw/m68k/next-cube: Move rtc into NeXTPC struct

[Peter Maydell]

Move the rtc into the NeXTPC struct. Since this is the last
use of the 'backdoor' NextState pointer we can now remove that.

Probably the RTC should be its own device at some point: in hardware
there is a separate MCS1850 RTC chip connected to the Peripheral
Controller via a 1-bit serial interface.  That goes beyond the remit
of the current refactoring, though.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 5a8fc24ed35..3c83b874c56 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -78,8 +78,6 @@ struct NeXTState {
     qemu_irq scsi_dma;
     qemu_irq scsi_reset;
     qemu_irq *fd_irq;
-
-    NextRtc rtc;
 };
 
 #define TYPE_NEXT_PC "next-pc"
@@ -88,9 +86,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(NeXTPC, NEXT_PC)
 struct NeXTPC {
     SysBusDevice parent_obj;
 
-    /* Temporary until all functionality has been moved into this device */
-    NeXTState *ns;
-
     M68kCPU *cpu;
 
     MemoryRegion mmiomem;
@@ -102,6 +97,8 @@ struct NeXTPC {
     uint8_t scsi_csr_2;
     uint32_t int_mask;
     uint32_t int_status;
+
+    NextRtc rtc;
 };
 
 /* Thanks to NeXT forums for this */
@@ -130,7 +127,7 @@ static void nextscr2_write(NeXTPC *s, uint32_t val, int size)
     static int phase;
     static uint8_t old_scr2;
     uint8_t scr2_2;
-    NextRtc *rtc = &s->ns->rtc;
+    NextRtc *rtc = &s->rtc;
 
     if (size == 4) {
         scr2_2 = (val >> 8) & 0xFF;
@@ -864,6 +861,11 @@ static void next_pc_reset(DeviceState *dev)
     /*     0x0000XX00 << vital bits */
     s->scr1 = 0x00011102;
     s->scr2 = 0x00ff0c80;
+
+    s->rtc.status = 0x90;
+
+    /* Load RTC RAM - TODO: provide possibility to load contents from file */
+    memcpy(s->rtc.ram, rtc_ram2, 32);
 }
 
 static void next_pc_realize(DeviceState *dev, Error **errp)
@@ -920,7 +922,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;
-    NeXTState *ns = NEXT_MACHINE(machine);
     DeviceState *dev;
     DeviceState *pcdev;
 
@@ -940,13 +941,6 @@ static void next_cube_init(MachineState *machine)
     pcdev = qdev_new(TYPE_NEXT_PC);
     object_property_set_link(OBJECT(pcdev), "cpu", OBJECT(cpu), &error_abort);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(pcdev), &error_fatal);
-    /* Temporary while we refactor this code */
-    NEXT_PC(pcdev)->ns = ns;
-
-    ns->rtc.status = 0x90;
-
-    /* Load RTC RAM - TODO: provide possibility to load contents from file */
-    memcpy(ns->rtc.ram, rtc_ram2, 32);
 
     /* 64MB RAM starting at 0x04000000  */
     memory_region_add_subregion(sysmem, 0x04000000, machine->ram);
-- 
2.20.1

[PATCH 09/11] hw/m68k/next-cube: Remove unused fields from NeXTState

[Peter Maydell]

The fields scsi_irq, scsi_dma, scsi_reset and fd_irq in
NeXTState are all unused, except in commented out
"this should do something like this" code. Remove the
unused fields. As and when the functionality that might
use them is added, we can put in the correct kind of
wiring (which might or might not need to be a qemu_irq,
but which in any case will need to be in the NeXTPC
device, not in NeXTState).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 3c83b874c56..dd0a2a5aea0 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -74,10 +74,6 @@ struct NeXTState {
     MachineState parent;
 
     next_dma dma[10];
-    qemu_irq *scsi_irq;
-    qemu_irq scsi_dma;
-    qemu_irq scsi_reset;
-    qemu_irq *fd_irq;
 };
 
 #define TYPE_NEXT_PC "next-pc"
-- 
2.20.1

[PATCH 10/11] hw/m68k/next-cube: Add vmstate for NeXTPC device

[Peter Maydell]

Add the vmstate for the new NeXTPC devic; this is in theory
a migration compatibility break, but this machine doesn't have
working migration currently anyway.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/m68k/next-cube.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index dd0a2a5aea0..9eb1e31752b 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -28,6 +28,7 @@
 #include "qapi/error.h"
 #include "ui/console.h"
 #include "target/m68k/cpu.h"
+#include "migration/vmstate.h"
 
 /* #define DEBUG_NEXT */
 #ifdef DEBUG_NEXT
@@ -890,6 +891,37 @@ static Property next_pc_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static const VMStateDescription next_rtc_vmstate = {
+    .name = "next-rtc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(ram, NextRtc, 32),
+        VMSTATE_UINT8(command, NextRtc),
+        VMSTATE_UINT8(value, NextRtc),
+        VMSTATE_UINT8(status, NextRtc),
+        VMSTATE_UINT8(control, NextRtc),
+        VMSTATE_UINT8(retval, NextRtc),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static const VMStateDescription next_pc_vmstate = {
+    .name = "next-pc",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(scr1, NeXTPC),
+        VMSTATE_UINT32(scr2, NeXTPC),
+        VMSTATE_UINT32(int_mask, NeXTPC),
+        VMSTATE_UINT32(int_status, NeXTPC),
+        VMSTATE_UINT8(scsi_csr_1, NeXTPC),
+        VMSTATE_UINT8(scsi_csr_2, NeXTPC),
+        VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NextRtc),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static void next_pc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -898,7 +930,7 @@ static void next_pc_class_init(ObjectClass *klass, void *data)
     dc->realize = next_pc_realize;
     dc->reset = next_pc_reset;
     device_class_set_props(dc, next_pc_properties);
-    /* We will add the VMState in a later commit */
+    dc->vmsd = &next_pc_vmstate;
 }
 
 static const TypeInfo next_pc_info = {
-- 
2.20.1

[PATCH 11/11] hw/m68k/next-cube: Add missing header comment to next-cube.h

[Peter Maydell]

The next-cube.h file is missing the usual copyright-and-license
header; add it (same as the next-cube.c one).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/m68k/next-cube.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/hw/m68k/next-cube.h b/include/hw/m68k/next-cube.h
index d38c52d540d..43577282d15 100644
--- a/include/hw/m68k/next-cube.h
+++ b/include/hw/m68k/next-cube.h
@@ -1,3 +1,13 @@
+/*
+ * NeXT Cube
+ *
+ * Copyright (c) 2011 Bryce Lanham
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; either version 2 of the License,
+ * or (at your option) any later version.
+ */
 
 #ifndef NEXT_CUBE_H
 #define NEXT_CUBE_H
-- 
2.20.1

Re: [PATCH 01/11] hw/m68k/next-cube: Make next_irq() function static

[Thomas Huth]

Am Fri, 15 Jan 2021 20:11:56 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> The next_irq() function is global, but isn't actually used anywhere
> outside next-cube.c. Make it static.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/m68k/next-cube.h | 2 --
>  hw/m68k/next-cube.c         | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 03/11] hw/m68k/next-cube: Move mmio_ops into NeXTPC device

[Thomas Huth]

Am Fri, 15 Jan 2021 20:11:58 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Move the registers handled by the mmio_ops struct into the NeXTPC
> device.  This allows us to also move the scr1 and scr2 data fields.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/next-cube.c | 80
> +++++++++++++++++++++++++-------------------- 1 file changed, 44
> insertions(+), 36 deletions(-)

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 11/11] hw/m68k/next-cube: Add missing header comment to next-cube.h

[Thomas Huth]

Am Fri, 15 Jan 2021 20:12:06 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> The next-cube.h file is missing the usual copyright-and-license
> header; add it (same as the next-cube.c one).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/m68k/next-cube.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/hw/m68k/next-cube.h b/include/hw/m68k/next-cube.h
> index d38c52d540d..43577282d15 100644
> --- a/include/hw/m68k/next-cube.h
> +++ b/include/hw/m68k/next-cube.h
> @@ -1,3 +1,13 @@
> +/*
> + * NeXT Cube
> + *
> + * Copyright (c) 2011 Bryce Lanham
> + *
> + * This code is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published
> + * by the Free Software Foundation; either version 2 of the License,
> + * or (at your option) any later version.
> + */
>  
>  #ifndef NEXT_CUBE_H
>  #define NEXT_CUBE_H

The file is based on Bryce's original version, indeed:

 https://github.com/blanham/qemu-NeXT/blob/next-cube/hw/next-cube.h

So I think that header comment is appropriate.

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 04/11] hw/m68k/next-cube: Move scr_ops into NeXTPC device

[Thomas Huth]

Am Fri, 15 Jan 2021 20:11:59 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Move the registers handled by the scr_ops struct into the NeXTPC
> device.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/next-cube.c | 50
> ++++++++++++++++++++++----------------------- 1 file changed, 25
> insertions(+), 25 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index ff121143e92..f5575cb43b8 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -76,8 +76,6 @@ struct NeXTState {
>      uint32_t int_mask;
>      uint32_t int_status;
>  
> -    uint8_t scsi_csr_1;
> -    uint8_t scsi_csr_2;
>      next_dma dma[10];
>      qemu_irq *scsi_irq;
>      qemu_irq scsi_dma;
> @@ -97,9 +95,12 @@ struct NeXTPC {
>      NeXTState *ns;
>  
>      MemoryRegion mmiomem;
> +    MemoryRegion scrmem;
>  
>      uint32_t scr1;
>      uint32_t scr2;
> +    uint8_t scsi_csr_1;
> +    uint8_t scsi_csr_2;
>  };
>  
>  /* Thanks to NeXT forums for this */
> @@ -402,7 +403,7 @@ static const MemoryRegionOps mmio_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static uint32_t scr_readb(NeXTState *s, hwaddr addr)
> +static uint32_t scr_readb(NeXTPC *s, hwaddr addr)
>  {
>      switch (addr) {
>      case 0x14108:
> @@ -436,13 +437,13 @@ static uint32_t scr_readb(NeXTState *s, hwaddr
> addr) }
>  }
>  
> -static uint32_t scr_readw(NeXTState *s, hwaddr addr)
> +static uint32_t scr_readw(NeXTPC *s, hwaddr addr)
>  {
>      DPRINTF("BMAP Read W @ %x\n", (unsigned int)addr);
>      return 0;
>  }
>  
> -static uint32_t scr_readl(NeXTState *s, hwaddr addr)
> +static uint32_t scr_readl(NeXTPC *s, hwaddr addr)
>  {
>      DPRINTF("BMAP Read L @ %x\n", (unsigned int)addr);
>      return 0;
> @@ -455,7 +456,7 @@ static uint32_t scr_readl(NeXTState *s, hwaddr
> addr) #define SCSICSR_CPUDMA  0x10  /* if set, dma enabled */
>  #define SCSICSR_INTMASK 0x20  /* if set, interrupt enabled */
>  
> -static void scr_writeb(NeXTState *s, hwaddr addr, uint32_t value)
> +static void scr_writeb(NeXTPC *s, hwaddr addr, uint32_t value)
>  {
>      switch (addr) {
>      case 0x14108:
> @@ -501,9 +502,9 @@ static void scr_writeb(NeXTState *s, hwaddr addr,
> uint32_t value) DPRINTF("SCSICSR CPUDMA\n");
>              /* qemu_irq_raise(s->scsi_dma); */
>  
> -            s->int_status |= 0x4000000;
> +            s->ns->int_status |= 0x4000000;
>          } else {
> -            s->int_status &= ~(0x4000000);
> +            s->ns->int_status &= ~(0x4000000);
>          }
>          if (value & SCSICSR_INTMASK) {
>              DPRINTF("SCSICSR INTMASK\n");
> @@ -533,27 +534,27 @@ static void scr_writeb(NeXTState *s, hwaddr
> addr, uint32_t value) }
>  }
>  
> -static void scr_writew(NeXTState *s, hwaddr addr, uint32_t value)
> +static void scr_writew(NeXTPC *s, hwaddr addr, uint32_t value)
>  {
>      DPRINTF("BMAP Write W @ %x with %x\n", (unsigned int)addr,
> value); }
>  
> -static void scr_writel(NeXTState *s, hwaddr addr, uint32_t value)
> +static void scr_writel(NeXTPC *s, hwaddr addr, uint32_t value)
>  {
>      DPRINTF("BMAP Write L @ %x with %x\n", (unsigned int)addr,
> value); }
>  
>  static uint64_t scr_readfn(void *opaque, hwaddr addr, unsigned size)
>  {
> -    NeXTState *ns = NEXT_MACHINE(opaque);
> +    NeXTPC *s = NEXT_PC(opaque);
>  
>      switch (size) {
>      case 1:
> -        return scr_readb(ns, addr);
> +        return scr_readb(s, addr);
>      case 2:
> -        return scr_readw(ns, addr);
> +        return scr_readw(s, addr);
>      case 4:
> -        return scr_readl(ns, addr);
> +        return scr_readl(s, addr);
>      default:
>          g_assert_not_reached();
>      }
> @@ -562,17 +563,17 @@ static uint64_t scr_readfn(void *opaque, hwaddr
> addr, unsigned size) static void scr_writefn(void *opaque, hwaddr
> addr, uint64_t value, unsigned size)
>  {
> -    NeXTState *ns = NEXT_MACHINE(opaque);
> +    NeXTPC *s = NEXT_PC(opaque);
>  
>      switch (size) {
>      case 1:
> -        scr_writeb(ns, addr, value);
> +        scr_writeb(s, addr, value);
>          break;
>      case 2:
> -        scr_writew(ns, addr, value);
> +        scr_writew(s, addr, value);
>          break;
>      case 4:
> -        scr_writel(ns, addr, value);
> +        scr_writel(s, addr, value);
>          break;
>      default:
>          g_assert_not_reached();
> @@ -886,8 +887,10 @@ static void next_pc_realize(DeviceState *dev,
> Error **errp) 
>      memory_region_init_io(&s->mmiomem, OBJECT(s), &mmio_ops, s,
>                            "next.mmio", 0xD0000);
> -
> +    memory_region_init_io(&s->scrmem, OBJECT(s), &scr_ops, s,
> +                          "next.scr", 0x20000);
>      sysbus_init_mmio(sbd, &s->mmiomem);
> +    sysbus_init_mmio(sbd, &s->scrmem);
>  }
>  
>  static void next_pc_class_init(ObjectClass *klass, void *data)
> @@ -912,7 +915,6 @@ static void next_cube_init(MachineState *machine)
>      M68kCPU *cpu;
>      CPUM68KState *env;
>      MemoryRegion *rom = g_new(MemoryRegion, 1);
> -    MemoryRegion *scrmem = g_new(MemoryRegion, 1);
>      MemoryRegion *dmamem = g_new(MemoryRegion, 1);
>      MemoryRegion *bmapm1 = g_new(MemoryRegion, 1);
>      MemoryRegion *bmapm2 = g_new(MemoryRegion, 1);
> @@ -956,6 +958,9 @@ static void next_cube_init(MachineState *machine)
>      /* MMIO */
>      sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 0, 0x02000000);
>  
> +    /* BMAP IO - acts as a catch-all for now */
> +    sysbus_mmio_map(SYS_BUS_DEVICE(pcdev), 1, 0x02100000);
> +
>      /* BMAP memory */
>      memory_region_init_ram_shared_nomigrate(bmapm1, NULL,
> "next.bmapmem", 64, true, &error_fatal);
> @@ -964,11 +969,6 @@ static void next_cube_init(MachineState *machine)
>      memory_region_init_alias(bmapm2, NULL, "next.bmapmem2", bmapm1,
> 0x0, 64); memory_region_add_subregion(sysmem, 0x820c0000, bmapm2);
>  
> -    /* BMAP IO - acts as a catch-all for now */
> -    memory_region_init_io(scrmem, NULL, &scr_ops, machine,
> "next.scr",
> -                          0x20000);
> -    memory_region_add_subregion(sysmem, 0x02100000, scrmem);
> -
>      /* KBD */
>      dev = qdev_new(TYPE_NEXTKBD);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 05/11] hw/m68k/next-cube: Make next_irq take NeXTPC* as its opaque

[Thomas Huth]

Am Fri, 15 Jan 2021 20:12:00 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Make the next_irq function take a NeXTPC* as its opaque rather than
> the M68kCPU*.  This will make it simpler to turn the next_irq
> function into a gpio input line of the NeXTPC device in the next
> commit.
> 
> For this to work we have to pass the CPU to the NeXTPC device via a
> link property, in the same way we do in q800.c (and for the same
> reason).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/next-cube.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index f5575cb43b8..a9e57304e04 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -94,6 +94,8 @@ struct NeXTPC {
>      /* Temporary until all functionality has been moved into this
> device */ NeXTState *ns;
>  
> +    M68kCPU *cpu;
> +
>      MemoryRegion mmiomem;
>      MemoryRegion scrmem;
>  
> @@ -739,9 +741,9 @@ static const MemoryRegionOps dma_ops = {
>   */
>  static void next_irq(void *opaque, int number, int level)
>  {
> -    M68kCPU *cpu = opaque;
> +    NeXTPC *s = NEXT_PC(opaque);
> +    M68kCPU *cpu = s->cpu;
>      int shift = 0;
> -    NeXTState *ns = NEXT_MACHINE(qdev_get_machine());
>  
>      /* first switch sets interupt status */
>      /* DPRINTF("IRQ %i\n",number); */
> @@ -796,14 +798,14 @@ static void next_irq(void *opaque, int number,
> int level)
>       * this HAS to be wrong, the interrupt handlers in mach and
> together
>       * int_status and int_mask and return if there is a hit
>       */
> -    if (ns->int_mask & (1 << shift)) {
> +    if (s->ns->int_mask & (1 << shift)) {
>          DPRINTF("%x interrupt masked @ %x\n", 1 << shift,
> cpu->env.pc); /* return; */
>      }
>  
>      /* second switch triggers the correct interrupt */
>      if (level) {
> -        ns->int_status |= 1 << shift;
> +        s->ns->int_status |= 1 << shift;
>  
>          switch (number) {
>          /* level 3 - floppy, kbd/mouse, power, ether rx/tx, scsi,
> clock */ @@ -832,7 +834,7 @@ static void next_irq(void *opaque, int
> number, int level) break;
>          }
>      } else {
> -        ns->int_status &= ~(1 << shift);
> +        s->ns->int_status &= ~(1 << shift);
>          cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
>      }
>  }
> @@ -847,9 +849,9 @@ static void next_serial_irq(void *opaque, int n,
> int level) }
>  }
>  
> -static void next_escc_init(M68kCPU *cpu)
> +static void next_escc_init(DeviceState *pcdev)
>  {
> -    qemu_irq *ser_irq = qemu_allocate_irqs(next_serial_irq, cpu, 2);
> +    qemu_irq *ser_irq = qemu_allocate_irqs(next_serial_irq, pcdev,
> 2); DeviceState *dev;
>      SysBusDevice *s;
>  
> @@ -893,6 +895,17 @@ static void next_pc_realize(DeviceState *dev,
> Error **errp) sysbus_init_mmio(sbd, &s->scrmem);
>  }
>  
> +/*
> + * If the m68k CPU implemented its inbound irq lines as GPIO lines
> + * rather than via the m68k_set_irq_level() function we would not
> need
> + * this cpu link property and could instead provide outbound IRQ
> lines
> + * that the board could wire up to the CPU.
> + */
> +static Property next_pc_properties[] = {
> +    DEFINE_PROP_LINK("cpu", NeXTPC, cpu, TYPE_M68K_CPU, M68kCPU *),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void next_pc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -900,6 +913,7 @@ static void next_pc_class_init(ObjectClass
> *klass, void *data) dc->desc = "NeXT Peripheral Controller";
>      dc->realize = next_pc_realize;
>      dc->reset = next_pc_reset;
> +    device_class_set_props(dc, next_pc_properties);
>      /* We will add the VMState in a later commit */
>  }
>  
> @@ -938,6 +952,7 @@ static void next_cube_init(MachineState *machine)
>  
>      /* Peripheral Controller */
>      pcdev = qdev_new(TYPE_NEXT_PC);
> +    object_property_set_link(OBJECT(pcdev), "cpu", OBJECT(cpu),
> &error_abort); sysbus_realize_and_unref(SYS_BUS_DEVICE(pcdev),
> &error_fatal); /* Temporary while we refactor this code */
>      NEXT_PC(pcdev)->ns = ns;
> @@ -996,7 +1011,7 @@ static void next_cube_init(MachineState *machine)
>      }
>  
>      /* Serial */
> -    next_escc_init(cpu);
> +    next_escc_init(pcdev);
>  
>      /* TODO: */
>      /* Network */

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 06/11] hw/m68k/next-cube: Move int_status and int_mask to NeXTPC struct

[Thomas Huth]

Am Fri, 15 Jan 2021 20:12:01 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> All the code which accesses int_status and int_mask is now doing
> so via the NeXTPC->NeXTState indirection, so we can move these
> fields into the NeXTPC struct where they belong.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/next-cube.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index a9e57304e04..6b4bcfd4b9b 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -73,9 +73,6 @@ typedef struct NextRtc {
>  struct NeXTState {
>      MachineState parent;
>  
> -    uint32_t int_mask;
> -    uint32_t int_status;
> -
>      next_dma dma[10];
>      qemu_irq *scsi_irq;
>      qemu_irq scsi_dma;
> @@ -103,6 +100,8 @@ struct NeXTPC {
>      uint32_t scr2;
>      uint8_t scsi_csr_1;
>      uint8_t scsi_csr_2;
> +    uint32_t int_mask;
> +    uint32_t int_status;
>  };
>  
>  /* Thanks to NeXT forums for this */
> @@ -243,7 +242,7 @@ static void nextscr2_write(NeXTPC *s, uint32_t
> val, int size) /* clear FTU */
>                      if (rtc->value & 0x04) {
>                          rtc->status = rtc->status & (~0x18);
> -                        s->ns->int_status = s->ns->int_status &
> (~0x04);
> +                        s->int_status = s->int_status & (~0x04);
>                      }
>                  }
>              }
> @@ -302,12 +301,12 @@ static uint32_t mmio_readl(NeXTPC *s, hwaddr
> addr) {
>      switch (addr) {
>      case 0x7000:
> -        /* DPRINTF("Read INT status: %x\n", s->ns->int_status); */
> -        return s->ns->int_status;
> +        /* DPRINTF("Read INT status: %x\n", s->int_status); */
> +        return s->int_status;
>  
>      case 0x7800:
> -        DPRINTF("MMIO Read INT mask: %x\n", s->ns->int_mask);
> -        return s->ns->int_mask;
> +        DPRINTF("MMIO Read INT mask: %x\n", s->int_mask);
> +        return s->int_mask;
>  
>      case 0xc000:
>          return s->scr1;
> @@ -342,12 +341,12 @@ static void mmio_writel(NeXTPC *s, hwaddr addr,
> uint32_t val) {
>      switch (addr) {
>      case 0x7000:
> -        DPRINTF("INT Status old: %x new: %x\n", s->ns->int_status,
> val);
> -        s->ns->int_status = val;
> +        DPRINTF("INT Status old: %x new: %x\n", s->int_status, val);
> +        s->int_status = val;
>          break;
>      case 0x7800:
> -        DPRINTF("INT Mask old: %x new: %x\n", s->ns->int_mask, val);
> -        s->ns->int_mask  = val;
> +        DPRINTF("INT Mask old: %x new: %x\n", s->int_mask, val);
> +        s->int_mask  = val;
>          break;
>      case 0xc000:
>          DPRINTF("SCR1 Write: %x\n", val);
> @@ -504,9 +503,9 @@ static void scr_writeb(NeXTPC *s, hwaddr addr,
> uint32_t value) DPRINTF("SCSICSR CPUDMA\n");
>              /* qemu_irq_raise(s->scsi_dma); */
>  
> -            s->ns->int_status |= 0x4000000;
> +            s->int_status |= 0x4000000;
>          } else {
> -            s->ns->int_status &= ~(0x4000000);
> +            s->int_status &= ~(0x4000000);
>          }
>          if (value & SCSICSR_INTMASK) {
>              DPRINTF("SCSICSR INTMASK\n");
> @@ -798,14 +797,14 @@ static void next_irq(void *opaque, int number,
> int level)
>       * this HAS to be wrong, the interrupt handlers in mach and
> together
>       * int_status and int_mask and return if there is a hit
>       */
> -    if (s->ns->int_mask & (1 << shift)) {
> +    if (s->int_mask & (1 << shift)) {
>          DPRINTF("%x interrupt masked @ %x\n", 1 << shift,
> cpu->env.pc); /* return; */
>      }
>  
>      /* second switch triggers the correct interrupt */
>      if (level) {
> -        s->ns->int_status |= 1 << shift;
> +        s->int_status |= 1 << shift;
>  
>          switch (number) {
>          /* level 3 - floppy, kbd/mouse, power, ether rx/tx, scsi,
> clock */ @@ -834,7 +833,7 @@ static void next_irq(void *opaque, int
> number, int level) break;
>          }
>      } else {
> -        s->ns->int_status &= ~(1 << shift);
> +        s->int_status &= ~(1 << shift);
>          cpu_reset_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
>      }
>  }

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 07/11] hw/m68k/next-cube: Make next_irq GPIO inputs to NEXT_PC device

[Thomas Huth]

Am Fri, 15 Jan 2021 20:12:02 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Make the next_irq function be GPIO inputs to the NEXT_PC
> device, rather than a freestanding set of qemu_irq lines.
> 
> This fixes a minor Coverity issue where it correctly points
> out the trivial memory leak of the memory allocated in the
> call to qemu_allocate_irqs().
> 
> Fixes: CID 1421962
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/m68k/next-cube.h |  3 ++-
>  hw/m68k/next-cube.c         | 21 ++++-----------------
>  2 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/include/hw/m68k/next-cube.h b/include/hw/m68k/next-cube.h
> index 5a56c354b8e..d38c52d540d 100644
> --- a/include/hw/m68k/next-cube.h
> +++ b/include/hw/m68k/next-cube.h
> @@ -39,7 +39,8 @@ enum next_irqs {
>      NEXT_ENRX_DMA_I,
>      NEXT_SCSI_DMA_I,
>      NEXT_SCC_DMA_I,
> -    NEXT_SND_I
> +    NEXT_SND_I,
> +    NEXT_NUM_IRQS
>  };
>  
>  #endif /* NEXT_CUBE_H */
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 6b4bcfd4b9b..5a8fc24ed35 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -734,10 +734,6 @@ static const MemoryRegionOps dma_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -/*
> - * TODO: set the shift numbers as values in the enum, so the first
> switch
> - * will not be needed
> - */
>  static void next_irq(void *opaque, int number, int level)
>  {
>      NeXTPC *s = NEXT_PC(opaque);
> @@ -838,19 +834,8 @@ static void next_irq(void *opaque, int number,
> int level) }
>  }
>  
> -static void next_serial_irq(void *opaque, int n, int level)
> -{
> -    /* DPRINTF("SCC IRQ NUM %i\n",n); */
> -    if (n) {
> -        next_irq(opaque, NEXT_SCC_DMA_I, level);
> -    } else {
> -        next_irq(opaque, NEXT_SCC_I, level);
> -    }
> -}
> -
>  static void next_escc_init(DeviceState *pcdev)
>  {
> -    qemu_irq *ser_irq = qemu_allocate_irqs(next_serial_irq, pcdev,
> 2); DeviceState *dev;
>      SysBusDevice *s;
>  
> @@ -866,8 +851,8 @@ static void next_escc_init(DeviceState *pcdev)
>  
>      s = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(s, &error_fatal);
> -    sysbus_connect_irq(s, 0, ser_irq[0]);
> -    sysbus_connect_irq(s, 1,  ser_irq[1]);
> +    sysbus_connect_irq(s, 0, qdev_get_gpio_in(pcdev, NEXT_SCC_I));
> +    sysbus_connect_irq(s, 1, qdev_get_gpio_in(pcdev,
> NEXT_SCC_DMA_I)); sysbus_mmio_map(s, 0, 0x2118000);
>  }
>  
> @@ -886,6 +871,8 @@ static void next_pc_realize(DeviceState *dev,
> Error **errp) NeXTPC *s = NEXT_PC(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  
> +    qdev_init_gpio_in(dev, next_irq, NEXT_NUM_IRQS);
> +
>      memory_region_init_io(&s->mmiomem, OBJECT(s), &mmio_ops, s,
>                            "next.mmio", 0xD0000);
>      memory_region_init_io(&s->scrmem, OBJECT(s), &scr_ops, s,

Acked-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 08/11] hw/m68k/next-cube: Move rtc into NeXTPC struct

[Thomas Huth]

Am Fri, 15 Jan 2021 20:12:03 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Move the rtc into the NeXTPC struct. Since this is the last
> use of the 'backdoor' NextState pointer we can now remove that.
> 
> Probably the RTC should be its own device at some point: in hardware
> there is a separate MCS1850 RTC chip connected to the Peripheral
> Controller via a 1-bit serial interface.  That goes beyond the remit
> of the current refactoring, though.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/next-cube.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 5a8fc24ed35..3c83b874c56 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -78,8 +78,6 @@ struct NeXTState {
>      qemu_irq scsi_dma;
>      qemu_irq scsi_reset;
>      qemu_irq *fd_irq;
> -
> -    NextRtc rtc;
>  };
>  
>  #define TYPE_NEXT_PC "next-pc"
> @@ -88,9 +86,6 @@ OBJECT_DECLARE_SIMPLE_TYPE(NeXTPC, NEXT_PC)
>  struct NeXTPC {
>      SysBusDevice parent_obj;
>  
> -    /* Temporary until all functionality has been moved into this
> device */
> -    NeXTState *ns;
> -
>      M68kCPU *cpu;
>  
>      MemoryRegion mmiomem;
> @@ -102,6 +97,8 @@ struct NeXTPC {
>      uint8_t scsi_csr_2;
>      uint32_t int_mask;
>      uint32_t int_status;
> +
> +    NextRtc rtc;
>  };
>  
>  /* Thanks to NeXT forums for this */
> @@ -130,7 +127,7 @@ static void nextscr2_write(NeXTPC *s, uint32_t
> val, int size) static int phase;
>      static uint8_t old_scr2;
>      uint8_t scr2_2;
> -    NextRtc *rtc = &s->ns->rtc;
> +    NextRtc *rtc = &s->rtc;
>  
>      if (size == 4) {
>          scr2_2 = (val >> 8) & 0xFF;
> @@ -864,6 +861,11 @@ static void next_pc_reset(DeviceState *dev)
>      /*     0x0000XX00 << vital bits */
>      s->scr1 = 0x00011102;
>      s->scr2 = 0x00ff0c80;
> +
> +    s->rtc.status = 0x90;
> +
> +    /* Load RTC RAM - TODO: provide possibility to load contents
> from file */
> +    memcpy(s->rtc.ram, rtc_ram2, 32);
>  }
>  
>  static void next_pc_realize(DeviceState *dev, Error **errp)
> @@ -920,7 +922,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;
> -    NeXTState *ns = NEXT_MACHINE(machine);
>      DeviceState *dev;
>      DeviceState *pcdev;
>  
> @@ -940,13 +941,6 @@ static void next_cube_init(MachineState *machine)
>      pcdev = qdev_new(TYPE_NEXT_PC);
>      object_property_set_link(OBJECT(pcdev), "cpu", OBJECT(cpu),
> &error_abort); sysbus_realize_and_unref(SYS_BUS_DEVICE(pcdev),
> &error_fatal);
> -    /* Temporary while we refactor this code */
> -    NEXT_PC(pcdev)->ns = ns;
> -
> -    ns->rtc.status = 0x90;
> -
> -    /* Load RTC RAM - TODO: provide possibility to load contents
> from file */
> -    memcpy(ns->rtc.ram, rtc_ram2, 32);
>  
>      /* 64MB RAM starting at 0x04000000  */
>      memory_region_add_subregion(sysmem, 0x04000000, machine->ram);

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 09/11] hw/m68k/next-cube: Remove unused fields from NeXTState

[Thomas Huth]

Am Fri, 15 Jan 2021 20:12:04 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> The fields scsi_irq, scsi_dma, scsi_reset and fd_irq in
> NeXTState are all unused, except in commented out
> "this should do something like this" code. Remove the
> unused fields. As and when the functionality that might
> use them is added, we can put in the correct kind of
> wiring (which might or might not need to be a qemu_irq,
> but which in any case will need to be in the NeXTPC
> device, not in NeXTState).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/next-cube.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index 3c83b874c56..dd0a2a5aea0 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -74,10 +74,6 @@ struct NeXTState {
>      MachineState parent;
>  
>      next_dma dma[10];
> -    qemu_irq *scsi_irq;
> -    qemu_irq scsi_dma;
> -    qemu_irq scsi_reset;
> -    qemu_irq *fd_irq;
>  };
>  
>  #define TYPE_NEXT_PC "next-pc"

Yeah, I've never managed to get the SCSI controller running. Old patch
is available here:

 https://gitlab.com/huth/qemu/-/commit/14e9ff46f8261203a15f13d8c8bfb7faf6471d44

But yes, we can add the variables back in case somebody gets this
working again, so:

Reviewed-by: Thomas Huth <huth@tuxfamily.org>

Re: [PATCH 10/11] hw/m68k/next-cube: Add vmstate for NeXTPC device

[Thomas Huth]

Am Fri, 15 Jan 2021 20:12:05 +0000
schrieb Peter Maydell <peter.maydell@linaro.org>:

> Add the vmstate for the new NeXTPC devic; this is in theory
> a migration compatibility break, but this machine doesn't have
> working migration currently anyway.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/next-cube.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
> index dd0a2a5aea0..9eb1e31752b 100644
> --- a/hw/m68k/next-cube.c
> +++ b/hw/m68k/next-cube.c
> @@ -28,6 +28,7 @@
>  #include "qapi/error.h"
>  #include "ui/console.h"
>  #include "target/m68k/cpu.h"
> +#include "migration/vmstate.h"
>  
>  /* #define DEBUG_NEXT */
>  #ifdef DEBUG_NEXT
> @@ -890,6 +891,37 @@ static Property next_pc_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static const VMStateDescription next_rtc_vmstate = {
> +    .name = "next-rtc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(ram, NextRtc, 32),
> +        VMSTATE_UINT8(command, NextRtc),
> +        VMSTATE_UINT8(value, NextRtc),
> +        VMSTATE_UINT8(status, NextRtc),
> +        VMSTATE_UINT8(control, NextRtc),
> +        VMSTATE_UINT8(retval, NextRtc),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static const VMStateDescription next_pc_vmstate = {
> +    .name = "next-pc",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(scr1, NeXTPC),
> +        VMSTATE_UINT32(scr2, NeXTPC),
> +        VMSTATE_UINT32(int_mask, NeXTPC),
> +        VMSTATE_UINT32(int_status, NeXTPC),
> +        VMSTATE_UINT8(scsi_csr_1, NeXTPC),
> +        VMSTATE_UINT8(scsi_csr_2, NeXTPC),
> +        VMSTATE_STRUCT(rtc, NeXTPC, 0, next_rtc_vmstate, NextRtc),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static void next_pc_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -898,7 +930,7 @@ static void next_pc_class_init(ObjectClass
> *klass, void *data) dc->realize = next_pc_realize;
>      dc->reset = next_pc_reset;
>      device_class_set_props(dc, next_pc_properties);
> -    /* We will add the VMState in a later commit */
> +    dc->vmsd = &next_pc_vmstate;
>  }
>  
>  static const TypeInfo next_pc_info = {

Acked-by: Thomas Huth <huth@tuxfamily.org>