On Mon, Jun 01, 2015 at 05:09:29PM +0100, Peter Maydell wrote:
> On 29 May 2015 at 07:43, Edgar E. Iglesias <edgar.igles...@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.igles...@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
> > ---
> > 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
