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

[Qemu-devel] [PATCH 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>

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
  ppc: move sdr1 value change detection logic to helper_store_sdr1()
  ppc: force update of all msr bits in cpu_post_load
  openpic: fix segfault on -M mac99 savevm
  openpic: fix up loadvm under -M mac99

 hw/input/adb.c           |   21 +++++++++++++++++----
 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, 76 insertions(+), 34 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 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] [PATCH 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 |   21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/input/adb.c b/hw/input/adb.c
index 34c8058..a5f813f 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,10 @@ 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] [PATCH 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] [PATCH 4/7] 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>
CC: 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] [PATCH 5/7] ppc: force update of all 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 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..9669063 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 all msr bits invalid before restoring */
+    msr = env->msr;
+    env->msr = ~env->msr;
+    ppc_store_msr(env, msr);
+
     hreg_compute_mem_idx(env);
 
     return 0;
-- 
1.7.10.4

[Qemu-devel] [PATCH 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] [PATCH 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] [PATCH 0/7] ppc: loadvm/savevm fixups for -M g3beige and -M mac99

[Mark Cave-Ayland]

On 23/12/14 00:36, Mark Cave-Ayland wrote:

> 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>
> 
> 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
>   ppc: move sdr1 value change detection logic to helper_store_sdr1()
>   ppc: force update of all msr bits in cpu_post_load
>   openpic: fix segfault on -M mac99 savevm
>   openpic: fix up loadvm under -M mac99
> 
>  hw/input/adb.c           |   21 +++++++++++++++++----
>  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, 76 insertions(+), 34 deletions(-)

Ping? Particularly patches 4 and 5 will require some review as they fix
general target-ppc bugs when loading/saving VM state and aren't just
specific to the g3beige/mac99 machines.


ATB,

Mark.

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

[Paolo Bonzini]

On 23/12/2014 01:36, Mark Cave-Ayland wrote:
> 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>
> CC: 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);

Possibly stupid question: should this tlb_flush be in ppc_store_sdr1,
maybe guarded by "if (tcg_enabled())"?

Apart from this, the patch is okay.

Paolo

> +        }
>      }
>  }
>  
> 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;
>      }
>  }
>  
> 

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

[Paolo Bonzini]

On 23/12/2014 01:36, 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 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..9669063 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 all msr bits invalid before restoring */
> +    msr = env->msr;
> +    env->msr = ~env->msr;
> +    ppc_store_msr(env, msr);
> +
>      hreg_compute_mem_idx(env);
>  
>      return 0;
> 

This is wrong for this line of hreg_store_msr:

    if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
                 ((value ^ env->msr) & (1 << MSR_TGPR)))) {
        /* Swap temporary saved registers with GPRs */
        hreg_swap_gpr_tgpr(env);
    }

I think you need to change the ~ with "^ ~(1 << MSR_TGPR)".

Paolo

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

[Mark Cave-Ayland]

On 20/01/15 14:57, Paolo Bonzini wrote:

> On 23/12/2014 01:36, Mark Cave-Ayland wrote:
>> 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>
>> CC: 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);
> 
> Possibly stupid question: should this tlb_flush be in ppc_store_sdr1,
> maybe guarded by "if (tcg_enabled())"?
> 
> Apart from this, the patch is okay.

Thanks Paolo. I based this patch upon a comment in a slightly earlier
thread here:
http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03146.html.

Is this still relevant or would you still like me to make the change?
This is a little beyond my area of knowledge, but at the very least I
can test any suggested changes under TCG fairly easily.


ATB,

Mark.

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

[Paolo Bonzini]

On 20/01/2015 16:23, Mark Cave-Ayland wrote:
> On 20/01/15 14:57, Paolo Bonzini wrote:
> 
>> On 23/12/2014 01:36, Mark Cave-Ayland wrote:
>>> 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>
>>> CC: 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);
>>
>> Possibly stupid question: should this tlb_flush be in ppc_store_sdr1,
>> maybe guarded by "if (tcg_enabled())"?
>>
>> Apart from this, the patch is okay.
> 
> Thanks Paolo. I based this patch upon a comment in a slightly earlier
> thread here:
> http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03146.html.
> 
> Is this still relevant or would you still like me to make the change?
> This is a little beyond my area of knowledge, but at the very least I
> can test any suggested changes under TCG fairly easily.

No big deal, it's really just personal preference and as you saw even
that can change easily...

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

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

[Mark Cave-Ayland]

On 20/01/15 15:01, Paolo Bonzini wrote:

> On 23/12/2014 01:36, 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 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..9669063 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 all msr bits invalid before restoring */
>> +    msr = env->msr;
>> +    env->msr = ~env->msr;
>> +    ppc_store_msr(env, msr);
>> +
>>      hreg_compute_mem_idx(env);
>>  
>>      return 0;
>>
> 
> This is wrong for this line of hreg_store_msr:
> 
>     if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
>                  ((value ^ env->msr) & (1 << MSR_TGPR)))) {
>         /* Swap temporary saved registers with GPRs */
>         hreg_swap_gpr_tgpr(env);
>     }
> 
> I think you need to change the ~ with "^ ~(1 << MSR_TGPR)".

It took me a little time to decipher how that worked, but yes it seems
fine in my tests here. I'll resend the series with with your Reviewed-by
added to the previous patch.


Many thanks,

Mark.