On Nov 30 10:19, Richard Henderson wrote:
> On 11/30/18 9:56 AM, Aaron Lindsay wrote:
> > On Nov 30 09:13, Richard Henderson wrote:
> >> On 11/20/18 1:26 PM, 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 <alind...@os.amperecomputing.com>
> >>> ---
> >>> target/arm/cpu.c | 12 +++++
> >>> target/arm/cpu.h | 7 +++
> >>> target/arm/helper.c | 126 +++++++++++++++++++++++++++++++++++++++++---
> >>> 3 files changed, 139 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> >>> index 208a08e867..7311a48e3c 100644
> >>> --- a/target/arm/cpu.c
> >>> +++ b/target/arm/cpu.c
> >>> @@ -827,6 +827,13 @@ 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) && cpu->pmu_timer) {
> >>
> >> No need for two tests here. Just check cpu->pmu_timer.
> >> (If it's set for any reason it should be freed, surely.)
> >>
> >>> @@ -1305,7 +1338,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)) {
> >>
> >> Fyi, this expression is
> >>
> >> env->cp15.c15_ccnt & ~new_pmccntr & overflow_mask
> >>
> >>> + env->cp15.c9_pmovsr |= (1 << 31);
> >>> + new_pmccntr &= ~overflow_mask;
> >>
> >> Why this line? You just checked that overflow_mask was unset in
> >> new_pmccntr above.
> >
> > This ensures that when overflow_bit == 31 (because PMCR.LC is not set)
> > the high 32 bits remain 0 even after an overflow has occurred. As you
> > point out, it's silly when overflow_bit == 64, but I didn't think it was
> > worth the extra conditional to avoid it.
>
> Eh? But we've set overflow_mask based on PMCR.LC, so what you say here
> doesn't
> make sense.
Sorry, I had an off-by-one-bit think-o I couldn't get past until I
started typing a concrete example to explain myself. I'll change this
line to be:
if (!(env->cp15.c9_pmcr & PMCRLC))
new_pmccntr &= 0xffffffff;
-Aaron
