[Qemu-devel] [PATCH v6 00/14] More fully implement ARM PMUv3

[Qemu-devel] [PATCH v6 00/14] More fully implement ARM PMUv3

[Aaron Lindsay]

The ARM PMU implementation currently contains a basic cycle counter, but
it is often useful to gather counts of other events, filter them based
on execution mode, and/or be notified on counter overflow. These patches
flesh out the implementations of various PMU registers including
PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to represent
arbitrary counter types, implement mode filtering, send interrupts on
counter overflow, and add instruction, cycle, and software increment
events.

Since v5 [1] I have:
* Taken a first pass at addressing migration
* Restructured the list of supported events, and ensured they're all
  initialized 
* Fixed aliasing for PMOVSSET
* Added ARM_CP_IO for PMINTENCLR and PMINTENCLR_EL1
* Addressed a few non-code issues (comment style, patch staging,
  spelling, etc.)

[1] - https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg06830.html

Aaron Lindsay (14):
  target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly
    doing IO
  target/arm: Mask PMOVSR writes based on supported counters
  migration: Add post_save function to VMStateDescription
  target/arm: Swap PMU values before/after migrations
  target/arm: Reorganize PMCCNTR accesses
  target/arm: Filter cycle counter based on PMCCFILTR_EL0
  target/arm: Allow AArch32 access for PMCCFILTR
  target/arm: Implement PMOVSSET
  target/arm: Add array for supported PMU events, generate PMCEID[01]
  target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER
  target/arm: PMU: Add instruction and cycle events
  target/arm: PMU: Set PMCR.N to 4
  target/arm: Implement PMSWINC
  target/arm: Send interrupts on PMU counter overflow

 docs/devel/migration.rst    |   9 +-
 include/migration/vmstate.h |   1 +
 migration/vmstate.c         |  10 +-
 target/arm/cpu.c            |  28 +-
 target/arm/cpu.h            |  68 +++-
 target/arm/cpu64.c          |   2 -
 target/arm/helper.c         | 781 ++++++++++++++++++++++++++++++++----
 target/arm/machine.c        |  19 +
 8 files changed, 817 insertions(+), 101 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH v6 01/14] target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly doing IO

[Aaron Lindsay]

I previously fixed this for PMINTENSET_EL1, but missed these.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
---
 target/arm/helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index c83f7c1109..52c76b7444 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1423,12 +1423,14 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .writefn = pmintenset_write, .raw_writefn = raw_write,
       .resetvalue = 0x0 },
     { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
-      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
+      .access = PL1_RW, .accessfn = access_tpm,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .writefn = pmintenclr_write, },
     { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
-      .access = PL1_RW, .accessfn = access_tpm, .type = ARM_CP_ALIAS,
+      .access = PL1_RW, .accessfn = access_tpm,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .writefn = pmintenclr_write },
     { .name = "CCSIDR", .state = ARM_CP_STATE_BOTH,
-- 
2.19.1

[Qemu-devel] [PATCH v6 02/14] target/arm: Mask PMOVSR writes based on supported counters

[Aaron Lindsay]

This is an amendment to my earlier patch:
    commit 7ece99b17e832065236c07a158dfac62619ef99b
    Author: Aaron Lindsay <alindsay@codeaurora.org>
    Date:   Thu Apr 26 11:04:39 2018 +0100

	target/arm: Mask PMU register writes based on PMCR_EL0.N

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 52c76b7444..8ca4d30797 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1179,6 +1179,7 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                          uint64_t value)
 {
+    value &= pmu_counter_mask(env);
     env->cp15.c9_pmovsr &= ~value;
 }
 
-- 
2.19.1

[Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Aaron Lindsay]

In some cases it may be helpful to modify state before saving it for
migration, and then modify the state back after it has been saved. The
existing pre_save function provides half of this functionality. This
patch adds a post_save function to provide the second half.

Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
---
 docs/devel/migration.rst    |  9 +++++++--
 include/migration/vmstate.h |  1 +
 migration/vmstate.c         | 10 +++++++++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index 687570754d..2a2533c9b3 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -419,8 +419,13 @@ The functions to do that are inside a vmstate definition, and are called:
 
   This function is called before we save the state of one device.
 
-Example: You can look at hpet.c, that uses the three function to
-massage the state that is transferred.
+- ``void (*post_save)(void *opaque);``
+
+  This function is called after we save the state of one device
+  (even upon failure, unless the call to pre_save returned and error).
+
+Example: You can look at hpet.c, that uses the first three functions
+to massage the state that is transferred.
 
 The ``VMSTATE_WITH_TMP`` macro may be useful when the migration
 data doesn't match the stored device data well; it allows an
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 2b501d0466..f6053b94e4 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -185,6 +185,7 @@ struct VMStateDescription {
     int (*pre_load)(void *opaque);
     int (*post_load)(void *opaque, int version_id);
     int (*pre_save)(void *opaque);
+    void (*post_save)(void *opaque);
     bool (*needed)(void *opaque);
     VMStateField *fields;
     const VMStateDescription **subsections;
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 0bc240a317..9afc9298f3 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -387,6 +387,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                 if (ret) {
                     error_report("Save of field %s/%s failed",
                                  vmsd->name, field->name);
+                    if (vmsd->post_save) {
+                        vmsd->post_save(opaque);
+                    }
                     return ret;
                 }
 
@@ -412,7 +415,12 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         json_end_array(vmdesc);
     }
 
-    return vmstate_subsection_save(f, vmsd, opaque, vmdesc);
+    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
+
+    if (vmsd->post_save) {
+        vmsd->post_save(opaque);
+    }
+    return ret;
 }
 
 static const VMStateDescription *
-- 
2.19.1

[Qemu-devel] [PATCH v6 04/14] target/arm: Swap PMU values before/after migrations

[Aaron Lindsay]

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 <aclindsa@gmail.com>
---
 target/arm/helper.c  |  6 ++++--
 target/arm/machine.c | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

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),
-- 
2.19.1

[Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay]

pmccntr_read and pmccntr_write contained duplicate code that was already
being handled by pmccntr_sync. Consolidate the duplicated code into two
functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to
c15_ccnt in CPUARMState so that we can simultaneously save both the
architectural register value and the last underlying cycle count - this
ensures time isn't lost and will also allow us to access the 'old'
architectural register value in order to detect overflows in later
patches.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.h     | 26 ++++++++----
 target/arm/helper.c  | 96 +++++++++++++++++++++++---------------------
 target/arm/machine.c |  8 ++--
 3 files changed, 73 insertions(+), 57 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 3a2aff1192..fdf672ca22 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -468,10 +468,20 @@ typedef struct CPUARMState {
         uint64_t oslsr_el1; /* OS Lock Status */
         uint64_t mdcr_el2;
         uint64_t mdcr_el3;
-        /* If the counter is enabled, this stores the last time the counter
-         * was reset. Otherwise it stores the counter value
+        /* Stores the architectural value of the counter *the last time it was
+         * updated* by pmccntr_op_start. Accesses should always be surrounded
+         * by pmccntr_op_start/pmccntr_op_finish to guarantee the latest
+         * architecturally-correct value is being read/set.
          */
         uint64_t c15_ccnt;
+        /* Stores the delta between the architectural value and the underlying
+         * cycle count during normal operation. It is used to update c15_ccnt
+         * to be the correct architectural value before accesses. During
+         * accesses, c15_ccnt_delta contains the underlying count being used
+         * for the access, after which it reverts to the delta value in
+         * pmccntr_op_finish.
+         */
+        uint64_t c15_ccnt_delta;
         uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
         uint64_t vpidr_el2; /* Virtualization Processor ID Register */
         uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
@@ -937,15 +947,15 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
                            void *puc);
 
 /**
- * pmccntr_sync
+ * pmccntr_op_start/finish
  * @env: CPUARMState
  *
- * Synchronises the counter in the PMCCNTR. This must always be called twice,
- * once before any action that might affect the timer and again afterwards.
- * The function is used to swap the state of the register if required.
- * This only happens when not in user mode (!CONFIG_USER_ONLY)
+ * Convert the counter in the PMCCNTR between its delta form (the typical mode
+ * when it's enabled) and the guest-visible value. These two calls must always
+ * surround any action which might affect the counter.
  */
-void pmccntr_sync(CPUARMState *env);
+void pmccntr_op_start(CPUARMState *env);
+void pmccntr_op_finish(CPUARMState *env);
 
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 12c53e54e9..91e4e4170b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1052,28 +1052,53 @@ static inline bool arm_ccnt_enabled(CPUARMState *env)
 
     return true;
 }
-
-void pmccntr_sync(CPUARMState *env)
+/*
+ * Ensure c15_ccnt is the guest-visible count so that operations such as
+ * enabling/disabling the counter or filtering, modifying the count itself,
+ * etc. can be done logically. This is essentially a no-op if the counter is
+ * not enabled at the time of the call.
+ */
+void pmccntr_op_start(CPUARMState *env)
 {
-    uint64_t temp_ticks;
-
-    temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+    uint64_t cycles = 0;
+    cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
 
-    if (env->cp15.c9_pmcr & PMCRD) {
-        /* Increment once every 64 processor clock cycles */
-        temp_ticks /= 64;
+    if (arm_ccnt_enabled(env)) {
+        uint64_t eff_cycles = cycles;
+        if (env->cp15.c9_pmcr & PMCRD) {
+            /* Increment once every 64 processor clock cycles */
+            eff_cycles /= 64;
+        }
+
+        env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt_delta;
     }
+    env->cp15.c15_ccnt_delta = cycles;
+}
 
+/*
+ * If PMCCNTR is enabled, recalculate the delta between the clock and the
+ * guest-visible count. A call to pmccntr_op_finish should follow every call to
+ * pmccntr_op_start.
+ */
+void pmccntr_op_finish(CPUARMState *env)
+{
     if (arm_ccnt_enabled(env)) {
-        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+        uint64_t prev_cycles = env->cp15.c15_ccnt_delta;
+
+        if (env->cp15.c9_pmcr & PMCRD) {
+            /* Increment once every 64 processor clock cycles */
+            prev_cycles /= 64;
+        }
+
+        env->cp15.c15_ccnt_delta = prev_cycles - env->cp15.c15_ccnt;
     }
 }
 
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    pmccntr_sync(env);
+    pmccntr_op_start(env);
 
     if (value & PMCRC) {
         /* The counter has been reset */
@@ -1084,26 +1109,16 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
 
-    pmccntr_sync(env);
+    pmccntr_op_finish(env);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    uint64_t total_ticks;
-
-    if (!arm_ccnt_enabled(env)) {
-        /* Counter is disabled, do not change value */
-        return env->cp15.c15_ccnt;
-    }
-
-    total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-    if (env->cp15.c9_pmcr & PMCRD) {
-        /* Increment once every 64 processor clock cycles */
-        total_ticks /= 64;
-    }
-    return total_ticks - env->cp15.c15_ccnt;
+    uint64_t ret;
+    pmccntr_op_start(env);
+    ret = env->cp15.c15_ccnt;
+    pmccntr_op_finish(env);
+    return ret;
 }
 
 static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1120,22 +1135,9 @@ static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
-    uint64_t total_ticks;
-
-    if (!arm_ccnt_enabled(env)) {
-        /* Counter is disabled, set the absolute value */
-        env->cp15.c15_ccnt = value;
-        return;
-    }
-
-    total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
-
-    if (env->cp15.c9_pmcr & PMCRD) {
-        /* Increment once every 64 processor clock cycles */
-        total_ticks /= 64;
-    }
-    env->cp15.c15_ccnt = total_ticks - value;
+    pmccntr_op_start(env);
+    env->cp15.c15_ccnt = value;
+    pmccntr_op_finish(env);
 }
 
 static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1148,7 +1150,11 @@ static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
 
 #else /* CONFIG_USER_ONLY */
 
-void pmccntr_sync(CPUARMState *env)
+void pmccntr_op_start(CPUARMState *env)
+{
+}
+
+void pmccntr_op_finish(CPUARMState *env)
 {
 }
 
@@ -1157,9 +1163,9 @@ void pmccntr_sync(CPUARMState *env)
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
-    pmccntr_sync(env);
+    pmccntr_op_start(env);
     env->cp15.pmccfiltr_el0 = value & 0xfc000000;
-    pmccntr_sync(env);
+    pmccntr_op_finish(env);
 }
 
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 8139b25be5..581c44cf08 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
-    pmccntr_sync(&cpu->env);
+    pmccntr_op_start(&cpu->env);
 
     if (kvm_enabled()) {
         if (!write_kvmstate_to_list(cpu)) {
@@ -610,13 +610,13 @@ static int cpu_pre_save(void *opaque)
 static void cpu_post_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
-    pmccntr_sync(&cpu->env);
+    pmccntr_op_finish(&cpu->env);
 }
 
 static int cpu_pre_load(void *opaque)
 {
     ARMCPU *cpu = opaque;
-    pmccntr_sync(&cpu->env);
+    pmccntr_op_start(&cpu->env);
     return 0;
 }
 
@@ -667,7 +667,7 @@ static int cpu_post_load(void *opaque, int version_id)
     hw_breakpoint_update_all(cpu);
     hw_watchpoint_update_all(cpu);
 
-    pmccntr_sync(&cpu->env);
+    pmccntr_op_finish(&cpu->env);
 
     return 0;
 }
-- 
2.19.1

[Qemu-devel] [PATCH v6 06/14] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

The pmu_counter_enabled and pmu_op_start/finish functions are generic
(as opposed to PMCCNTR-specific) to allow for the implementation of
other events.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c     |   3 ++
 target/arm/cpu.h     |  22 +++++++-
 target/arm/helper.c  | 118 +++++++++++++++++++++++++++++++++++++++----
 target/arm/machine.c |   8 +--
 4 files changed, 137 insertions(+), 14 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b5e61cc177..f69addb961 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -948,6 +948,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     if (!cpu->has_pmu) {
         unset_feature(env, ARM_FEATURE_PMU);
         cpu->id_aa64dfr0 &= ~0xf00;
+    } else if (!kvm_enabled()) {
+        arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
+        arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
     }
 
     if (!arm_feature(env, ARM_FEATURE_EL2)) {
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index fdf672ca22..d9cd8dd92c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -957,6 +957,24 @@ int cpu_arm_signal_handler(int host_signum, void *pinfo,
 void pmccntr_op_start(CPUARMState *env);
 void pmccntr_op_finish(CPUARMState *env);
 
+/**
+ * pmu_op_start/finish
+ * @env: CPUARMState
+ *
+ * Convert all PMU counters between their delta form (the typical mode when
+ * they are enabled) and the guest-visible values. These two calls must
+ * surround any action which might affect the counters, and the return value
+ * from pmu_op_start must be supplied as the second argument to pmu_op_finish.
+ */
+void pmu_op_start(CPUARMState *env);
+void pmu_op_finish(CPUARMState *env);
+
+/**
+ * Functions to register as EL change hooks for PMU mode filtering
+ */
+void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
+void pmu_post_el_change(ARMCPU *cpu, void *ignored);
+
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
  * for both old and new bit meanings. Code which tests against those
@@ -1018,7 +1036,8 @@ void pmccntr_op_finish(CPUARMState *env);
 
 #define MDCR_EPMAD    (1U << 21)
 #define MDCR_EDAD     (1U << 20)
-#define MDCR_SPME     (1U << 17)
+#define MDCR_SPME     (1U << 17)  /* MDCR_EL3 */
+#define MDCR_HPMD     (1U << 17)  /* MDCR_EL2 */
 #define MDCR_SDD      (1U << 16)
 #define MDCR_SPD      (3U << 14)
 #define MDCR_TDRA     (1U << 11)
@@ -1028,6 +1047,7 @@ void pmccntr_op_finish(CPUARMState *env);
 #define MDCR_HPME     (1U << 7)
 #define MDCR_TPM      (1U << 6)
 #define MDCR_TPMCR    (1U << 5)
+#define MDCR_HPMN     (0x1fU)
 
 /* Not all of the MDCR_EL3 bits are present in the 32-bit SDCR */
 #define SDCR_VALID_MASK (MDCR_EPMAD | MDCR_EDAD | MDCR_SPME | MDCR_SPD)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 91e4e4170b..52bd13fdde 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -943,10 +943,24 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Definitions for the PMU registers */
 #define PMCRN_MASK  0xf800
 #define PMCRN_SHIFT 11
+#define PMCRDP  0x10
 #define PMCRD   0x8
 #define PMCRC   0x4
 #define PMCRE   0x1
 
+#define PMXEVTYPER_P          0x80000000
+#define PMXEVTYPER_U          0x40000000
+#define PMXEVTYPER_NSK        0x20000000
+#define PMXEVTYPER_NSU        0x10000000
+#define PMXEVTYPER_NSH        0x08000000
+#define PMXEVTYPER_M          0x04000000
+#define PMXEVTYPER_MT         0x02000000
+#define PMXEVTYPER_EVTCOUNT   0x0000ffff
+#define PMXEVTYPER_MASK       (PMXEVTYPER_P | PMXEVTYPER_U | PMXEVTYPER_NSK | \
+                               PMXEVTYPER_NSU | PMXEVTYPER_NSH | \
+                               PMXEVTYPER_M | PMXEVTYPER_MT | \
+                               PMXEVTYPER_EVTCOUNT)
+
 static inline uint32_t pmu_num_counters(CPUARMState *env)
 {
   return (env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT;
@@ -1042,16 +1056,66 @@ static CPAccessResult pmreg_access_ccntr(CPUARMState *env,
     return pmreg_access(env, ri, isread);
 }
 
-static inline bool arm_ccnt_enabled(CPUARMState *env)
+/* Returns true if the counter (pass 31 for PMCCNTR) should count events using
+ * the current EL, security state, and register configuration.
+ */
+static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
 {
-    /* This does not support checking PMCCFILTR_EL0 register */
+    uint64_t filter;
+    bool e, p, u, nsk, nsu, nsh, m;
+    bool enabled, prohibited, filtered;
+    bool secure = arm_is_secure(env);
+    int el = arm_current_el(env);
+    uint8_t hpmn = env->cp15.mdcr_el2 & MDCR_HPMN;
 
-    if (!(env->cp15.c9_pmcr & PMCRE) || !(env->cp15.c9_pmcnten & (1 << 31))) {
-        return false;
+    if (!arm_feature(env, ARM_FEATURE_EL2) ||
+            (counter < hpmn || counter == 31)) {
+        e = env->cp15.c9_pmcr & PMCRE;
+    } else {
+        e = env->cp15.mdcr_el2 & MDCR_HPME;
     }
+    enabled = e && (env->cp15.c9_pmcnten & (1 << counter));
 
-    return true;
+    if (!secure) {
+        if (el == 2 && (counter < hpmn || counter == 31)) {
+            prohibited = env->cp15.mdcr_el2 & MDCR_HPMD;
+        } else {
+            prohibited = false;
+        }
+    } else {
+        prohibited = arm_feature(env, ARM_FEATURE_EL3) &&
+           (env->cp15.mdcr_el3 & MDCR_SPME);
+    }
+
+    if (prohibited && counter == 31) {
+        prohibited = env->cp15.c9_pmcr & PMCRDP;
+    }
+
+    /* TODO Remove assert, set filter to correct PMEVTYPER */
+    assert(counter == 31);
+    filter = env->cp15.pmccfiltr_el0;
+
+    p   = filter & PMXEVTYPER_P;
+    u   = filter & PMXEVTYPER_U;
+    nsk = arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_NSK);
+    nsu = arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_NSU);
+    nsh = arm_feature(env, ARM_FEATURE_EL2) && (filter & PMXEVTYPER_NSH);
+    m   = arm_el_is_aa64(env, 1) &&
+              arm_feature(env, ARM_FEATURE_EL3) && (filter & PMXEVTYPER_M);
+
+    if (el == 0) {
+        filtered = secure ? u : u != nsu;
+    } else if (el == 1) {
+        filtered = secure ? p : p != nsk;
+    } else if (el == 2) {
+        filtered = !nsh;
+    } else { /* EL3 */
+        filtered = m != p;
+    }
+
+    return enabled && !prohibited && !filtered;
 }
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,
@@ -1064,7 +1128,7 @@ void pmccntr_op_start(CPUARMState *env)
     cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
                           ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
 
-    if (arm_ccnt_enabled(env)) {
+    if (pmu_counter_enabled(env, 31)) {
         uint64_t eff_cycles = cycles;
         if (env->cp15.c9_pmcr & PMCRD) {
             /* Increment once every 64 processor clock cycles */
@@ -1083,7 +1147,7 @@ void pmccntr_op_start(CPUARMState *env)
  */
 void pmccntr_op_finish(CPUARMState *env)
 {
-    if (arm_ccnt_enabled(env)) {
+    if (pmu_counter_enabled(env, 31)) {
         uint64_t prev_cycles = env->cp15.c15_ccnt_delta;
 
         if (env->cp15.c9_pmcr & PMCRD) {
@@ -1095,10 +1159,30 @@ void pmccntr_op_finish(CPUARMState *env)
     }
 }
 
+void pmu_op_start(CPUARMState *env)
+{
+    pmccntr_op_start(env);
+}
+
+void pmu_op_finish(CPUARMState *env)
+{
+    pmccntr_op_finish(env);
+}
+
+void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
+{
+    pmu_op_start(&cpu->env);
+}
+
+void pmu_post_el_change(ARMCPU *cpu, void *ignored)
+{
+    pmu_op_finish(&cpu->env);
+}
+
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    pmccntr_op_start(env);
+    pmu_op_start(env);
 
     if (value & PMCRC) {
         /* The counter has been reset */
@@ -1109,7 +1193,7 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
 
-    pmccntr_op_finish(env);
+    pmu_op_finish(env);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
@@ -1158,6 +1242,22 @@ void pmccntr_op_finish(CPUARMState *env)
 {
 }
 
+void pmu_op_start(CPUARMState *env)
+{
+}
+
+void pmu_op_finish(CPUARMState *env)
+{
+}
+
+void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
+{
+}
+
+void pmu_post_el_change(ARMCPU *cpu, void *ignored)
+{
+}
+
 #endif
 
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
diff --git a/target/arm/machine.c b/target/arm/machine.c
index 581c44cf08..bb9e47f602 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
 
-    pmccntr_op_start(&cpu->env);
+    pmu_op_start(&cpu->env);
 
     if (kvm_enabled()) {
         if (!write_kvmstate_to_list(cpu)) {
@@ -610,13 +610,13 @@ static int cpu_pre_save(void *opaque)
 static void cpu_post_save(void *opaque)
 {
     ARMCPU *cpu = opaque;
-    pmccntr_op_finish(&cpu->env);
+    pmu_op_finish(&cpu->env);
 }
 
 static int cpu_pre_load(void *opaque)
 {
     ARMCPU *cpu = opaque;
-    pmccntr_op_start(&cpu->env);
+    pmu_op_start(&cpu->env);
     return 0;
 }
 
@@ -667,7 +667,7 @@ static int cpu_post_load(void *opaque, int version_id)
     hw_breakpoint_update_all(cpu);
     hw_watchpoint_update_all(cpu);
 
-    pmccntr_op_finish(&cpu->env);
+    pmu_op_finish(&cpu->env);
 
     return 0;
 }
-- 
2.19.1

[Qemu-devel] [PATCH v6 07/14] target/arm: Allow AArch32 access for PMCCFILTR

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 52bd13fdde..e804caaced 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -961,6 +961,10 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
                                PMXEVTYPER_M | PMXEVTYPER_MT | \
                                PMXEVTYPER_EVTCOUNT)
 
+#define PMCCFILTR             0xf8000000
+#define PMCCFILTR_M           PMXEVTYPER_M
+#define PMCCFILTR_EL0         (PMCCFILTR | PMCCFILTR_M)
+
 static inline uint32_t pmu_num_counters(CPUARMState *env)
 {
   return (env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT;
@@ -1264,10 +1268,26 @@ static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
     pmccntr_op_start(env);
-    env->cp15.pmccfiltr_el0 = value & 0xfc000000;
+    env->cp15.pmccfiltr_el0 = value & PMCCFILTR_EL0;
+    pmccntr_op_finish(env);
+}
+
+static void pmccfiltr_write_a32(CPUARMState *env, const ARMCPRegInfo *ri,
+                            uint64_t value)
+{
+    pmccntr_op_start(env);
+    /* M is not accessible from AArch32 */
+    env->cp15.pmccfiltr_el0 = (env->cp15.pmccfiltr_el0 & PMCCFILTR_M) |
+        (value & PMCCFILTR);
     pmccntr_op_finish(env);
 }
 
+static uint64_t pmccfiltr_read_a32(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /* M is not visible in AArch32 */
+    return env->cp15.pmccfiltr_el0 & PMCCFILTR;
+}
+
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -1489,6 +1509,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .readfn = pmccntr_read, .writefn = pmccntr_write,
       .raw_readfn = raw_read, .raw_writefn = raw_write, },
 #endif
+    { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7,
+      .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
+      .resetvalue = 0, },
     { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
       .writefn = pmccfiltr_write, .raw_writefn = raw_write,
-- 
2.19.1

[Qemu-devel] [PATCH v6 08/14] target/arm: Implement PMOVSSET

[Aaron Lindsay]

Add an array for PMOVSSET so we only define it for v7ve+ platforms

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index e804caaced..f3c00c3db0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1309,6 +1309,13 @@ static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmovsr &= ~value;
 }
 
+static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                         uint64_t value)
+{
+    value &= pmu_counter_mask(env);
+    env->cp15.c9_pmovsr |= value;
+}
+
 static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
                              uint64_t value)
 {
@@ -1662,6 +1669,24 @@ static const ARMCPRegInfo v7mp_cp_reginfo[] = {
     REGINFO_SENTINEL
 };
 
+static const ARMCPRegInfo pmovsset_cp_reginfo[] = {
+    /* PMOVSSET is not implemented in v7 before v7ve */
+    { .name = "PMOVSSET", .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 3,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
+      .writefn = pmovsset_write,
+      .raw_writefn = raw_write },
+    { .name = "PMOVSSET_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 3,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+      .writefn = pmovsset_write,
+      .raw_writefn = raw_write },
+    REGINFO_SENTINEL
+};
+
 static void teecr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
@@ -5116,6 +5141,9 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         !arm_feature(env, ARM_FEATURE_PMSA)) {
         define_arm_cp_regs(cpu, v7mp_cp_reginfo);
     }
+    if (arm_feature(env, ARM_FEATURE_V7VE)) {
+        define_arm_cp_regs(cpu, pmovsset_cp_reginfo);
+    }
     if (arm_feature(env, ARM_FEATURE_V7)) {
         /* v7 performance monitor control register: same implementor
          * field as main ID register, and we implement only the cycle
-- 
2.19.1

[Qemu-devel] [PATCH v6 09/14] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay]

This commit doesn't add any supported events, but provides the framework
for adding them. We store the pm_event structs in a simple array, and
provide the mapping from the event numbers to array indexes in the
supported_event_map array. Because the value of PMCEID[01] depends upon
which events are supported at runtime, generate it dynamically.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c    | 20 +++++++++++++-------
 target/arm/cpu.h    | 10 ++++++++++
 target/arm/cpu64.c  |  2 --
 target/arm/helper.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index f69addb961..7f39f25f51 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -948,9 +948,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     if (!cpu->has_pmu) {
         unset_feature(env, ARM_FEATURE_PMU);
         cpu->id_aa64dfr0 &= ~0xf00;
-    } else if (!kvm_enabled()) {
-        arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
-        arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
+    }
+    if (arm_feature(env, ARM_FEATURE_PMU)) {
+        uint64_t pmceid = get_pmceid(&cpu->env);
+        cpu->pmceid0 = pmceid & 0xffffffff;
+        cpu->pmceid1 = (pmceid >> 32) & 0xffffffff;
+
+        if (!kvm_enabled()) {
+            arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
+            arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
+        }
+    } else {
+        cpu->pmceid0 = 0x00000000;
+        cpu->pmceid1 = 0x00000000;
     }
 
     if (!arm_feature(env, ARM_FEATURE_EL2)) {
@@ -1583,8 +1593,6 @@ static void cortex_a7_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
-    cpu->pmceid0 = 0x00000000;
-    cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10101105;
     cpu->id_mmfr1 = 0x40000000;
@@ -1626,8 +1634,6 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
-    cpu->pmceid0 = 0x0000000;
-    cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
     cpu->id_mmfr1 = 0x20000000;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d9cd8dd92c..cc026f0b75 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -975,6 +975,16 @@ void pmu_op_finish(CPUARMState *env);
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
 void pmu_post_el_change(ARMCPU *cpu, void *ignored);
 
+/*
+ * get_pmceid
+ * @env: CPUARMState
+ *
+ * Return the PMCEID[01] register values corresponding to the counters which
+ * are supported given the current configuration (0 is low 32, 1 is high 32
+ * bits)
+ */
+uint64_t get_pmceid(CPUARMState *env);
+
 /* SCTLR bit meanings. Several bits have been reused in newer
  * versions of the architecture; in that case we define constants
  * for both old and new bit meanings. Code which tests against those
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index db71504cb5..440d874c17 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -143,8 +143,6 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar6 = 0;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
-    cpu->pmceid0 = 0x00000000;
-    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index f3c00c3db0..375b6dcda5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -976,6 +976,48 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
   return (1 << 31) | ((1 << pmu_num_counters(env)) - 1);
 }
 
+typedef struct pm_event {
+    uint16_t number; /* PMEVTYPER.evtCount is 16 bits wide */
+    /* If the event is supported on this CPU (used to generate PMCEID[01]) */
+    bool (*supported)(CPUARMState *);
+    /*
+     * Retrieve the current count of the underlying event. The programmed
+     * counters hold a difference from the return value from this function
+     */
+    uint64_t (*get_count)(CPUARMState *);
+} pm_event;
+
+static const pm_event pm_events[] = {
+};
+#define MAX_EVENT_ID 0x0
+#define UNSUPPORTED_EVENT UINT16_MAX
+static uint16_t supported_event_map[MAX_EVENT_ID + 1];
+
+/*
+ * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high
+ * 32). We also use it to build a map of ARM event numbers to indices in
+ * our pm_events array.
+ */
+uint64_t get_pmceid(CPUARMState *env)
+{
+    uint64_t pmceid = 0;
+    unsigned int i;
+
+    for (i = 0; i <= MAX_EVENT_ID; i++) {
+        supported_event_map[i] = UNSUPPORTED_EVENT;
+    }
+
+    for (i = 0; i < ARRAY_SIZE(pm_events); i++) {
+        const pm_event *cnt = &pm_events[i];
+        assert(cnt->number <= MAX_EVENT_ID);
+        if (cnt->supported(env)) {
+            pmceid |= (1 << cnt->number);
+            supported_event_map[cnt->number] = i;
+        }
+    }
+    return pmceid;
+}
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                    bool isread)
 {
-- 
2.19.1

[Qemu-devel] [PATCH v6 10/14] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER

[Aaron Lindsay]

Add arrays to hold the registers, the definitions themselves, access
functions, and logic to reset counters when PMCR.P is set. Update
filtering code to support counters other than PMCCNTR. Support migration
with raw read/write functions.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
---
 target/arm/cpu.h    |   3 +
 target/arm/helper.c | 296 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 282 insertions(+), 17 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index cc026f0b75..f4317f87c9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -482,6 +482,9 @@ typedef struct CPUARMState {
          * pmccntr_op_finish.
          */
         uint64_t c15_ccnt_delta;
+        uint64_t c14_pmevcntr[31];
+        uint64_t c14_pmevcntr_delta[31];
+        uint64_t c14_pmevtyper[31];
         uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
         uint64_t vpidr_el2; /* Virtualization Processor ID Register */
         uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 375b6dcda5..f0798f7a8c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -946,6 +946,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMCRDP  0x10
 #define PMCRD   0x8
 #define PMCRC   0x4
+#define PMCRP   0x2
 #define PMCRE   0x1
 
 #define PMXEVTYPER_P          0x80000000
@@ -1018,6 +1019,17 @@ uint64_t get_pmceid(CPUARMState *env)
     return pmceid;
 }
 
+/*
+ * Check at runtime whether a PMU event is supported for the current machine
+ */
+static bool event_supported(uint16_t number)
+{
+    if (number > MAX_EVENT_ID) {
+        return false;
+    }
+    return supported_event_map[number] != UNSUPPORTED_EVENT;
+}
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                    bool isread)
 {
@@ -1137,9 +1149,11 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
         prohibited = env->cp15.c9_pmcr & PMCRDP;
     }
 
-    /* TODO Remove assert, set filter to correct PMEVTYPER */
-    assert(counter == 31);
-    filter = env->cp15.pmccfiltr_el0;
+    if (counter == 31) {
+        filter = env->cp15.pmccfiltr_el0;
+    } else {
+        filter = env->cp15.c14_pmevtyper[counter];
+    }
 
     p   = filter & PMXEVTYPER_P;
     u   = filter & PMXEVTYPER_U;
@@ -1159,6 +1173,17 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
         filtered = m != p;
     }
 
+    if (counter != 31) {
+        /*
+         * If not checking PMCCNTR, ensure the counter is setup to an event we
+         * support
+         */
+        uint16_t event = filter & PMXEVTYPER_EVTCOUNT;
+        if (!event_supported(event)) {
+            return false;
+        }
+    }
+
     return enabled && !prohibited && !filtered;
 }
 
@@ -1205,14 +1230,47 @@ void pmccntr_op_finish(CPUARMState *env)
     }
 }
 
+static void pmevcntr_op_start(CPUARMState *env, uint8_t counter)
+{
+
+    uint16_t event = env->cp15.c14_pmevtyper[counter] & PMXEVTYPER_EVTCOUNT;
+    uint64_t count = 0;
+    if (event_supported(event)) {
+        uint16_t event_idx = supported_event_map[event];
+        count = pm_events[event_idx].get_count(env);
+    }
+
+    if (pmu_counter_enabled(env, counter)) {
+        env->cp15.c14_pmevcntr[counter] =
+            count - env->cp15.c14_pmevcntr_delta[counter];
+    }
+    env->cp15.c14_pmevcntr_delta[counter] = count;
+}
+
+static void pmevcntr_op_finish(CPUARMState *env, uint8_t counter)
+{
+    if (pmu_counter_enabled(env, counter)) {
+        env->cp15.c14_pmevcntr_delta[counter] -=
+            env->cp15.c14_pmevcntr[counter];
+    }
+}
+
 void pmu_op_start(CPUARMState *env)
 {
+    unsigned int i;
     pmccntr_op_start(env);
+    for (i = 0; i < pmu_num_counters(env); i++) {
+        pmevcntr_op_start(env, i);
+    }
 }
 
 void pmu_op_finish(CPUARMState *env)
 {
+    unsigned int i;
     pmccntr_op_finish(env);
+    for (i = 0; i < pmu_num_counters(env); i++) {
+        pmevcntr_op_finish(env, i);
+    }
 }
 
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
@@ -1235,6 +1293,13 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         env->cp15.c15_ccnt = 0;
     }
 
+    if (value & PMCRP) {
+        unsigned int i;
+        for (i = 0; i < pmu_num_counters(env); i++) {
+            env->cp15.c14_pmevcntr[i] = 0;
+        }
+    }
+
     /* only the DP, X, D and E bits are writable */
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
@@ -1288,6 +1353,14 @@ void pmccntr_op_finish(CPUARMState *env)
 {
 }
 
+void pmevcntr_op_start(CPUARMState *env, uint8_t i)
+{
+}
+
+void pmevcntr_op_finish(CPUARMState *env, uint8_t i)
+{
+}
+
 void pmu_op_start(CPUARMState *env)
 {
 }
@@ -1358,30 +1431,174 @@ static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmovsr |= value;
 }
 
-static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
-                             uint64_t value)
+static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value, const uint8_t counter)
 {
+    if (counter == 31) {
+        pmccfiltr_write(env, ri, value);
+    } else if (counter < pmu_num_counters(env)) {
+        pmevcntr_op_start(env, counter);
+
+        /*
+         * If this counter's event type is changing, store the current
+         * underlying count for the new type in c14_pmevcntr_delta[counter] so
+         * pmevcntr_op_finish has the correct baseline when it converts back to
+         * a delta.
+         */
+        uint16_t old_event = env->cp15.c14_pmevtyper[counter] &
+            PMXEVTYPER_EVTCOUNT;
+        uint16_t new_event = value & PMXEVTYPER_EVTCOUNT;
+        if (old_event != new_event) {
+            uint64_t count = 0;
+            if (event_supported(new_event)) {
+                uint16_t event_idx = supported_event_map[new_event];
+                count = pm_events[event_idx].get_count(env);
+            }
+            env->cp15.c14_pmevcntr_delta[counter] = count;
+        }
+
+        env->cp15.c14_pmevtyper[counter] = value & PMXEVTYPER_MASK;
+        pmevcntr_op_finish(env, counter);
+    }
     /* Attempts to access PMXEVTYPER are CONSTRAINED UNPREDICTABLE when
      * PMSELR value is equal to or greater than the number of implemented
      * counters, but not equal to 0x1f. We opt to behave as a RAZ/WI.
      */
-    if (env->cp15.c9_pmselr == 0x1f) {
-        pmccfiltr_write(env, ri, value);
+}
+
+static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                               const uint8_t counter)
+{
+    if (counter == 31) {
+        return env->cp15.pmccfiltr_el0;
+    } else if (counter < pmu_num_counters(env)) {
+        return env->cp15.c14_pmevtyper[counter];
+    } else {
+      /*
+       * We opt to behave as a RAZ/WI when attempts to access PMXEVTYPER
+       * are CONSTRAINED UNPREDICTABLE. See comments in pmevtyper_write().
+       */
+        return 0;
     }
 }
 
+static void pmevtyper_writefn(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    pmevtyper_write(env, ri, value, counter);
+}
+
+static void pmevtyper_rawwrite(CPUARMState *env, const ARMCPRegInfo *ri,
+                               uint64_t value)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    env->cp15.c14_pmevtyper[counter] = value;
+
+    /*
+     * pmevtyper_rawwrite is called between a pair of pmu_op_start and
+     * pmu_op_finish calls when loading saved state for a migration. Because
+     * we're potentially updating the type of event here, the value written to
+     * c14_pmevcntr_delta by the preceeding pmu_op_start call may be for a
+     * different counter type. Therefore, we need to set this value to the
+     * current count for the counter type we're writing so that pmu_op_finish
+     * has the correct count for its calculation.
+     */
+    uint16_t event = value & PMXEVTYPER_EVTCOUNT;
+    if (event_supported(event)) {
+        uint16_t event_idx = supported_event_map[event];
+        env->cp15.c14_pmevcntr_delta[counter] =
+            pm_events[event_idx].get_count(env);
+    }
+}
+
+static uint64_t pmevtyper_readfn(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    return pmevtyper_read(env, ri, counter);
+}
+
+static void pmxevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value)
+{
+    pmevtyper_write(env, ri, value, env->cp15.c9_pmselr & 31);
+}
+
 static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    /* We opt to behave as a RAZ/WI when attempts to access PMXEVTYPER
-     * are CONSTRAINED UNPREDICTABLE. See comments in pmxevtyper_write().
+    return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
+static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value, uint8_t counter)
+{
+    if (counter < pmu_num_counters(env)) {
+        pmevcntr_op_start(env, counter);
+        env->cp15.c14_pmevcntr[counter] = value;
+        pmevcntr_op_finish(env, counter);
+    }
+    /*
+     * We opt to behave as a RAZ/WI when attempts to access PM[X]EVCNTR
+     * are CONSTRAINED UNPREDICTABLE.
      */
-    if (env->cp15.c9_pmselr == 0x1f) {
-        return env->cp15.pmccfiltr_el0;
+}
+
+static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint8_t counter)
+{
+    if (counter < pmu_num_counters(env)) {
+        uint64_t ret;
+        pmevcntr_op_start(env, counter);
+        ret = env->cp15.c14_pmevcntr[counter];
+        pmevcntr_op_finish(env, counter);
+        return ret;
     } else {
+      /* We opt to behave as a RAZ/WI when attempts to access PM[X]EVCNTR
+       * are CONSTRAINED UNPREDICTABLE. */
         return 0;
     }
 }
 
+static void pmevcntr_writefn(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    pmevcntr_write(env, ri, value, counter);
+}
+
+static uint64_t pmevcntr_readfn(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    return pmevcntr_read(env, ri, counter);
+}
+
+static void pmevcntr_rawwrite(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    assert(counter < pmu_num_counters(env));
+    env->cp15.c14_pmevcntr[counter] = value;
+    pmevcntr_write(env, ri, value, counter);
+}
+
+static uint64_t pmevcntr_rawread(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    uint8_t counter = ((ri->crm & 3) << 3) | (ri->opc2 & 7);
+    assert(counter < pmu_num_counters(env));
+    return env->cp15.c14_pmevcntr[counter];
+}
+
+static void pmxevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value)
+{
+    pmevcntr_write(env, ri, value, env->cp15.c9_pmselr & 31);
+}
+
+static uint64_t pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
 static void pmuserenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -1571,16 +1788,23 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
       .resetvalue = 0, },
     { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
-      .access = PL0_RW, .type = ARM_CP_NO_RAW, .accessfn = pmreg_access,
+      .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .accessfn = pmreg_access,
       .writefn = pmxevtyper_write, .readfn = pmxevtyper_read },
     { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1,
-      .access = PL0_RW, .type = ARM_CP_NO_RAW, .accessfn = pmreg_access,
+      .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .accessfn = pmreg_access,
       .writefn = pmxevtyper_write, .readfn = pmxevtyper_read },
-    /* Unimplemented, RAZ/WI. */
     { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
-      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
-      .accessfn = pmreg_access_xevcntr },
+      .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .accessfn = pmreg_access_xevcntr,
+      .writefn = pmxevcntr_write, .readfn = pmxevcntr_read },
+    { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2,
+      .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+      .accessfn = pmreg_access_xevcntr,
+      .writefn = pmxevcntr_write, .readfn = pmxevcntr_read },
     { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
       .access = PL0_R | PL1_RW, .accessfn = access_tpm,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmuserenr),
@@ -4339,7 +4563,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
 #endif
     /* The only field of MDCR_EL2 that has a defined architectural reset value
      * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we
-     * don't impelment any PMU event counters, so using zero as a reset
+     * don't implement any PMU event counters, so using zero as a reset
      * value for MDCR_EL2 is okay
      */
     { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
@@ -5191,6 +5415,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
          * field as main ID register, and we implement only the cycle
          * count register.
          */
+        unsigned int i, pmcrn = 4;
 #ifndef CONFIG_USER_ONLY
         ARMCPRegInfo pmcr = {
             .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
@@ -5211,6 +5436,43 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         };
         define_one_arm_cp_reg(cpu, &pmcr);
         define_one_arm_cp_reg(cpu, &pmcr64);
+        for (i = 0; i < pmcrn; i++) {
+            char *pmevcntr_name = g_strdup_printf("PMEVCNTR%d", i);
+            char *pmevcntr_el0_name = g_strdup_printf("PMEVCNTR%d_EL0", i);
+            char *pmevtyper_name = g_strdup_printf("PMEVTYPER%d", i);
+            char *pmevtyper_el0_name = g_strdup_printf("PMEVTYPER%d_EL0", i);
+            ARMCPRegInfo pmev_regs[] = {
+                { .name = pmevcntr_name, .cp = 15, .crn = 15,
+                  .crm = 8 | (3 & (i >> 3)), .opc1 = 0, .opc2 = i & 7,
+                  .access = PL0_RW, .type = ARM_CP_IO | ARM_CP_ALIAS,
+                  .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn,
+                  .accessfn = pmreg_access },
+                { .name = pmevcntr_el0_name, .state = ARM_CP_STATE_AA64,
+                  .opc0 = 3, .opc1 = 3, .crn = 15, .crm = 8 | (3 & (i >> 3)),
+                  .opc2 = i & 7, .access = PL0_RW, .accessfn = pmreg_access,
+                  .type = ARM_CP_IO,
+                  .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn,
+                  .raw_readfn = pmevcntr_rawread,
+                  .raw_writefn = pmevcntr_rawwrite },
+                { .name = pmevtyper_name, .cp = 15, .crn = 15,
+                  .crm = 12 | (3 & (i >> 3)), .opc1 = 0, .opc2 = i & 7,
+                  .access = PL0_RW, .type = ARM_CP_IO | ARM_CP_ALIAS,
+                  .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn,
+                  .accessfn = pmreg_access },
+                { .name = pmevtyper_el0_name, .state = ARM_CP_STATE_AA64,
+                  .opc0 = 3, .opc1 = 3, .crn = 15, .crm = 12 | (3 & (i >> 3)),
+                  .opc2 = i & 7, .access = PL0_RW, .accessfn = pmreg_access,
+                  .type = ARM_CP_IO,
+                  .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn,
+                  .raw_writefn = pmevtyper_rawwrite },
+                REGINFO_SENTINEL
+            };
+            define_arm_cp_regs(cpu, pmev_regs);
+            g_free(pmevcntr_name);
+            g_free(pmevcntr_el0_name);
+            g_free(pmevtyper_name);
+            g_free(pmevtyper_el0_name);
+        }
 #endif
         ARMCPRegInfo clidr = {
             .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
-- 
2.19.1

[Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

The instruction event is only enabled when icount is used, cycles are
always supported. Always defining get_cycle_count (but altering its
behavior depending on CONFIG_USER_ONLY) allows us to remove some
CONFIG_USER_ONLY #defines throughout the rest of the code.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 90 ++++++++++++++++++++++-----------------------
 1 file changed, 44 insertions(+), 46 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f0798f7a8c..d6501de1ba 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -15,6 +15,7 @@
 #include "arm_ldst.h"
 #include <zlib.h> /* For crc32 */
 #include "exec/semihost.h"
+#include "sysemu/cpus.h"
 #include "sysemu/kvm.h"
 #include "fpu/softfloat.h"
 #include "qemu/range.h"
@@ -988,9 +989,50 @@ typedef struct pm_event {
     uint64_t (*get_count)(CPUARMState *);
 } pm_event;
 
+static bool event_always_supported(CPUARMState *env)
+{
+    return true;
+}
+
+/*
+ * Return the underlying cycle count for the PMU cycle counters. If we're in
+ * usermode, simply return 0.
+ */
+static uint64_t cycles_get_count(CPUARMState *env)
+{
+#ifndef CONFIG_USER_ONLY
+    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+#else
+    return 0;
+#endif
+}
+
+#ifndef CONFIG_USER_ONLY
+static bool instructions_supported(CPUARMState *env)
+{
+    return use_icount == 1 /* Precise instruction counting */;
+}
+
+static uint64_t instructions_get_count(CPUARMState *env)
+{
+    return (uint64_t)cpu_get_icount_raw();
+}
+#endif
+
 static const pm_event pm_events[] = {
+#ifndef CONFIG_USER_ONLY
+    { .number = 0x008, /* INST_RETIRED, Instruction architecturally executed */
+      .supported = instructions_supported,
+      .get_count = instructions_get_count,
+    },
+    { .number = 0x011, /* CPU_CYCLES, Cycle */
+      .supported = event_always_supported,
+      .get_count = cycles_get_count,
+    }
+#endif
 };
-#define MAX_EVENT_ID 0x0
+#define MAX_EVENT_ID 0x11
 #define UNSUPPORTED_EVENT UINT16_MAX
 static uint16_t supported_event_map[MAX_EVENT_ID + 1];
 
@@ -1083,8 +1125,6 @@ static CPAccessResult pmreg_access_swinc(CPUARMState *env,
     return pmreg_access(env, ri, isread);
 }
 
-#ifndef CONFIG_USER_ONLY
-
 static CPAccessResult pmreg_access_selr(CPUARMState *env,
                                         const ARMCPRegInfo *ri,
                                         bool isread)
@@ -1195,9 +1235,7 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
  */
 void pmccntr_op_start(CPUARMState *env)
 {
-    uint64_t cycles = 0;
-    cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                          ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
+    uint64_t cycles = cycles_get_count(env);
 
     if (pmu_counter_enabled(env, 31)) {
         uint64_t eff_cycles = cycles;
@@ -1343,42 +1381,6 @@ static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri,
     pmccntr_write(env, ri, deposit64(cur_val, 0, 32, value));
 }
 
-#else /* CONFIG_USER_ONLY */
-
-void pmccntr_op_start(CPUARMState *env)
-{
-}
-
-void pmccntr_op_finish(CPUARMState *env)
-{
-}
-
-void pmevcntr_op_start(CPUARMState *env, uint8_t i)
-{
-}
-
-void pmevcntr_op_finish(CPUARMState *env, uint8_t i)
-{
-}
-
-void pmu_op_start(CPUARMState *env)
-{
-}
-
-void pmu_op_finish(CPUARMState *env)
-{
-}
-
-void pmu_pre_el_change(ARMCPU *cpu, void *ignored)
-{
-}
-
-void pmu_post_el_change(ARMCPU *cpu, void *ignored)
-{
-}
-
-#endif
-
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -1752,7 +1754,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
     /* Unimplemented so WI. */
     { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
       .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
-#ifndef CONFIG_USER_ONLY
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
       .access = PL0_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
@@ -1774,7 +1775,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
       .readfn = pmccntr_read, .writefn = pmccntr_write,
       .raw_readfn = raw_read, .raw_writefn = raw_write, },
-#endif
     { .name = "PMCCFILTR", .cp = 15, .opc1 = 0, .crn = 14, .crm = 15, .opc2 = 7,
       .writefn = pmccfiltr_write_a32, .readfn = pmccfiltr_read_a32,
       .access = PL0_RW, .accessfn = pmreg_access,
@@ -5416,7 +5416,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
          * count register.
          */
         unsigned int i, pmcrn = 4;
-#ifndef CONFIG_USER_ONLY
         ARMCPRegInfo pmcr = {
             .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
             .access = PL0_RW,
@@ -5473,7 +5472,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             g_free(pmevtyper_name);
             g_free(pmevtyper_el0_name);
         }
-#endif
         ARMCPRegInfo clidr = {
             .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
             .opc0 = 3, .crn = 0, .crm = 0, .opc1 = 1, .opc2 = 1,
-- 
2.19.1

[Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

This both advertises that we support four counters and enables them
because the pmu_num_counters() reads this value from PMCR.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d6501de1ba..89ceb34cb9 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1706,7 +1706,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_W, .type = ARM_CP_NOP },
     /* Performance monitors are implementation defined in v7,
      * but with an ARM recommended set of registers, which we
-     * follow (although we don't actually implement any counters)
+     * follow.
      *
      * Performance registers fall into three categories:
      *  (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR)
@@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
     }
     if (arm_feature(env, ARM_FEATURE_V7)) {
         /* v7 performance monitor control register: same implementor
-         * field as main ID register, and we implement only the cycle
-         * count register.
+         * field as main ID register, and we implement four counters in
+         * addition to the cycle count register.
          */
         unsigned int i, pmcrn = 4;
         ARMCPRegInfo pmcr = {
@@ -5430,7 +5430,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             .access = PL0_RW, .accessfn = pmreg_access,
             .type = ARM_CP_IO,
             .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
-            .resetvalue = cpu->midr & 0xff000000,
+            .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT),
             .writefn = pmcr_write, .raw_writefn = raw_write,
         };
         define_one_arm_cp_reg(cpu, &pmcr);
-- 
2.19.1

[Qemu-devel] [PATCH v6 13/14] target/arm: Implement PMSWINC

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 89ceb34cb9..6c2a899009 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -994,6 +994,15 @@ static bool event_always_supported(CPUARMState *env)
     return true;
 }
 
+static uint64_t swinc_get_count(CPUARMState *env)
+{
+    /*
+     * SW_INCR events are written directly to the pmevcntr's by writes to
+     * PMSWINC, so there is no underlying count maintained by the PMU itself
+     */
+    return 0;
+}
+
 /*
  * Return the underlying cycle count for the PMU cycle counters. If we're in
  * usermode, simply return 0.
@@ -1021,6 +1030,10 @@ static uint64_t instructions_get_count(CPUARMState *env)
 #endif
 
 static const pm_event pm_events[] = {
+    { .number = 0x000, /* SW_INCR */
+      .supported = event_always_supported,
+      .get_count = swinc_get_count,
+    },
 #ifndef CONFIG_USER_ONLY
     { .number = 0x008, /* INST_RETIRED, Instruction architecturally executed */
       .supported = instructions_supported,
@@ -1345,6 +1358,24 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     pmu_op_finish(env);
 }
 
+static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                          uint64_t value)
+{
+    unsigned int i;
+    for (i = 0; i < pmu_num_counters(env); i++) {
+        /* Increment a counter's count iff: */
+        if ((value & (1 << i)) && /* counter's bit is set */
+                /* counter is enabled and not filtered */
+                pmu_counter_enabled(env, i) &&
+                /* counter is SW_INCR */
+                (env->cp15.c14_pmevtyper[i] & PMXEVTYPER_EVTCOUNT) == 0x0) {
+            pmevcntr_op_start(env, i);
+            env->cp15.c14_pmevcntr[i]++;
+            pmevcntr_op_finish(env, i);
+        }
+    }
+}
+
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     uint64_t ret;
@@ -1751,9 +1782,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
       .writefn = pmovsr_write,
       .raw_writefn = raw_write },
-    /* Unimplemented so WI. */
     { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
-      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NOP },
+      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+      .writefn = pmswinc_write },
+    { .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 4,
+      .access = PL0_W, .accessfn = pmreg_access_swinc, .type = ARM_CP_NO_RAW,
+      .writefn = pmswinc_write },
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
       .access = PL0_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmselr),
-- 
2.19.1

[Qemu-devel] [PATCH v6 14/14] target/arm: Send interrupts on PMU counter overflow

[Aaron Lindsay]

Setup a QEMUTimer to get a callback when we expect counters to next
overflow and trigger an interrupt at that time.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c    |  11 ++++
 target/arm/cpu.h    |   7 +++
 target/arm/helper.c | 126 +++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 138 insertions(+), 6 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7f39f25f51..c89c7c776c 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -764,6 +764,12 @@ static void arm_cpu_finalizefn(Object *obj)
         QLIST_REMOVE(hook, node);
         g_free(hook);
     }
+#ifndef CONFIG_USER_ONLY
+    if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
+        timer_deinit(cpu->pmu_timer);
+        timer_free(cpu->pmu_timer);
+    }
+#endif
 }
 
 static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
@@ -958,6 +964,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
             arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
             arm_register_el_change_hook(cpu, &pmu_post_el_change, 0);
         }
+
+#ifndef CONFIG_USER_ONLY
+        cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
+                cpu);
+#endif
     } else {
         cpu->pmceid0 = 0x00000000;
         cpu->pmceid1 = 0x00000000;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index f4317f87c9..a27481658c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -721,6 +721,8 @@ struct ARMCPU {
 
     /* Timers used by the generic (architected) timer */
     QEMUTimer *gt_timer[NUM_GTIMERS];
+    /* Timer used by the PMU */
+    QEMUTimer *pmu_timer;
     /* GPIO outputs for generic timer */
     qemu_irq gt_timer_outputs[NUM_GTIMERS];
     /* GPIO output for GICv3 maintenance interrupt signal */
@@ -972,6 +974,11 @@ void pmccntr_op_finish(CPUARMState *env);
 void pmu_op_start(CPUARMState *env);
 void pmu_op_finish(CPUARMState *env);
 
+/**
+ * Called when a PMU counter is due to overflow
+ */
+void arm_pmu_timer_cb(void *opaque);
+
 /**
  * Functions to register as EL change hooks for PMU mode filtering
  */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 6c2a899009..9699e43f0c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -944,6 +944,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 /* Definitions for the PMU registers */
 #define PMCRN_MASK  0xf800
 #define PMCRN_SHIFT 11
+#define PMCRLC  0x40
 #define PMCRDP  0x10
 #define PMCRD   0x8
 #define PMCRC   0x4
@@ -963,6 +964,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
                                PMXEVTYPER_M | PMXEVTYPER_MT | \
                                PMXEVTYPER_EVTCOUNT)
 
+#define PMEVCNTR_OVERFLOW_MASK ((uint64_t)1 << 31)
+
 #define PMCCFILTR             0xf8000000
 #define PMCCFILTR_M           PMXEVTYPER_M
 #define PMCCFILTR_EL0         (PMCCFILTR | PMCCFILTR_M)
@@ -987,6 +990,11 @@ typedef struct pm_event {
      * counters hold a difference from the return value from this function
      */
     uint64_t (*get_count)(CPUARMState *);
+    /* Return how many nanoseconds it will take (at a minimum) for count events
+     * to occur. A negative value indicates the counter will never overflow, or
+     * that the counter has otherwise arranged for the overflow bit to be set
+     * and the PMU interrupt to be raised on overflow. */
+    int64_t (*ns_per_count)(uint64_t);
 } pm_event;
 
 static bool event_always_supported(CPUARMState *env)
@@ -1003,6 +1011,11 @@ static uint64_t swinc_get_count(CPUARMState *env)
     return 0;
 }
 
+static int64_t swinc_ns_per(uint64_t ignored)
+{
+    return -1;
+}
+
 /*
  * Return the underlying cycle count for the PMU cycle counters. If we're in
  * usermode, simply return 0.
@@ -1018,6 +1031,11 @@ static uint64_t cycles_get_count(CPUARMState *env)
 }
 
 #ifndef CONFIG_USER_ONLY
+static int64_t cycles_ns_per(uint64_t cycles)
+{
+    return (ARM_CPU_FREQ / NANOSECONDS_PER_SECOND) * cycles;
+}
+
 static bool instructions_supported(CPUARMState *env)
 {
     return use_icount == 1 /* Precise instruction counting */;
@@ -1027,21 +1045,29 @@ static uint64_t instructions_get_count(CPUARMState *env)
 {
     return (uint64_t)cpu_get_icount_raw();
 }
+
+static int64_t instructions_ns_per(uint64_t icount)
+{
+    return cpu_icount_to_ns((int64_t)icount);
+}
 #endif
 
 static const pm_event pm_events[] = {
     { .number = 0x000, /* SW_INCR */
       .supported = event_always_supported,
       .get_count = swinc_get_count,
+      .ns_per_count = swinc_ns_per,
     },
 #ifndef CONFIG_USER_ONLY
     { .number = 0x008, /* INST_RETIRED, Instruction architecturally executed */
       .supported = instructions_supported,
       .get_count = instructions_get_count,
+      .ns_per_count = instructions_ns_per,
     },
     { .number = 0x011, /* CPU_CYCLES, Cycle */
       .supported = event_always_supported,
       .get_count = cycles_get_count,
+      .ns_per_count = cycles_ns_per,
     }
 #endif
 };
@@ -1240,6 +1266,13 @@ static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)
     return enabled && !prohibited && !filtered;
 }
 
+static void pmu_update_irq(CPUARMState *env)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    qemu_set_irq(cpu->pmu_interrupt, (env->cp15.c9_pmcr & PMCRE) &&
+            (env->cp15.c9_pminten & env->cp15.c9_pmovsr));
+}
+
 /*
  * Ensure c15_ccnt is the guest-visible count so that operations such as
  * enabling/disabling the counter or filtering, modifying the count itself,
@@ -1257,7 +1290,18 @@ void pmccntr_op_start(CPUARMState *env)
             eff_cycles /= 64;
         }
 
-        env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt_delta;
+        uint64_t new_pmccntr = eff_cycles - env->cp15.c15_ccnt_delta;
+
+        unsigned int overflow_bit = (env->cp15.c9_pmcr & PMCRLC) ? 63 : 31;
+        uint64_t overflow_mask = (uint64_t)1 << overflow_bit;
+        if (!(new_pmccntr & overflow_mask) &&
+                (env->cp15.c15_ccnt & overflow_mask)) {
+            env->cp15.c9_pmovsr |= (1 << 31);
+            new_pmccntr &= ~overflow_mask;
+            pmu_update_irq(env);
+        }
+
+        env->cp15.c15_ccnt = new_pmccntr;
     }
     env->cp15.c15_ccnt_delta = cycles;
 }
@@ -1270,13 +1314,28 @@ void pmccntr_op_start(CPUARMState *env)
 void pmccntr_op_finish(CPUARMState *env)
 {
     if (pmu_counter_enabled(env, 31)) {
-        uint64_t prev_cycles = env->cp15.c15_ccnt_delta;
+#ifndef CONFIG_USER_ONLY
+        uint64_t delta;
+        if (env->cp15.c9_pmcr & PMCRLC) {
+            delta = UINT64_MAX - env->cp15.c15_ccnt + 1;
+        } else {
+            delta = UINT32_MAX - (uint32_t)env->cp15.c15_ccnt + 1;
+        }
+        int64_t overflow_in = cycles_ns_per(delta);
+
+        if (overflow_in > 0) {
+            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                overflow_in;
+            ARMCPU *cpu = arm_env_get_cpu(env);
+            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+        }
+#endif
 
+        uint64_t prev_cycles = env->cp15.c15_ccnt_delta;
         if (env->cp15.c9_pmcr & PMCRD) {
             /* Increment once every 64 processor clock cycles */
             prev_cycles /= 64;
         }
-
         env->cp15.c15_ccnt_delta = prev_cycles - env->cp15.c15_ccnt;
     }
 }
@@ -1292,8 +1351,15 @@ static void pmevcntr_op_start(CPUARMState *env, uint8_t counter)
     }
 
     if (pmu_counter_enabled(env, counter)) {
-        env->cp15.c14_pmevcntr[counter] =
-            count - env->cp15.c14_pmevcntr_delta[counter];
+        uint64_t new_pmevcntr = count - env->cp15.c14_pmevcntr_delta[counter];
+
+        if (!(new_pmevcntr & PMEVCNTR_OVERFLOW_MASK) &&
+                (env->cp15.c14_pmevcntr[counter] & PMEVCNTR_OVERFLOW_MASK)) {
+            env->cp15.c9_pmovsr |= (1 << counter);
+            new_pmevcntr &= ~PMEVCNTR_OVERFLOW_MASK;
+            pmu_update_irq(env);
+        }
+        env->cp15.c14_pmevcntr[counter] = new_pmevcntr;
     }
     env->cp15.c14_pmevcntr_delta[counter] = count;
 }
@@ -1301,6 +1367,21 @@ static void pmevcntr_op_start(CPUARMState *env, uint8_t counter)
 static void pmevcntr_op_finish(CPUARMState *env, uint8_t counter)
 {
     if (pmu_counter_enabled(env, counter)) {
+#ifndef CONFIG_USER_ONLY
+        uint16_t event = env->cp15.c14_pmevtyper[counter] & PMXEVTYPER_EVTCOUNT;
+        uint16_t event_idx = supported_event_map[event];
+        uint64_t delta = UINT32_MAX -
+            (uint32_t)env->cp15.c14_pmevcntr[counter] + 1;
+        int64_t overflow_in = pm_events[event_idx].ns_per_count(delta);
+
+        if (overflow_in > 0) {
+            int64_t overflow_at = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+                overflow_in;
+            ARMCPU *cpu = arm_env_get_cpu(env);
+            timer_mod_anticipate_ns(cpu->pmu_timer, overflow_at);
+        }
+#endif
+
         env->cp15.c14_pmevcntr_delta[counter] -=
             env->cp15.c14_pmevcntr[counter];
     }
@@ -1334,6 +1415,19 @@ void pmu_post_el_change(ARMCPU *cpu, void *ignored)
     pmu_op_finish(&cpu->env);
 }
 
+void arm_pmu_timer_cb(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    /* Update all the counter values based on the current underlying counts,
+     * triggering interrupts to be raised, if necessary. pmu_op_finish() also
+     * has the effect of setting the cpu->pmu_timer to the next earliest time a
+     * counter may expire.
+     */
+    pmu_op_start(&cpu->env);
+    pmu_op_finish(&cpu->env);
+}
+
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
@@ -1370,7 +1464,21 @@ static void pmswinc_write(CPUARMState *env, const ARMCPRegInfo *ri,
                 /* counter is SW_INCR */
                 (env->cp15.c14_pmevtyper[i] & PMXEVTYPER_EVTCOUNT) == 0x0) {
             pmevcntr_op_start(env, i);
-            env->cp15.c14_pmevcntr[i]++;
+
+            /* Detect if this write causes an overflow since we can't predict
+             * PMSWINC overflows like we can for other events
+             */
+            uint64_t new_pmswinc = env->cp15.c14_pmevcntr[i] + 1;
+
+            if (!(new_pmswinc & PMEVCNTR_OVERFLOW_MASK) &&
+                    (env->cp15.c14_pmevcntr[i] & PMEVCNTR_OVERFLOW_MASK)) {
+                env->cp15.c9_pmovsr |= (1 << i);
+                new_pmswinc &= ~PMEVCNTR_OVERFLOW_MASK;
+                pmu_update_irq(env);
+            }
+
+            env->cp15.c14_pmevcntr[i] = new_pmswinc;
+
             pmevcntr_op_finish(env, i);
         }
     }
@@ -1441,6 +1549,7 @@ static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmcnten |= value;
+    pmu_update_irq(env);
 }
 
 static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1448,6 +1557,7 @@ static void pmcntenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmcnten &= ~value;
+    pmu_update_irq(env);
 }
 
 static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1455,6 +1565,7 @@ static void pmovsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmovsr &= ~value;
+    pmu_update_irq(env);
 }
 
 static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1462,6 +1573,7 @@ static void pmovsset_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pmovsr |= value;
+    pmu_update_irq(env);
 }
 
 static void pmevtyper_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1648,6 +1760,7 @@ static void pmintenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
     /* We have no event counters so only the C bit can be changed */
     value &= pmu_counter_mask(env);
     env->cp15.c9_pminten |= value;
+    pmu_update_irq(env);
 }
 
 static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -1655,6 +1768,7 @@ static void pmintenclr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     value &= pmu_counter_mask(env);
     env->cp15.c9_pminten &= ~value;
+    pmu_update_irq(env);
 }
 
 static void vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
-- 
2.19.1

Re: [Qemu-devel] [PATCH v6 01/14] target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly doing IO

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> I previously fixed this for PMINTENSET_EL1, but missed these.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> ---
>  target/arm/helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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


r~

Re: [Qemu-devel] [PATCH v6 02/14] target/arm: Mask PMOVSR writes based on supported counters

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> This is an amendment to my earlier patch:
>     commit 7ece99b17e832065236c07a158dfac62619ef99b
>     Author: Aaron Lindsay <alindsay@codeaurora.org>
>     Date:   Thu Apr 26 11:04:39 2018 +0100
> 
> 	target/arm: Mask PMU register writes based on PMCR_EL0.N
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)

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


r~

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> In some cases it may be helpful to modify state before saving it for
> migration, and then modify the state back after it has been saved. The
> existing pre_save function provides half of this functionality. This
> patch adds a post_save function to provide the second half.
> 
> Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> ---
>  docs/devel/migration.rst    |  9 +++++++--
>  include/migration/vmstate.h |  1 +
>  migration/vmstate.c         | 10 +++++++++-
>  3 files changed, 17 insertions(+), 3 deletions(-)

Hmm, maybe.  I believe the common practice is for pre_save to copy state into a
separate member on the side, so that conversion back isn't necessary.

Ccing in the migration maintainers for a second opinion.


r~

> 
> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> index 687570754d..2a2533c9b3 100644
> --- a/docs/devel/migration.rst
> +++ b/docs/devel/migration.rst
> @@ -419,8 +419,13 @@ The functions to do that are inside a vmstate definition, and are called:
>  
>    This function is called before we save the state of one device.
>  
> -Example: You can look at hpet.c, that uses the three function to
> -massage the state that is transferred.
> +- ``void (*post_save)(void *opaque);``
> +
> +  This function is called after we save the state of one device
> +  (even upon failure, unless the call to pre_save returned and error).
> +
> +Example: You can look at hpet.c, that uses the first three functions
> +to massage the state that is transferred.
>  
>  The ``VMSTATE_WITH_TMP`` macro may be useful when the migration
>  data doesn't match the stored device data well; it allows an
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 2b501d0466..f6053b94e4 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -185,6 +185,7 @@ struct VMStateDescription {
>      int (*pre_load)(void *opaque);
>      int (*post_load)(void *opaque, int version_id);
>      int (*pre_save)(void *opaque);
> +    void (*post_save)(void *opaque);
>      bool (*needed)(void *opaque);
>      VMStateField *fields;
>      const VMStateDescription **subsections;
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 0bc240a317..9afc9298f3 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -387,6 +387,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  if (ret) {
>                      error_report("Save of field %s/%s failed",
>                                   vmsd->name, field->name);
> +                    if (vmsd->post_save) {
> +                        vmsd->post_save(opaque);
> +                    }
>                      return ret;
>                  }
>  
> @@ -412,7 +415,12 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>          json_end_array(vmdesc);
>      }
>  
> -    return vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> +    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> +
> +    if (vmsd->post_save) {
> +        vmsd->post_save(opaque);
> +    }
> +    return ret;
>  }
>  
>  static const VMStateDescription *
> 

Re: [Qemu-devel] [PATCH v6 04/14] target/arm: Swap PMU values before/after migrations

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> pmccntr_read and pmccntr_write contained duplicate code that was already
> being handled by pmccntr_sync. Consolidate the duplicated code into two
> functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to
> c15_ccnt in CPUARMState so that we can simultaneously save both the
> architectural register value and the last underlying cycle count - this
> ensures time isn't lost and will also allow us to access the 'old'
> architectural register value in order to detect overflows in later
> patches.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.h     | 26 ++++++++----
>  target/arm/helper.c  | 96 +++++++++++++++++++++++---------------------
>  target/arm/machine.c |  8 ++--
>  3 files changed, 73 insertions(+), 57 deletions(-)

Ok, looking at this follow-up makes more sense than the previous patch.  Would
it make sense to squash these two together?

It also makes sense why you'd need the post_save hook.


r~

Re: [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses

[Richard Henderson]

On 10/15/18 12:50 PM, Richard Henderson wrote:
> Ok, looking at this follow-up makes more sense than the previous patch.  Would
> it make sense to squash these two together?

Or, rather, split into two different patches: the first splits pmccntr_sync and
updates all of the existing uses, and the second which adds the migration hooks.


r~

Re: [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay]

On Oct 15 12:50, Richard Henderson wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > pmccntr_read and pmccntr_write contained duplicate code that was already
> > being handled by pmccntr_sync. Consolidate the duplicated code into two
> > functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to
> > c15_ccnt in CPUARMState so that we can simultaneously save both the
> > architectural register value and the last underlying cycle count - this
> > ensures time isn't lost and will also allow us to access the 'old'
> > architectural register value in order to detect overflows in later
> > patches.
> > 
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.h     | 26 ++++++++----
> >  target/arm/helper.c  | 96 +++++++++++++++++++++++---------------------
> >  target/arm/machine.c |  8 ++--
> >  3 files changed, 73 insertions(+), 57 deletions(-)
> 
> Ok, looking at this follow-up makes more sense than the previous patch.  Would
> it make sense to squash these two together?

I was attempting to keep the migration plumbing separate from the PMU
implementation details, but I'm not particularly partial to this
staging.

> It also makes sense why you'd need the post_save hook.

Okay. I attempted to describe this in the commit message in a way that
communicated the need for the hook without being overly verbose - but
suggestions in that area are very welcome if you think a different
commit message would help.

-Aaron

Re: [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay]

On Oct 15 13:19, Richard Henderson wrote:
> On 10/15/18 12:50 PM, Richard Henderson wrote:
> > Ok, looking at this follow-up makes more sense than the previous patch.  Would
> > it make sense to squash these two together?
> 
> Or, rather, split into two different patches: the first splits pmccntr_sync and
> updates all of the existing uses, and the second which adds the migration hooks.

To make sure I understand, you're suggesting essentially reversing the
order of patches 4 and 5 (and fixing everything up to be consistent)?

-Aaron

Re: [Qemu-devel] [PATCH v6 04/14] target/arm: Swap PMU values before/after migrations

[Aaron Lindsay]

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

Re: [Qemu-devel] [PATCH v6 05/14] target/arm: Reorganize PMCCNTR accesses

[Richard Henderson]

On 10/15/18 1:30 PM, Aaron Lindsay wrote:
> On Oct 15 13:19, Richard Henderson wrote:
>> On 10/15/18 12:50 PM, Richard Henderson wrote:
>>> Ok, looking at this follow-up makes more sense than the previous patch.  Would
>>> it make sense to squash these two together?
>>
>> Or, rather, split into two different patches: the first splits pmccntr_sync and
>> updates all of the existing uses, and the second which adds the migration hooks.
> 
> To make sure I understand, you're suggesting essentially reversing the
> order of patches 4 and 5 (and fixing everything up to be consistent)?

Yes.


r~

Re: [Qemu-devel] [PATCH v6 06/14] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
>  
> -    pmccntr_op_start(&cpu->env);
> +    pmu_op_start(&cpu->env);

Does it make sense to move this patch earlier so that these hooks are modified
once?  No big deal if not.


> +static inline bool pmu_counter_enabled(CPUARMState *env, uint8_t counter)

Drop the inline.  This function is pretty big; we should let the compiler choose.

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


r~

Re: [Qemu-devel] [PATCH v6 07/14] target/arm: Allow AArch32 access for PMCCFILTR

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)

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


r~

Re: [Qemu-devel] [PATCH v6 08/14] target/arm: Implement PMOVSSET

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> Add an array for PMOVSSET so we only define it for v7ve+ platforms
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

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


r~

Re: [Qemu-devel] [PATCH v6 09/14] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> +        cpu->pmceid0 = pmceid & 0xffffffff;
> +        cpu->pmceid1 = (pmceid >> 32) & 0xffffffff;

extract64(pmceid, 0, 32) and extract64(pmceid, 32, 32).

> +static const pm_event pm_events[] = {
> +};
> +#define MAX_EVENT_ID 0x0

Is this going to be ARRAY_SIZE(pm_events) - 1 in the end?

> +static uint16_t supported_event_map[MAX_EVENT_ID + 1];

Better as ARRAY_SIZE(pm_events)?

> +    for (i = 0; i <= MAX_EVENT_ID; i++) {
> +        supported_event_map[i] = UNSUPPORTED_EVENT;
> +    }

I think maybe

  for (i = 0; i < ARRAY_SIZE(supported_map_event); ++i) {

would be better.


r~

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Dr. David Alan Gilbert]

* Richard Henderson (richard.henderson@linaro.org) wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > In some cases it may be helpful to modify state before saving it for
> > migration, and then modify the state back after it has been saved. The
> > existing pre_save function provides half of this functionality. This
> > patch adds a post_save function to provide the second half.
> > 
> > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> > ---
> >  docs/devel/migration.rst    |  9 +++++++--
> >  include/migration/vmstate.h |  1 +
> >  migration/vmstate.c         | 10 +++++++++-
> >  3 files changed, 17 insertions(+), 3 deletions(-)
> 
> Hmm, maybe.  I believe the common practice is for pre_save to copy state into a
> separate member on the side, so that conversion back isn't necessary.
> 
> Ccing in the migration maintainers for a second opinion.

It is common to copy stuff into a separate member; however we do
occasionally think that post_save would be a useful addition; so I think
we should take it (if nothing else it actually makes stuff symmetric!).

Please make it return 'int' in the same way that pre_save/pre_load
does, so that it can fail and stop the migration.

Dave

> 
> 
> r~
> 
> > 
> > diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> > index 687570754d..2a2533c9b3 100644
> > --- a/docs/devel/migration.rst
> > +++ b/docs/devel/migration.rst
> > @@ -419,8 +419,13 @@ The functions to do that are inside a vmstate definition, and are called:
> >  
> >    This function is called before we save the state of one device.
> >  
> > -Example: You can look at hpet.c, that uses the three function to
> > -massage the state that is transferred.
> > +- ``void (*post_save)(void *opaque);``
> > +
> > +  This function is called after we save the state of one device
> > +  (even upon failure, unless the call to pre_save returned and error).
> > +
> > +Example: You can look at hpet.c, that uses the first three functions
> > +to massage the state that is transferred.
> >  
> >  The ``VMSTATE_WITH_TMP`` macro may be useful when the migration
> >  data doesn't match the stored device data well; it allows an
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 2b501d0466..f6053b94e4 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -185,6 +185,7 @@ struct VMStateDescription {
> >      int (*pre_load)(void *opaque);
> >      int (*post_load)(void *opaque, int version_id);
> >      int (*pre_save)(void *opaque);
> > +    void (*post_save)(void *opaque);
> >      bool (*needed)(void *opaque);
> >      VMStateField *fields;
> >      const VMStateDescription **subsections;
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 0bc240a317..9afc9298f3 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -387,6 +387,9 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >                  if (ret) {
> >                      error_report("Save of field %s/%s failed",
> >                                   vmsd->name, field->name);
> > +                    if (vmsd->post_save) {
> > +                        vmsd->post_save(opaque);
> > +                    }
> >                      return ret;
> >                  }
> >  
> > @@ -412,7 +415,12 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> >          json_end_array(vmdesc);
> >      }
> >  
> > -    return vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> > +    ret = vmstate_subsection_save(f, vmsd, opaque, vmdesc);
> > +
> > +    if (vmsd->post_save) {
> > +        vmsd->post_save(opaque);
> > +    }
> > +    return ret;
> >  }
> >  
> >  static const VMStateDescription *
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 09/14] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay]

On Oct 15 14:35, Richard Henderson wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > +static const pm_event pm_events[] = {
> > +};
> > +#define MAX_EVENT_ID 0x0
> 
> Is this going to be ARRAY_SIZE(pm_events) - 1 in the end?

No, this ends up being a 'sparse' array. It maps the ARM architected PMU
event numbers to their implementations. So, MAX_EVENT_ID ends up being
the maximum architected event number we implement when my later patches
add those.

-Aaron

Re: [Qemu-devel] [PATCH v6 00/14] More fully implement ARM PMUv3

[Peter Maydell]

On 10 October 2018 at 21:37, Aaron Lindsay <aclindsa@gmail.com> wrote:
> The ARM PMU implementation currently contains a basic cycle counter, but
> it is often useful to gather counts of other events, filter them based
> on execution mode, and/or be notified on counter overflow. These patches
> flesh out the implementations of various PMU registers including
> PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to represent
> arbitrary counter types, implement mode filtering, send interrupts on
> counter overflow, and add instruction, cycle, and software increment
> events.
>
> Since v5 [1] I have:
> * Taken a first pass at addressing migration
> * Restructured the list of supported events, and ensured they're all
>   initialized
> * Fixed aliasing for PMOVSSET
> * Added ARM_CP_IO for PMINTENCLR and PMINTENCLR_EL1
> * Addressed a few non-code issues (comment style, patch staging,
>   spelling, etc.)
>
> [1] - https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg06830.html
>
> Aaron Lindsay (14):
>   target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly
>     doing IO
>   target/arm: Mask PMOVSR writes based on supported counters

Hi; Richard has reviewed most of this series and suggested some
changes (thanks!); I'll just take these first two patches into
target-arm.next, since they're simple fixes that have been reviewed.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 00/14] More fully implement ARM PMUv3

[Aaron Lindsay]

On Oct 16 13:01, Peter Maydell wrote:
> On 10 October 2018 at 21:37, Aaron Lindsay <aclindsa@gmail.com> wrote:
> > The ARM PMU implementation currently contains a basic cycle counter, but
> > it is often useful to gather counts of other events, filter them based
> > on execution mode, and/or be notified on counter overflow. These patches
> > flesh out the implementations of various PMU registers including
> > PM[X]EVCNTR and PM[X]EVTYPER, add a struct definition to represent
> > arbitrary counter types, implement mode filtering, send interrupts on
> > counter overflow, and add instruction, cycle, and software increment
> > events.
> >
> > Since v5 [1] I have:
> > * Taken a first pass at addressing migration
> > * Restructured the list of supported events, and ensured they're all
> >   initialized
> > * Fixed aliasing for PMOVSSET
> > * Added ARM_CP_IO for PMINTENCLR and PMINTENCLR_EL1
> > * Addressed a few non-code issues (comment style, patch staging,
> >   spelling, etc.)
> >
> > [1] - https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg06830.html
> >
> > Aaron Lindsay (14):
> >   target/arm: Mark PMINTENCLR and PMINTENCLR_EL1 accesses as possibly
> >     doing IO
> >   target/arm: Mask PMOVSR writes based on supported counters
> 
> Hi; Richard has reviewed most of this series and suggested some
> changes (thanks!); I'll just take these first two patches into
> target-arm.next, since they're simple fixes that have been reviewed.

Thanks, Peter and Richard!

Is anyone willing to take a glance at the final patch in this series,
"target/arm: Send interrupts on PMU counter overflow", before my next
iteration? I'm particularly interested in a review of the approach I
took for detecting overflow.

-Aaron

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Aaron Lindsay]

On Oct 16 09:21, Dr. David Alan Gilbert wrote:
> * Richard Henderson (richard.henderson@linaro.org) wrote:
> > On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > > In some cases it may be helpful to modify state before saving it for
> > > migration, and then modify the state back after it has been saved. The
> > > existing pre_save function provides half of this functionality. This
> > > patch adds a post_save function to provide the second half.
> > > 
> > > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> > > ---
> > >  docs/devel/migration.rst    |  9 +++++++--
> > >  include/migration/vmstate.h |  1 +
> > >  migration/vmstate.c         | 10 +++++++++-
> > >  3 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > Hmm, maybe.  I believe the common practice is for pre_save to copy state into a
> > separate member on the side, so that conversion back isn't necessary.
> > 
> > Ccing in the migration maintainers for a second opinion.
> 
> It is common to copy stuff into a separate member; however we do
> occasionally think that post_save would be a useful addition; so I think
> we should take it (if nothing else it actually makes stuff symmetric!).
> 
> Please make it return 'int' in the same way that pre_save/pre_load
> does, so that it can fail and stop the migration.

This patch calls post_save *even if the save operation fails*. My
reasoning was that I didn't want a failed migration to leave a
still-running original QEMU instance in an invalid state. Was this
misguided?

If it was not, which error do you prefer to be returned from
vmstate_save_state_v() in the case that both the save operation itself
and the post_save call returned errors?

-Aaron

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Dr. David Alan Gilbert]

* Aaron Lindsay (aclindsa@gmail.com) wrote:
> On Oct 16 09:21, Dr. David Alan Gilbert wrote:
> > * Richard Henderson (richard.henderson@linaro.org) wrote:
> > > On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > > > In some cases it may be helpful to modify state before saving it for
> > > > migration, and then modify the state back after it has been saved. The
> > > > existing pre_save function provides half of this functionality. This
> > > > patch adds a post_save function to provide the second half.
> > > > 
> > > > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> > > > ---
> > > >  docs/devel/migration.rst    |  9 +++++++--
> > > >  include/migration/vmstate.h |  1 +
> > > >  migration/vmstate.c         | 10 +++++++++-
> > > >  3 files changed, 17 insertions(+), 3 deletions(-)
> > > 
> > > Hmm, maybe.  I believe the common practice is for pre_save to copy state into a
> > > separate member on the side, so that conversion back isn't necessary.
> > > 
> > > Ccing in the migration maintainers for a second opinion.
> > 
> > It is common to copy stuff into a separate member; however we do
> > occasionally think that post_save would be a useful addition; so I think
> > we should take it (if nothing else it actually makes stuff symmetric!).
> > 
> > Please make it return 'int' in the same way that pre_save/pre_load
> > does, so that it can fail and stop the migration.
> 
> This patch calls post_save *even if the save operation fails*. My
> reasoning was that I didn't want a failed migration to leave a
> still-running original QEMU instance in an invalid state. Was this
> misguided?

That's fine - my only issue is that I want post_save to be able to fail
itself even if pre_save failed.

> If it was not, which error do you prefer to be returned from
> vmstate_save_state_v() in the case that both the save operation itself
> and the post_save call returned errors?

The return value from the save operation.
I did wonder about suggesting that you pass the return value from the
save operation as a parameter to post_save.

Dave

> -Aaron
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Aaron Lindsay]

On Oct 16 15:06, Dr. David Alan Gilbert wrote:
> I did wonder about suggesting that you pass the return value from the
> save operation as a parameter to post_save.

I feel like having that information in post_save may be useful, though
I'm having trouble coming up with any concrete uses for it. But, let me
know if you decide that you prefer it and I can add that for the next
revision.

-Aaron

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Dr. David Alan Gilbert]

* Aaron Lindsay (aclindsa@gmail.com) wrote:
> On Oct 16 15:06, Dr. David Alan Gilbert wrote:
> > I did wonder about suggesting that you pass the return value from the
> > save operation as a parameter to post_save.
> 
> I feel like having that information in post_save may be useful, though
> I'm having trouble coming up with any concrete uses for it. But, let me
> know if you decide that you prefer it and I can add that for the next
> revision.

I'm happy either way with the input value; it sounded useful to me but
not enough to ask you for, but if you agree then throw it in.

Dave

> -Aaron
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 06/14] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Aaron Lindsay]

On Oct 16 08:25, Aaron Lindsay wrote:
> On Oct 15 13:51, Richard Henderson wrote:
> > On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > > --- a/target/arm/machine.c
> > > +++ b/target/arm/machine.c
> > > @@ -584,7 +584,7 @@ static int cpu_pre_save(void *opaque)
> > >  {
> > >      ARMCPU *cpu = opaque;
> > >  
> > > -    pmccntr_op_start(&cpu->env);
> > > +    pmu_op_start(&cpu->env);
> > 
> > Does it make sense to move this patch earlier so that these hooks are modified
> > once?  No big deal if not.

I took another look at this, and I think it makes sense to move the
pmccntr_op -> pmu_op changes into "target/arm: Reorganize PMCCNTR
accesses", independent of the filtering changes.

-Aaron

Re: [Qemu-devel] [PATCH v6 00/14] More fully implement ARM PMUv3

[Richard Henderson]

On 10/16/18 5:46 AM, Aaron Lindsay wrote:
> Is anyone willing to take a glance at the final patch in this series,
> "target/arm: Send interrupts on PMU counter overflow", before my next
> iteration? I'm particularly interested in a review of the approach I
> took for detecting overflow.

I intend to finish reviewing the patch series today.


r~

Re: [Qemu-devel] [PATCH v6 10/14] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> Add arrays to hold the registers, the definitions themselves, access
> functions, and logic to reset counters when PMCR.P is set. Update
> filtering code to support counters other than PMCCNTR. Support migration
> with raw read/write functions.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> ---
>  target/arm/cpu.h    |   3 +
>  target/arm/helper.c | 296 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 282 insertions(+), 17 deletions(-)

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


r~

Re: [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> + * Return the underlying cycle count for the PMU cycle counters. If we're in
> + * usermode, simply return 0.
> + */
> +static uint64_t cycles_get_count(CPUARMState *env)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> +#else
> +    return 0;
> +#endif
> +}

Usually we pass through the host cycle counter.
See cpu_get_host_ticks().


r~

Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> This both advertises that we support four counters and enables them
> because the pmu_num_counters() reads this value from PMCR.
> 
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

It looks like this should have been folded into patch 10,
or patch 10 split to include the code changes that go with
these comment changes.


r~

Re: [Qemu-devel] [PATCH v6 13/14] target/arm: Implement PMSWINC

[Richard Henderson]

On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Juan Quintela]

Richard Henderson <richard.henderson@linaro.org> wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
>> In some cases it may be helpful to modify state before saving it for
>> migration, and then modify the state back after it has been saved. The
>> existing pre_save function provides half of this functionality. This
>> patch adds a post_save function to provide the second half.
>> 
>> Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
>> ---
>>  docs/devel/migration.rst    |  9 +++++++--
>>  include/migration/vmstate.h |  1 +
>>  migration/vmstate.c         | 10 +++++++++-
>>  3 files changed, 17 insertions(+), 3 deletions(-)
>
> Hmm, maybe.  I believe the common practice is for pre_save to copy state into a
> separate member on the side, so that conversion back isn't necessary.

Hi

Originally we have that function.  We removed it because we had not
remaining uses for it on tree.  I am not againt getting it if you need
it.

Once told that, I think you can add a return value as David saide.

And once there, it ia good thing that you document it if it is called
"before" or "after" the subsections_save.  There are arguments for doing
it either way, just document it.

Thanks, Juan.

Re: [Qemu-devel] [PATCH v6 03/14] migration: Add post_save function to VMStateDescription

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Aaron Lindsay (aclindsa@gmail.com) wrote:
>> On Oct 16 09:21, Dr. David Alan Gilbert wrote:
>> > * Richard Henderson (richard.henderson@linaro.org) wrote:
>> > > On 10/10/18 1:37 PM, Aaron Lindsay wrote:
>> > > > In some cases it may be helpful to modify state before saving it for
>> > > > migration, and then modify the state back after it has been saved. The
>> > > > existing pre_save function provides half of this functionality. This
>> > > > patch adds a post_save function to provide the second half.
>> > > > 
>> > > > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
>> > > > ---
>> > > >  docs/devel/migration.rst    |  9 +++++++--
>> > > >  include/migration/vmstate.h |  1 +
>> > > >  migration/vmstate.c         | 10 +++++++++-
>> > > >  3 files changed, 17 insertions(+), 3 deletions(-)
>> > > 
>> > > Hmm, maybe.  I believe the common practice is for pre_save to
>> > > copy state into a
>> > > separate member on the side, so that conversion back isn't necessary.
>> > > 
>> > > Ccing in the migration maintainers for a second opinion.
>> > 
>> > It is common to copy stuff into a separate member; however we do
>> > occasionally think that post_save would be a useful addition; so I think
>> > we should take it (if nothing else it actually makes stuff symmetric!).
>> > 
>> > Please make it return 'int' in the same way that pre_save/pre_load
>> > does, so that it can fail and stop the migration.
>> 
>> This patch calls post_save *even if the save operation fails*. My
>> reasoning was that I didn't want a failed migration to leave a
>> still-running original QEMU instance in an invalid state. Was this
>> misguided?
>
> That's fine - my only issue is that I want post_save to be able to fail
> itself even if pre_save failed.
>
>> If it was not, which error do you prefer to be returned from
>> vmstate_save_state_v() in the case that both the save operation itself
>> and the post_save call returned errors?
>
> The return value from the save operation.
> I did wonder about suggesting that you pass the return value from the
> save operation as a parameter to post_save.

The one from save.  In general, this one shouldn't be called, and if it
gets an error, we are really in big trouble, no?  By big treauble I mean
that we can basically only stop the guest?

Later, Juan.

>
> Dave
>
>> -Aaron
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

On Oct 16 17:09, Richard Henderson wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > This both advertises that we support four counters and enables them
> > because the pmu_num_counters() reads this value from PMCR.
> > 
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/helper.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> It looks like this should have been folded into patch 10,
> or patch 10 split to include the code changes that go with
> these comment changes.

By setting PMCR.N to 4, this commit is what causes all the previous
implementation of non-PMCCNTR counters to become usable by the guest -
both because the OS can now detect their presence in PMCR, and because
the system registers used to setup/access the counters are gated on
pmu_num_counters(), which uses PMCR.N to determine if a counter is
implemented.

That said, I'm not against merging the two patches if you feel that is
cleaner.

-Aaron

Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Richard Henderson]

On 10/17/18 12:20 PM, Aaron Lindsay wrote:
> On Oct 16 17:09, Richard Henderson wrote:
>> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
>>> This both advertises that we support four counters and enables them
>>> because the pmu_num_counters() reads this value from PMCR.
>>>
>>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>>> ---
>>>  target/arm/helper.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> It looks like this should have been folded into patch 10,
>> or patch 10 split to include the code changes that go with
>> these comment changes.
> 
> By setting PMCR.N to 4, this commit is what causes all the previous
> implementation of non-PMCCNTR counters to become usable by the guest -
> both because the OS can now detect their presence in PMCR, and because
> the system registers used to setup/access the counters are gated on
> pmu_num_counters(), which uses PMCR.N to determine if a counter is
> implemented.

But e.g.

> @@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>      }
>      if (arm_feature(env, ARM_FEATURE_V7)) {
>          /* v7 performance monitor control register: same implementor
> -         * field as main ID register, and we implement only the cycle
> -         * count register.
> +         * field as main ID register, and we implement four counters in
> +         * addition to the cycle count register.
>           */
>          unsigned int i, pmcrn = 4;
>          ARMCPRegInfo pmcr = {

clearly belongs with the previous patch, since pmcrn is already 4.

> @@ -1706,7 +1706,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .access = PL1_W, .type = ARM_CP_NOP },
>      /* Performance monitors are implementation defined in v7,
>       * but with an ARM recommended set of registers, which we
> -     * follow (although we don't actually implement any counters)
> +     * follow.
>       *
>       * Performance registers fall into three categories:
>       *  (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR)

And this is out of date as well, and probably belonged elsewhere.
Which leaves only

> @@ -5430,7 +5430,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              .access = PL0_RW, .accessfn = pmreg_access,
>              .type = ARM_CP_IO,
>              .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
> -            .resetvalue = cpu->midr & 0xff000000,
> +            .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT),
>              .writefn = pmcr_write, .raw_writefn = raw_write,
>          };

which suggests that this one line belonged to the patch that set pmcrn in the
first place.


r~

Re: [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

On Oct 16 17:04, Richard Henderson wrote:
> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> > + * Return the underlying cycle count for the PMU cycle counters. If we're in
> > + * usermode, simply return 0.
> > + */
> > +static uint64_t cycles_get_count(CPUARMState *env)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > +                   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> > +#else
> > +    return 0;
> > +#endif
> > +}
> 
> Usually we pass through the host cycle counter.
> See cpu_get_host_ticks().

Why do you prefer cpu_get_host_ticks()? And are you suggesting this for
just user-mode, or both system and user?

PMCCNTR used this same qemu_clock_get_ns() call previous to my patch
(see where this patch replaces that call with one to cycles_get_count()
in pmccntr_op_start()). Of course, we could keep the preexisting PMCCNTR
behavior while making the new cycle counter use cpu_get_host_ticks(),
but having two ways through the same interface which count cycles
differently feels wrong.

-Aaron

Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

On Oct 17 12:34, Richard Henderson wrote:
> On 10/17/18 12:20 PM, Aaron Lindsay wrote:
> > On Oct 16 17:09, Richard Henderson wrote:
> >> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> >>> This both advertises that we support four counters and enables them
> >>> because the pmu_num_counters() reads this value from PMCR.
> >>>
> >>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> >>> ---
> >>>  target/arm/helper.c | 8 ++++----
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> It looks like this should have been folded into patch 10,
> >> or patch 10 split to include the code changes that go with
> >> these comment changes.
> > 
> > By setting PMCR.N to 4, this commit is what causes all the previous
> > implementation of non-PMCCNTR counters to become usable by the guest -
> > both because the OS can now detect their presence in PMCR, and because
> > the system registers used to setup/access the counters are gated on
> > pmu_num_counters(), which uses PMCR.N to determine if a counter is
> > implemented.
> 
> But e.g.
> 
> > @@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> >      }
> >      if (arm_feature(env, ARM_FEATURE_V7)) {
> >          /* v7 performance monitor control register: same implementor
> > -         * field as main ID register, and we implement only the cycle
> > -         * count register.
> > +         * field as main ID register, and we implement four counters in
> > +         * addition to the cycle count register.
> >           */
> >          unsigned int i, pmcrn = 4;
> >          ARMCPRegInfo pmcr = {
> 
> clearly belongs with the previous patch, since pmcrn is already 4.

I suppose pmcrn (the local variable) should've been set to 0 before this
patch and updated here to be 4. However, even though it was already set
to 4 my amendment to the comment was still not true until this patch,
because it wasn't written into PMCR.N (the CP register).

> > @@ -1706,7 +1706,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >        .access = PL1_W, .type = ARM_CP_NOP },
> >      /* Performance monitors are implementation defined in v7,
> >       * but with an ARM recommended set of registers, which we
> > -     * follow (although we don't actually implement any counters)
> > +     * follow.
> >       *
> >       * Performance registers fall into three categories:
> >       *  (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR)
> 
> And this is out of date as well, and probably belonged elsewhere.
> Which leaves only

Why is this out of date? Because PMCR.N still contained zero, no
counters were implemented from the guest's standpoint until this patch.

> > @@ -5430,7 +5430,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> >              .access = PL0_RW, .accessfn = pmreg_access,
> >              .type = ARM_CP_IO,
> >              .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
> > -            .resetvalue = cpu->midr & 0xff000000,
> > +            .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << PMCRN_SHIFT),
> >              .writefn = pmcr_write, .raw_writefn = raw_write,
> >          };
> 
> which suggests that this one line belonged to the patch that set pmcrn in the
> first place.

I get the feeling we may be talking past each other/misunderstanding
somehow, but I'm not sure why/how. Maybe I'm thinking about whether or
not counters are implemented from the guest's standpoint and you're
thinking about it from the standpoint of whether the code to implement
them exists in QEMU?

Anyway, by splitting these patches apart, my intention was to separate
the plumbing work from turning on the water. If you believe it is better
to do both in the same patch, I'll make that change for v7.

-Aaron

Re: [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events

[Richard Henderson]

On 10/17/18 12:47 PM, Aaron Lindsay wrote:
> On Oct 16 17:04, Richard Henderson wrote:
>> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
>>> + * Return the underlying cycle count for the PMU cycle counters. If we're in
>>> + * usermode, simply return 0.
>>> + */
>>> +static uint64_t cycles_get_count(CPUARMState *env)
>>> +{
>>> +#ifndef CONFIG_USER_ONLY
>>> +    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>>> +                   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
>>> +#else
>>> +    return 0;
>>> +#endif
>>> +}
>>
>> Usually we pass through the host cycle counter.
>> See cpu_get_host_ticks().
> 
> Why do you prefer cpu_get_host_ticks()? And are you suggesting this for
> just user-mode, or both system and user?

Just user-mode.  Providing a clock with unknown scaling is more useful than a
constant 0.


r~

Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Richard Henderson]

On 10/17/18 1:25 PM, Aaron Lindsay wrote:
> I suppose pmcrn (the local variable) should've been set to 0 before this
> patch and updated here to be 4.

That's plausible.

> Anyway, by splitting these patches apart, my intention was to separate
> the plumbing work from turning on the water. If you believe it is better
> to do both in the same patch, I'll make that change for v7.

Fair.  I'll let Peter weigh in for final decision.


r~

Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Peter Maydell]

On 17 October 2018 at 22:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 10/17/18 1:25 PM, Aaron Lindsay wrote:
>> I suppose pmcrn (the local variable) should've been set to 0 before this
>> patch and updated here to be 4.
>
> That's plausible.
>
>> Anyway, by splitting these patches apart, my intention was to separate
>> the plumbing work from turning on the water. If you believe it is better
>> to do both in the same patch, I'll make that change for v7.
>
> Fair.  I'll let Peter weigh in for final decision.

No strong opinion -- rth has reviewed this set, not me.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6 11/14] target/arm: PMU: Add instruction and cycle events

[Aaron Lindsay]

On Oct 17 14:12, Richard Henderson wrote:
> On 10/17/18 12:47 PM, Aaron Lindsay wrote:
> > On Oct 16 17:04, Richard Henderson wrote:
> >> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> >>> + * Return the underlying cycle count for the PMU cycle counters. If we're in
> >>> + * usermode, simply return 0.
> >>> + */
> >>> +static uint64_t cycles_get_count(CPUARMState *env)
> >>> +{
> >>> +#ifndef CONFIG_USER_ONLY
> >>> +    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> >>> +                   ARM_CPU_FREQ, NANOSECONDS_PER_SECOND);
> >>> +#else
> >>> +    return 0;
> >>> +#endif
> >>> +}
> >>
> >> Usually we pass through the host cycle counter.
> >> See cpu_get_host_ticks().
> > 
> > Why do you prefer cpu_get_host_ticks()? And are you suggesting this for
> > just user-mode, or both system and user?
> 
> Just user-mode.  Providing a clock with unknown scaling is more useful than a
> constant 0.

Okay, that makes sense I think. I slightly dislike the fact that the
behavior is silently different between user and system mode, but perhaps
some documentation could alleviate my concern - is there an appropriate
place to document this difference?

It occurs to me that one argument for always returning 0 is that it's
immediately obvious that the behavior is different from system mode,
eliminating the possibility of a user assuming results from user and
system mode are comparable.

-Aaron

Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

On Oct 17 14:14, Richard Henderson wrote:
> On 10/17/18 1:25 PM, Aaron Lindsay wrote:
> > I suppose pmcrn (the local variable) should've been set to 0 before this
> > patch and updated here to be 4.
> 
> That's plausible.
> 
> > Anyway, by splitting these patches apart, my intention was to separate
> > the plumbing work from turning on the water. If you believe it is better
> > to do both in the same patch, I'll make that change for v7.
> 
> Fair.  I'll let Peter weigh in for final decision.

At a minimum, I've updated my local tree so that pmcrn is initially set
to 0 in 'target/arm: Finish implementation of PM[X]EVCNTR and
PM[X]EVTYPER' and updated to 4 in this patch. And I'll squash them
instead if that ends up being the decision. 

-Aaron