diff mbox

[v3,11/15] target-arm: Add CNTVOFF_EL2

Message ID 1432881807-18164-12-git-send-email-edgar.iglesias@gmail.com
State New
Headers show

Commit Message

Edgar E. Iglesias May 29, 2015, 6:43 a.m. UTC 
From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/cpu.h    |  1 +
 target-arm/helper.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 42 insertions(+), 6 deletions(-)

Comments

Peter Maydell June 1, 2015, 4:09 p.m. UTC | #1 
On 29 May 2015 at 07:43, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@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.

-- PMM
Edgar E. Iglesias June 2, 2015, 1:45 a.m. UTC | #2 
On Mon, Jun 01, 2015 at 05:09:29PM +0100, Peter Maydell wrote:
> On 29 May 2015 at 07:43, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@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
diff mbox

Patch

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;
         uint64_t nexttick;
 
         gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
@@ -1229,7 +1231,7 @@  static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
             nexttick = UINT64_MAX;
         } else {
             /* Next transition is when we hit cval */
-            nexttick = gt->cval;
+            nexttick = gt->cval + offset;
         }
         /* Note that the desired next expiry time might be beyond the
          * signed-64-bit range of a QEMUTimer -- in this case we just
@@ -1261,6 +1263,11 @@  static uint64_t gt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
     return gt_get_countervalue(env);
 }
 
+static uint64_t gt_virt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_get_countervalue(env) - env->cp15.cntvoff_el2;
+}
+
 static void gt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
                           uint64_t value)
 {
@@ -1273,17 +1280,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);
 }
 
 static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
                           uint64_t value)
 {
     int timeridx = ri->crm & 1;
+    uint64_t offset = timeridx == GTIMER_VIRT ? env->cp15.cntvoff_el2 : 0;
 
-    env->cp15.c14_timer[timeridx].cval = gt_get_countervalue(env) +
+    env->cp15.c14_timer[timeridx].cval = gt_get_countervalue(env) - offset +
                                          sextract64(value, 0, 32);
     gt_recalc_timer(arm_env_get_cpu(env), timeridx);
 }
@@ -1308,6 +1317,15 @@  static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }
 
+static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+
+    raw_write(env, ri, value);
+    gt_recalc_timer(cpu, GTIMER_VIRT);
+}
+
 void arm_gt_ptimer_cb(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -1417,13 +1435,13 @@  static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
     { .name = "CNTVCT", .cp = 15, .crm = 14, .opc1 = 1,
       .access = PL0_R, .type = ARM_CP_64BIT | ARM_CP_NO_RAW | ARM_CP_IO,
       .accessfn = gt_vct_access,
-      .readfn = gt_cnt_read, .resetfn = arm_cp_reset_ignore,
+      .readfn = gt_virt_cnt_read, .resetfn = arm_cp_reset_ignore,
     },
     { .name = "CNTVCT_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 2,
       .access = PL0_R, .type = ARM_CP_NO_RAW | ARM_CP_IO,
       .accessfn = gt_vct_access,
-      .readfn = gt_cnt_read, .resetfn = gt_cnt_reset,
+      .readfn = gt_virt_cnt_read, .resetfn = gt_cnt_reset,
     },
     /* Comparison value, indicating when the timer goes off */
     { .name = "CNTP_CVAL", .cp = 15, .crm = 14, .opc1 = 2,
@@ -2547,6 +2565,12 @@  static const ARMCPRegInfo v8_el3_no_el2_cp_reginfo[] = {
     { .name = "HTTBR", .cp = 15, .crm = 2, .opc1 = 4,
       .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST,
       .resetvalue = 0 },
+    { .name = "CNTVOFF_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 0, .opc2 = 3,
+      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "CNTVOFF", .cp = 15, .opc1 = 4, .crm = 14,
+      .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST,
+      .resetvalue = 0 },
     REGINFO_SENTINEL
 };
 
@@ -2659,6 +2683,17 @@  static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
       .opc0 = 1, .opc1 = 4, .crn = 8, .crm = 3, .opc2 = 1,
       .type = ARM_CP_NO_RAW, .access = PL2_W,
       .writefn = tlbi_aa64_vaa_write },
+#ifndef CONFIG_USER_ONLY
+    { .name = "CNTVOFF_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 0, .opc2 = 3,
+      .access = PL2_RW, .type = ARM_CP_IO, .resetvalue = 0,
+      .writefn = gt_cntvoff_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.cntvoff_el2) },
+    { .name = "CNTVOFF", .cp = 15, .opc1 = 4, .crm = 14,
+      .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_ALIAS | ARM_CP_IO,
+      .writefn = gt_cntvoff_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.cntvoff_el2) },
+#endif
     REGINFO_SENTINEL
 };