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

[Aaron Lindsay <aaron@os.amperecomputing.com>]

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 <alindsay@codeaurora.org>
> >>> Signed-off-by: Aaron Lindsay <alindsay@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