Re: [Qemu-devel] [PATCH v4 1/6] target-arm: Add CNTVOFF_EL2

[Peter Maydell]

On 5 June 2015 at 11:33, Edgar E. Iglesias <address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Adds support for the virtual timer offset controlled by EL2.
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---

> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1208,9 +1208,20 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>          /* Timer enabled: calculate and set current ISTATUS, irq, and
>           * reset timer to when ISTATUS next has to change
>           */
> +        uint64_t offset = timeridx == GTIMER_VIRT ?
> +                                      cpu->env.cp15.cntvoff_el2 : 0;
>          uint64_t count = gt_get_countervalue(&cpu->env);
> -        /* Note that this must be unsigned 64 bit arithmetic: */
> -        int istatus = count >= gt->cval;
> +        /* The ARM spec says that count, offset and gt->cval are all
> +         * unsigned 64bit values.
> +         * The event trig is described as:
> +         * (Counter[63:0] - Offset[63:0])[63:0] - CompareValue[63:0]) >= 0
> +         *
> +         * We do the subtractions as unsigned values to avoid 
> under/overflowing
> +         * signed integers (undefined behaviour in C).
> +         * To be able to do the compare >= 0 we cast the result into a
> +         * signed int64_t.
> +         */
> +        int istatus = (int64_t) (count - offset - gt->cval) >= 0;

This is wrong. Consider the case where:
 count is 0x1000,0000,0000,0002 (it's a really large unsigned number)
 offset is zero
 cval is 1

The ARM ARM required calculation gives you
  0x1000,0000,0000,0002 - 1 >= 0
ie 0x1000,0000,0000,0001 >= 0
which is true. (Note that ARM ARM pseudocode works with infinite
precision integers, not 2s-complement.)

With your code:
  (count - offset - gt->cval) is 0x1000,0000,0000,0001
  Cast to an int64_t this is negative (top bit is set)
  Comparison against 0 is done as a signed value, and returns false.

This is exactly the tricky case which is why we must do this as unsigned
arithmetic.

What you want is
    int istatus = count - offset >= gt->cval;

which comes out to
    0x1000,0000,0000,0002 >= 1
which is true.

(That's the code we had before, but just "use 'count - offset' rather than
'count'".)

> @@ -1265,17 +1281,19 @@ static void gt_cval_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
>      int timeridx = ri->crm & 1;
> +    uint64_t offset = timeridx == GTIMER_VIRT ? env->cp15.cntvoff_el2 : 0;
>
>      return (uint32_t)(env->cp15.c14_timer[timeridx].cval -
> -                      gt_get_countervalue(env));
> +                      gt_get_countervalue(env) - offset);

The docs say that the timerval read view is
   (comparevalue - (counter - offset))
not (comparevalue - counter - offset)...

>  }

Looks OK otherwise.

thanks
-- PMM