[v6,04/14] target/arm: Swap PMU values before/after migrations

Commit Message

Aaron Lindsay Oct. 10, 2018, 8:37 p.m. UTC

Because of the PMU's design, many register accesses have side effects
which are inter-related, meaning that the normal method of saving CP
registers can result in inconsistent state. These side-effects are
largely handled in *op_start and *op_finish functions which can be
called globally once before and after the state is saved/restored. By
doing this and adding raw read/write functions for the affected
registers, we avoid such inconsistencies.

Signed-off-by: Aaron Lindsay <[e-mail]>
---
 target/arm/helper.c  |  6 ++++--
 target/arm/machine.c | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

Comments

Richard Henderson Oct. 15, 2018, 7:45 p.m. UTC | #1

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> +static void cpu_post_save(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    pmccntr_sync(&cpu->env);
> +}

I'm confused about the need for this.
Can you explain the sequence of events that requires it?


r~

Aaron Lindsay Oct. 15, 2018, 8:44 p.m. UTC | #2

On Oct 15 12:45, Richard Henderson wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > +static void cpu_post_save(void *opaque)
> > +{
> > +    ARMCPU *cpu = opaque;
> > +    pmccntr_sync(&cpu->env);
> > +}
> 
> I'm confused about the need for this.
> Can you explain the sequence of events that requires it?

It sounds like you're more okay with this now based on your comments on
my later patch, but for others...

The PMU implementation is event-driven - the counters and other
associated PMU/sysreg state are only updated when needed to ensure the
architectural state observed by the system is correct (i.e. on system
register reads). Otherwise, the counters are stored as differences from
the underlying counts rather than the raw counter values themselves.
Upon migration, we want to convert the counters and other state to their
architectural versions, but then want to swap it all back to the
differential versions again when we're done saving/loading.

-Aaron

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8ca4d30797..12c53e54e9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1379,11 +1379,13 @@  static const ARMCPRegInfo v7_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 0,
       .access = PL0_RW, .accessfn = pmreg_access_ccntr,
       .type = ARM_CP_IO,
-      .readfn = pmccntr_read, .writefn = pmccntr_write, },
+      .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
+      .readfn = pmccntr_read, .writefn = pmccntr_write,
+      .raw_readfn = raw_read, .raw_writefn = raw_write, },
 #endif
     { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
-      .writefn = pmccfiltr_write,
+      .writefn = pmccfiltr_write, .raw_writefn = raw_write,
       .access = PL0_RW, .accessfn = pmreg_access,
       .type = ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
diff --git a/target/arm/machine.c b/target/arm/machine.c
index ff4ec22bf7..8139b25be5 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -584,6 +584,8 @@  static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
+    pmccntr_sync(&cpu->env);
+
     if (kvm_enabled()) {
         if (!write_kvmstate_to_list(cpu)) {
             /* This should never fail */
@@ -605,6 +607,19 @@  static int cpu_pre_save(void *opaque)
     return 0;
 }
 
+static void cpu_post_save(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+    pmccntr_sync(&cpu->env);
+}
+
+static int cpu_pre_load(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+    pmccntr_sync(&cpu->env);
+    return 0;
+}
+
 static int cpu_post_load(void *opaque, int version_id)
 {
     ARMCPU *cpu = opaque;
@@ -652,6 +667,8 @@  static int cpu_post_load(void *opaque, int version_id)
     hw_breakpoint_update_all(cpu);
     hw_watchpoint_update_all(cpu);
 
+    pmccntr_sync(&cpu->env);
+
     return 0;
 }
 
@@ -660,6 +677,8 @@  const VMStateDescription vmstate_arm_cpu = {
     .version_id = 22,
     .minimum_version_id = 22,
     .pre_save = cpu_pre_save,
+    .post_save = cpu_post_save,
+    .pre_load = cpu_pre_load,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16),