[Qemu-devel] [PATCH] target/arm: Don't clear supported PMU events when initializing PMCEID1

[Qemu-devel] [PATCH] target/arm: Don't clear supported PMU events when initializing PMCEID1

[Aaron Lindsay OS]

A bug was introduced during a respin of:

	commit 57a4a11b2b281bb548b419ca81bfafb214e4c77a
	target/arm: Add array for supported PMU events, generate PMCEID[01]_EL0

This patch introduced two calls to get_pmceid() during CPU
initialization - one each for PMCEID0 and PMCEID1. In addition to
building the register values, get_pmceid() clears an internal array
mapping event numbers to their implementations (supported_event_map)
before rebuilding it. This is an optimization since much of the logic is
shared. However, since it was called twice, the contents of
supported_event_map reflect only the events in PMCEID1 (the second call
to get_pmceid()).

Fix this bug by moving the initialization of PMCEID0 and PMCEID1 back
into a single function call, and name it more appropriately since it is
doing more than simply generating the contents of the PMCEID[01]
registers.

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/cpu.c    |  3 +--
 target/arm/cpu.h    | 11 +++++------
 target/arm/helper.c | 27 ++++++++++++++++-----------
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7e1f3dd637..d6da3f4fed 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1039,8 +1039,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         unset_feature(env, ARM_FEATURE_PMU);
     }
     if (arm_feature(env, ARM_FEATURE_PMU)) {
-        cpu->pmceid0 = get_pmceid(&cpu->env, 0);
-        cpu->pmceid1 = get_pmceid(&cpu->env, 1);
+        pmu_init(cpu);
 
         if (!kvm_enabled()) {
             arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index ff81db420d..b8161cb6d7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1012,14 +1012,13 @@ void pmu_pre_el_change(ARMCPU *cpu, void *ignored);
 void pmu_post_el_change(ARMCPU *cpu, void *ignored);
 
 /*
- * get_pmceid
- * @env: CPUARMState
- * @which: which PMCEID register to return (0 or 1)
+ * pmu_init
+ * @cpu: ARMCPU
  *
- * Return the PMCEID[01]_EL0 register values corresponding to the counters
- * which are supported given the current configuration
+ * Initialize the CPU's PMCEID[01]_EL0 registers and associated internal state
+ * for the current configuration
  */
-uint64_t get_pmceid(CPUARMState *env, unsigned which);
+void pmu_init(ARMCPU *cpu);
 
 /* 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 92666e5208..31273fb8de 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1090,22 +1090,24 @@ static const pm_event pm_events[] = {
 static uint16_t supported_event_map[MAX_EVENT_ID + 1];
 
 /*
- * Called upon initialization to build PMCEID0_EL0 or PMCEID1_EL0 (indicated by
- * 'which'). We also use it to build a map of ARM event numbers to indices in
- * our pm_events array.
+ * Called upon CPU initialization to initialize PMCEID[01]_EL0 and build a map
+ * of ARM event numbers to indices in our pm_events array.
  *
  * Note: Events in the 0x40XX range are not currently supported.
  */
-uint64_t get_pmceid(CPUARMState *env, unsigned which)
+void pmu_init(ARMCPU *cpu)
 {
-    uint64_t pmceid = 0;
     unsigned int i;
 
-    assert(which <= 1);
-
+    /*
+     * Empty supported_event_map and cpu->pmceid[01] before adding supported
+     * events to them
+     */
     for (i = 0; i < ARRAY_SIZE(supported_event_map); i++) {
         supported_event_map[i] = UNSUPPORTED_EVENT;
     }
+    cpu->pmceid0 = 0;
+    cpu->pmceid1 = 0;
 
     for (i = 0; i < ARRAY_SIZE(pm_events); i++) {
         const pm_event *cnt = &pm_events[i];
@@ -1113,13 +1115,16 @@ uint64_t get_pmceid(CPUARMState *env, unsigned which)
         /* We do not currently support events in the 0x40xx range */
         assert(cnt->number <= 0x3f);
 
-        if ((cnt->number & 0x20) == (which << 6) &&
-                cnt->supported(env)) {
-            pmceid |= (1 << (cnt->number & 0x1f));
+        if (cnt->supported(&cpu->env)) {
             supported_event_map[cnt->number] = i;
+            uint64_t event_mask = 1 << (cnt->number & 0x1f);
+            if (cnt->number & 0x20) {
+                cpu->pmceid1 |= event_mask;
+            } else {
+                cpu->pmceid0 |= event_mask;
+            }
         }
     }
-    return pmceid;
 }
 
 /*
-- 
2.20.1

Re: [Qemu-devel] [PATCH] target/arm: Don't clear supported PMU events when initializing PMCEID1

[Richard Henderson]

On 1/23/19 11:59 AM, Aaron Lindsay OS wrote:
> A bug was introduced during a respin of:
> 
> 	commit 57a4a11b2b281bb548b419ca81bfafb214e4c77a
> 	target/arm: Add array for supported PMU events, generate PMCEID[01]_EL0
> 
> This patch introduced two calls to get_pmceid() during CPU
> initialization - one each for PMCEID0 and PMCEID1. In addition to
> building the register values, get_pmceid() clears an internal array
> mapping event numbers to their implementations (supported_event_map)
> before rebuilding it. This is an optimization since much of the logic is
> shared. However, since it was called twice, the contents of
> supported_event_map reflect only the events in PMCEID1 (the second call
> to get_pmceid()).
> 
> Fix this bug by moving the initialization of PMCEID0 and PMCEID1 back
> into a single function call, and name it more appropriately since it is
> doing more than simply generating the contents of the PMCEID[01]
> registers.
> 
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/cpu.c    |  3 +--
>  target/arm/cpu.h    | 11 +++++------
>  target/arm/helper.c | 27 ++++++++++++++++-----------
>  3 files changed, 22 insertions(+), 19 deletions(-)


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


r~

Re: [Qemu-devel] [PATCH] target/arm: Don't clear supported PMU events when initializing PMCEID1

[Peter Maydell]

On Wed, 23 Jan 2019 at 19:59, Aaron Lindsay OS
<aaron@os.amperecomputing.com> wrote:
>
> A bug was introduced during a respin of:
>
>         commit 57a4a11b2b281bb548b419ca81bfafb214e4c77a
>         target/arm: Add array for supported PMU events, generate PMCEID[01]_EL0
>
> This patch introduced two calls to get_pmceid() during CPU
> initialization - one each for PMCEID0 and PMCEID1. In addition to
> building the register values, get_pmceid() clears an internal array
> mapping event numbers to their implementations (supported_event_map)
> before rebuilding it. This is an optimization since much of the logic is
> shared. However, since it was called twice, the contents of
> supported_event_map reflect only the events in PMCEID1 (the second call
> to get_pmceid()).
>
> Fix this bug by moving the initialization of PMCEID0 and PMCEID1 back
> into a single function call, and name it more appropriately since it is
> doing more than simply generating the contents of the PMCEID[01]
> registers.
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>



Applied to target-arm.next, thanks.

-- PMM