[Qemu-devel] [PATCHv2 0/7] ppc: loadvm/savevm fixups for -M g3beige and -M mac99

[Qemu-devel] [PATCHv2 0/7] ppc: loadvm/savevm fixups for -M g3beige and -M mac99

[Mark Cave-Ayland]

This patchset fixes up various bugs in loadvm/savevm for -M g3beige and
-M mac99 so that it is becomes possible to save and restore image snapshots.

The focus of this patchset is on -M g3beige since this matches the majority
of my test images, but there were some easy fixes to be made to -M mac99
at the same time.

With this patchset applied both -M g3beige and -M mac99 images can be
saved/restored whilst booted into OpenBIOS with no issues. I tested -M g3beige
with a paused, disk-inactive Darwin 6 image and was able to resume
successfully which was good enough for my needs.

I noticed some hangs can still occur when trying to restore an image
where the disk is active which makes me believe that there is still some
extra macio/dbdma state which needs to be included if someone is interested
enough to pursue this further.

Most of the patches are straightforward except for patch 4 which came out of
a discussion on-list between Alex and Paolo, and patch 5 which is a similar
error except this time for the MSR register. I suspect patch 5 can be
improved by someone with more PPC knowledge than myself.

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

v2:
- Minor subject line changes for patches 4+5
- Update patches 4+5 based upon feedback from Paolo
- Fix line width exceeding 80 characters in patch 2


Mark Cave-Ayland (7):
  macio.c: include parent PCIDevice state in VMStateDescription
  adb.c: include ADBDevice parent state in KBDState and MouseState
  cuda.c: include adb_poll_timer in VMStateDescription
  target-ppc: move sdr1 value change detection logic to
    helper_store_sdr1()
  target-ppc: force update of msr bits in cpu_post_load
  openpic: fix segfault on -M mac99 savevm
  openpic: fix up loadvm under -M mac99

 hw/input/adb.c           |   22 ++++++++++++++++++----
 hw/intc/openpic.c        |   10 ++++------
 hw/misc/macio/cuda.c     |    5 +++--
 hw/misc/macio/macio.c    |   24 ++++++++++++++++++++++++
 target-ppc/machine.c     |    8 +++++++-
 target-ppc/misc_helper.c |    7 ++++++-
 target-ppc/mmu_helper.c  |   35 +++++++++++++++--------------------
 7 files changed, 77 insertions(+), 34 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCHv2 1/7] macio.c: include parent PCIDevice state in VMStateDescription

[Mark Cave-Ayland]

This ensures that the macio PCI device is correctly configured when restoring
from a VM snapshot.

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

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index e0f1e88..9bc3f2d 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -336,20 +336,44 @@ static void macio_instance_init(Object *obj)
     memory_region_add_subregion(&s->bar, 0x08000, dbdma_mem);
 }
 
+static const VMStateDescription vmstate_macio_oldworld = {
+    .name = "macio-oldworld",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(parent_obj.parent, OldWorldMacIOState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void macio_oldworld_class_init(ObjectClass *oc, void *data)
 {
     PCIDeviceClass *pdc = PCI_DEVICE_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
     pdc->init = macio_oldworld_initfn;
     pdc->device_id = PCI_DEVICE_ID_APPLE_343S1201;
+    dc->vmsd = &vmstate_macio_oldworld;
 }
 
+static const VMStateDescription vmstate_macio_newworld = {
+    .name = "macio-newworld",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(parent_obj.parent, NewWorldMacIOState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void macio_newworld_class_init(ObjectClass *oc, void *data)
 {
     PCIDeviceClass *pdc = PCI_DEVICE_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
     pdc->init = macio_newworld_initfn;
     pdc->device_id = PCI_DEVICE_ID_APPLE_UNI_N_KEYL;
+    dc->vmsd = &vmstate_macio_newworld;
 }
 
 static Property macio_properties[] = {
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 2/7] adb.c: include ADBDevice parent state in KBDState and MouseState

[Mark Cave-Ayland]

The parent ADBDevice contains the device id on the ADB bus. Make sure that
this state is included in both its subclasses since some clients (such as
OpenBIOS) reprogram each device id after enumeration.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/input/adb.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/input/adb.c b/hw/input/adb.c
index 34c8058..a18eea2 100644
--- a/hw/input/adb.c
+++ b/hw/input/adb.c
@@ -118,6 +118,17 @@ static const TypeInfo adb_bus_type_info = {
     .instance_size = sizeof(ADBBusState),
 };
 
+static const VMStateDescription vmstate_adb_device = {
+    .name = "adb_device",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(devaddr, ADBDevice),
+        VMSTATE_INT32(handler, ADBDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void adb_device_realizefn(DeviceState *dev, Error **errp)
 {
     ADBDevice *d = ADB_DEVICE(dev);
@@ -301,9 +312,10 @@ static int adb_kbd_request(ADBDevice *d, uint8_t *obuf,
 
 static const VMStateDescription vmstate_adb_kbd = {
     .name = "adb_kbd",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(parent_obj, KBDState, 0, vmstate_adb_device, ADBDevice),
         VMSTATE_BUFFER(data, KBDState),
         VMSTATE_INT32(rptr, KBDState),
         VMSTATE_INT32(wptr, KBDState),
@@ -515,9 +527,11 @@ static void adb_mouse_reset(DeviceState *dev)
 
 static const VMStateDescription vmstate_adb_mouse = {
     .name = "adb_mouse",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(parent_obj, MouseState, 0, vmstate_adb_device,
+                       ADBDevice),
         VMSTATE_INT32(buttons_state, MouseState),
         VMSTATE_INT32(last_buttons_state, MouseState),
         VMSTATE_INT32(dx, MouseState),
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 3/7] cuda.c: include adb_poll_timer in VMStateDescription

[Mark Cave-Ayland]

Make sure that we include the adb_poll_timer when saving the VM state for
client OSs that use it, e.g. Darwin.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index b4273aa..dd9d76a 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -638,8 +638,8 @@ static const VMStateDescription vmstate_cuda_timer = {
 
 static const VMStateDescription vmstate_cuda = {
     .name = "cuda",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(a, CUDAState),
         VMSTATE_UINT8(b, CUDAState),
@@ -660,6 +660,7 @@ static const VMStateDescription vmstate_cuda = {
         VMSTATE_UINT32(tick_offset, CUDAState),
         VMSTATE_STRUCT_ARRAY(timers, CUDAState, 2, 1,
                              vmstate_cuda_timer, CUDATimer),
+        VMSTATE_TIMER(adb_poll_timer, CUDAState),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 4/7] target-ppc: move sdr1 value change detection logic to helper_store_sdr1()

[Mark Cave-Ayland]

Otherwise when cpu_post_load calls ppc_store_sdr1() when restoring a VM
snapshot the value is deemed unchanged and so the internal env->htab*
variables aren't set correctly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-ppc/misc_helper.c |    7 ++++++-
 target-ppc/mmu_helper.c  |   35 +++++++++++++++--------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index a577b3a..6b12ca8 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -77,8 +77,13 @@ void helper_msr_facility_check(CPUPPCState *env, uint32_t bit,
 
 void helper_store_sdr1(CPUPPCState *env, target_ulong val)
 {
+    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+
     if (!env->external_htab) {
-        ppc_store_sdr1(env, val);
+        if (env->spr[SPR_SDR1] != val) {
+            ppc_store_sdr1(env, val);
+            tlb_flush(CPU(cpu), 1);
+        }
     }
 }
 
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 660be7f..527c6ad 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2036,31 +2036,26 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
 /* Special registers manipulation */
 void ppc_store_sdr1(CPUPPCState *env, target_ulong value)
 {
-    PowerPCCPU *cpu = ppc_env_get_cpu(env);
-
     qemu_log_mask(CPU_LOG_MMU, "%s: " TARGET_FMT_lx "\n", __func__, value);
     assert(!env->external_htab);
-    if (env->spr[SPR_SDR1] != value) {
-        env->spr[SPR_SDR1] = value;
+    env->spr[SPR_SDR1] = value;
 #if defined(TARGET_PPC64)
-        if (env->mmu_model & POWERPC_MMU_64) {
-            target_ulong htabsize = value & SDR_64_HTABSIZE;
+    if (env->mmu_model & POWERPC_MMU_64) {
+        target_ulong htabsize = value & SDR_64_HTABSIZE;
 
-            if (htabsize > 28) {
-                fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
-                        " stored in SDR1\n", htabsize);
-                htabsize = 28;
-            }
-            env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
-            env->htab_base = value & SDR_64_HTABORG;
-        } else
-#endif /* defined(TARGET_PPC64) */
-        {
-            /* FIXME: Should check for valid HTABMASK values */
-            env->htab_mask = ((value & SDR_32_HTABMASK) << 16) | 0xFFFF;
-            env->htab_base = value & SDR_32_HTABORG;
+        if (htabsize > 28) {
+            fprintf(stderr, "Invalid HTABSIZE 0x" TARGET_FMT_lx
+                    " stored in SDR1\n", htabsize);
+            htabsize = 28;
         }
-        tlb_flush(CPU(cpu), 1);
+        env->htab_mask = (1ULL << (htabsize + 18 - 7)) - 1;
+        env->htab_base = value & SDR_64_HTABORG;
+    } else
+#endif /* defined(TARGET_PPC64) */
+    {
+        /* FIXME: Should check for valid HTABMASK values */
+        env->htab_mask = ((value & SDR_32_HTABMASK) << 16) | 0xFFFF;
+        env->htab_base = value & SDR_32_HTABORG;
     }
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 5/7] target-ppc: force update of msr bits in cpu_post_load

[Mark Cave-Ayland]

Since env->msr has already been restored by the time cpu_post_load is called,
make sure that ppc_store_msr() is explicitly called with all msr bits except
MSR_TGPR marked as invalid.

This solves the issue where MSR flags aren't set correctly when restoring a VM
snapshot, in particular the internal env->excp_prefix value when MSR_EP has
been altered by a guest.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target-ppc/machine.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index c801b82..fc8ddcd 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -159,6 +159,7 @@ static int cpu_post_load(void *opaque, int version_id)
     PowerPCCPU *cpu = opaque;
     CPUPPCState *env = &cpu->env;
     int i;
+    target_ulong msr;
 
     /*
      * We always ignore the source PVR. The user or management
@@ -190,7 +191,12 @@ static int cpu_post_load(void *opaque, int version_id)
         /* Restore htab_base and htab_mask variables */
         ppc_store_sdr1(env, env->spr[SPR_SDR1]);
     }
-    hreg_compute_hflags(env);
+
+    /* Mark msr bits except MSR_TGPR invalid before restoring */
+    msr = env->msr;
+    env->msr ^= ~(1 << MSR_TGPR);
+    ppc_store_msr(env, msr);
+
     hreg_compute_mem_idx(env);
 
     return 0;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCHv2 5/7] target-ppc: force update of msr bits in cpu_post_load

[Alexander Graf]

On 21.01.15 17:01, Mark Cave-Ayland wrote:
> Since env->msr has already been restored by the time cpu_post_load is called,
> make sure that ppc_store_msr() is explicitly called with all msr bits except
> MSR_TGPR marked as invalid.
> 
> This solves the issue where MSR flags aren't set correctly when restoring a VM
> snapshot, in particular the internal env->excp_prefix value when MSR_EP has
> been altered by a guest.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target-ppc/machine.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index c801b82..fc8ddcd 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -159,6 +159,7 @@ static int cpu_post_load(void *opaque, int version_id)
>      PowerPCCPU *cpu = opaque;
>      CPUPPCState *env = &cpu->env;
>      int i;
> +    target_ulong msr;
>  
>      /*
>       * We always ignore the source PVR. The user or management
> @@ -190,7 +191,12 @@ static int cpu_post_load(void *opaque, int version_id)
>          /* Restore htab_base and htab_mask variables */
>          ppc_store_sdr1(env, env->spr[SPR_SDR1]);
>      }
> -    hreg_compute_hflags(env);
> +
> +    /* Mark msr bits except MSR_TGPR invalid before restoring */
> +    msr = env->msr;
> +    env->msr ^= ~(1 << MSR_TGPR);

Doesn't this need to be 1ULL?


Alex

> +    ppc_store_msr(env, msr);
> +
>      hreg_compute_mem_idx(env);
>  
>      return 0;
> 

Re: [Qemu-devel] [PATCHv2 5/7] target-ppc: force update of msr bits in cpu_post_load

[Mark Cave-Ayland]

On 22/01/15 13:37, Alexander Graf wrote:

> On 21.01.15 17:01, Mark Cave-Ayland wrote:
>> Since env->msr has already been restored by the time cpu_post_load is called,
>> make sure that ppc_store_msr() is explicitly called with all msr bits except
>> MSR_TGPR marked as invalid.
>>
>> This solves the issue where MSR flags aren't set correctly when restoring a VM
>> snapshot, in particular the internal env->excp_prefix value when MSR_EP has
>> been altered by a guest.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  target-ppc/machine.c |    8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>> index c801b82..fc8ddcd 100644
>> --- a/target-ppc/machine.c
>> +++ b/target-ppc/machine.c
>> @@ -159,6 +159,7 @@ static int cpu_post_load(void *opaque, int version_id)
>>      PowerPCCPU *cpu = opaque;
>>      CPUPPCState *env = &cpu->env;
>>      int i;
>> +    target_ulong msr;
>>  
>>      /*
>>       * We always ignore the source PVR. The user or management
>> @@ -190,7 +191,12 @@ static int cpu_post_load(void *opaque, int version_id)
>>          /* Restore htab_base and htab_mask variables */
>>          ppc_store_sdr1(env, env->spr[SPR_SDR1]);
>>      }
>> -    hreg_compute_hflags(env);
>> +
>> +    /* Mark msr bits except MSR_TGPR invalid before restoring */
>> +    msr = env->msr;
>> +    env->msr ^= ~(1 << MSR_TGPR);
> 
> Doesn't this need to be 1ULL?

Yes, you're probably right. Are you able to fix this, or do you need me
to respin a v3?


ATB,

Mark.

Re: [Qemu-devel] [PATCHv2 5/7] target-ppc: force update of msr bits in cpu_post_load

[Alexander Graf]

> Am 26.01.2015 um 22:41 schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
> 
>> On 22/01/15 13:37, Alexander Graf wrote:
>> 
>>> On 21.01.15 17:01, Mark Cave-Ayland wrote:
>>> Since env->msr has already been restored by the time cpu_post_load is called,
>>> make sure that ppc_store_msr() is explicitly called with all msr bits except
>>> MSR_TGPR marked as invalid.
>>> 
>>> This solves the issue where MSR flags aren't set correctly when restoring a VM
>>> snapshot, in particular the internal env->excp_prefix value when MSR_EP has
>>> been altered by a guest.
>>> 
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> target-ppc/machine.c |    8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>>> index c801b82..fc8ddcd 100644
>>> --- a/target-ppc/machine.c
>>> +++ b/target-ppc/machine.c
>>> @@ -159,6 +159,7 @@ static int cpu_post_load(void *opaque, int version_id)
>>>     PowerPCCPU *cpu = opaque;
>>>     CPUPPCState *env = &cpu->env;
>>>     int i;
>>> +    target_ulong msr;
>>> 
>>>     /*
>>>      * We always ignore the source PVR. The user or management
>>> @@ -190,7 +191,12 @@ static int cpu_post_load(void *opaque, int version_id)
>>>         /* Restore htab_base and htab_mask variables */
>>>         ppc_store_sdr1(env, env->spr[SPR_SDR1]);
>>>     }
>>> -    hreg_compute_hflags(env);
>>> +
>>> +    /* Mark msr bits except MSR_TGPR invalid before restoring */
>>> +    msr = env->msr;
>>> +    env->msr ^= ~(1 << MSR_TGPR);
>> 
>> Doesn't this need to be 1ULL?
> 
> Yes, you're probably right. Are you able to fix this, or do you need me
> to respin a v3?

Depends on your answer to the other mail ;). If this is as far as you want to push the patches for now I can certainly apply it and change the line as I go.


Alex

[Qemu-devel] [PATCHv2 6/7] openpic: fix segfault on -M mac99 savevm

[Mark Cave-Ayland]

A simple copy/paste error causes savevm on -M mac99 to segfault.

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

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 7d1f3b9..8699a4a 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1335,7 +1335,7 @@ static void openpic_save(QEMUFile* f, void *opaque)
     for (i = 0; i < opp->max_irq; i++) {
         qemu_put_be32s(f, &opp->src[i].ivpr);
         qemu_put_be32s(f, &opp->src[i].idr);
-        qemu_get_be32s(f, &opp->src[i].destmask);
+        qemu_put_be32s(f, &opp->src[i].destmask);
         qemu_put_sbe32s(f, &opp->src[i].last_cpu);
         qemu_put_sbe32s(f, &opp->src[i].pending);
     }
-- 
1.7.10.4

[Qemu-devel] [PATCHv2 7/7] openpic: fix up loadvm under -M mac99

[Mark Cave-Ayland]

Issuing loadvm under -M mac99 would fail for two reasons: firstly an incorrect
version number for openpic would cause openpic_load() to abort, and secondly
a cut/paste error when restoring the IVPR and IDR registers caused subsequent
vmstate sections to become misaligned and abort early.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/intc/openpic.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
index 8699a4a..4194cef 100644
--- a/hw/intc/openpic.c
+++ b/hw/intc/openpic.c
@@ -1366,7 +1366,7 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id)
     OpenPICState *opp = (OpenPICState *)opaque;
     unsigned int i, nb_cpus;
 
-    if (version_id != 1) {
+    if (version_id != 2) {
         return -EINVAL;
     }
 
@@ -1399,12 +1399,10 @@ static int openpic_load(QEMUFile* f, void *opaque, int version_id)
         uint32_t val;
 
         val = qemu_get_be32(f);
-        write_IRQreg_idr(opp, i, val);
-        val = qemu_get_be32(f);
         write_IRQreg_ivpr(opp, i, val);
+        val = qemu_get_be32(f);
+        write_IRQreg_idr(opp, i, val);
 
-        qemu_get_be32s(f, &opp->src[i].ivpr);
-        qemu_get_be32s(f, &opp->src[i].idr);
         qemu_get_be32s(f, &opp->src[i].destmask);
         qemu_get_sbe32s(f, &opp->src[i].last_cpu);
         qemu_get_sbe32s(f, &opp->src[i].pending);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCHv2 7/7] openpic: fix up loadvm under -M mac99

[Alexander Graf]

On 21.01.15 17:01, Mark Cave-Ayland wrote:
> Issuing loadvm under -M mac99 would fail for two reasons: firstly an incorrect
> version number for openpic would cause openpic_load() to abort, and secondly
> a cut/paste error when restoring the IVPR and IDR registers caused subsequent
> vmstate sections to become misaligned and abort early.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Thanks a lot for all the work you put into this. Do you think you
understand enough to the OpenPIC code by now to be able to convert it to
VMState instead?

That would get rid of the whole class of problems altogether and make
sure we didn't overlook something subtle somewhere else in the code.


Alex

Re: [Qemu-devel] [PATCHv2 7/7] openpic: fix up loadvm under -M mac99

[Mark Cave-Ayland]

On 22/01/15 13:39, Alexander Graf wrote:

> On 21.01.15 17:01, Mark Cave-Ayland wrote:
>> Issuing loadvm under -M mac99 would fail for two reasons: firstly an incorrect
>> version number for openpic would cause openpic_load() to abort, and secondly
>> a cut/paste error when restoring the IVPR and IDR registers caused subsequent
>> vmstate sections to become misaligned and abort early.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Thanks a lot for all the work you put into this. Do you think you
> understand enough to the OpenPIC code by now to be able to convert it to
> VMState instead?
> 
> That would get rid of the whole class of problems altogether and make
> sure we didn't overlook something subtle somewhere else in the code.

I'm interested to have a go in time for the 2.3 release, but I'm not
exactly sure when that will be. This was mainly a festive distraction in
an attempt to help work out why the existing macio code doesn't work
correctly in its current form.

I'd be fine with you applying the existing patchset and I'll submit an
attempt to convert to VMState a bit later, hopefully after your
migration stream patches have also been merged to make the job a bit
easier ;)

Also slightly off-topic, but my rework of the macio code
(https://github.com/mcayland/qemu/commits/for-kevin), while being much
more readable in my opinion, also sadly appears to suffer from the same
corruption bug as the existing implementation.

If you think that what I have there is an improvement in terms of
readability then I'll aim to get that tidied up and submitted at some
point during the 2.3 cycle too.


ATB,

Mark.