[Qemu-devel] [PATCH 0/4] cuda/mos6522 migration fixes

[Qemu-devel] [PATCH 0/4] cuda/mos6522 migration fixes

[Mark Cave-Ayland]

Whilst performing a random migration test for the Mac machines I noticed
a regression (patch 1) which prevented the loadvm from completing
successfully. A big thank you to Peter and David on IRC who pointed me
in the right direction in order to fix the bug.

Once that was working I spent a bit more time analysing the migration
stream and realised that the mos6522 device state wasn't being embedded
within the CUDA device, but instead being maintained separately which is
solved by patch 2.

Patch 3 is something I noticed whilst rearranging the existing code based
upon my better understanding of QOM/qdev and ensures that the timer frequency
is always set correctly post-migration for the device and its parent class.
This leaves no remaining functionality in the mos6522 realize function and so
allows it to be removed.

Finally patch 4 was suggested by Peter on IRC whilst helping me investigate
the original migration issue, and removes the last remaining user of
VMSTATE_TIMER_PTR_TEST from the codebase.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (4):
  mos6522: fix vmstate_mos6522_timer version in vmstate_mos6522
  cuda: embed mos6522_cuda device directly rather than using QOM object
    link
  mos6522: move timer frequency initialisation to mos6522_reset
  mos6522: convert VMSTATE_TIMER_PTR_TEST to VMSTATE_TIMER_PTR

 hw/misc/macio/cuda.c         | 50 +++++++++++++++++++-------------------------
 hw/misc/mos6522.c            | 26 ++++++-----------------
 include/hw/misc/macio/cuda.h | 27 +++++++++++-------------
 include/hw/misc/mos6522.h    |  4 +++-
 4 files changed, 42 insertions(+), 65 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/4] mos6522: fix vmstate_mos6522_timer version in vmstate_mos6522

[Mark Cave-Ayland]

This was accidentally introduced when extracting the 6522 VIA functionality
from the CUDA device, and prevents loadvm from completing successfully.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mos6522.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 6163cea6ab..dd56c796e4 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -405,7 +405,7 @@ static const VMStateDescription vmstate_mos6522 = {
         VMSTATE_UINT8(ifr, MOS6522State),
         VMSTATE_UINT8(ier, MOS6522State),
         VMSTATE_UINT8(anh, MOS6522State),
-        VMSTATE_STRUCT_ARRAY(timers, MOS6522State, 2, 1,
+        VMSTATE_STRUCT_ARRAY(timers, MOS6522State, 2, 0,
                              vmstate_mos6522_timer, MOS6522Timer),
         VMSTATE_END_OF_LIST()
     }
-- 
2.11.0

[Qemu-devel] [PATCH 2/4] cuda: embed mos6522_cuda device directly rather than using QOM object link

[Mark Cave-Ayland]

Examining the migration stream it can be seen that the mos6522 device state is
being stored separately rather than as part of the CUDA device which is
incorrect (and likely to cause issues if another mos6522 device is added to
the machine).

Resolve this by embedding the mos6522_cuda device directly within the CUDA
device rather than using a QOM object link to reference the device separately.

Note that we also bump the version in vmstate_cuda to reflect this change: this
isn't particularly important for the moment as the Mac machine migration isn't
100% reliable due to issues migrating the timebase under TCG.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c         | 44 ++++++++++++++++++--------------------------
 hw/misc/mos6522.c            |  2 +-
 include/hw/misc/macio/cuda.h | 27 ++++++++++++---------------
 include/hw/misc/mos6522.h    |  2 ++
 4 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index bd9b862034..8aba2e63ec 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -65,7 +65,7 @@ static void cuda_receive_packet_from_host(CUDAState *s,
 static uint64_t cuda_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
 {
     MOS6522CUDAState *mcs = container_of(s, MOS6522CUDAState, parent_obj);
-    CUDAState *cs = mcs->cuda;
+    CUDAState *cs = container_of(mcs, CUDAState, mos6522_cuda);
 
     /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup */
     uint64_t tb_diff = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
@@ -78,7 +78,7 @@ static uint64_t cuda_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
 static uint64_t cuda_get_load_time(MOS6522State *s, MOS6522Timer *ti)
 {
     MOS6522CUDAState *mcs = container_of(s, MOS6522CUDAState, parent_obj);
-    CUDAState *cs = mcs->cuda;
+    CUDAState *cs = container_of(mcs, CUDAState, mos6522_cuda);
 
     uint64_t load_time = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
                                   cs->tb_frequency, NANOSECONDS_PER_SECOND);
@@ -88,7 +88,7 @@ static uint64_t cuda_get_load_time(MOS6522State *s, MOS6522Timer *ti)
 static void cuda_set_sr_int(void *opaque)
 {
     CUDAState *s = opaque;
-    MOS6522CUDAState *mcs = s->mos6522_cuda;
+    MOS6522CUDAState *mcs = &s->mos6522_cuda;
     MOS6522State *ms = MOS6522(mcs);
     MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
 
@@ -97,7 +97,7 @@ static void cuda_set_sr_int(void *opaque)
 
 static void cuda_delay_set_sr_int(CUDAState *s)
 {
-    MOS6522CUDAState *mcs = s->mos6522_cuda;
+    MOS6522CUDAState *mcs = &s->mos6522_cuda;
     MOS6522State *ms = MOS6522(mcs);
     MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
     int64_t expire;
@@ -117,7 +117,7 @@ static void cuda_delay_set_sr_int(CUDAState *s)
 /* NOTE: TIP and TREQ are negated */
 static void cuda_update(CUDAState *s)
 {
-    MOS6522CUDAState *mcs = s->mos6522_cuda;
+    MOS6522CUDAState *mcs = &s->mos6522_cuda;
     MOS6522State *ms = MOS6522(mcs);
     int packet_received, len;
 
@@ -462,7 +462,7 @@ static void cuda_receive_packet_from_host(CUDAState *s,
 static uint64_t mos6522_cuda_read(void *opaque, hwaddr addr, unsigned size)
 {
     CUDAState *s = opaque;
-    MOS6522CUDAState *mcs = s->mos6522_cuda;
+    MOS6522CUDAState *mcs = &s->mos6522_cuda;
     MOS6522State *ms = MOS6522(mcs);
 
     addr = (addr >> 9) & 0xf;
@@ -473,7 +473,7 @@ static void mos6522_cuda_write(void *opaque, hwaddr addr, uint64_t val,
                                unsigned size)
 {
     CUDAState *s = opaque;
-    MOS6522CUDAState *mcs = s->mos6522_cuda;
+    MOS6522CUDAState *mcs = &s->mos6522_cuda;
     MOS6522State *ms = MOS6522(mcs);
 
     addr = (addr >> 9) & 0xf;
@@ -492,9 +492,11 @@ static const MemoryRegionOps mos6522_cuda_ops = {
 
 static const VMStateDescription vmstate_cuda = {
     .name = "cuda",
-    .version_id = 4,
-    .minimum_version_id = 4,
+    .version_id = 5,
+    .minimum_version_id = 5,
     .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(mos6522_cuda.parent_obj, CUDAState, 0, vmstate_mos6522,
+                       MOS6522State),
         VMSTATE_UINT8(last_b, CUDAState),
         VMSTATE_UINT8(last_acr, CUDAState),
         VMSTATE_INT32(data_in_size, CUDAState),
@@ -530,12 +532,8 @@ static void cuda_realize(DeviceState *dev, Error **errp)
     DeviceState *d;
     struct tm tm;
 
-    d = qdev_create(NULL, TYPE_MOS6522_CUDA);
-    object_property_set_link(OBJECT(d), OBJECT(s), "cuda", errp);
-    qdev_init_nofail(d);
-    s->mos6522_cuda = MOS6522_CUDA(d);
-
     /* Pass IRQ from 6522 */
+    d = DEVICE(&s->mos6522_cuda);
     ms = MOS6522(d);
     sbd = SYS_BUS_DEVICE(s);
     sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
@@ -556,6 +554,10 @@ static void cuda_init(Object *obj)
     CUDAState *s = CUDA(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
+    object_initialize(&s->mos6522_cuda, sizeof(s->mos6522_cuda),
+                      TYPE_MOS6522_CUDA);
+    qdev_set_parent_bus(DEVICE(&s->mos6522_cuda), sysbus_get_default());
+
     memory_region_init_io(&s->mem, obj, &mos6522_cuda_ops, s, "cuda", 0x2000);
     sysbus_init_mmio(sbd, &s->mem);
 
@@ -590,8 +592,9 @@ static const TypeInfo cuda_type_info = {
 static void mos6522_cuda_portB_write(MOS6522State *s)
 {
     MOS6522CUDAState *mcs = container_of(s, MOS6522CUDAState, parent_obj);
+    CUDAState *cs = container_of(mcs, CUDAState, mos6522_cuda);
 
-    cuda_update(mcs->cuda);
+    cuda_update(cs);
 }
 
 static void mos6522_cuda_realize(DeviceState *dev, Error **errp)
@@ -605,16 +608,6 @@ static void mos6522_cuda_realize(DeviceState *dev, Error **errp)
     ms->timers[1].frequency = (SCALE_US * 6000) / 4700;
 }
 
-static void mos6522_cuda_init(Object *obj)
-{
-    MOS6522CUDAState *s = MOS6522_CUDA(obj);
-
-    object_property_add_link(obj, "cuda", TYPE_CUDA,
-                             (Object **) &s->cuda,
-                             qdev_prop_allow_set_link_before_realize,
-                             0, NULL);
-}
-
 static void mos6522_cuda_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -632,7 +625,6 @@ static const TypeInfo mos6522_cuda_type_info = {
     .name = TYPE_MOS6522_CUDA,
     .parent = TYPE_MOS6522,
     .instance_size = sizeof(MOS6522CUDAState),
-    .instance_init = mos6522_cuda_init,
     .class_init = mos6522_cuda_class_init,
 };
 
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index dd56c796e4..facdaed5d5 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -390,7 +390,7 @@ static const VMStateDescription vmstate_mos6522_timer = {
     }
 };
 
-static const VMStateDescription vmstate_mos6522 = {
+const VMStateDescription vmstate_mos6522 = {
     .name = "mos6522",
     .version_id = 0,
     .minimum_version_id = 0,
diff --git a/include/hw/misc/macio/cuda.h b/include/hw/misc/macio/cuda.h
index 494b709579..7dad469142 100644
--- a/include/hw/misc/macio/cuda.h
+++ b/include/hw/misc/macio/cuda.h
@@ -54,12 +54,21 @@
 #define CUDA_TIMER_TICKLE              0x24
 #define CUDA_COMBINED_FORMAT_IIC       0x25
 
+
+/* MOS6522 CUDA */
+typedef struct MOS6522CUDAState {
+    /*< private >*/
+    MOS6522State parent_obj;
+} MOS6522CUDAState;
+
+#define TYPE_MOS6522_CUDA "mos6522-cuda"
+#define MOS6522_CUDA(obj) OBJECT_CHECK(MOS6522CUDAState, (obj), \
+                                       TYPE_MOS6522_CUDA)
+
 /* Cuda */
 #define TYPE_CUDA "cuda"
 #define CUDA(obj) OBJECT_CHECK(CUDAState, (obj), TYPE_CUDA)
 
-typedef struct MOS6522CUDAState MOS6522CUDAState;
-
 typedef struct CUDAState {
     /*< private >*/
     SysBusDevice parent_obj;
@@ -67,7 +76,7 @@ typedef struct CUDAState {
     MemoryRegion mem;
 
     ADBBusState adb_bus;
-    MOS6522CUDAState *mos6522_cuda;
+    MOS6522CUDAState mos6522_cuda;
 
     uint32_t tick_offset;
     uint64_t tb_frequency;
@@ -92,16 +101,4 @@ typedef struct CUDAState {
     QEMUTimer *adb_poll_timer;
 } CUDAState;
 
-/* MOS6522 CUDA */
-struct MOS6522CUDAState {
-    /*< private >*/
-    MOS6522State parent_obj;
-
-    CUDAState *cuda;
-};
-
-#define TYPE_MOS6522_CUDA "mos6522-cuda"
-#define MOS6522_CUDA(obj) OBJECT_CHECK(MOS6522CUDAState, (obj), \
-                                       TYPE_MOS6522_CUDA)
-
 #endif /* CUDA_H */
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index a53c161b00..cb0fd7db78 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -146,6 +146,8 @@ typedef struct MOS6522DeviceClass {
 #define MOS6522_DEVICE_GET_CLASS(obj) \
     OBJECT_GET_CLASS(MOS6522DeviceClass, (obj), TYPE_MOS6522)
 
+extern const VMStateDescription vmstate_mos6522;
+
 uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
 void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/4] cuda: embed mos6522_cuda device directly rather than using QOM object link

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:17:49PM +0100, Mark Cave-Ayland wrote:
> Examining the migration stream it can be seen that the mos6522 device state is
> being stored separately rather than as part of the CUDA device which is
> incorrect (and likely to cause issues if another mos6522 device is added to
> the machine).
> 
> Resolve this by embedding the mos6522_cuda device directly within the CUDA
> device rather than using a QOM object link to reference the device separately.
> 
> Note that we also bump the version in vmstate_cuda to reflect this change: this
> isn't particularly important for the moment as the Mac machine migration isn't
> 100% reliable due to issues migrating the timebase under TCG.

And apart from that the mac machine types aren't yet versioned, so
we're not really trying to support migration between different qemu
versions anyway.

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/macio/cuda.c         | 44 ++++++++++++++++++--------------------------
>  hw/misc/mos6522.c            |  2 +-
>  include/hw/misc/macio/cuda.h | 27 ++++++++++++---------------
>  include/hw/misc/mos6522.h    |  2 ++
>  4 files changed, 33 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index bd9b862034..8aba2e63ec 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -65,7 +65,7 @@ static void cuda_receive_packet_from_host(CUDAState *s,
>  static uint64_t cuda_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
>  {
>      MOS6522CUDAState *mcs = container_of(s, MOS6522CUDAState, parent_obj);
> -    CUDAState *cs = mcs->cuda;
> +    CUDAState *cs = container_of(mcs, CUDAState, mos6522_cuda);
>  
>      /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup */
>      uint64_t tb_diff = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> @@ -78,7 +78,7 @@ static uint64_t cuda_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
>  static uint64_t cuda_get_load_time(MOS6522State *s, MOS6522Timer *ti)
>  {
>      MOS6522CUDAState *mcs = container_of(s, MOS6522CUDAState, parent_obj);
> -    CUDAState *cs = mcs->cuda;
> +    CUDAState *cs = container_of(mcs, CUDAState, mos6522_cuda);
>  
>      uint64_t load_time = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>                                    cs->tb_frequency, NANOSECONDS_PER_SECOND);
> @@ -88,7 +88,7 @@ static uint64_t cuda_get_load_time(MOS6522State *s, MOS6522Timer *ti)
>  static void cuda_set_sr_int(void *opaque)
>  {
>      CUDAState *s = opaque;
> -    MOS6522CUDAState *mcs = s->mos6522_cuda;
> +    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>      MOS6522State *ms = MOS6522(mcs);
>      MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>  
> @@ -97,7 +97,7 @@ static void cuda_set_sr_int(void *opaque)
>  
>  static void cuda_delay_set_sr_int(CUDAState *s)
>  {
> -    MOS6522CUDAState *mcs = s->mos6522_cuda;
> +    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>      MOS6522State *ms = MOS6522(mcs);
>      MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
>      int64_t expire;
> @@ -117,7 +117,7 @@ static void cuda_delay_set_sr_int(CUDAState *s)
>  /* NOTE: TIP and TREQ are negated */
>  static void cuda_update(CUDAState *s)
>  {
> -    MOS6522CUDAState *mcs = s->mos6522_cuda;
> +    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>      MOS6522State *ms = MOS6522(mcs);
>      int packet_received, len;
>  
> @@ -462,7 +462,7 @@ static void cuda_receive_packet_from_host(CUDAState *s,
>  static uint64_t mos6522_cuda_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      CUDAState *s = opaque;
> -    MOS6522CUDAState *mcs = s->mos6522_cuda;
> +    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>      MOS6522State *ms = MOS6522(mcs);
>  
>      addr = (addr >> 9) & 0xf;
> @@ -473,7 +473,7 @@ static void mos6522_cuda_write(void *opaque, hwaddr addr, uint64_t val,
>                                 unsigned size)
>  {
>      CUDAState *s = opaque;
> -    MOS6522CUDAState *mcs = s->mos6522_cuda;
> +    MOS6522CUDAState *mcs = &s->mos6522_cuda;
>      MOS6522State *ms = MOS6522(mcs);
>  
>      addr = (addr >> 9) & 0xf;
> @@ -492,9 +492,11 @@ static const MemoryRegionOps mos6522_cuda_ops = {
>  
>  static const VMStateDescription vmstate_cuda = {
>      .name = "cuda",
> -    .version_id = 4,
> -    .minimum_version_id = 4,
> +    .version_id = 5,
> +    .minimum_version_id = 5,
>      .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT(mos6522_cuda.parent_obj, CUDAState, 0, vmstate_mos6522,
> +                       MOS6522State),
>          VMSTATE_UINT8(last_b, CUDAState),
>          VMSTATE_UINT8(last_acr, CUDAState),
>          VMSTATE_INT32(data_in_size, CUDAState),
> @@ -530,12 +532,8 @@ static void cuda_realize(DeviceState *dev, Error **errp)
>      DeviceState *d;
>      struct tm tm;
>  
> -    d = qdev_create(NULL, TYPE_MOS6522_CUDA);
> -    object_property_set_link(OBJECT(d), OBJECT(s), "cuda", errp);
> -    qdev_init_nofail(d);
> -    s->mos6522_cuda = MOS6522_CUDA(d);
> -
>      /* Pass IRQ from 6522 */
> +    d = DEVICE(&s->mos6522_cuda);
>      ms = MOS6522(d);
>      sbd = SYS_BUS_DEVICE(s);
>      sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
> @@ -556,6 +554,10 @@ static void cuda_init(Object *obj)
>      CUDAState *s = CUDA(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>  
> +    object_initialize(&s->mos6522_cuda, sizeof(s->mos6522_cuda),
> +                      TYPE_MOS6522_CUDA);
> +    qdev_set_parent_bus(DEVICE(&s->mos6522_cuda), sysbus_get_default());
> +
>      memory_region_init_io(&s->mem, obj, &mos6522_cuda_ops, s, "cuda", 0x2000);
>      sysbus_init_mmio(sbd, &s->mem);
>  
> @@ -590,8 +592,9 @@ static const TypeInfo cuda_type_info = {
>  static void mos6522_cuda_portB_write(MOS6522State *s)
>  {
>      MOS6522CUDAState *mcs = container_of(s, MOS6522CUDAState, parent_obj);
> +    CUDAState *cs = container_of(mcs, CUDAState, mos6522_cuda);
>  
> -    cuda_update(mcs->cuda);
> +    cuda_update(cs);
>  }
>  
>  static void mos6522_cuda_realize(DeviceState *dev, Error **errp)
> @@ -605,16 +608,6 @@ static void mos6522_cuda_realize(DeviceState *dev, Error **errp)
>      ms->timers[1].frequency = (SCALE_US * 6000) / 4700;
>  }
>  
> -static void mos6522_cuda_init(Object *obj)
> -{
> -    MOS6522CUDAState *s = MOS6522_CUDA(obj);
> -
> -    object_property_add_link(obj, "cuda", TYPE_CUDA,
> -                             (Object **) &s->cuda,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0, NULL);
> -}
> -
>  static void mos6522_cuda_class_init(ObjectClass *oc, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -632,7 +625,6 @@ static const TypeInfo mos6522_cuda_type_info = {
>      .name = TYPE_MOS6522_CUDA,
>      .parent = TYPE_MOS6522,
>      .instance_size = sizeof(MOS6522CUDAState),
> -    .instance_init = mos6522_cuda_init,
>      .class_init = mos6522_cuda_class_init,
>  };
>  
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index dd56c796e4..facdaed5d5 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -390,7 +390,7 @@ static const VMStateDescription vmstate_mos6522_timer = {
>      }
>  };
>  
> -static const VMStateDescription vmstate_mos6522 = {
> +const VMStateDescription vmstate_mos6522 = {
>      .name = "mos6522",
>      .version_id = 0,
>      .minimum_version_id = 0,
> diff --git a/include/hw/misc/macio/cuda.h b/include/hw/misc/macio/cuda.h
> index 494b709579..7dad469142 100644
> --- a/include/hw/misc/macio/cuda.h
> +++ b/include/hw/misc/macio/cuda.h
> @@ -54,12 +54,21 @@
>  #define CUDA_TIMER_TICKLE              0x24
>  #define CUDA_COMBINED_FORMAT_IIC       0x25
>  
> +
> +/* MOS6522 CUDA */
> +typedef struct MOS6522CUDAState {
> +    /*< private >*/
> +    MOS6522State parent_obj;
> +} MOS6522CUDAState;
> +
> +#define TYPE_MOS6522_CUDA "mos6522-cuda"
> +#define MOS6522_CUDA(obj) OBJECT_CHECK(MOS6522CUDAState, (obj), \
> +                                       TYPE_MOS6522_CUDA)
> +
>  /* Cuda */
>  #define TYPE_CUDA "cuda"
>  #define CUDA(obj) OBJECT_CHECK(CUDAState, (obj), TYPE_CUDA)
>  
> -typedef struct MOS6522CUDAState MOS6522CUDAState;
> -
>  typedef struct CUDAState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -67,7 +76,7 @@ typedef struct CUDAState {
>      MemoryRegion mem;
>  
>      ADBBusState adb_bus;
> -    MOS6522CUDAState *mos6522_cuda;
> +    MOS6522CUDAState mos6522_cuda;
>  
>      uint32_t tick_offset;
>      uint64_t tb_frequency;
> @@ -92,16 +101,4 @@ typedef struct CUDAState {
>      QEMUTimer *adb_poll_timer;
>  } CUDAState;
>  
> -/* MOS6522 CUDA */
> -struct MOS6522CUDAState {
> -    /*< private >*/
> -    MOS6522State parent_obj;
> -
> -    CUDAState *cuda;
> -};
> -
> -#define TYPE_MOS6522_CUDA "mos6522-cuda"
> -#define MOS6522_CUDA(obj) OBJECT_CHECK(MOS6522CUDAState, (obj), \
> -                                       TYPE_MOS6522_CUDA)
> -
>  #endif /* CUDA_H */
> diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
> index a53c161b00..cb0fd7db78 100644
> --- a/include/hw/misc/mos6522.h
> +++ b/include/hw/misc/mos6522.h
> @@ -146,6 +146,8 @@ typedef struct MOS6522DeviceClass {
>  #define MOS6522_DEVICE_GET_CLASS(obj) \
>      OBJECT_GET_CLASS(MOS6522DeviceClass, (obj), TYPE_MOS6522)
>  
> +extern const VMStateDescription vmstate_mos6522;
> +
>  uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
>  void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);
>  

-- 
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 3/4] mos6522: move timer frequency initialisation to mos6522_reset

[Mark Cave-Ayland]

The 6522 VIA timer frequency cannot be set by altering registers within the
device itself and hence it is a fixed property of the machine.

Move the initialisation of the timer frequency to the mos6522 reset function
and ensure that any subclasses always call the parent reset function so that
it isn't required to store the timer frequency within vmstate_mos6522_timer
itself.

By moving the frequency initialisation to the device reset function then we
find that the realize function for both mos6522 and mos6522_cuda becomes
obsolete and can simply be removed.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c      |  6 +++---
 hw/misc/mos6522.c         | 13 +++----------
 include/hw/misc/mos6522.h |  2 +-
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 8aba2e63ec..9651ed9744 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -597,12 +597,12 @@ static void mos6522_cuda_portB_write(MOS6522State *s)
     cuda_update(cs);
 }
 
-static void mos6522_cuda_realize(DeviceState *dev, Error **errp)
+static void mos6522_cuda_reset(DeviceState *dev)
 {
     MOS6522State *ms = MOS6522(dev);
     MOS6522DeviceClass *mdc = MOS6522_DEVICE_GET_CLASS(ms);
 
-    mdc->parent_realize(dev, errp);
+    mdc->parent_reset(dev);
 
     ms->timers[0].frequency = CUDA_TIMER_FREQ;
     ms->timers[1].frequency = (SCALE_US * 6000) / 4700;
@@ -613,7 +613,7 @@ static void mos6522_cuda_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     MOS6522DeviceClass *mdc = MOS6522_DEVICE_CLASS(oc);
 
-    dc->realize = mos6522_cuda_realize;
+    dc->reset = mos6522_cuda_reset;
     mdc->portB_write = mos6522_cuda_portB_write;
     mdc->get_timer1_counter_value = cuda_get_counter_value;
     mdc->get_timer2_counter_value = cuda_get_counter_value;
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index facdaed5d5..d0a0c9e5d9 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -427,18 +427,12 @@ static void mos6522_reset(DeviceState *dev)
     /* s->ier = T1_INT | SR_INT; */
     s->anh = 0;
 
+    s->timers[0].frequency = s->frequency;
     s->timers[0].latch = 0xffff;
     set_counter(s, &s->timers[0], 0xffff);
 
-    s->timers[1].latch = 0xffff;
-}
-
-static void mos6522_realize(DeviceState *dev, Error **errp)
-{
-    MOS6522State *s = MOS6522(dev);
-
-    s->timers[0].frequency = s->frequency;
     s->timers[1].frequency = s->frequency;
+    s->timers[1].latch = 0xffff;
 }
 
 static void mos6522_init(Object *obj)
@@ -469,11 +463,10 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
     MOS6522DeviceClass *mdc = MOS6522_DEVICE_CLASS(oc);
 
-    dc->realize = mos6522_realize;
     dc->reset = mos6522_reset;
     dc->vmsd = &vmstate_mos6522;
     dc->props = mos6522_properties;
-    mdc->parent_realize = dc->realize;
+    mdc->parent_reset = dc->reset;
     mdc->set_sr_int = mos6522_set_sr_int;
     mdc->portB_write = mos6522_portB_write;
     mdc->portA_write = mos6522_portA_write;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index cb0fd7db78..f52b41920b 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -130,7 +130,7 @@ typedef struct MOS6522State {
 typedef struct MOS6522DeviceClass {
     DeviceClass parent_class;
 
-    DeviceRealize parent_realize;
+    DeviceReset parent_reset;
     void (*set_sr_int)(MOS6522State *dev);
     void (*portB_write)(MOS6522State *dev);
     void (*portA_write)(MOS6522State *dev);
-- 
2.11.0

[Qemu-devel] [PATCH 4/4] mos6522: convert VMSTATE_TIMER_PTR_TEST to VMSTATE_TIMER_PTR

[Mark Cave-Ayland]

The timers are configured in the mos6522 init function and therefore will
always exist, so the function can never return false.

Peter also pointed out that this is the only remaining user of
VMSTATE_TIMER_PTR_TEST in the codebase, so we might as well just convert it
over to VMSTATE_TIMER_PTR and remove mos6522_timer_exist() as it is no
longer required.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mos6522.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index d0a0c9e5d9..e9b686ac92 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -369,13 +369,6 @@ static const MemoryRegionOps mos6522_ops = {
     },
 };
 
-static bool mos6522_timer_exist(void *opaque, int version_id)
-{
-    MOS6522Timer *s = opaque;
-
-    return s->timer != NULL;
-}
-
 static const VMStateDescription vmstate_mos6522_timer = {
     .name = "mos6522_timer",
     .version_id = 0,
@@ -385,7 +378,7 @@ static const VMStateDescription vmstate_mos6522_timer = {
         VMSTATE_UINT16(counter_value, MOS6522Timer),
         VMSTATE_INT64(load_time, MOS6522Timer),
         VMSTATE_INT64(next_irq_time, MOS6522Timer),
-        VMSTATE_TIMER_PTR_TEST(timer, MOS6522Timer, mos6522_timer_exist),
+        VMSTATE_TIMER_PTR(timer, MOS6522Timer),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
2.11.0

Re: [Qemu-devel] [PATCH 4/4] mos6522: convert VMSTATE_TIMER_PTR_TEST to VMSTATE_TIMER_PTR

[Peter Maydell]

On 7 June 2018 at 18:17, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote:
> The timers are configured in the mos6522 init function and therefore will
> always exist, so the function can never return false.
>
> Peter also pointed out that this is the only remaining user of
> VMSTATE_TIMER_PTR_TEST in the codebase, so we might as well just convert it
> over to VMSTATE_TIMER_PTR and remove mos6522_timer_exist() as it is no
> longer required.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mos6522.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index d0a0c9e5d9..e9b686ac92 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -369,13 +369,6 @@ static const MemoryRegionOps mos6522_ops = {
>      },
>  };
>
> -static bool mos6522_timer_exist(void *opaque, int version_id)
> -{
> -    MOS6522Timer *s = opaque;
> -
> -    return s->timer != NULL;
> -}
> -
>  static const VMStateDescription vmstate_mos6522_timer = {
>      .name = "mos6522_timer",
>      .version_id = 0,
> @@ -385,7 +378,7 @@ static const VMStateDescription vmstate_mos6522_timer = {
>          VMSTATE_UINT16(counter_value, MOS6522Timer),
>          VMSTATE_INT64(load_time, MOS6522Timer),
>          VMSTATE_INT64(next_irq_time, MOS6522Timer),
> -        VMSTATE_TIMER_PTR_TEST(timer, MOS6522Timer, mos6522_timer_exist),
> +        VMSTATE_TIMER_PTR(timer, MOS6522Timer),
>          VMSTATE_END_OF_LIST()
>      }
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] cuda/mos6522 migration fixes

[Philippe Mathieu-Daudé]

On 06/07/2018 02:17 PM, Mark Cave-Ayland wrote:
> Whilst performing a random migration test for the Mac machines I noticed
> a regression (patch 1) which prevented the loadvm from completing
> successfully. A big thank you to Peter and David on IRC who pointed me
> in the right direction in order to fix the bug.
> 
> Once that was working I spent a bit more time analysing the migration
> stream and realised that the mos6522 device state wasn't being embedded
> within the CUDA device, but instead being maintained separately which is
> solved by patch 2.
> 
> Patch 3 is something I noticed whilst rearranging the existing code based
> upon my better understanding of QOM/qdev and ensures that the timer frequency
> is always set correctly post-migration for the device and its parent class.
> This leaves no remaining functionality in the mos6522 realize function and so
> allows it to be removed.
> 
> Finally patch 4 was suggested by Peter on IRC whilst helping me investigate
> the original migration issue, and removes the last remaining user of
> VMSTATE_TIMER_PTR_TEST from the codebase.

You forgot patch 5 "vmstate: Remove VMSTATE_TIMER_PTR_TEST" :)

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (4):
>   mos6522: fix vmstate_mos6522_timer version in vmstate_mos6522
>   cuda: embed mos6522_cuda device directly rather than using QOM object
>     link
>   mos6522: move timer frequency initialisation to mos6522_reset
>   mos6522: convert VMSTATE_TIMER_PTR_TEST to VMSTATE_TIMER_PTR
> 
>  hw/misc/macio/cuda.c         | 50 +++++++++++++++++++-------------------------
>  hw/misc/mos6522.c            | 26 ++++++-----------------
>  include/hw/misc/macio/cuda.h | 27 +++++++++++-------------
>  include/hw/misc/mos6522.h    |  4 +++-
>  4 files changed, 42 insertions(+), 65 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] cuda/mos6522 migration fixes

[Peter Maydell]

On 7 June 2018 at 20:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> You forgot patch 5 "vmstate: Remove VMSTATE_TIMER_PTR_TEST" :)

It's a generic macro that might have utility in future,
and it fits into a slot in the matrix of possible VMSTATE
macros, so I wouldn't bother. (It's kind of irritating that we
have an almost but not quite orthogonal set of VMSTATE
macros.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] cuda/mos6522 migration fixes

[Philippe Mathieu-Daudé]

On 06/07/2018 05:17 PM, Peter Maydell wrote:
> On 7 June 2018 at 20:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> You forgot patch 5 "vmstate: Remove VMSTATE_TIMER_PTR_TEST" :)
> 
> It's a generic macro that might have utility in future,
> and it fits into a slot in the matrix of possible VMSTATE
> macros, so I wouldn't bother. (It's kind of irritating that we
> have an almost but not quite orthogonal set of VMSTATE
> macros.)

OK!

Re: [Qemu-devel] [PATCH 0/4] cuda/mos6522 migration fixes

[David Gibson]

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

On Thu, Jun 07, 2018 at 06:17:47PM +0100, Mark Cave-Ayland wrote:
> Whilst performing a random migration test for the Mac machines I noticed
> a regression (patch 1) which prevented the loadvm from completing
> successfully. A big thank you to Peter and David on IRC who pointed me
> in the right direction in order to fix the bug.
> 
> Once that was working I spent a bit more time analysing the migration
> stream and realised that the mos6522 device state wasn't being embedded
> within the CUDA device, but instead being maintained separately which is
> solved by patch 2.
> 
> Patch 3 is something I noticed whilst rearranging the existing code based
> upon my better understanding of QOM/qdev and ensures that the timer frequency
> is always set correctly post-migration for the device and its parent class.
> This leaves no remaining functionality in the mos6522 realize function and so
> allows it to be removed.
> 
> Finally patch 4 was suggested by Peter on IRC whilst helping me investigate
> the original migration issue, and removes the last remaining user of
> VMSTATE_TIMER_PTR_TEST from the codebase.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-3.0.

> 
> 
> Mark Cave-Ayland (4):
>   mos6522: fix vmstate_mos6522_timer version in vmstate_mos6522
>   cuda: embed mos6522_cuda device directly rather than using QOM object
>     link
>   mos6522: move timer frequency initialisation to mos6522_reset
>   mos6522: convert VMSTATE_TIMER_PTR_TEST to VMSTATE_TIMER_PTR
> 
>  hw/misc/macio/cuda.c         | 50 +++++++++++++++++++-------------------------
>  hw/misc/mos6522.c            | 26 ++++++-----------------
>  include/hw/misc/macio/cuda.h | 27 +++++++++++-------------
>  include/hw/misc/mos6522.h    |  4 +++-
>  4 files changed, 42 insertions(+), 65 deletions(-)
> 

-- 
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 --]