[Qemu-devel] [PATCH 0/7] mac_dbdma: tidy-up and QOMify

[Qemu-devel] [PATCH 0/7] mac_dbdma: tidy-up and QOMify

[Mark Cave-Ayland]

Whilst looking at implementing another DBDMA device for the Mac machines
I noticed a couple of things: firstly there were some unused fields still
in DBDMAState, and secondly the existing code still used global functions
to register DMA channels and handle the relationship between macio IDE and
DBDMA.

This patchset removes the now-unused fields from DBDMA state, QOMifys the
DBDMA device, uses a QOM object link to allow the macio IDE object to
reference the DBDMA device, and then finally removes the global DBDMA_*
functions substituting them instead for QOM methods.

Note: this patchset does not apply to master but on top of David's
ppc-for-2.11 branch since there are merge conflicts with my previous
patchset. Hopefully the Based-On line below is enough to keep patchew
happy, even though it wasn't the final version applied to the ppc-for-2.11
branch.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Based-on: 1505668548-16616-1-git-send-email-mark.cave-ayland@ilande.co.uk (ppc: more Mac-related fixups)


Mark Cave-Ayland (7):
  mac_dbdma: remove unused IO fields from DBDMAState
  mac_dbdma: QOMify
  mac_dbdma: remove DBDMA_init() function
  macio: pass channel into MACIOIDEState via qdev property
  macio: use object link between MACIO_IDE and MAC_DBDMA object
  mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method
  mac_dbdma: change DBDMA_kick to a MAC_DBDMA type method

 hw/ide/macio.c             |   26 ++++++++++-----
 hw/misc/macio/mac_dbdma.c  |   79 +++++++++++++++++++++++++++++---------------
 hw/misc/macio/macio.c      |   20 ++++++++---
 hw/ppc/mac.h               |    4 +--
 include/hw/ppc/mac_dbdma.h |   22 ++++++------
 5 files changed, 97 insertions(+), 54 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/7] mac_dbdma: remove unused IO fields from DBDMAState

[Mark Cave-Ayland]

These fields were used to manually handle IO requests that weren't aligned
to a sector boundary before this feature was supported by the block API.

Once the block API changed to support byte-aligned IO requests, the macio
controller was switched over to use it in commit be1e343 but these fields
were accidentally left behind. Remove them, including the initialisation
in DBDMA_init().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/mac_dbdma.c  |    2 --
 include/hw/ppc/mac_dbdma.h |    4 ----
 2 files changed, 6 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 3fe5073..9795172 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -893,9 +893,7 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
     s = g_malloc0(sizeof(DBDMAState));
 
     for (i = 0; i < DBDMA_CHANNELS; i++) {
-        DBDMA_io *io = &s->channels[i].io;
         DBDMA_channel *ch = &s->channels[i];
-        qemu_iovec_init(&io->iov, 1);
 
         ch->rw = dbdma_unassigned_rw;
         ch->flush = dbdma_unassigned_flush;
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index a860387..21bd66f 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -42,10 +42,6 @@ struct DBDMA_io {
     DBDMA_end dma_end;
     /* DMA is in progress, don't start another one */
     bool processing;
-    /* unaligned last sector of a request */
-    uint8_t head_remainder[0x200];
-    uint8_t tail_remainder[0x200];
-    QEMUIOVector iov;
     /* DMA request */
     void *dma_mem;
     dma_addr_t dma_len;
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/7] mac_dbdma: QOMify

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/mac_dbdma.c  |   59 ++++++++++++++++++++++++++++++++++++--------
 include/hw/ppc/mac_dbdma.h |    6 +++++
 2 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 9795172..302f131 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -851,13 +851,14 @@ static const VMStateDescription vmstate_dbdma = {
     }
 };
 
-static void dbdma_reset(void *opaque)
+static void mac_dbdma_reset(DeviceState *d)
 {
-    DBDMAState *s = opaque;
+    DBDMAState *s = MAC_DBDMA(d);
     int i;
 
-    for (i = 0; i < DBDMA_CHANNELS; i++)
+    for (i = 0; i < DBDMA_CHANNELS; i++) {
         memset(s->channels[i].regs, 0, DBDMA_SIZE);
+    }
 }
 
 static void dbdma_unassigned_rw(DBDMA_io *io)
@@ -888,9 +889,22 @@ static void dbdma_unassigned_flush(DBDMA_io *io)
 void* DBDMA_init (MemoryRegion **dbdma_mem)
 {
     DBDMAState *s;
-    int i;
+    SysBusDevice *sbd;
+
+    s = MAC_DBDMA(object_new(TYPE_MAC_DBDMA));
+    object_property_set_bool(OBJECT(s), true, "realized", NULL);
+
+    sbd = SYS_BUS_DEVICE(s);
+    *dbdma_mem = sysbus_mmio_get_region(sbd, 0);
 
-    s = g_malloc0(sizeof(DBDMAState));
+    return s;
+}
+
+static void mac_dbdma_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    DBDMAState *s = MAC_DBDMA(obj);
+    int i;
 
     for (i = 0; i < DBDMA_CHANNELS; i++) {
         DBDMA_channel *ch = &s->channels[i];
@@ -901,12 +915,37 @@ void* DBDMA_init (MemoryRegion **dbdma_mem)
         ch->io.channel = ch;
     }
 
-    memory_region_init_io(&s->mem, NULL, &dbdma_ops, s, "dbdma", 0x1000);
-    *dbdma_mem = &s->mem;
-    vmstate_register(NULL, -1, &vmstate_dbdma, s);
-    qemu_register_reset(dbdma_reset, s);
+    memory_region_init_io(&s->mem, obj, &dbdma_ops, s, "dbdma", 0x1000);
+    sysbus_init_mmio(sbd, &s->mem);
+}
+
+static void mac_dbdma_realize(DeviceState *dev, Error **errp)
+{
+    DBDMAState *s = MAC_DBDMA(dev);
 
     s->bh = qemu_bh_new(DBDMA_run_bh, s);
+}
 
-    return s;
+static void mac_dbdma_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = mac_dbdma_realize;
+    dc->reset = mac_dbdma_reset;
+    dc->vmsd = &vmstate_dbdma;
 }
+
+static const TypeInfo mac_dbdma_type_info = {
+    .name = TYPE_MAC_DBDMA,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(DBDMAState),
+    .instance_init = mac_dbdma_init,
+    .class_init = mac_dbdma_class_init
+};
+
+static void mac_dbdma_register_types(void)
+{
+    type_register_static(&mac_dbdma_type_info);
+}
+
+type_init(mac_dbdma_register_types)
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 21bd66f..4bc6274 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -26,6 +26,7 @@
 #include "exec/memory.h"
 #include "qemu/iov.h"
 #include "sysemu/dma.h"
+#include "hw/sysbus.h"
 
 typedef struct DBDMA_io DBDMA_io;
 
@@ -160,6 +161,8 @@ typedef struct DBDMA_channel {
 } DBDMA_channel;
 
 typedef struct {
+    SysBusDevice parent_obj;
+
     MemoryRegion mem;
     DBDMA_channel channels[DBDMA_CHANNELS];
     QEMUBH *bh;
@@ -173,4 +176,7 @@ void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
 void DBDMA_kick(DBDMAState *dbdma);
 void* DBDMA_init (MemoryRegion **dbdma_mem);
 
+#define TYPE_MAC_DBDMA "mac-dbdma"
+#define MAC_DBDMA(obj) OBJECT_CHECK(DBDMAState, (obj), TYPE_MAC_DBDMA)
+
 #endif
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/7] mac_dbdma: remove DBDMA_init() function

[Mark Cave-Ayland]

Instead we can now instantiate the MAC_DBDMA object directly within the
macio device. We also add the DBDMA device as a child property so that
it is possible to retrieve later.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/mac_dbdma.c  |   14 --------------
 hw/misc/macio/macio.c      |   16 ++++++++++++----
 include/hw/ppc/mac_dbdma.h |    1 -
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 302f131..0eddf2e 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -886,20 +886,6 @@ static void dbdma_unassigned_flush(DBDMA_io *io)
                   __func__, ch->channel);
 }
 
-void* DBDMA_init (MemoryRegion **dbdma_mem)
-{
-    DBDMAState *s;
-    SysBusDevice *sbd;
-
-    s = MAC_DBDMA(object_new(TYPE_MAC_DBDMA));
-    object_property_set_bool(OBJECT(s), true, "realized", NULL);
-
-    sbd = SYS_BUS_DEVICE(s);
-    *dbdma_mem = sysbus_mmio_get_region(sbd, 0);
-
-    return s;
-}
-
 static void mac_dbdma_init(Object *obj)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 5d57f45..f459f17 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -41,7 +41,7 @@ typedef struct MacIOState
 
     MemoryRegion bar;
     CUDAState cuda;
-    void *dbdma;
+    DBDMAState *dbdma;
     MemoryRegion *pic_mem;
     MemoryRegion *escc_mem;
     uint64_t frequency;
@@ -127,10 +127,15 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
     MacIOState *s = MACIO(d);
     SysBusDevice *sysbus_dev;
     Error *err = NULL;
-    MemoryRegion *dbdma_mem;
 
-    s->dbdma = DBDMA_init(&dbdma_mem);
-    memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem);
+    object_property_set_bool(OBJECT(s->dbdma), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    sysbus_dev = SYS_BUS_DEVICE(s->dbdma);
+    memory_region_add_subregion(&s->bar, 0x08000,
+                                sysbus_mmio_get_region(sysbus_dev, 0));
 
     object_property_set_bool(OBJECT(&s->cuda), true, "realized", &err);
     if (err) {
@@ -334,6 +339,9 @@ static void macio_instance_init(Object *obj)
     object_initialize(&s->cuda, sizeof(s->cuda), TYPE_CUDA);
     qdev_set_parent_bus(DEVICE(&s->cuda), sysbus_get_default());
     object_property_add_child(obj, "cuda", OBJECT(&s->cuda), NULL);
+
+    s->dbdma = MAC_DBDMA(object_new(TYPE_MAC_DBDMA));
+    object_property_add_child(obj, "dbdma", OBJECT(s->dbdma), NULL);
 }
 
 static const VMStateDescription vmstate_macio_oldworld = {
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 4bc6274..26cc469 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -174,7 +174,6 @@ void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
                             DBDMA_rw rw, DBDMA_flush flush,
                             void *opaque);
 void DBDMA_kick(DBDMAState *dbdma);
-void* DBDMA_init (MemoryRegion **dbdma_mem);
 
 #define TYPE_MAC_DBDMA "mac-dbdma"
 #define MAC_DBDMA(obj) OBJECT_CHECK(DBDMAState, (obj), TYPE_MAC_DBDMA)
-- 
1.7.10.4

[Qemu-devel] [PATCH 4/7] macio: pass channel into MACIOIDEState via qdev property

[Mark Cave-Ayland]

One of the reasons macio_ide_register_dma() needs to exist is because the
channel id isn't passed into the MACIO_IDE object. Pass in the channel id
using a qdev property to remove this requirement.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c        |   10 ++++++++--
 hw/misc/macio/macio.c |    4 +++-
 hw/ppc/mac.h          |    4 ++--
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 18ae952..19d5f5a 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -452,12 +452,18 @@ static void macio_ide_initfn(Object *obj)
     s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
 }
 
+static Property macio_ide_properties[] = {
+    DEFINE_PROP_UINT32("channel", MACIOIDEState, channel, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void macio_ide_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = macio_ide_realizefn;
     dc->reset = macio_ide_reset;
+    dc->props = macio_ide_properties;
     dc->vmsd = &vmstate_pmac;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
 }
@@ -487,10 +493,10 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
     }
 }
 
-void macio_ide_register_dma(MACIOIDEState *s, void *dbdma, int channel)
+void macio_ide_register_dma(MACIOIDEState *s, void *dbdma)
 {
     s->dbdma = dbdma;
-    DBDMA_register_channel(dbdma, channel, s->dma_irq,
+    DBDMA_register_channel(dbdma, s->channel, s->dma_irq,
                            pmac_ide_transfer, pmac_ide_flush, s);
 }
 
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index f459f17..41b377e 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -159,7 +159,9 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
     sysbus_dev = SYS_BUS_DEVICE(ide);
     sysbus_connect_irq(sysbus_dev, 0, irq0);
     sysbus_connect_irq(sysbus_dev, 1, irq1);
-    macio_ide_register_dma(ide, s->dbdma, dmaid);
+    qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
+    macio_ide_register_dma(ide, s->dbdma);
+
     object_property_set_bool(OBJECT(ide), true, "realized", errp);
 }
 
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index 300fc8a..b3a26c4 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -131,7 +131,7 @@ typedef struct MACIOIDEState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
-
+    uint32_t channel;
     qemu_irq real_ide_irq;
     qemu_irq real_dma_irq;
     qemu_irq ide_irq;
@@ -147,7 +147,7 @@ typedef struct MACIOIDEState {
 } MACIOIDEState;
 
 void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
-void macio_ide_register_dma(MACIOIDEState *ide, void *dbdma, int channel);
+void macio_ide_register_dma(MACIOIDEState *ide, void *dbdma);
 
 void macio_init(PCIDevice *dev,
                 MemoryRegion *pic_mem,
-- 
1.7.10.4

[Qemu-devel] [PATCH 5/7] macio: use object link between MACIO_IDE and MAC_DBDMA object

[Mark Cave-Ayland]

Using a standard QOM object link we can pass a reference to the MAC_DBDMA
controller to the MACIO_IDE object which removes the last external parameter
to macio_ide_register_dma().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c        |    9 ++++++---
 hw/misc/macio/macio.c |    3 ++-
 hw/ppc/mac.h          |    2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 19d5f5a..ce194c6 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -450,6 +450,10 @@ static void macio_ide_initfn(Object *obj)
     sysbus_init_irq(d, &s->real_dma_irq);
     s->dma_irq = qemu_allocate_irq(pmac_ide_irq, s, 0);
     s->ide_irq = qemu_allocate_irq(pmac_ide_irq, s, 1);
+
+    object_property_add_link(obj, "dbdma", TYPE_MAC_DBDMA,
+                             (Object **) &s->dbdma,
+                             qdev_prop_allow_set_link_before_realize, 0, NULL);
 }
 
 static Property macio_ide_properties[] = {
@@ -493,10 +497,9 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
     }
 }
 
-void macio_ide_register_dma(MACIOIDEState *s, void *dbdma)
+void macio_ide_register_dma(MACIOIDEState *s)
 {
-    s->dbdma = dbdma;
-    DBDMA_register_channel(dbdma, s->channel, s->dma_irq,
+    DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
                            pmac_ide_transfer, pmac_ide_flush, s);
 }
 
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 41b377e..9aa7e75 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -160,7 +160,8 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
     sysbus_connect_irq(sysbus_dev, 0, irq0);
     sysbus_connect_irq(sysbus_dev, 1, irq1);
     qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
-    macio_ide_register_dma(ide, s->dbdma);
+    object_property_set_link(OBJECT(ide), OBJECT(s->dbdma), "dbdma", errp);
+    macio_ide_register_dma(ide);
 
     object_property_set_bool(OBJECT(ide), true, "realized", errp);
 }
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index b3a26c4..b501af1 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -147,7 +147,7 @@ typedef struct MACIOIDEState {
 } MACIOIDEState;
 
 void macio_ide_init_drives(MACIOIDEState *ide, DriveInfo **hd_table);
-void macio_ide_register_dma(MACIOIDEState *ide, void *dbdma);
+void macio_ide_register_dma(MACIOIDEState *ide);
 
 void macio_init(PCIDevice *dev,
                 MemoryRegion *pic_mem,
-- 
1.7.10.4

[Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

[Mark Cave-Ayland]

Using this we can change the MACIO_IDE instance to register the channel
itself via a type method instead of requiring a separate
DBDMA_register_channel() function.

As a consequence of this it is now possible to remove the old external
macio_ide_register_dma() function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c             |   12 ++++++------
 hw/misc/macio/mac_dbdma.c  |    9 +++++----
 hw/misc/macio/macio.c      |    1 -
 include/hw/ppc/mac_dbdma.h |    9 ++++-----
 4 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index ce194c6..b296017 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -411,12 +411,18 @@ static const IDEDMAOps dbdma_ops = {
 static void macio_ide_realizefn(DeviceState *dev, Error **errp)
 {
     MACIOIDEState *s = MACIO_IDE(dev);
+    DBDMAState *dbdma;
 
     ide_init2(&s->bus, s->ide_irq);
 
     /* Register DMA callbacks */
     s->dma.ops = &dbdma_ops;
     s->bus.dma = &s->dma;
+
+    /* Register DBDMA channel */
+    dbdma = MAC_DBDMA(object_property_get_link(OBJECT(dev), "dbdma", errp));
+    dbdma->register_channel(dbdma, s->channel, s->dma_irq,
+                            pmac_ide_transfer, pmac_ide_flush, s);
 }
 
 static void pmac_ide_irq(void *opaque, int n, int level)
@@ -497,10 +503,4 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
     }
 }
 
-void macio_ide_register_dma(MACIOIDEState *s)
-{
-    DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
-                           pmac_ide_transfer, pmac_ide_flush, s);
-}
-
 type_init(macio_ide_register_types)
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 0eddf2e..addb97d 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -557,11 +557,10 @@ void DBDMA_kick(DBDMAState *dbdma)
     qemu_bh_schedule(dbdma->bh);
 }
 
-void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
-                            DBDMA_rw rw, DBDMA_flush flush,
-                            void *opaque)
+static void
+dbdma_register_channel(DBDMAState *s, int nchan, qemu_irq irq,
+                       DBDMA_rw rw, DBDMA_flush flush, void *opaque)
 {
-    DBDMAState *s = dbdma;
     DBDMA_channel *ch = &s->channels[nchan];
 
     DBDMA_DPRINTFCH(ch, "DBDMA_register_channel 0x%x\n", nchan);
@@ -903,6 +902,8 @@ static void mac_dbdma_init(Object *obj)
 
     memory_region_init_io(&s->mem, obj, &dbdma_ops, s, "dbdma", 0x1000);
     sysbus_init_mmio(sbd, &s->mem);
+
+    s->register_channel = dbdma_register_channel;
 }
 
 static void mac_dbdma_realize(DeviceState *dev, Error **errp)
diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 9aa7e75..533331a 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -161,7 +161,6 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
     sysbus_connect_irq(sysbus_dev, 1, irq1);
     qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
     object_property_set_link(OBJECT(ide), OBJECT(s->dbdma), "dbdma", errp);
-    macio_ide_register_dma(ide);
 
     object_property_set_bool(OBJECT(ide), true, "realized", errp);
 }
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index 26cc469..d6a38c5 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -160,19 +160,18 @@ typedef struct DBDMA_channel {
     dbdma_cmd current;
 } DBDMA_channel;
 
-typedef struct {
+typedef struct DBDMAState {
     SysBusDevice parent_obj;
 
     MemoryRegion mem;
     DBDMA_channel channels[DBDMA_CHANNELS];
     QEMUBH *bh;
+
+    void (*register_channel)(struct DBDMAState *s, int nchan, qemu_irq irq,
+                             DBDMA_rw rw, DBDMA_flush flush, void *opaque);
 } DBDMAState;
 
 /* Externally callable functions */
-
-void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
-                            DBDMA_rw rw, DBDMA_flush flush,
-                            void *opaque);
 void DBDMA_kick(DBDMAState *dbdma);
 
 #define TYPE_MAC_DBDMA "mac-dbdma"
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

[David Gibson]

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

On Sun, Sep 24, 2017 at 03:47:45PM +0100, Mark Cave-Ayland wrote:
> Using this we can change the MACIO_IDE instance to register the channel
> itself via a type method instead of requiring a separate
> DBDMA_register_channel() function.
> 
> As a consequence of this it is now possible to remove the old external
> macio_ide_register_dma() function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Ok, two concerns about this.

First, you've added the function pointer to the instance, not to the
class, which is not how a QOM method would normally be done.

More generally, though, why is a method preferable to a plain
function?  AFAICT it's not plausible that there will ever be more than
one implementation of the method.

Same comments apply to patch 7/7.

> ---
>  hw/ide/macio.c             |   12 ++++++------
>  hw/misc/macio/mac_dbdma.c  |    9 +++++----
>  hw/misc/macio/macio.c      |    1 -
>  include/hw/ppc/mac_dbdma.h |    9 ++++-----
>  4 files changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index ce194c6..b296017 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -411,12 +411,18 @@ static const IDEDMAOps dbdma_ops = {
>  static void macio_ide_realizefn(DeviceState *dev, Error **errp)
>  {
>      MACIOIDEState *s = MACIO_IDE(dev);
> +    DBDMAState *dbdma;
>  
>      ide_init2(&s->bus, s->ide_irq);
>  
>      /* Register DMA callbacks */
>      s->dma.ops = &dbdma_ops;
>      s->bus.dma = &s->dma;
> +
> +    /* Register DBDMA channel */
> +    dbdma = MAC_DBDMA(object_property_get_link(OBJECT(dev), "dbdma", errp));
> +    dbdma->register_channel(dbdma, s->channel, s->dma_irq,
> +                            pmac_ide_transfer, pmac_ide_flush, s);
>  }
>  
>  static void pmac_ide_irq(void *opaque, int n, int level)
> @@ -497,10 +503,4 @@ void macio_ide_init_drives(MACIOIDEState *s, DriveInfo **hd_table)
>      }
>  }
>  
> -void macio_ide_register_dma(MACIOIDEState *s)
> -{
> -    DBDMA_register_channel(s->dbdma, s->channel, s->dma_irq,
> -                           pmac_ide_transfer, pmac_ide_flush, s);
> -}
> -
>  type_init(macio_ide_register_types)
> diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
> index 0eddf2e..addb97d 100644
> --- a/hw/misc/macio/mac_dbdma.c
> +++ b/hw/misc/macio/mac_dbdma.c
> @@ -557,11 +557,10 @@ void DBDMA_kick(DBDMAState *dbdma)
>      qemu_bh_schedule(dbdma->bh);
>  }
>  
> -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
> -                            DBDMA_rw rw, DBDMA_flush flush,
> -                            void *opaque)
> +static void
> +dbdma_register_channel(DBDMAState *s, int nchan, qemu_irq irq,
> +                       DBDMA_rw rw, DBDMA_flush flush, void *opaque)
>  {
> -    DBDMAState *s = dbdma;
>      DBDMA_channel *ch = &s->channels[nchan];
>  
>      DBDMA_DPRINTFCH(ch, "DBDMA_register_channel 0x%x\n", nchan);
> @@ -903,6 +902,8 @@ static void mac_dbdma_init(Object *obj)
>  
>      memory_region_init_io(&s->mem, obj, &dbdma_ops, s, "dbdma", 0x1000);
>      sysbus_init_mmio(sbd, &s->mem);
> +
> +    s->register_channel = dbdma_register_channel;
>  }
>  
>  static void mac_dbdma_realize(DeviceState *dev, Error **errp)
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 9aa7e75..533331a 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -161,7 +161,6 @@ static void macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>      sysbus_connect_irq(sysbus_dev, 1, irq1);
>      qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
>      object_property_set_link(OBJECT(ide), OBJECT(s->dbdma), "dbdma", errp);
> -    macio_ide_register_dma(ide);
>  
>      object_property_set_bool(OBJECT(ide), true, "realized", errp);
>  }
> diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
> index 26cc469..d6a38c5 100644
> --- a/include/hw/ppc/mac_dbdma.h
> +++ b/include/hw/ppc/mac_dbdma.h
> @@ -160,19 +160,18 @@ typedef struct DBDMA_channel {
>      dbdma_cmd current;
>  } DBDMA_channel;
>  
> -typedef struct {
> +typedef struct DBDMAState {
>      SysBusDevice parent_obj;
>  
>      MemoryRegion mem;
>      DBDMA_channel channels[DBDMA_CHANNELS];
>      QEMUBH *bh;
> +
> +    void (*register_channel)(struct DBDMAState *s, int nchan, qemu_irq irq,
> +                             DBDMA_rw rw, DBDMA_flush flush, void *opaque);
>  } DBDMAState;
>  
>  /* Externally callable functions */
> -
> -void DBDMA_register_channel(void *dbdma, int nchan, qemu_irq irq,
> -                            DBDMA_rw rw, DBDMA_flush flush,
> -                            void *opaque);
>  void DBDMA_kick(DBDMAState *dbdma);
>  
>  #define TYPE_MAC_DBDMA "mac-dbdma"

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

[Mark Cave-Ayland]

On 26/09/17 04:47, David Gibson wrote:

> On Sun, Sep 24, 2017 at 03:47:45PM +0100, Mark Cave-Ayland wrote:
>> Using this we can change the MACIO_IDE instance to register the channel
>> itself via a type method instead of requiring a separate
>> DBDMA_register_channel() function.
>>
>> As a consequence of this it is now possible to remove the old external
>> macio_ide_register_dma() function.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Ok, two concerns about this.
> 
> First, you've added the function pointer to the instance, not to the
> class, which is not how a QOM method would normally be done.

Yeah I did think about whether I needed to create a full class but was
torn since as you say there is only one implementation.

> More generally, though, why is a method preferable to a plain
> function?  AFAICT it's not plausible that there will ever be more than
> one implementation of the method.
> 
> Same comments apply to patch 7/7.

For me it's really for encapsulation. It seems a little odd requiring a
global function to configure a QOM object to which I already have a
reference.

If I were to redo the last 2 patches using a proper class, would you
accept them?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

[David Gibson]

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

On Thu, Sep 28, 2017 at 07:40:18AM +0100, Mark Cave-Ayland wrote:
> On 26/09/17 04:47, David Gibson wrote:
> 
> > On Sun, Sep 24, 2017 at 03:47:45PM +0100, Mark Cave-Ayland wrote:
> >> Using this we can change the MACIO_IDE instance to register the channel
> >> itself via a type method instead of requiring a separate
> >> DBDMA_register_channel() function.
> >>
> >> As a consequence of this it is now possible to remove the old external
> >> macio_ide_register_dma() function.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > 
> > Ok, two concerns about this.
> > 
> > First, you've added the function pointer to the instance, not to the
> > class, which is not how a QOM method would normally be done.
> 
> Yeah I did think about whether I needed to create a full class but was
> torn since as you say there is only one implementation.
> 
> > More generally, though, why is a method preferable to a plain
> > function?  AFAICT it's not plausible that there will ever be more than
> > one implementation of the method.
> > 
> > Same comments apply to patch 7/7.
> 
> For me it's really for encapsulation. It seems a little odd requiring a
> global function to configure a QOM object to which I already have a
> reference.

Instead you're using the method which is defined in a global type
definition.  I don't think it really makes any different to
encapsulation.

> If I were to redo the last 2 patches using a proper class, would you
> accept them?
> 
> 
> ATB,
> 
> Mark.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[Qemu-devel] [PATCH 7/7] mac_dbdma: change DBDMA_kick to a MAC_DBDMA type method

[Mark Cave-Ayland]

With this we can now remove the last external method used to interface
between macio and DBDMA.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ide/macio.c             |    3 ++-
 hw/misc/macio/mac_dbdma.c  |   19 ++++++++++---------
 include/hw/ppc/mac_dbdma.h |    4 +---
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index b296017..6f7f286 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -384,6 +384,7 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
                             BlockCompletionFunc *cb)
 {
     MACIOIDEState *m = container_of(dma, MACIOIDEState, dma);
+    DBDMAState *dbdma = (DBDMAState *)m->dbdma;
 
     s->io_buffer_index = 0;
     if (s->drive_kind == IDE_CD) {
@@ -399,7 +400,7 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
     MACIO_DPRINTF("-------------------------\n");
 
     m->dma_active = true;
-    DBDMA_kick(m->dbdma);
+    dbdma->kick(dbdma);
 }
 
 static const IDEDMAOps dbdma_ops = {
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index addb97d..f8375db 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -301,6 +301,11 @@ wait:
         channel_run(ch);
 }
 
+static void dbdma_kick(DBDMAState *dbdma)
+{
+    qemu_bh_schedule(dbdma->bh);
+}
+
 static void start_output(DBDMA_channel *ch, int key, uint32_t addr,
                         uint16_t req_count, int is_last)
 {
@@ -381,7 +386,7 @@ static void load_word(DBDMA_channel *ch, int key, uint32_t addr,
     next(ch);
 
 wait:
-    DBDMA_kick(dbdma_from_ch(ch));
+    dbdma_kick(dbdma_from_ch(ch));
 }
 
 static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
@@ -413,7 +418,7 @@ static void store_word(DBDMA_channel *ch, int key, uint32_t addr,
     next(ch);
 
 wait:
-    DBDMA_kick(dbdma_from_ch(ch));
+    dbdma_kick(dbdma_from_ch(ch));
 }
 
 static void nop(DBDMA_channel *ch)
@@ -430,7 +435,7 @@ static void nop(DBDMA_channel *ch)
     conditional_branch(ch);
 
 wait:
-    DBDMA_kick(dbdma_from_ch(ch));
+    dbdma_kick(dbdma_from_ch(ch));
 }
 
 static void stop(DBDMA_channel *ch)
@@ -552,11 +557,6 @@ static void DBDMA_run_bh(void *opaque)
     DBDMA_DPRINTF("<- DBDMA_run_bh\n");
 }
 
-void DBDMA_kick(DBDMAState *dbdma)
-{
-    qemu_bh_schedule(dbdma->bh);
-}
-
 static void
 dbdma_register_channel(DBDMAState *s, int nchan, qemu_irq irq,
                        DBDMA_rw rw, DBDMA_flush flush, void *opaque)
@@ -686,7 +686,7 @@ static void dbdma_control_write(DBDMA_channel *ch)
 
     /* If active, make sure the BH gets to run */
     if (status & ACTIVE) {
-        DBDMA_kick(dbdma_from_ch(ch));
+        dbdma_kick(dbdma_from_ch(ch));
     }
 }
 
@@ -904,6 +904,7 @@ static void mac_dbdma_init(Object *obj)
     sysbus_init_mmio(sbd, &s->mem);
 
     s->register_channel = dbdma_register_channel;
+    s->kick = dbdma_kick;
 }
 
 static void mac_dbdma_realize(DeviceState *dev, Error **errp)
diff --git a/include/hw/ppc/mac_dbdma.h b/include/hw/ppc/mac_dbdma.h
index d6a38c5..a30f8d8 100644
--- a/include/hw/ppc/mac_dbdma.h
+++ b/include/hw/ppc/mac_dbdma.h
@@ -169,11 +169,9 @@ typedef struct DBDMAState {
 
     void (*register_channel)(struct DBDMAState *s, int nchan, qemu_irq irq,
                              DBDMA_rw rw, DBDMA_flush flush, void *opaque);
+    void (*kick)(struct DBDMAState *s);
 } DBDMAState;
 
-/* Externally callable functions */
-void DBDMA_kick(DBDMAState *dbdma);
-
 #define TYPE_MAC_DBDMA "mac-dbdma"
 #define MAC_DBDMA(obj) OBJECT_CHECK(DBDMAState, (obj), TYPE_MAC_DBDMA)
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/7] mac_dbdma: tidy-up and QOMify

[David Gibson]

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

On Sun, Sep 24, 2017 at 03:47:39PM +0100, Mark Cave-Ayland wrote:
> Whilst looking at implementing another DBDMA device for the Mac machines
> I noticed a couple of things: firstly there were some unused fields still
> in DBDMAState, and secondly the existing code still used global functions
> to register DMA channels and handle the relationship between macio IDE and
> DBDMA.
> 
> This patchset removes the now-unused fields from DBDMA state, QOMifys the
> DBDMA device, uses a QOM object link to allow the macio IDE object to
> reference the DBDMA device, and then finally removes the global DBDMA_*
> functions substituting them instead for QOM methods.
> 
> Note: this patchset does not apply to master but on top of David's
> ppc-for-2.11 branch since there are merge conflicts with my previous
> patchset. Hopefully the Based-On line below is enough to keep patchew
> happy, even though it wasn't the final version applied to the ppc-for-2.11
> branch.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Based-on: 1505668548-16616-1-git-send-email-mark.cave-ayland@ilande.co.uk (ppc: more Mac-related fixups)

I've applied 1-5/7.  There are a couple of details I'm not 100%
convinced on, but it's still better than what was there before.  6 & 7
I'm still thinking about.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]