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