qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Richard Henderson
Subject: Re: [Qemu-devel] [PATCH v10 14/14] target/arm: Send interrupts on PMU counter overflow
Date: Fri, 18 Jan 2019 07:26:11 +1100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0

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.


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]