On 5 June 2015 at 11:33, Edgar E. Iglesias <edgar.igles...@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com>
>
> Adds support for the virtual timer offset controlled by EL2.
>
> Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
> ---
> --- 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
