[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/6] target-arm: Add CNTVOFF_EL2
From: |
Edgar E. Iglesias |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/6] target-arm: Add CNTVOFF_EL2 |
Date: |
Mon, 15 Jun 2015 10:52:00 +1000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
[Qemu-devel] [PATCH v4 3/6] target-arm: Pass timeridx as argument to various timer functions, Edgar E. Iglesias, 2015/06/05
[Qemu-devel] [PATCH v4 4/6] target-arm: Add the Hypervisor timer, Edgar E. Iglesias, 2015/06/05