[Qemu-devel] [PATCH for-3.0 v2 0/5] Arm migration fixes for 3.0

[Qemu-devel] [PATCH for-3.0 v2 0/5] Arm migration fixes for 3.0

[Peter Maydell]

This patchset primarily fixes problems with Arm migration
induced by a bug in the core vmstate handling of subsections:
currently the migration code incorrectly treats a subsection with
no .needed function pointer as if it was the subsection list
terminator -- it is ignored and so is everything after it.

I did an audit of all uses of subsections in QEMU, and found
that we had four which didn't define a .needed function (assuming
that this meant "always needed", same as the semantics for
not providing a .needed function for a toplevel vmsd).
This patchset fixes them all up by providing a dummy needed
function.

It also fixes an error in vmstate_gicv3_cpu which was accidentally
initializing .subsections twice and so ignoring one of the subsections.

Patches 1..3 are the same as for v1 and have been reviewed.
Patches 4 and 5 fix a further bug which I discovered during
testing of 1..3: the GICv3 migration structs had several
uses of the pre_load and post_load hooks which assumed that the
hooks were run whether the subsection was present or not. In
fact the migration code only runs the hooks when the subsection
is present in the incoming migration stream, and so for correct
behaviour we need to move the code we were running in the hooks
up to the parent VMSDs.

I've now tested this by doing a save/load of:
 2.12.0 QEMU -> new QEMU
 new QEMU -> new QEMU

An on-trunk QEMU prior to these bugfixes won't migrate
to a QEMU with the bugfixes, but we never released any
final QEMU version with the bugs in.

thanks
-- PMM

Peter Maydell (5):
  hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a
    needed function
  hw/intc/arm_gicv3_common: Combine duplicate .subsections in
    vmstate_gicv3_cpu
  target/arm: Add dummy needed functions to M profile vmstate
    subsections
  hw/intc/arm_gicv3_common: Move post_load hooks to top-level VMSD
  hw/intc/arm_gicv3_common: Move gicd shift bug handling to
    gicv3_post_load

 hw/intc/arm_gicv3_common.c | 284 ++++++++++++++++++-------------------
 target/arm/machine.c       |   3 +
 2 files changed, 145 insertions(+), 142 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH for-3.0 v2 1/5] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function

[Peter Maydell]

Currently the migration code incorrectly treats a subsection with
no .needed function pointer as if it was the subsection list
terminator -- it is ignored and so is everything after it.
Work around this by giving vmstate_gicv3_gicd_no_migration_shift_bug
a 'needed' function that always returns true.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
This should go into 3.0 to avoid awkward migration compat problems:
the no-migration-shift-bug subsection is new in 3.0.
---
 hw/intc/arm_gicv3_common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index ff326b374ad..e58bc8b8105 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -203,10 +203,16 @@ static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque,
     return 0;
 }
 
+static bool needed_always(void *opaque)
+{
+    return true;
+}
+
 const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = {
     .name = "arm_gicv3/gicd_no_migration_shift_bug",
     .version_id = 1,
     .minimum_version_id = 1,
+    .needed = needed_always,
     .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load,
     .post_load = gicv3_gicd_no_migration_shift_bug_post_load,
     .fields = (VMStateField[]) {
-- 
2.17.1

[Qemu-devel] [PATCH for-3.0 v2 2/5] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu

[Peter Maydell]

Commit 6692aac411199064 accidentally introduced a second initialization
of the .subsections field of vmstate_gicv3_cpu, instead of adding
the new subsection to the existing list. The effect of this was
probably that migration of GICv3 with virtualization enabled was
broken (or alternatively that migration of ICC_SRE_EL1 was broken,
depending on which of the two initializers the compiler used).
Combine the two into a single list.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
Not strictly a 2.12 regression.
---
 hw/intc/arm_gicv3_common.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index e58bc8b8105..e1a8999cf5b 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -134,9 +134,6 @@ static const VMStateDescription vmstate_gicv3_cpu = {
     },
     .subsections = (const VMStateDescription * []) {
         &vmstate_gicv3_cpu_virt,
-        NULL
-    },
-    .subsections = (const VMStateDescription * []) {
         &vmstate_gicv3_cpu_sre_el1,
         NULL
     }
-- 
2.17.1

[Qemu-devel] [PATCH for-3.0 v2 3/5] target/arm: Add dummy needed functions to M profile vmstate subsections

[Peter Maydell]

Currently the migration code incorrectly treats a subsection with
no .needed function pointer as if it was the subsection list
terminator -- it is ignored and so is everything after it.
Work around this by giving various M profile vmstate structs
a 'needed' function that always returns true.
We reuse m_needed() for this, since it's always true here.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
Not strictly a regression as it only affects M profile CPUs
with the security extensions, and migration of those was
broken anyway in 2.12 due to a different bug.
---
 target/arm/machine.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/machine.c b/target/arm/machine.c
index 2e28d086bdf..ff4ec22bf75 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -184,6 +184,7 @@ static const VMStateDescription vmstate_m_faultmask_primask = {
     .name = "cpu/m/faultmask-primask",
     .version_id = 1,
     .minimum_version_id = 1,
+    .needed = m_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(env.v7m.faultmask[M_REG_NS], ARMCPU),
         VMSTATE_UINT32(env.v7m.primask[M_REG_NS], ARMCPU),
@@ -230,6 +231,7 @@ static const VMStateDescription vmstate_m_scr = {
     .name = "cpu/m/scr",
     .version_id = 1,
     .minimum_version_id = 1,
+    .needed = m_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(env.v7m.scr[M_REG_NS], ARMCPU),
         VMSTATE_END_OF_LIST()
@@ -240,6 +242,7 @@ static const VMStateDescription vmstate_m_other_sp = {
     .name = "cpu/m/other-sp",
     .version_id = 1,
     .minimum_version_id = 1,
+    .needed = m_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(env.v7m.other_sp, ARMCPU),
         VMSTATE_END_OF_LIST()
-- 
2.17.1

[Qemu-devel] [PATCH for-3.0 v2 4/5] hw/intc/arm_gicv3_common: Move post_load hooks to top-level VMSD

[Peter Maydell]

Contrary to the the impression given in docs/devel/migration.rst,
the migration code does not run the pre_load hook for a
subsection unless the subsection appears on the wire, and so
this is not a place where you can set the default value for
state for the "subsection not present" case. Instead this needs
to be done in a pre_load hook for whatever is the parent VMSD
of the subsection.

We got this wrong in two of the subsection definitions in
the GICv3 migration structs; fix this.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_common.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index e1a8999cf5b..8175889f1e7 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -73,7 +73,7 @@ static const VMStateDescription vmstate_gicv3_cpu_virt = {
     }
 };
 
-static int icc_sre_el1_reg_pre_load(void *opaque)
+static int vmstate_gicv3_cpu_pre_load(void *opaque)
 {
     GICv3CPUState *cs = opaque;
 
@@ -97,7 +97,6 @@ const VMStateDescription vmstate_gicv3_cpu_sre_el1 = {
     .name = "arm_gicv3_cpu/sre_el1",
     .version_id = 1,
     .minimum_version_id = 1,
-    .pre_load = icc_sre_el1_reg_pre_load,
     .needed = icc_sre_el1_reg_needed,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(icc_sre_el1, GICv3CPUState),
@@ -109,6 +108,7 @@ static const VMStateDescription vmstate_gicv3_cpu = {
     .name = "arm_gicv3_cpu",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_load = vmstate_gicv3_cpu_pre_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(level, GICv3CPUState),
         VMSTATE_UINT32(gicr_ctlr, GICv3CPUState),
@@ -139,7 +139,7 @@ static const VMStateDescription vmstate_gicv3_cpu = {
     }
 };
 
-static int gicv3_gicd_no_migration_shift_bug_pre_load(void *opaque)
+static int gicv3_pre_load(void *opaque)
 {
     GICv3State *cs = opaque;
 
@@ -210,7 +210,6 @@ const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = {
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = needed_always,
-    .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load,
     .post_load = gicv3_gicd_no_migration_shift_bug_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_BOOL(gicd_no_migration_shift_bug, GICv3State),
@@ -222,6 +221,7 @@ static const VMStateDescription vmstate_gicv3 = {
     .name = "arm_gicv3",
     .version_id = 1,
     .minimum_version_id = 1,
+    .pre_load = gicv3_pre_load,
     .pre_save = gicv3_pre_save,
     .post_load = gicv3_post_load,
     .priority = MIG_PRI_GICV3,
-- 
2.17.1

[Qemu-devel] [PATCH for-3.0 v2 5/5] hw/intc/arm_gicv3_common: Move gicd shift bug handling to gicv3_post_load

[Peter Maydell]

The code currently in gicv3_gicd_no_migration_shift_bug_post_load()
that handles migration from older QEMU versions with a particular
bug is misplaced. We need to run this after migration in all cases,
not just the cases where the "arm_gicv3/gicd_no_migration_shift_bug"
subsection is present, so it must go in a post_load hook for the
top level VMSD, not for the subsection. Move it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gicv3_common.c | 77 ++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 8175889f1e7..52480c3b4cf 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -29,6 +29,41 @@
 #include "hw/arm/linux-boot-if.h"
 #include "sysemu/kvm.h"
 
+
+static void gicv3_gicd_no_migration_shift_bug_post_load(GICv3State *cs)
+{
+    if (cs->gicd_no_migration_shift_bug) {
+        return;
+    }
+
+    /* Older versions of QEMU had a bug in the handling of state save/restore
+     * to the KVM GICv3: they got the offset in the bitmap arrays wrong,
+     * so that instead of the data for external interrupts 32 and up
+     * starting at bit position 32 in the bitmap, it started at bit
+     * position 64. If we're receiving data from a QEMU with that bug,
+     * we must move the data down into the right place.
+     */
+    memmove(cs->group, (uint8_t *)cs->group + GIC_INTERNAL / 8,
+            sizeof(cs->group) - GIC_INTERNAL / 8);
+    memmove(cs->grpmod, (uint8_t *)cs->grpmod + GIC_INTERNAL / 8,
+            sizeof(cs->grpmod) - GIC_INTERNAL / 8);
+    memmove(cs->enabled, (uint8_t *)cs->enabled + GIC_INTERNAL / 8,
+            sizeof(cs->enabled) - GIC_INTERNAL / 8);
+    memmove(cs->pending, (uint8_t *)cs->pending + GIC_INTERNAL / 8,
+            sizeof(cs->pending) - GIC_INTERNAL / 8);
+    memmove(cs->active, (uint8_t *)cs->active + GIC_INTERNAL / 8,
+            sizeof(cs->active) - GIC_INTERNAL / 8);
+    memmove(cs->edge_trigger, (uint8_t *)cs->edge_trigger + GIC_INTERNAL / 8,
+            sizeof(cs->edge_trigger) - GIC_INTERNAL / 8);
+
+    /*
+     * While this new version QEMU doesn't have this kind of bug as we fix it,
+     * so it needs to set the flag to true to indicate that and it's necessary
+     * for next migration to work from this new version QEMU.
+     */
+    cs->gicd_no_migration_shift_bug = true;
+}
+
 static int gicv3_pre_save(void *opaque)
 {
     GICv3State *s = (GICv3State *)opaque;
@@ -46,6 +81,8 @@ static int gicv3_post_load(void *opaque, int version_id)
     GICv3State *s = (GICv3State *)opaque;
     ARMGICv3CommonClass *c = ARM_GICV3_COMMON_GET_CLASS(s);
 
+    gicv3_gicd_no_migration_shift_bug_post_load(s);
+
     if (c->post_load) {
         c->post_load(s);
     }
@@ -161,45 +198,6 @@ static int gicv3_pre_load(void *opaque)
     return 0;
 }
 
-static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque,
-                                                       int version_id)
-{
-    GICv3State *cs = opaque;
-
-    if (cs->gicd_no_migration_shift_bug) {
-        return 0;
-    }
-
-    /* Older versions of QEMU had a bug in the handling of state save/restore
-     * to the KVM GICv3: they got the offset in the bitmap arrays wrong,
-     * so that instead of the data for external interrupts 32 and up
-     * starting at bit position 32 in the bitmap, it started at bit
-     * position 64. If we're receiving data from a QEMU with that bug,
-     * we must move the data down into the right place.
-     */
-    memmove(cs->group, (uint8_t *)cs->group + GIC_INTERNAL / 8,
-            sizeof(cs->group) - GIC_INTERNAL / 8);
-    memmove(cs->grpmod, (uint8_t *)cs->grpmod + GIC_INTERNAL / 8,
-            sizeof(cs->grpmod) - GIC_INTERNAL / 8);
-    memmove(cs->enabled, (uint8_t *)cs->enabled + GIC_INTERNAL / 8,
-            sizeof(cs->enabled) - GIC_INTERNAL / 8);
-    memmove(cs->pending, (uint8_t *)cs->pending + GIC_INTERNAL / 8,
-            sizeof(cs->pending) - GIC_INTERNAL / 8);
-    memmove(cs->active, (uint8_t *)cs->active + GIC_INTERNAL / 8,
-            sizeof(cs->active) - GIC_INTERNAL / 8);
-    memmove(cs->edge_trigger, (uint8_t *)cs->edge_trigger + GIC_INTERNAL / 8,
-            sizeof(cs->edge_trigger) - GIC_INTERNAL / 8);
-
-    /*
-     * While this new version QEMU doesn't have this kind of bug as we fix it,
-     * so it needs to set the flag to true to indicate that and it's necessary
-     * for next migration to work from this new version QEMU.
-     */
-    cs->gicd_no_migration_shift_bug = true;
-
-    return 0;
-}
-
 static bool needed_always(void *opaque)
 {
     return true;
@@ -210,7 +208,6 @@ const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = {
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = needed_always,
-    .post_load = gicv3_gicd_no_migration_shift_bug_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_BOOL(gicd_no_migration_shift_bug, GICv3State),
         VMSTATE_END_OF_LIST()
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 v2 0/5] Arm migration fixes for 3.0

[Richard Henderson]

On 08/06/2018 05:34 AM, Peter Maydell wrote:
> Peter Maydell (5):
>   hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a
>     needed function
>   hw/intc/arm_gicv3_common: Combine duplicate .subsections in
>     vmstate_gicv3_cpu
>   target/arm: Add dummy needed functions to M profile vmstate
>     subsections
>   hw/intc/arm_gicv3_common: Move post_load hooks to top-level VMSD
>   hw/intc/arm_gicv3_common: Move gicd shift bug handling to
>     gicv3_post_load

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH for-3.0 v2 4/5] hw/intc/arm_gicv3_common: Move post_load hooks to top-level VMSD

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Contrary to the the impression given in docs/devel/migration.rst,
> the migration code does not run the pre_load hook for a
> subsection unless the subsection appears on the wire, and so
> this is not a place where you can set the default value for
> state for the "subsection not present" case. Instead this needs
> to be done in a pre_load hook for whatever is the parent VMSD
> of the subsection.
> 
> We got this wrong in two of the subsection definitions in
> the GICv3 migration structs; fix this.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/intc/arm_gicv3_common.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index e1a8999cf5b..8175889f1e7 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -73,7 +73,7 @@ static const VMStateDescription vmstate_gicv3_cpu_virt = {
>      }
>  };
>  
> -static int icc_sre_el1_reg_pre_load(void *opaque)
> +static int vmstate_gicv3_cpu_pre_load(void *opaque)
>  {
>      GICv3CPUState *cs = opaque;
>  
> @@ -97,7 +97,6 @@ const VMStateDescription vmstate_gicv3_cpu_sre_el1 = {
>      .name = "arm_gicv3_cpu/sre_el1",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .pre_load = icc_sre_el1_reg_pre_load,
>      .needed = icc_sre_el1_reg_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64(icc_sre_el1, GICv3CPUState),
> @@ -109,6 +108,7 @@ static const VMStateDescription vmstate_gicv3_cpu = {
>      .name = "arm_gicv3_cpu",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .pre_load = vmstate_gicv3_cpu_pre_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(level, GICv3CPUState),
>          VMSTATE_UINT32(gicr_ctlr, GICv3CPUState),
> @@ -139,7 +139,7 @@ static const VMStateDescription vmstate_gicv3_cpu = {
>      }
>  };
>  
> -static int gicv3_gicd_no_migration_shift_bug_pre_load(void *opaque)
> +static int gicv3_pre_load(void *opaque)
>  {
>      GICv3State *cs = opaque;
>  
> @@ -210,7 +210,6 @@ const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = needed_always,
> -    .pre_load = gicv3_gicd_no_migration_shift_bug_pre_load,
>      .post_load = gicv3_gicd_no_migration_shift_bug_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_BOOL(gicd_no_migration_shift_bug, GICv3State),
> @@ -222,6 +221,7 @@ static const VMStateDescription vmstate_gicv3 = {
>      .name = "arm_gicv3",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .pre_load = gicv3_pre_load,
>      .pre_save = gicv3_pre_save,
>      .post_load = gicv3_post_load,
>      .priority = MIG_PRI_GICV3,
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-3.0 v2 5/5] hw/intc/arm_gicv3_common: Move gicd shift bug handling to gicv3_post_load

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> The code currently in gicv3_gicd_no_migration_shift_bug_post_load()
> that handles migration from older QEMU versions with a particular
> bug is misplaced. We need to run this after migration in all cases,
> not just the cases where the "arm_gicv3/gicd_no_migration_shift_bug"
> subsection is present, so it must go in a post_load hook for the
> top level VMSD, not for the subsection. Move it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/intc/arm_gicv3_common.c | 77 ++++++++++++++++++--------------------
>  1 file changed, 37 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 8175889f1e7..52480c3b4cf 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -29,6 +29,41 @@
>  #include "hw/arm/linux-boot-if.h"
>  #include "sysemu/kvm.h"
>  
> +
> +static void gicv3_gicd_no_migration_shift_bug_post_load(GICv3State *cs)
> +{
> +    if (cs->gicd_no_migration_shift_bug) {
> +        return;
> +    }
> +
> +    /* Older versions of QEMU had a bug in the handling of state save/restore
> +     * to the KVM GICv3: they got the offset in the bitmap arrays wrong,
> +     * so that instead of the data for external interrupts 32 and up
> +     * starting at bit position 32 in the bitmap, it started at bit
> +     * position 64. If we're receiving data from a QEMU with that bug,
> +     * we must move the data down into the right place.
> +     */
> +    memmove(cs->group, (uint8_t *)cs->group + GIC_INTERNAL / 8,
> +            sizeof(cs->group) - GIC_INTERNAL / 8);
> +    memmove(cs->grpmod, (uint8_t *)cs->grpmod + GIC_INTERNAL / 8,
> +            sizeof(cs->grpmod) - GIC_INTERNAL / 8);
> +    memmove(cs->enabled, (uint8_t *)cs->enabled + GIC_INTERNAL / 8,
> +            sizeof(cs->enabled) - GIC_INTERNAL / 8);
> +    memmove(cs->pending, (uint8_t *)cs->pending + GIC_INTERNAL / 8,
> +            sizeof(cs->pending) - GIC_INTERNAL / 8);
> +    memmove(cs->active, (uint8_t *)cs->active + GIC_INTERNAL / 8,
> +            sizeof(cs->active) - GIC_INTERNAL / 8);
> +    memmove(cs->edge_trigger, (uint8_t *)cs->edge_trigger + GIC_INTERNAL / 8,
> +            sizeof(cs->edge_trigger) - GIC_INTERNAL / 8);
> +
> +    /*
> +     * While this new version QEMU doesn't have this kind of bug as we fix it,
> +     * so it needs to set the flag to true to indicate that and it's necessary
> +     * for next migration to work from this new version QEMU.
> +     */
> +    cs->gicd_no_migration_shift_bug = true;
> +}
> +
>  static int gicv3_pre_save(void *opaque)
>  {
>      GICv3State *s = (GICv3State *)opaque;
> @@ -46,6 +81,8 @@ static int gicv3_post_load(void *opaque, int version_id)
>      GICv3State *s = (GICv3State *)opaque;
>      ARMGICv3CommonClass *c = ARM_GICV3_COMMON_GET_CLASS(s);
>  
> +    gicv3_gicd_no_migration_shift_bug_post_load(s);
> +
>      if (c->post_load) {
>          c->post_load(s);
>      }
> @@ -161,45 +198,6 @@ static int gicv3_pre_load(void *opaque)
>      return 0;
>  }
>  
> -static int gicv3_gicd_no_migration_shift_bug_post_load(void *opaque,
> -                                                       int version_id)
> -{
> -    GICv3State *cs = opaque;
> -
> -    if (cs->gicd_no_migration_shift_bug) {
> -        return 0;
> -    }
> -
> -    /* Older versions of QEMU had a bug in the handling of state save/restore
> -     * to the KVM GICv3: they got the offset in the bitmap arrays wrong,
> -     * so that instead of the data for external interrupts 32 and up
> -     * starting at bit position 32 in the bitmap, it started at bit
> -     * position 64. If we're receiving data from a QEMU with that bug,
> -     * we must move the data down into the right place.
> -     */
> -    memmove(cs->group, (uint8_t *)cs->group + GIC_INTERNAL / 8,
> -            sizeof(cs->group) - GIC_INTERNAL / 8);
> -    memmove(cs->grpmod, (uint8_t *)cs->grpmod + GIC_INTERNAL / 8,
> -            sizeof(cs->grpmod) - GIC_INTERNAL / 8);
> -    memmove(cs->enabled, (uint8_t *)cs->enabled + GIC_INTERNAL / 8,
> -            sizeof(cs->enabled) - GIC_INTERNAL / 8);
> -    memmove(cs->pending, (uint8_t *)cs->pending + GIC_INTERNAL / 8,
> -            sizeof(cs->pending) - GIC_INTERNAL / 8);
> -    memmove(cs->active, (uint8_t *)cs->active + GIC_INTERNAL / 8,
> -            sizeof(cs->active) - GIC_INTERNAL / 8);
> -    memmove(cs->edge_trigger, (uint8_t *)cs->edge_trigger + GIC_INTERNAL / 8,
> -            sizeof(cs->edge_trigger) - GIC_INTERNAL / 8);
> -
> -    /*
> -     * While this new version QEMU doesn't have this kind of bug as we fix it,
> -     * so it needs to set the flag to true to indicate that and it's necessary
> -     * for next migration to work from this new version QEMU.
> -     */
> -    cs->gicd_no_migration_shift_bug = true;
> -
> -    return 0;
> -}
> -
>  static bool needed_always(void *opaque)
>  {
>      return true;
> @@ -210,7 +208,6 @@ const VMStateDescription vmstate_gicv3_gicd_no_migration_shift_bug = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = needed_always,
> -    .post_load = gicv3_gicd_no_migration_shift_bug_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_BOOL(gicd_no_migration_shift_bug, GICv3State),
>          VMSTATE_END_OF_LIST()
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH for-3.0 v2 0/5] Arm migration fixes for 3.0

[Peter Maydell]

On 6 August 2018 at 15:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 08/06/2018 05:34 AM, Peter Maydell wrote:
>> Peter Maydell (5):
>>   hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a
>>     needed function
>>   hw/intc/arm_gicv3_common: Combine duplicate .subsections in
>>     vmstate_gicv3_cpu
>>   target/arm: Add dummy needed functions to M profile vmstate
>>     subsections
>>   hw/intc/arm_gicv3_common: Move post_load hooks to top-level VMSD
>>   hw/intc/arm_gicv3_common: Move gicd shift bug handling to
>>     gicv3_post_load
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks; series applied to master for rc4.

-- PMM

Re: [Qemu-devel] [PATCH for-3.0 v2 1/5] hw/intc/arm_gicv3_common: Give no-migration-shift-bug subsection a needed function

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> Currently the migration code incorrectly treats a subsection with
> no .needed function pointer as if it was the subsection list
> terminator -- it is ignored and so is everything after it.
> Work around this by giving vmstate_gicv3_gicd_no_migration_shift_bug
> a 'needed' function that always returns true.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH for-3.0 v2 2/5] hw/intc/arm_gicv3_common: Combine duplicate .subsections in vmstate_gicv3_cpu

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> Commit 6692aac411199064 accidentally introduced a second initialization
> of the .subsections field of vmstate_gicv3_cpu, instead of adding
> the new subsection to the existing list. The effect of this was
> probably that migration of GICv3 with virtualization enabled was
> broken (or alternatively that migration of ICC_SRE_EL1 was broken,
> depending on which of the two initializers the compiler used).
> Combine the two into a single list.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH for-3.0 v2 3/5] target/arm: Add dummy needed functions to M profile vmstate subsections

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> Currently the migration code incorrectly treats a subsection with
> no .needed function pointer as if it was the subsection list
> terminator -- it is ignored and so is everything after it.
> Work around this by giving various M profile vmstate structs
> a 'needed' function that always returns true.
> We reuse m_needed() for this, since it's always true here.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>


> ---
> Not strictly a regression as it only affects M profile CPUs
> with the security extensions, and migration of those was
> broken anyway in 2.12 due to a different bug.
> ---
>  target/arm/machine.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 2e28d086bdf..ff4ec22bf75 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -184,6 +184,7 @@ static const VMStateDescription vmstate_m_faultmask_primask = {
>      .name = "cpu/m/faultmask-primask",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .needed = m_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(env.v7m.faultmask[M_REG_NS], ARMCPU),
>          VMSTATE_UINT32(env.v7m.primask[M_REG_NS], ARMCPU),
> @@ -230,6 +231,7 @@ static const VMStateDescription vmstate_m_scr = {
>      .name = "cpu/m/scr",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .needed = m_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(env.v7m.scr[M_REG_NS], ARMCPU),
>          VMSTATE_END_OF_LIST()
> @@ -240,6 +242,7 @@ static const VMStateDescription vmstate_m_other_sp = {
>      .name = "cpu/m/other-sp",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .needed = m_needed,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(env.v7m.other_sp, ARMCPU),
>          VMSTATE_END_OF_LIST()

But having 3 subsections with the same needed function ... we could have
a single subsection.

Later, Juan.

Re: [Qemu-devel] [PATCH for-3.0 v2 4/5] hw/intc/arm_gicv3_common: Move post_load hooks to top-level VMSD

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> Contrary to the the impression given in docs/devel/migration.rst,
> the migration code does not run the pre_load hook for a
> subsection unless the subsection appears on the wire, and so
> this is not a place where you can set the default value for
> state for the "subsection not present" case. Instead this needs
> to be done in a pre_load hook for whatever is the parent VMSD
> of the subsection.
>
> We got this wrong in two of the subsection definitions in
> the GICv3 migration structs; fix this.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH for-3.0 v2 3/5] target/arm: Add dummy needed functions to M profile vmstate subsections

[Peter Maydell]

On 7 August 2018 at 15:40, Juan Quintela <quintela@redhat.com> wrote:
> But having 3 subsections with the same needed function ... we could have
> a single subsection.

I wanted to keep the change small here since we're very
late in the release cycle, so I kept it to a change
that restores the intent of the code, ie "always transfer
the subsection".

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-3.0 v2 3/5] target/arm: Add dummy needed functions to M profile vmstate subsections

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 August 2018 at 15:40, Juan Quintela <quintela@redhat.com> wrote:
>> But having 3 subsections with the same needed function ... we could have
>> a single subsection.
>
> I wanted to keep the change small here since we're very
> late in the release cycle, so I kept it to a change
> that restores the intent of the code, ie "always transfer
> the subsection".

ok with minimizing changes.  But I can't understand why that is going
that way.  Seeing this, I am making a patch that at registration time
check that we are not doing something stupid, like not having needed
function, bad names, whatever.

Thanks.