On 12/12/18 2:20 AM, Aaron Lindsay 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 <alind...@codeaurora.org>
> Signed-off-by: Aaron Lindsay <aa...@os.amperecomputing.com>
> ---
> target/arm/cpu.c | 12 +++++
> target/arm/cpu.h | 8 +++
> target/arm/helper.c | 126 +++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 140 insertions(+), 6 deletions(-)
Well, this patch is doing several things at once -- adding the timer, adding
the ns_per_count hook, updating irqs. Not ideal, but I won't insist it be
split.
You'll need to re-run against scripts/checkpatch, it would seem.
The goal-posts with respect to comments have been changed since
you started this.
> @@ -1305,7 +1338,19 @@ 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;
Could just as easily be
uint64_t overflow_mask = env->cp15.c9_pmcr & PMCRLC ? INT64_MIN : INT32_MIN;
> + if (env->cp15.c15_ccnt & ~new_pmccntr & overflow_mask) {
> + env->cp15.c9_pmovsr |= (1 << 31);
> + if (!(env->cp15.c9_pmcr & PMCRLC)) {
> + new_pmccntr &= 0xffffffff;
> + }
Why is this truncation buried within the overflow condition? Simply because
the high bits can't be set without overflow being noticed? That could use a
comment, because it looks odd.
> @@ -1340,8 +1399,15 @@ static void pmevcntr_op_start(CPUARMState *env,
> uint8_t counter)
> }
>
> if (pmu_counter_enabled(env, counter)) {
> - env->cp15.c14_pmevcntr[counter] =
> - count - env->cp15.c14_pmevcntr_delta[counter];
> + uint64_t new_pmevcntr = count -
> env->cp15.c14_pmevcntr_delta[counter];
> +
> + if (!(new_pmevcntr & PMEVCNTR_OVERFLOW_MASK) &&
> + (env->cp15.c14_pmevcntr[counter] & PMEVCNTR_OVERFLOW_MASK)) {
> + env->cp15.c9_pmovsr |= (1 << counter);
> + new_pmevcntr &= ~PMEVCNTR_OVERFLOW_MASK;
That, surely, does not do what you intend. I can only imagine that you meant
new_pmevcntr = (uint32_t)new_pmevcntr;
or
new_pmevcntr &= PMEVCNTR_OVERFLOW_MASK - 1;
depending on how much you want to depend on the symbol defining the width.
Given that it is architecturally defined to 32-bits, I think you could really
just drop the define and use
uint32_t new_pmevcntr = ...;
if (env->cp15.c14_pmevcntr[counter] & ~new_pmevcntr & INT32_MIN)
with equal clarity. The type of new_pmevcntr means you don't have to clear any
high bits either.
> + /* 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;
Likewise.
r~
