| Message ID | 20190123195814.29253-1-aaron@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Don't clear supported PMU events when initializing PMCEID1 | expand |
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~
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
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; } /*
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(-)