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

[Peter Maydell]

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.

-- PMM