[Qemu-devel] [PATCH v5 00/13] More fully implement ARM PMUv3

[Qemu-devel] [PATCH v5 00/13] 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 and filter them based on
execution mode. 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 v4 I've added improved V7VE handling with Peter's direction, fixed up a
few patch staging issues, and fixed a bug causing cycle counter overflow to be
checked every instruction.

-Aaron

Aaron Lindsay (13):
  target/arm: Reorganize PMCCNTR accesses
  target/arm: Filter cycle counter based on PMCCFILTR_EL0
  target/arm: Allow AArch32 access for PMCCFILTR
  target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions
  target/arm: Remove redundant DIV detection for KVM
  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: Mark PMINTENSET accesses as possibly doing IO
  target/arm: Send interrupts on PMU counter overflow

 target/arm/cpu.c    |  49 +++-
 target/arm/cpu.h    |  71 +++++-
 target/arm/cpu64.c  |   2 -
 target/arm/helper.c | 702 +++++++++++++++++++++++++++++++++++++++++++++-------
 target/arm/kvm32.c  |  27 +-
 5 files changed, 720 insertions(+), 131 deletions(-)

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v5 01/13] 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
ensure 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    |  28 ++++++++++-----
 target/arm/helper.c | 100 ++++++++++++++++++++++++++++------------------------
 2 files changed, 73 insertions(+), 55 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8488273..ba2c876 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -467,10 +467,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-corect 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 */
@@ -924,15 +934,17 @@ 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, and the return value
+ * from pmccntr_op_start must be supplied as the second argument to
+ * pmccntr_op_finish.
  */
-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 1248d84..23b3a16 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1044,28 +1044,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)) {
-        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+        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)) {
+        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 */
@@ -1076,26 +1101,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,
@@ -1112,22 +1127,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,
@@ -1140,7 +1142,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)
 {
 }
 
@@ -1149,9 +1155,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,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 01/13] target/arm: Reorganize PMCCNTR accesses

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> 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
> ensure 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    |  28 ++++++++++-----
>  target/arm/helper.c | 100 ++++++++++++++++++++++++++++------------------------
>  2 files changed, 73 insertions(+), 55 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8488273..ba2c876 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -467,10 +467,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-corect value is being read/set.

"correct"

>           */
>          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;

I had a look through the patchset, and I still can't see
anything here which indicates how we're handling migration.

If this field is only ever used as a temporary while we're
between op_start and op_finish, then we can just return it from
start and take it as an argument to finish, and we're fine
(migration of the PMCCNTR_EL0 register will be done by calling
its readfn on the source and its writefn on the destination).

If there's a reason for having this be a field in the state struct
(I think you said counter overflow)? then we need to be sure that
migration is going to do the right thing and that we don't for instance
lose overflow indications during a migration.

>          uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
>          uint64_t vpidr_el2; /* Virtualization Processor ID Register */
>          uint64_t vmpidr_el2; /* Virtualization Multiprocessor ID Register */
> @@ -924,15 +934,17 @@ 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, and the return value
> + * from pmccntr_op_start must be supplied as the second argument to
> + * pmccntr_op_finish.
>   */
> -void pmccntr_sync(CPUARMState *env);
> +void pmccntr_op_start(CPUARMState *env);
> +void pmccntr_op_finish(CPUARMState *env);

The comment says pmccntr_op_start() returns a value but the
prototype says it doesn't...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 01/13] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay]

On Jun 28 17:40, Peter Maydell wrote:
> On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 8488273..ba2c876 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -467,10 +467,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-corect value is being read/set.
> 
> "correct"

Thanks - fixed for v6.

> >           */
> >          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;
> 
> I had a look through the patchset, and I still can't see
> anything here which indicates how we're handling migration.

I'll admit that I had not previously thought through everything
exhaustively, but I think I've convinced myself that *almost* everything
is already handled by calling .readfn/writefn on the registers for
migration... more below.

> If this field is only ever used as a temporary while we're
> between op_start and op_finish, then we can just return it from
> start and take it as an argument to finish, and we're fine
> (migration of the PMCCNTR_EL0 register will be done by calling
> its readfn on the source and its writefn on the destination).
> 
> If there's a reason for having this be a field in the state struct
> (I think you said counter overflow)? then we need to be sure that
> migration is going to do the right thing and that we don't for instance
> lose overflow indications during a migration.

I think we do need additional state for supporting counter overflow
during normal operation (more than just between op_start and op_finish),
but I don't think sending additional state over the wire is necessary
for migration. Let me lay out my reasoning to see if you think it makes
sense:

I observe that we seem to deal mostly with the following values with
respect to PMU counters, and I think it might be easier to discuss this
in terms of values rather than variables:

	AC = current architectural value of PMCCNTR
	AL = architectural value of PMCCNTR when last observed/updated by
	     the OS
	UC = current underlying counter value
	UL = underlying counter value when PMCCNTR last observed/updated by
	     OS
	D = difference between underlying counter value and architectural
	    value

At any given point in time, the following should then be true during
normal execution (i.e. between OS accesses to the counter):
	UL - AL == D == UC - AC

The approach taken previous to my patchset was to store D in
cp15.c15_ccnt most of the time, updating that variable to hold AC when
read (by doing `c15_ccnt = UC - c15_ccnt`... an implementation using `AC
= UC - D`, which is a different way to write `D = UC - AC` from above).
This operation was reversed after a write completes, so that c15_ccnt
again holds a value corresponding to D.

Only storing D works well and minimizes memory usage if you don't care
about detecting overflows. However, without some frame of reference for
what the last observed architectural value was relative to its current
value, we have no way to know if the counter has overflowed in the
intervening time. In other words, we need to know both AC and AL to know
if the counter has overflowed. Assuming we store D and can obtain UC we
can calculate AC, but we need to store at least one additional piece of
information to obtain AL (either AL itself or UL).

In my patchset so far, I'm storing AL in c15_ccnt and D in
c15_ccnt_delta during normal operation and AL/AC in c15_ccnt, UL/UC in
c15_ccnt_delta between op_start and op_finish (note that AL == AC, UL ==
UC, and D == 0 during this time). Also note that UL/UC are only stored
between op_start and op_finish calls to avoid losing time, and are not
needed to maintain architectural state across migrations.

Now for migration - The read/writefn's already marshal/unmarshal both
the counter value and overflow state to their architectural values in
PMCCNTR and PMOVSCLR. I think this would work on its own if we made
two big (and wrong) assumptions:
	1. PMOVSCLR is read after PMCCNTR on save (ensures all overflows
	   which have occurred are recorded in PMOVSCLR) and written before
	   it on load (ensures any overflow bits in PMOVSCLR cause the
	   interrupt line to be raised)
	2. Time (as measured by `qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)`) does
	   not change during the process of saving or loading CP registers
	   (ensures PMCCNTR can't have overflowed between when it was saved
	   and when PMOVSCLR was later saved).

One possible solution for this is to add calls to op_start in both
cpu_pre_load and cpu_pre_save, add calls to op_finish in cpu_post_load
and cpu_post_save, and ensure that all CP registers which would call
op_start/finish on their own have raw_read/writefn's which assume
they're between calls to op_start and op_finish.

Thoughts? (and I apologize for the wall of text)

-Aaron

[Qemu-devel] [PATCH v5 02/13] 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>
---
 target/arm/cpu.c    |   3 ++
 target/arm/cpu.h    |  22 +++++++++-
 target/arm/helper.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 129 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index e1de45e..08ce1bc 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -916,6 +916,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 ba2c876..800c4ec 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -946,6 +946,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
@@ -1007,7 +1025,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)
@@ -1017,6 +1036,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 23b3a16..7c66977 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -935,10 +935,20 @@ 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   0x000003ff
+
 static inline uint32_t pmu_num_counters(CPUARMState *env)
 {
   return (env->cp15.c9_pmcr & PMCRN_MASK) >> PMCRN_SHIFT;
@@ -1034,16 +1044,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));
+
+    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);
     }
 
-    return true;
+    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,
@@ -1056,7 +1116,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 */
@@ -1075,7 +1135,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) {
@@ -1087,10 +1147,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 */
@@ -1101,7 +1181,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)
@@ -1150,6 +1230,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,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 02/13] target/arm: Filter cycle counter based on PMCCFILTR_EL0

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> 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>

thanks
-- PMM

[Qemu-devel] [PATCH v5 03/13] target/arm: Allow AArch32 access for PMCCFILTR

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.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 7c66977..7d63bb2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -949,6 +949,10 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMXEVTYPER_MT         0x02000000
 #define PMXEVTYPER_EVTCOUNT   0x000003ff
 
+#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;
@@ -1252,10 +1256,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)
 {
@@ -1474,6 +1494,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .type = ARM_CP_IO,
       .readfn = pmccntr_read, .writefn = pmccntr_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,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 03/13] target/arm: Allow AArch32 access for PMCCFILTR

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v5 04/13] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.c   | 21 ++++++++++++++-------
 target/arm/cpu.h   |  1 +
 target/arm/kvm32.c |  8 ++++----
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 08ce1bc..98790b1 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -793,9 +793,20 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
     /* Some features automatically imply others: */
     if (arm_feature(env, ARM_FEATURE_V8)) {
-        set_feature(env, ARM_FEATURE_V7);
+        set_feature(env, ARM_FEATURE_V7VE);
+    }
+    if (arm_feature(env, ARM_FEATURE_V7VE)) {
+        /* v7 Virtualization Extensions. In real hardware this implies
+         * EL2 and also the presence of the Security Extensions.
+         * For QEMU, for backwards-compatibility we implement some
+         * CPUs or CPU configs which have no actual EL2 or EL3 but do
+         * include the various other features that V7VE implies.
+         * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
+         * Security Extensions is ARM_FEATURE_EL3.
+         */
         set_feature(env, ARM_FEATURE_ARM_DIV);
         set_feature(env, ARM_FEATURE_LPAE);
+        set_feature(env, ARM_FEATURE_V7);
     }
     if (arm_feature(env, ARM_FEATURE_V7)) {
         set_feature(env, ARM_FEATURE_VAPA);
@@ -1509,15 +1520,13 @@ static void cortex_a7_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
 
     cpu->dtb_compatible = "arm,cortex-a7";
-    set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
-    set_feature(&cpu->env, ARM_FEATURE_ARM_DIV);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
-    set_feature(&cpu->env, ARM_FEATURE_LPAE);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A7;
     cpu->midr = 0x410fc075;
@@ -1554,15 +1563,13 @@ static void cortex_a15_initfn(Object *obj)
     ARMCPU *cpu = ARM_CPU(obj);
 
     cpu->dtb_compatible = "arm,cortex-a15";
-    set_feature(&cpu->env, ARM_FEATURE_V7);
+    set_feature(&cpu->env, ARM_FEATURE_V7VE);
     set_feature(&cpu->env, ARM_FEATURE_VFP4);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
-    set_feature(&cpu->env, ARM_FEATURE_ARM_DIV);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
-    set_feature(&cpu->env, ARM_FEATURE_LPAE);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_CORTEX_A15;
     cpu->midr = 0x412fc0f1;
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 800c4ec..852743b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1474,6 +1474,7 @@ enum arm_features {
     ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
     ARM_FEATURE_THUMB2EE,
     ARM_FEATURE_V7MP,    /* v7 Multiprocessing Extensions */
+    ARM_FEATURE_V7VE, /* v7 Virtualization Extensions (non-EL2 parts) */
     ARM_FEATURE_V4T,
     ARM_FEATURE_V5,
     ARM_FEATURE_STRONGARM,
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 1740cda..fb9ea37 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -98,12 +98,12 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     /* Now we've retrieved all the register information we can
      * set the feature bits based on the ID register fields.
      * We can assume any KVM supporting CPU is at least a v7
-     * with VFPv3, LPAE and the generic timers; this in turn implies
-     * most of the other feature bits, but a few must be tested.
+     * with VFPv3, virtualization extensions, and the generic
+     * timers; this in turn implies most of the other feature
+     * bits, but a few must be tested.
      */
-    set_feature(&features, ARM_FEATURE_V7);
+    set_feature(&features, ARM_FEATURE_V7VE);
     set_feature(&features, ARM_FEATURE_VFP3);
-    set_feature(&features, ARM_FEATURE_LPAE);
     set_feature(&features, ARM_FEATURE_GENERIC_TIMER);
 
     switch (extract32(id_isar0, 24, 4)) {
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 04/13] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.c   | 21 ++++++++++++++-------
>  target/arm/cpu.h   |  1 +
>  target/arm/kvm32.c |  8 ++++----
>  3 files changed, 19 insertions(+), 11 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v5 05/13] target/arm: Remove redundant DIV detection for KVM

[Aaron Lindsay]

KVM implies V7VE, which implies ARM_DIV and THUMB_DIV. The conditional
detection here is therefore unnecessary. Because V7VE is already
unconditionally specified for all KVM hosts, ARM_DIV and THUMB_DIV are
already indirectly specified and do not need to be included here at all.

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

diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index fb9ea37..4e91c11 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -36,7 +36,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      * and then query that CPU for the relevant ID registers.
      */
     int i, ret, fdarray[3];
-    uint32_t midr, id_pfr0, id_isar0, mvfr1;
+    uint32_t midr, id_pfr0, mvfr1;
     uint64_t features = 0;
     /* Old kernels may not know about the PREFERRED_TARGET ioctl: however
      * we know these will only support creating one kind of guest CPU,
@@ -60,11 +60,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
         },
         {
             .id = KVM_REG_ARM | KVM_REG_SIZE_U32
-            | ENCODE_CP_REG(15, 0, 0, 0, 2, 0, 0),
-            .addr = (uintptr_t)&id_isar0,
-        },
-        {
-            .id = KVM_REG_ARM | KVM_REG_SIZE_U32
             | KVM_REG_ARM_VFP | KVM_REG_ARM_VFP_MVFR1,
             .addr = (uintptr_t)&mvfr1,
         },
@@ -106,18 +101,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     set_feature(&features, ARM_FEATURE_VFP3);
     set_feature(&features, ARM_FEATURE_GENERIC_TIMER);
 
-    switch (extract32(id_isar0, 24, 4)) {
-    case 1:
-        set_feature(&features, ARM_FEATURE_THUMB_DIV);
-        break;
-    case 2:
-        set_feature(&features, ARM_FEATURE_ARM_DIV);
-        set_feature(&features, ARM_FEATURE_THUMB_DIV);
-        break;
-    default:
-        break;
-    }
-
     if (extract32(id_pfr0, 12, 4) == 1) {
         set_feature(&features, ARM_FEATURE_THUMB2EE);
     }
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 05/13] target/arm: Remove redundant DIV detection for KVM

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> KVM implies V7VE, which implies ARM_DIV and THUMB_DIV. The conditional
> detection here is therefore unnecessary. Because V7VE is already
> unconditionally specified for all KVM hosts, ARM_DIV and THUMB_DIV are
> already indirectly specified and do not need to be included here at all.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v5 06/13] 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 7d63bb2..5d83446 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1293,9 +1293,17 @@ 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;
 }
 
+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)
 {
@@ -1645,6 +1653,23 @@ 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,
+      .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)
 {
@@ -4996,6 +5021,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
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 06/13] target/arm: Implement PMOVSSET

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> 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(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7d63bb2..5d83446 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1293,9 +1293,17 @@ 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;
>  }

This change doesn't look like it should be in this patch ?

>
> +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)
>  {
> @@ -1645,6 +1653,23 @@ 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,
> +      .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> +      .writefn = pmovsset_write,
> +      .raw_writefn = raw_write },

This should be marked ARM_CP_ALIAS, beacuse its underlying
state in c9_pmovsr is just an alias into PMOVSR, and that
register is handling reset and migration.

> +    { .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)
>  {
> @@ -4996,6 +5021,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
> --

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 06/13] target/arm: Implement PMOVSSET

[Aaron Lindsay]

On Jun 28 17:23, Peter Maydell wrote:
> On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> 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(+)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index 7d63bb2..5d83446 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -1293,9 +1293,17 @@ 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;
> >  }
> 
> This change doesn't look like it should be in this patch ?

This has been appropriately split off into a separate patch for v6. I
must've seen "pmovsr" and staged it here by accident.

> >
> > +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)
> >  {
> > @@ -1645,6 +1653,23 @@ 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,
> > +      .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pmovsr),
> > +      .writefn = pmovsset_write,
> > +      .raw_writefn = raw_write },
> 
> This should be marked ARM_CP_ALIAS, beacuse its underlying
> state in c9_pmovsr is just an alias into PMOVSR, and that
> register is handling reset and migration.

Thanks for catching this, too!

-Aaron

[Qemu-devel] [PATCH v5 07/13] 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 | 37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 98790b1..2f5b16a 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -927,9 +927,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)) {
@@ -1538,8 +1548,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;
@@ -1581,8 +1589,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 852743b..430b8d5 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -964,6 +964,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 c50dcd4..28482a0 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -141,8 +141,6 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     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 5d83446..9f81747 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -964,6 +964,43 @@ 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 10 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;
+
+#define SUPPORTED_EVENT_SENTINEL UINT16_MAX
+static const pm_event pm_events[] = {
+    { .number = SUPPORTED_EVENT_SENTINEL }
+};
+static uint16_t supported_event_map[0x3f];
+
+/*
+ * 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 = 0;
+    while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) {
+        const pm_event *cnt = &pm_events[i];
+        if (cnt->number < 0x3f && cnt->supported(env)) {
+            pmceid |= (1 << cnt->number);
+            supported_event_map[cnt->number] = i;
+        } else {
+            supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL;
+        }
+        i++;
+    }
+    return pmceid;
+}
+
 static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri,
                                    bool isread)
 {
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 07/13] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> 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 | 37 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 98790b1..2f5b16a 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -927,9 +927,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;

This is ok for now, but we really need to sort out what we're
doing for the interplay between feature bits and ID register
values more generally (notably for '-cpu max'), so depending on
when we do that work this might end up needing to change a bit
to fit in.

>      }
>
>      if (!arm_feature(env, ARM_FEATURE_EL2)) {
> @@ -1538,8 +1548,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;
> @@ -1581,8 +1589,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 852743b..430b8d5 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -964,6 +964,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 c50dcd4..28482a0 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -141,8 +141,6 @@ static void aarch64_a57_initfn(Object *obj)
>      cpu->id_isar5 = 0x00011121;
>      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 5d83446..9f81747 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -964,6 +964,43 @@ 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 10 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 */

Don't leave the */ dangling on the RHS of a multiline comment.
(CODING_STYLE now recommends Linux-kernel style.)

> +    uint64_t (*get_count)(CPUARMState *);
> +} pm_event;
> +
> +#define SUPPORTED_EVENT_SENTINEL UINT16_MAX
> +static const pm_event pm_events[] = {
> +    { .number = SUPPORTED_EVENT_SENTINEL }
> +};
> +static uint16_t supported_event_map[0x3f];

What is this hardcoded 0x3f ?

> +
> +/*
> + * 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 = 0;
> +    while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) {

Are we ever going to run through some other array than the
constant pm_events[] ? We could just use ARRAY_SIZE rather
than requiring a sentinel member.

> +        const pm_event *cnt = &pm_events[i];
> +        if (cnt->number < 0x3f && cnt->supported(env)) {

...another 0x3f. Are we ever really going to see an
out-of-range .number field, or should we just assert()
that it's in range ?

> +            pmceid |= (1 << cnt->number);
> +            supported_event_map[cnt->number] = i;
> +        } else {
> +            supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL;

This will leave us with supported_event_map[] entries in
one of three states:
 * SUPPORTED_EVENT_SENTINEL if they have a specific pm_events[]
   entry marked as not supported
 * an index if they are supported
 * zero if there is no pm_events[] entry

Don't the ones in the last category get confused with the
real index 0?

Maybe this should initialize the whole array first to the
sentinel value and then just fill in the supported ones with
their index values?

> +        }
> +        i++;
> +    }
> +    return pmceid;
> +}
> +

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 07/13] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay]

On Jul 17 17:06, Peter Maydell wrote:
> On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> > 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 | 37 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 60 insertions(+), 9 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 98790b1..2f5b16a 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -927,9 +927,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;
> 
> This is ok for now, but we really need to sort out what we're
> doing for the interplay between feature bits and ID register
> values more generally (notably for '-cpu max'), so depending on
> when we do that work this might end up needing to change a bit
> to fit in.

Okay, I'll try to be on the lookout for this work.

> >      }
> >
> >      if (!arm_feature(env, ARM_FEATURE_EL2)) {
> > @@ -1538,8 +1548,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;
> > @@ -1581,8 +1589,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 852743b..430b8d5 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -964,6 +964,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 c50dcd4..28482a0 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -141,8 +141,6 @@ static void aarch64_a57_initfn(Object *obj)
> >      cpu->id_isar5 = 0x00011121;
> >      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 5d83446..9f81747 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -964,6 +964,43 @@ 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 10 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 */
> 
> Don't leave the */ dangling on the RHS of a multiline comment.
> (CODING_STYLE now recommends Linux-kernel style.)
> 
> > +    uint64_t (*get_count)(CPUARMState *);
> > +} pm_event;
> > +
> > +#define SUPPORTED_EVENT_SENTINEL UINT16_MAX
> > +static const pm_event pm_events[] = {
> > +    { .number = SUPPORTED_EVENT_SENTINEL }
> > +};
> > +static uint16_t supported_event_map[0x3f];
> 
> What is this hardcoded 0x3f ?

This was the maximum common event ID (i.e. bits in PMCEID[01]) as of
ARMv8.0. Obviously it should be a macro instead of a magic constant. I'm
also debating whether this should be the maximum supported by the
architecture or just the maximum supported by QEMU (particularly because
later ARM versions raise this number significantly).

> > +
> > +/*
> > + * 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 = 0;
> > +    while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) {
> 
> Are we ever going to run through some other array than the
> constant pm_events[] ? We could just use ARRAY_SIZE rather
> than requiring a sentinel member.

No, I don't think so.

> 
> > +        const pm_event *cnt = &pm_events[i];
> > +        if (cnt->number < 0x3f && cnt->supported(env)) {
> 
> ...another 0x3f. Are we ever really going to see an
> out-of-range .number field, or should we just assert()
> that it's in range ?

In some places this can be set by user code via PMEVTYPER.evtCount, but
I think in this code an assert is the right way to handle it.

> 
> > +            pmceid |= (1 << cnt->number);
> > +            supported_event_map[cnt->number] = i;
> > +        } else {
> > +            supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL;
> 
> This will leave us with supported_event_map[] entries in
> one of three states:
>  * SUPPORTED_EVENT_SENTINEL if they have a specific pm_events[]
>    entry marked as not supported
>  * an index if they are supported
>  * zero if there is no pm_events[] entry
> 
> Don't the ones in the last category get confused with the
> real index 0?
> 
> Maybe this should initialize the whole array first to the
> sentinel value and then just fill in the supported ones with
> their index values?

I've re-written this code based on your feedback for v6, eliminating the
sentinel and initializing the entirety of supported_event_map.

-Aaron

[Qemu-devel] [PATCH v5 08/13] 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.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
---
 target/arm/cpu.h    |   3 +
 target/arm/helper.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 209 insertions(+), 18 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 430b8d5..c240b38 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -481,6 +481,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 9f81747..f1fd21c 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -938,6 +938,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
@@ -1120,9 +1121,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;
@@ -1142,6 +1145,21 @@ 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 > 0x3f) {
+            return false; /* We only support common architectural and
+                             microarchitectural events */
+        }
+
+        uint16_t event_idx = supported_event_map[event];
+        if (event_idx == SUPPORTED_EVENT_SENTINEL) {
+            return false;
+        }
+    }
+
     return enabled && !prohibited && !filtered;
 }
 
@@ -1188,14 +1206,44 @@ 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;
+    uint16_t event_idx = supported_event_map[event];
+    uint64_t 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)
@@ -1218,6 +1266,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);
@@ -1271,6 +1326,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)
 {
 }
@@ -1341,30 +1404,113 @@ 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 == 0x1f) {
+        pmccfiltr_write(env, ri, value);
+    } else if (counter < pmu_num_counters(env)) {
+        pmevcntr_op_start(env, counter);
+        env->cp15.c14_pmevtyper[counter] = value & 0xfe0003ff;
+        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 == 0x1f) {
+        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 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().
-     */
-    if (env->cp15.c9_pmselr == 0x1f) {
-        return env->cp15.pmccfiltr_el0;
+    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. */
+}
+
+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 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)
 {
@@ -1552,16 +1698,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),
@@ -4250,7 +4403,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,
@@ -5062,6 +5215,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
         define_arm_cp_regs(cpu, pmovsset_cp_reginfo);
     }
     if (arm_feature(env, ARM_FEATURE_V7)) {
+        unsigned int i;
         /* v7 performance monitor control register: same implementor
          * field as main ID register, and we implement only the cycle
          * count register.
@@ -5086,6 +5240,40 @@ 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 < 31; 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_NO_RAW | ARM_CP_IO,
+                  .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_NO_RAW | ARM_CP_IO,
+                  .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn },
+                { .name = pmevtyper_name, .cp = 15, .crn = 15,
+                  .crm = 12 | (3 & (i >> 3)), .opc1 = 0, .opc2 = i & 7,
+                  .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
+                  .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_NO_RAW | ARM_CP_IO,
+                  .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn },
+                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,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 08/13] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> 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.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/cpu.h    |   3 +
>  target/arm/helper.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 209 insertions(+), 18 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 430b8d5..c240b38 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -481,6 +481,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 9f81747..f1fd21c 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -938,6 +938,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
> @@ -1120,9 +1121,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;
> @@ -1142,6 +1145,21 @@ 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 */

Please use the right format for multiline comments (here and below).

>  static void pmuserenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> @@ -1552,16 +1698,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),
> @@ -4250,7 +4403,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,
> @@ -5062,6 +5215,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_arm_cp_regs(cpu, pmovsset_cp_reginfo);
>      }
>      if (arm_feature(env, ARM_FEATURE_V7)) {
> +        unsigned int i;
>          /* v7 performance monitor control register: same implementor
>           * field as main ID register, and we implement only the cycle
>           * count register.
> @@ -5086,6 +5240,40 @@ 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 < 31; 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_NO_RAW | ARM_CP_IO,
> +                  .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_NO_RAW | ARM_CP_IO,
> +                  .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn },
> +                { .name = pmevtyper_name, .cp = 15, .crn = 15,
> +                  .crm = 12 | (3 & (i >> 3)), .opc1 = 0, .opc2 = i & 7,
> +                  .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
> +                  .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_NO_RAW | ARM_CP_IO,
> +                  .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn },
> +                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

Because all these registers are marked as NO_RAW we're going to fail
to migrate their state, I think.

This patchset in general needs to address how the extra CPU state
involved with the PMU is going to be migrated.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 08/13] target/arm: Finish implementation of PM[X]EVCNTR and PM[X]EVTYPER

[Aaron Lindsay]

On Jul 17 17:17, Peter Maydell wrote:
> On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> 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.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > ---
> >  target/arm/cpu.h    |   3 +
> >  target/arm/helper.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 209 insertions(+), 18 deletions(-)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 430b8d5..c240b38 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -481,6 +481,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 9f81747..f1fd21c 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -938,6 +938,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
> > @@ -1120,9 +1121,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;
> > @@ -1142,6 +1145,21 @@ 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 */
> 
> Please use the right format for multiline comments (here and below).

Fixed.

> 
> >  static void pmuserenr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >                              uint64_t value)
> >  {
> > @@ -1552,16 +1698,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),
> > @@ -4250,7 +4403,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,
> > @@ -5062,6 +5215,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> >          define_arm_cp_regs(cpu, pmovsset_cp_reginfo);
> >      }
> >      if (arm_feature(env, ARM_FEATURE_V7)) {
> > +        unsigned int i;
> >          /* v7 performance monitor control register: same implementor
> >           * field as main ID register, and we implement only the cycle
> >           * count register.
> > @@ -5086,6 +5240,40 @@ 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 < 31; 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_NO_RAW | ARM_CP_IO,
> > +                  .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_NO_RAW | ARM_CP_IO,
> > +                  .readfn = pmevcntr_readfn, .writefn = pmevcntr_writefn },
> > +                { .name = pmevtyper_name, .cp = 15, .crn = 15,
> > +                  .crm = 12 | (3 & (i >> 3)), .opc1 = 0, .opc2 = i & 7,
> > +                  .access = PL0_RW, .type = ARM_CP_NO_RAW | ARM_CP_IO,
> > +                  .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_NO_RAW | ARM_CP_IO,
> > +                  .readfn = pmevtyper_readfn, .writefn = pmevtyper_writefn },
> > +                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
> 
> Because all these registers are marked as NO_RAW we're going to fail
> to migrate their state, I think.

A bad copy-paste job from the PMXEVCNTR variants, I'll warrant. Fixed
now.

> This patchset in general needs to address how the extra CPU state
> involved with the PMU is going to be migrated.

I've now commented elsewhere about general ideas for migration, but I'm
wondering if there is a good way to set up unit tests for this code
because I don't trust myself to get migration right (in particular
because it's not my use case, so I won't be "naturally" testing it on my
own).

-Aaron

[Qemu-devel] [PATCH v5 09/13] 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>
---
 target/arm/helper.c | 88 ++++++++++++++++++++++++++---------------------------
 1 file changed, 43 insertions(+), 45 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f1fd21c..92ebd21 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"
 
@@ -974,8 +975,49 @@ 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
+
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 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
     { .number = SUPPORTED_EVENT_SENTINEL }
 };
 static uint16_t supported_event_map[0x3f];
@@ -1055,8 +1097,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)
@@ -1171,9 +1211,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;
@@ -1316,42 +1354,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)
 {
@@ -1664,7 +1666,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),
@@ -1684,7 +1685,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL0_RW, .accessfn = pmreg_access_ccntr,
       .type = ARM_CP_IO,
       .readfn = pmccntr_read, .writefn = pmccntr_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,
@@ -5220,7 +5220,6 @@ void register_cp_regs_for_features(ARMCPU *cpu)
          * field as main ID register, and we implement only the cycle
          * count register.
          */
-#ifndef CONFIG_USER_ONLY
         ARMCPRegInfo pmcr = {
             .name = "PMCR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 0,
             .access = PL0_RW,
@@ -5274,7 +5273,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,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 09/13] target/arm: PMU: Add instruction and cycle events

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> 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>

thanks
-- PMM

[Qemu-devel] [PATCH v5 10/13] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

This both advertises that we support four counters and adds them to the
implementation because the PMU_NUM_COUNTERS macro reads this value from
the PMCR.

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 92ebd21..3720239 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1618,7 +1618,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)
@@ -5234,7 +5234,8 @@ 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,
+            /* 4 counters enabled */
+            .resetvalue = (cpu->midr & 0xff000000) | (0x4 << PMCRN_SHIFT),
             .writefn = pmcr_write, .raw_writefn = raw_write,
         };
         define_one_arm_cp_reg(cpu, &pmcr);
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v5 11/13] 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 3720239..96667e6 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -980,6 +980,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.
@@ -1008,6 +1017,10 @@ static uint64_t instructions_get_count(CPUARMState *env)
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 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,
@@ -1318,6 +1331,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;
@@ -1663,9 +1694,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),
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [PATCH v5 12/13] target/arm: Mark PMINTENSET accesses as possibly doing IO

[Aaron Lindsay]

This makes it match its AArch64 equivalent, PMINTENSET_EL1

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

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 96667e6..38fb6a2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1763,7 +1763,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
     { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW, .accessfn = access_tpm,
-      .type = ARM_CP_ALIAS,
+      .type = ARM_CP_ALIAS | ARM_CP_IO,
       .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pminten),
       .resetvalue = 0,
       .writefn = pmintenset_write, .raw_writefn = raw_write },
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 12/13] target/arm: Mark PMINTENSET accesses as possibly doing IO

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> This makes it match its AArch64 equivalent, PMINTENSET_EL1
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 96667e6..38fb6a2 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -1763,7 +1763,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
>      { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
>        .access = PL1_RW, .accessfn = access_tpm,
> -      .type = ARM_CP_ALIAS,
> +      .type = ARM_CP_ALIAS | ARM_CP_IO,
>        .fieldoffset = offsetoflow32(CPUARMState, cp15.c9_pminten),
>        .resetvalue = 0,
>        .writefn = pmintenset_write, .raw_writefn = raw_write },

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Shouldn't PMINTENCLR and PMINTENCLR_EL1 also be ARM_CP_IO ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 12/13] target/arm: Mark PMINTENSET accesses as possibly doing IO

[Aaron Lindsay]

On Jun 28 17:25, Peter Maydell wrote:
> Shouldn't PMINTENCLR and PMINTENCLR_EL1 also be ARM_CP_IO ?

Yes, that was an oversight - I've added a patch addressing this for v6.

-Aaron

[Qemu-devel] [PATCH v5 13/13] 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 | 132 ++++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 141 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 2f5b16a..7b3c137 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -743,6 +743,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)
@@ -937,6 +943,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(QEMU_CLOCK_VIRTUAL, 1, 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 c240b38..583b008 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -720,6 +720,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 */
@@ -962,6 +964,11 @@ 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
  */
 void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 38fb6a2..0ddbcac 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -936,6 +936,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
@@ -951,6 +952,8 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
 #define PMXEVTYPER_MT         0x02000000
 #define PMXEVTYPER_EVTCOUNT   0x000003ff
 
+#define PMEVCNTR_OVERFLOW_MASK ((uint64_t)1 << 31)
+
 #define PMCCFILTR             0xf8000000
 #define PMCCFILTR_M           PMXEVTYPER_M
 #define PMCCFILTR_EL0         (PMCCFILTR | PMCCFILTR_M)
@@ -973,6 +976,11 @@ typedef struct pm_event {
     /* 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 *);
+    /* 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)
@@ -989,6 +997,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.
@@ -1004,6 +1017,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 */;
@@ -1013,22 +1031,30 @@ 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
 
 #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
 static const pm_event pm_events[] = {
     { .number = 0x000, /* SW_INCR */
       .supported = event_always_supported,
-      .get_count = swinc_get_count
+      .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
+      .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
+      .get_count = cycles_get_count,
+      .ns_per_count = cycles_ns_per
     },
 #endif
     { .number = SUPPORTED_EVENT_SENTINEL }
@@ -1216,6 +1242,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,
@@ -1233,7 +1266,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;
 }
@@ -1246,13 +1290,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;
     }
 }
@@ -1265,8 +1324,15 @@ static void pmevcntr_op_start(CPUARMState *env, uint8_t counter)
     uint64_t 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];
+        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;
 }
@@ -1274,6 +1340,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];
     }
@@ -1307,6 +1388,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)
 {
@@ -1343,7 +1437,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);
         }
     }
@@ -1414,6 +1522,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,
@@ -1421,6 +1530,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,
@@ -1428,6 +1538,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,
@@ -1435,6 +1546,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,
@@ -1560,6 +1672,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,
@@ -1567,6 +1680,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,
-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [PATCH v5 13/13] target/arm: Send interrupts on PMU counter overflow

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> 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 | 132 ++++++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 141 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2f5b16a..7b3c137 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -743,6 +743,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)
> @@ -937,6 +943,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(QEMU_CLOCK_VIRTUAL, 1, arm_pmu_timer_cb,
> +                cpu);

This is a confusing way to write timer_new_ns().

> +#endif

>  #define SUPPORTED_EVENT_SENTINEL UINT16_MAX
>  static const pm_event pm_events[] = {
>      { .number = 0x000, /* SW_INCR */
>        .supported = event_always_supported,
> -      .get_count = swinc_get_count
> +      .get_count = swinc_get_count,
> +      .ns_per_count = swinc_ns_per

If you always put trailing commas on the end of the last line in
a struct initializer, then you don't need to modify the preceding
line in later patches which add a new line to the initializer.

The meat of the patch is beyond me at this time of the
afternoon :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 00/13] More fully implement ARM PMUv3

[Peter Maydell]

On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> The ARM PMU implementation currently contains a basic cycle counter, but it is
> often useful to gather counts of other events and filter them based on
> execution mode. 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 v4 I've added improved V7VE handling with Peter's direction, fixed up a
> few patch staging issues, and fixed a bug causing cycle counter overflow to be
> checked every instruction.

Hi; I've reviewed some of the simpler parts of this, but I
suspect I'm not going to get to the rest of it for a bit (the
softfreeze deadline is next Tuesday, so it's a busy time of
the release cycle for me). For the moment I'm going to take
patches 4, 5 and 12 into target-arm.next, which should at least
reduce the size of the patchset for next time.

thanks
-- PMM