Re: [Qemu-arm] [PATCH v10 14/14] target/arm: Send interrupts on PMU counter overflow

[Aaron Lindsay]

On Jan 18 07:26, Richard Henderson wrote:
> 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 <address@hidden>
> > Signed-off-by: Aaron Lindsay <address@hidden>
> > ---
> >  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.

Okay, I'll check that again before I send the next version out.

> > @@ -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;

Updated.

> > +        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.

Upon re-reading the spec, I don't think this is needed (or even correct
behavior). I must've been thinking that PMCR.LC == 0 implied that upper
32 bits could never be updated by the hardware and made PMCCNTR act like
its high bits didn't even exist, like one of the PMXEVCNTRs. I no longer
believe that is true and I'll remove this.

> > @@ -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.

In practice, I think only the 32nd bit would ever need to be cleared,
but I agree it is more correct to clear them all.

> 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.

I don't know whether it is important for the resolution of this patch,
but what did you mean by the following?:

> 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.

Thanks,

Aaron