Re: [Qemu-devel] [PATCH v3 11/15] target-arm: Add CNTVOFF_EL2

[Edgar E. Iglesias]

On Mon, Jun 01, 2015 at 05:09:29PM +0100, Peter Maydell wrote:
> On 29 May 2015 at 07:43, Edgar E. Iglesias <address@hidden> wrote:
> > From: "Edgar E. Iglesias" <address@hidden>
> >
> > Signed-off-by: Edgar E. Iglesias <address@hidden>
> > ---
> >  target-arm/cpu.h    |  1 +
> >  target-arm/helper.c | 47 +++++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 42 insertions(+), 6 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 21b5b8e..1a66aa4 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -355,6 +355,7 @@ typedef struct CPUARMState {
> >          };
> >          uint64_t c14_cntfrq; /* Counter Frequency register */
> >          uint64_t c14_cntkctl; /* Timer Control register */
> > +        uint64_t cntvoff_el2; /* Counter Virtual Offset register */
> >          ARMGenericTimer c14_timer[NUM_GTIMERS];
> >          uint32_t c15_cpar; /* XScale Coprocessor Access Register */
> >          uint32_t c15_ticonfig; /* TI925T configuration byte.  */
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index a5c0363..f5579fc 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -1216,9 +1216,11 @@ 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;
> > +        int istatus = (int64_t) (count - offset - gt->cval) >= 0;
> 
> The comment says "must be unsigned" and your change is adding
> a cast to force signed comparison -- one of them must be wrong.
> 
> I'm going to apply patches 1..10 to target-arm.next; this is
> where I ran out of time to review.

Thanks Peter,

The manual (Operation of the CompareValue views of the timers) says:

EventTriggered = (((Counter[63:0] – Offset[63:0])[63:0] - CompareValue[63:0]) 
>= 0)

It also says:
In this view of a timer, Counter , Offset , and CompareValue are all 64-bit 
unsigned values.

My interpretation is that the arithmetics are done unsigned but the compare (>= 
0) has to be signed (if not it is always true).
Does that make sense?

I can modify the comment to make that clear.

Thanks,
Edgar