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

[Edgar E. Iglesias]

On Fri, Jun 12, 2015 at 05:44:24PM +0100, Peter Maydell wrote:
> 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'".)

Thanks, I've changed it to what you suggest allthough I'm probably missing
something cause I'm still finding the spec confusing :S


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

Fixed for next version.

Cheers,
Edgar


> 
> >  }
> 
> Looks OK otherwise.
> 
> thanks
> -- PMM