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