[Qemu-devel] [PATCH v5 0/6] arm: Steps towards EL2 support round 3

[Qemu-devel] [PATCH v5 0/6] arm: Steps towards EL2 support round 3

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

This is what is left of round 3 of our series towards support for EL2 for
AArch64.

Comments welcome!

Best regards,
Edgar

v4 -> v5:
* Fix timer compare logic
* Fix cval reads
* Fix CNTHCTL exception priority
* Comment on CNTHCTL reset value
* Added AArch32 HYP timer regs
* Reorder HYP timer regs
* Use CP_CONST for RES0 HYP timer regs (EL3 but no EL2)

v3 -> v4:
* Add comment clarifing the unsigned/signed timer hit arithmetics
* Replace GIC magic constants with macros
* Slightly expanded commit messages

v2 -> v3:
* Use CP_ACCESS_TRAP_EL2 instead of setting target_el

v1 -> v2:
* Drop PAR_EL1
* Add AArch32 mappings of MAIR_EL2
* Add AArch32 mappings of TCR_EL2
* Add AArch32 mappings of SCTLR_EL2
* Add AArch32 mappings of TTBR0_EL2
* Add AArch32 mappings of TPIDR_EL2
* Add AArch32 mappings of CNTHCTL_EL2
* Add AArch32 mappings of CNTVOFF_EL2
* Tag CNTVOFF_EL2 and CNTVOFF as ARM_CP_IO
* Rename TLIBALLE2 -> TLBI_ALLE2
* Break down TLB_LOCKDOWN to v7 level

Edgar E. Iglesias (6):
  target-arm: Add CNTVOFF_EL2
  target-arm: Add CNTHCTL_EL2
  target-arm: Pass timeridx as argument to various timer functions
  target-arm: Add the Hypervisor timer
  hw/arm/virt: Replace magic IRQ constants with macros
  hw/arm/virt: Connect the Hypervisor timer

 hw/arm/virt.c        |  13 ++-
 target-arm/cpu-qom.h |   1 +
 target-arm/cpu.c     |   2 +
 target-arm/cpu.h     |   5 +-
 target-arm/helper.c  | 245 +++++++++++++++++++++++++++++++++++++++++++++------
 5 files changed, 233 insertions(+), 33 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v5 1/6] target-arm: Add CNTVOFF_EL2

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Adds support for the virtual timer offset controlled by EL2.

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 3da0c05..41cfad8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1208,9 +1208,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 = count - offset >= gt->cval;
         uint64_t nexttick;
 
         gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
@@ -1221,7 +1223,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
@@ -1253,6 +1255,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)
 {
@@ -1265,17 +1272,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);
 }
@@ -1300,6 +1309,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;
@@ -1409,13 +1427,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,
@@ -2539,6 +2557,12 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
     { .name = "HTTBR", .cp = 15, .opc1 = 4, .crm = 2,
       .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
 };
 
@@ -2651,6 +2675,17 @@ static const ARMCPRegInfo 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
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH v5 2/6] target-arm: Add CNTHCTL_EL2

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Adds control for trapping selected timer and counter accesses to EL2.

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

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 1a66aa4..f39c32b 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 */
+        uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
         uint64_t cntvoff_el2; /* Counter Virtual Offset register */
         ARMGenericTimer c14_timer[NUM_GTIMERS];
         uint32_t c15_cpar; /* XScale Coprocessor Access Register */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 41cfad8..282f9fb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1153,23 +1153,42 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
 
 static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
 {
+    unsigned int cur_el = arm_current_el(env);
+    bool secure = arm_is_secure(env);
+
     /* CNT[PV]CT: not visible from PL0 if ELO[PV]CTEN is zero */
-    if (arm_current_el(env) == 0 &&
+    if (cur_el == 0 &&
         !extract32(env->cp15.c14_cntkctl, timeridx, 1)) {
         return CP_ACCESS_TRAP;
     }
+
+    if (arm_feature(env, ARM_FEATURE_EL2) &&
+        timeridx == GTIMER_PHYS && !secure && cur_el < 2 &&
+        !extract32(env->cp15.cnthctl_el2, 0, 1)) {
+        return CP_ACCESS_TRAP_EL2;
+    }
     return CP_ACCESS_OK;
 }
 
 static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
 {
+    unsigned int cur_el = arm_current_el(env);
+    bool secure = arm_is_secure(env);
+
     /* CNT[PV]_CVAL, CNT[PV]_CTL, CNT[PV]_TVAL: not visible from PL0 if
      * EL0[PV]TEN is zero.
      */
-    if (arm_current_el(env) == 0 &&
+    if (cur_el == 0 &&
         !extract32(env->cp15.c14_cntkctl, 9 - timeridx, 1)) {
         return CP_ACCESS_TRAP;
     }
+
+    if (arm_feature(env, ARM_FEATURE_EL2)) {
+        if (timeridx == GTIMER_PHYS && !secure && cur_el < 2 &&
+            !extract32(env->cp15.cnthctl_el2, 1, 1)) {
+            return CP_ACCESS_TRAP_EL2;
+        }
+    }
     return CP_ACCESS_OK;
 }
 
@@ -2557,6 +2576,9 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
     { .name = "HTTBR", .cp = 15, .opc1 = 4, .crm = 2,
       .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST,
       .resetvalue = 0 },
+    { .name = "CNTHCTL_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 1, .opc2 = 0,
+      .access = PL2_RW, .type = 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 },
@@ -2676,6 +2698,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .type = ARM_CP_NO_RAW, .access = PL2_W,
       .writefn = tlbi_aa64_vaa_write },
 #ifndef CONFIG_USER_ONLY
+    { .name = "CNTHCTL_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 1, .opc2 = 0,
+      /* ARMv7 requires bit 0 and 1 to reset to 1. ARMv8 defines the
+       * reset values as IMPDEF. We chose to reset to 3 to comply with
+       * both ARMv7 and ARMv8.
+       */
+      .access = PL2_RW, .resetvalue = 3,
+      .fieldoffset = offsetof(CPUARMState, cp15.cnthctl_el2) },
     { .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,
-- 
1.9.1

[Qemu-devel] [PATCH v5 3/6] target-arm: Pass timeridx as argument to various timer functions

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Prepare for adding the Hypervisor timer, no functional change.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c | 98 +++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 76 insertions(+), 22 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 282f9fb..92dbb28 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1261,10 +1261,9 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
     }
 }
 
-static void gt_cnt_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+static void gt_cnt_reset(CPUARMState *env, const ARMCPRegInfo *ri, int timeridx)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
-    int timeridx = ri->opc1 & 1;
 
     timer_del(cpu->gt_timer[timeridx]);
 }
@@ -1280,17 +1279,16 @@ static uint64_t gt_virt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri)
 }
 
 static void gt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                          int timeridx,
                           uint64_t value)
 {
-    int timeridx = ri->opc1 & 1;
-
     env->cp15.c14_timer[timeridx].cval = value;
     gt_recalc_timer(arm_env_get_cpu(env), timeridx);
 }
 
-static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                             int timeridx)
 {
-    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 -
@@ -1298,9 +1296,9 @@ static uint64_t gt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
 }
 
 static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                          int timeridx,
                           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) - offset +
@@ -1309,10 +1307,10 @@ static void gt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 
 static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                         int timeridx,
                          uint64_t value)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
-    int timeridx = ri->crm & 1;
     uint32_t oldval = env->cp15.c14_timer[timeridx].ctl;
 
     env->cp15.c14_timer[timeridx].ctl = deposit64(oldval, 0, 2, value);
@@ -1328,6 +1326,62 @@ static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 }
 
+static void gt_phys_cnt_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    gt_cnt_reset(env, ri, GTIMER_PHYS);
+}
+
+static void gt_phys_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                               uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_PHYS, value);
+}
+
+static uint64_t gt_phys_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_PHYS);
+}
+
+static void gt_phys_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                               uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_PHYS, value);
+}
+
+static void gt_phys_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_PHYS, value);
+}
+
+static void gt_virt_cnt_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    gt_cnt_reset(env, ri, GTIMER_VIRT);
+}
+
+static void gt_virt_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                               uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_VIRT, value);
+}
+
+static uint64_t gt_virt_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_VIRT);
+}
+
+static void gt_virt_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                               uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_VIRT, value);
+}
+
+static void gt_virt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_VIRT, value);
+}
+
 static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri,
                               uint64_t value)
 {
@@ -1382,7 +1436,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .fieldoffset = offsetoflow32(CPUARMState,
                                    cp15.c14_timer[GTIMER_PHYS].ctl),
       .resetfn = arm_cp_reset_ignore,
-      .writefn = gt_ctl_write, .raw_writefn = raw_write,
+      .writefn = gt_phys_ctl_write, .raw_writefn = raw_write,
     },
     { .name = "CNTP_CTL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 2, .opc2 = 1,
@@ -1390,7 +1444,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .accessfn = gt_ptimer_access,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_PHYS].ctl),
       .resetvalue = 0,
-      .writefn = gt_ctl_write, .raw_writefn = raw_write,
+      .writefn = gt_phys_ctl_write, .raw_writefn = raw_write,
     },
     { .name = "CNTV_CTL", .cp = 15, .crn = 14, .crm = 3, .opc1 = 0, .opc2 = 1,
       .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
@@ -1398,7 +1452,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .fieldoffset = offsetoflow32(CPUARMState,
                                    cp15.c14_timer[GTIMER_VIRT].ctl),
       .resetfn = arm_cp_reset_ignore,
-      .writefn = gt_ctl_write, .raw_writefn = raw_write,
+      .writefn = gt_virt_ctl_write, .raw_writefn = raw_write,
     },
     { .name = "CNTV_CTL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 1,
@@ -1406,30 +1460,30 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .accessfn = gt_vtimer_access,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_VIRT].ctl),
       .resetvalue = 0,
-      .writefn = gt_ctl_write, .raw_writefn = raw_write,
+      .writefn = gt_virt_ctl_write, .raw_writefn = raw_write,
     },
     /* TimerValue views: a 32 bit downcounting view of the underlying state */
     { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0,
       .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
       .accessfn = gt_ptimer_access,
-      .readfn = gt_tval_read, .writefn = gt_tval_write,
+      .readfn = gt_phys_tval_read, .writefn = gt_phys_tval_write,
     },
     { .name = "CNTP_TVAL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 2, .opc2 = 0,
       .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
       .accessfn = gt_ptimer_access,
-      .readfn = gt_tval_read, .writefn = gt_tval_write,
+      .readfn = gt_phys_tval_read, .writefn = gt_phys_tval_write,
     },
     { .name = "CNTV_TVAL", .cp = 15, .crn = 14, .crm = 3, .opc1 = 0, .opc2 = 0,
       .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
       .accessfn = gt_vtimer_access,
-      .readfn = gt_tval_read, .writefn = gt_tval_write,
+      .readfn = gt_virt_tval_read, .writefn = gt_virt_tval_write,
     },
     { .name = "CNTV_TVAL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 0,
       .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
       .accessfn = gt_vtimer_access,
-      .readfn = gt_tval_read, .writefn = gt_tval_write,
+      .readfn = gt_virt_tval_read, .writefn = gt_virt_tval_write,
     },
     /* The counter itself */
     { .name = "CNTPCT", .cp = 15, .crm = 14, .opc1 = 0,
@@ -1441,7 +1495,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 1,
       .access = PL0_R, .type = ARM_CP_NO_RAW | ARM_CP_IO,
       .accessfn = gt_pct_access,
-      .readfn = gt_cnt_read, .resetfn = gt_cnt_reset,
+      .readfn = gt_cnt_read, .resetfn = gt_phys_cnt_reset,
     },
     { .name = "CNTVCT", .cp = 15, .crm = 14, .opc1 = 1,
       .access = PL0_R, .type = ARM_CP_64BIT | ARM_CP_NO_RAW | ARM_CP_IO,
@@ -1452,7 +1506,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .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_virt_cnt_read, .resetfn = gt_cnt_reset,
+      .readfn = gt_virt_cnt_read, .resetfn = gt_virt_cnt_reset,
     },
     /* Comparison value, indicating when the timer goes off */
     { .name = "CNTP_CVAL", .cp = 15, .crm = 14, .opc1 = 2,
@@ -1460,7 +1514,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_PHYS].cval),
       .accessfn = gt_ptimer_access, .resetfn = arm_cp_reset_ignore,
-      .writefn = gt_cval_write, .raw_writefn = raw_write,
+      .writefn = gt_phys_cval_write, .raw_writefn = raw_write,
     },
     { .name = "CNTP_CVAL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 2, .opc2 = 2,
@@ -1468,14 +1522,14 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .type = ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_PHYS].cval),
       .resetvalue = 0, .accessfn = gt_ptimer_access,
-      .writefn = gt_cval_write, .raw_writefn = raw_write,
+      .writefn = gt_phys_cval_write, .raw_writefn = raw_write,
     },
     { .name = "CNTV_CVAL", .cp = 15, .crm = 14, .opc1 = 3,
       .access = PL1_RW | PL0_R,
       .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_VIRT].cval),
       .accessfn = gt_vtimer_access, .resetfn = arm_cp_reset_ignore,
-      .writefn = gt_cval_write, .raw_writefn = raw_write,
+      .writefn = gt_virt_cval_write, .raw_writefn = raw_write,
     },
     { .name = "CNTV_CVAL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 2,
@@ -1483,7 +1537,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .type = ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_VIRT].cval),
       .resetvalue = 0, .accessfn = gt_vtimer_access,
-      .writefn = gt_cval_write, .raw_writefn = raw_write,
+      .writefn = gt_virt_cval_write, .raw_writefn = raw_write,
     },
     REGINFO_SENTINEL
 };
-- 
1.9.1

[Qemu-devel] [PATCH v5 4/6] target-arm: Add the Hypervisor timer

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/cpu-qom.h |  1 +
 target-arm/cpu.c     |  2 ++
 target-arm/cpu.h     |  3 ++-
 target-arm/helper.c  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index ed5a644..3aaa7b6 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -214,6 +214,7 @@ int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 /* Callback functions for the generic timer's timers. */
 void arm_gt_ptimer_cb(void *opaque);
 void arm_gt_vtimer_cb(void *opaque);
+void arm_gt_htimer_cb(void *opaque);
 
 #ifdef TARGET_AARCH64
 int aarch64_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 4a888ab..b631482 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -409,6 +409,8 @@ static void arm_cpu_initfn(Object *obj)
                                                 arm_gt_ptimer_cb, cpu);
     cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
                                                 arm_gt_vtimer_cb, cpu);
+    cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
+                                                arm_gt_htimer_cb, cpu);
     qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs,
                        ARRAY_SIZE(cpu->gt_timer_outputs));
 #endif
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index f39c32b..dfa9d77 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -113,7 +113,8 @@ typedef struct ARMGenericTimer {
 
 #define GTIMER_PHYS 0
 #define GTIMER_VIRT 1
-#define NUM_GTIMERS 2
+#define GTIMER_HYP  2
+#define NUM_GTIMERS 3
 
 typedef struct {
     uint64_t raw_tcr;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 92dbb28..32df2f5 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1391,6 +1391,34 @@ static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri,
     gt_recalc_timer(cpu, GTIMER_VIRT);
 }
 
+static void gt_hyp_cnt_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    gt_cnt_reset(env, ri, GTIMER_HYP);
+}
+
+static void gt_hyp_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_HYP, value);
+}
+
+static uint64_t gt_hyp_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_HYP);
+}
+
+static void gt_hyp_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_HYP, value);
+}
+
+static void gt_hyp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_HYP, value);
+}
+
 void arm_gt_ptimer_cb(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -1405,6 +1433,13 @@ void arm_gt_vtimer_cb(void *opaque)
     gt_recalc_timer(cpu, GTIMER_VIRT);
 }
 
+void arm_gt_htimer_cb(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    gt_recalc_timer(cpu, GTIMER_HYP);
+}
+
 static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
     /* Note that CNTFRQ is purely reads-as-written for the benefit
      * of software; writing it doesn't actually change the timer frequency.
@@ -2639,6 +2674,18 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
     { .name = "CNTVOFF", .cp = 15, .opc1 = 4, .crm = 14,
       .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST,
       .resetvalue = 0 },
+    { .name = "CNTHP_CVAL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 2,
+      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "CNTHP_CVAL", .cp = 15, .opc1 = 6, .crm = 14,
+      .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST,
+      .resetvalue = 0 },
+    { .name = "CNTHP_TVAL_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 0,
+      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
+      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
     REGINFO_SENTINEL
 };
 
@@ -2769,6 +2816,27 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
       .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_ALIAS | ARM_CP_IO,
       .writefn = gt_cntvoff_write,
       .fieldoffset = offsetof(CPUARMState, cp15.cntvoff_el2) },
+    { .name = "CNTHP_CVAL_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 2,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_HYP].cval),
+      .type = ARM_CP_IO, .access = PL2_RW,
+      .writefn = gt_hyp_cval_write, .raw_writefn = raw_write },
+    { .name = "CNTHP_CVAL", .cp = 15, .opc1 = 6, .crm = 14,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_HYP].cval),
+      .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_IO,
+      .writefn = gt_hyp_cval_write, .raw_writefn = raw_write },
+    { .name = "CNTHP_TVAL_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 0,
+      .type = ARM_CP_IO, .access = PL2_RW,
+      .resetfn = gt_hyp_cnt_reset,
+      .readfn = gt_hyp_tval_read, .writefn = gt_hyp_tval_write },
+    { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
+      .type = ARM_CP_IO,
+      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
+      .access = PL2_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_HYP].ctl),
+      .resetvalue = 0,
+      .writefn = gt_hyp_ctl_write, .raw_writefn = raw_write },
 #endif
     REGINFO_SENTINEL
 };
-- 
1.9.1

[Qemu-devel] [PATCH v5 5/6] hw/arm/virt: Replace magic IRQ constants with macros

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Replace magic constants with macros from
hw/arm/virt.h and hw/intc/arm_gic_common.h.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/virt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1b1cc71..f822ea0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -47,6 +47,7 @@
 #include "hw/arm/virt-acpi-build.h"
 #include "hw/arm/sysbus-fdt.h"
 #include "hw/platform-bus.h"
+#include "hw/intc/arm_gic_common.h"
 
 /* Number of external interrupt lines to configure the GIC with */
 #define NUM_IRQS 256
@@ -395,15 +396,17 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
      */
     for (i = 0; i < smp_cpus; i++) {
         DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
-        int ppibase = NUM_IRQS + i * 32;
+        int ppibase = NUM_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS;
         /* physical timer; we wire it up to the non-secure timer's ID,
          * since a real A15 always has TrustZone but QEMU doesn't.
          */
         qdev_connect_gpio_out(cpudev, 0,
-                              qdev_get_gpio_in(gicdev, ppibase + 30));
+                              qdev_get_gpio_in(gicdev,
+                                             ppibase + ARCH_TIMER_NS_EL1_IRQ));
         /* virtual timer */
         qdev_connect_gpio_out(cpudev, 1,
-                              qdev_get_gpio_in(gicdev, ppibase + 27));
+                              qdev_get_gpio_in(gicdev,
+                                               ppibase + ARCH_TIMER_VIRT_IRQ));
 
         sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
         sysbus_connect_irq(gicbusdev, i + smp_cpus,
-- 
1.9.1

[Qemu-devel] [PATCH v5 6/6] hw/arm/virt: Connect the Hypervisor timer

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/arm/virt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f822ea0..76f4611 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -407,6 +407,10 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
         qdev_connect_gpio_out(cpudev, 1,
                               qdev_get_gpio_in(gicdev,
                                                ppibase + ARCH_TIMER_VIRT_IRQ));
+        /* Hypervisor timer.  */
+        qdev_connect_gpio_out(cpudev, 2,
+                              qdev_get_gpio_in(gicdev,
+                                             ppibase + ARCH_TIMER_NS_EL2_IRQ));
 
         sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
         sysbus_connect_irq(gicbusdev, i + smp_cpus,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v5 0/6] arm: Steps towards EL2 support round 3

[Peter Maydell]

On 16 June 2015 at 02:51, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Hi,
>
> This is what is left of round 3 of our series towards support for EL2 for
> AArch64.
>
> Comments welcome!

Given that we're now in softfreeze, and these patches don't actually
enable a new feature by themselves, my current thought is that we
should postpone them until after 2.4, unless they'd be particularly
inconvenient for you to carry out of tree until then. Thoughts?

-- PMM

Re: [Qemu-devel] [PATCH v5 0/6] arm: Steps towards EL2 support round 3

[Edgar E. Iglesias]

On Thu, Jun 18, 2015 at 05:27:54PM +0100, Peter Maydell wrote:
> On 16 June 2015 at 02:51, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Hi,
> >
> > This is what is left of round 3 of our series towards support for EL2 for
> > AArch64.
> >
> > Comments welcome!
> 
> Given that we're now in softfreeze, and these patches don't actually
> enable a new feature by themselves, my current thought is that we
> should postpone them until after 2.4, unless they'd be particularly
> inconvenient for you to carry out of tree until then. Thoughts?

Hi Peter,

That's OK, No hurry from my side.

Best regards,
Edgar

Re: [Qemu-devel] [PATCH v5 1/6] target-arm: Add CNTVOFF_EL2

[Peter Maydell]

On 16 June 2015 at 02:51, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Adds support for the virtual timer offset controlled by EL2.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 2/6] target-arm: Add CNTHCTL_EL2

[Peter Maydell]

On 16 June 2015 at 02:51, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Adds control for trapping selected timer and counter accesses to EL2.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 34 ++++++++++++++++++++++++++++++++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1a66aa4..f39c32b 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 */
> +        uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
>          uint64_t cntvoff_el2; /* Counter Virtual Offset register */
>          ARMGenericTimer c14_timer[NUM_GTIMERS];
>          uint32_t c15_cpar; /* XScale Coprocessor Access Register */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 41cfad8..282f9fb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1153,23 +1153,42 @@ static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri)
>
>  static CPAccessResult gt_counter_access(CPUARMState *env, int timeridx)
>  {
> +    unsigned int cur_el = arm_current_el(env);
> +    bool secure = arm_is_secure(env);
> +
>      /* CNT[PV]CT: not visible from PL0 if ELO[PV]CTEN is zero */
> -    if (arm_current_el(env) == 0 &&
> +    if (cur_el == 0 &&
>          !extract32(env->cp15.c14_cntkctl, timeridx, 1)) {
>          return CP_ACCESS_TRAP;
>      }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL2) &&
> +        timeridx == GTIMER_PHYS && !secure && cur_el < 2 &&
> +        !extract32(env->cp15.cnthctl_el2, 0, 1)) {
> +        return CP_ACCESS_TRAP_EL2;
> +    }
>      return CP_ACCESS_OK;
>  }
>
>  static CPAccessResult gt_timer_access(CPUARMState *env, int timeridx)
>  {
> +    unsigned int cur_el = arm_current_el(env);
> +    bool secure = arm_is_secure(env);
> +
>      /* CNT[PV]_CVAL, CNT[PV]_CTL, CNT[PV]_TVAL: not visible from PL0 if
>       * EL0[PV]TEN is zero.
>       */
> -    if (arm_current_el(env) == 0 &&
> +    if (cur_el == 0 &&
>          !extract32(env->cp15.c14_cntkctl, 9 - timeridx, 1)) {
>          return CP_ACCESS_TRAP;
>      }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL2)) {
> +        if (timeridx == GTIMER_PHYS && !secure && cur_el < 2 &&
> +            !extract32(env->cp15.cnthctl_el2, 1, 1)) {
> +            return CP_ACCESS_TRAP_EL2;
> +        }
> +    }

It would be nice to be consistent about how we lay this conditional
out with the near-equivalent one in the previous function...

>      return CP_ACCESS_OK;
>  }
>
> @@ -2557,6 +2576,9 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>      { .name = "HTTBR", .cp = 15, .opc1 = 4, .crm = 2,
>        .access = PL2_RW, .type = ARM_CP_64BIT | ARM_CP_CONST,
>        .resetvalue = 0 },
> +    { .name = "CNTHCTL_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 1, .opc2 = 0,
> +      .access = PL2_RW, .type = 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 },
> @@ -2676,6 +2698,14 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
>        .type = ARM_CP_NO_RAW, .access = PL2_W,
>        .writefn = tlbi_aa64_vaa_write },
>  #ifndef CONFIG_USER_ONLY
> +    { .name = "CNTHCTL_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 1, .opc2 = 0,
> +      /* ARMv7 requires bit 0 and 1 to reset to 1. ARMv8 defines the
> +       * reset values as IMPDEF. We chose to reset to 3 to comply with

"choose".

> +       * both ARMv7 and ARMv8.
> +       */
> +      .access = PL2_RW, .resetvalue = 3,
> +      .fieldoffset = offsetof(CPUARMState, cp15.cnthctl_el2) },
>      { .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,

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 4/6] target-arm: Add the Hypervisor timer

[Peter Maydell]

On 16 June 2015 at 02:51, 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>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 4/6] target-arm: Add the Hypervisor timer

[Peter Maydell]

On 16 June 2015 at 02:51, 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-qom.h |  1 +
>  target-arm/cpu.c     |  2 ++
>  target-arm/cpu.h     |  3 ++-
>  target-arm/helper.c  | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index ed5a644..3aaa7b6 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -214,6 +214,7 @@ int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  /* Callback functions for the generic timer's timers. */
>  void arm_gt_ptimer_cb(void *opaque);
>  void arm_gt_vtimer_cb(void *opaque);
> +void arm_gt_htimer_cb(void *opaque);
>
>  #ifdef TARGET_AARCH64
>  int aarch64_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 4a888ab..b631482 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -409,6 +409,8 @@ static void arm_cpu_initfn(Object *obj)
>                                                  arm_gt_ptimer_cb, cpu);
>      cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
>                                                  arm_gt_vtimer_cb, cpu);
> +    cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> +                                                arm_gt_htimer_cb, cpu);
>      qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs,
>                         ARRAY_SIZE(cpu->gt_timer_outputs));
>  #endif
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index f39c32b..dfa9d77 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -113,7 +113,8 @@ typedef struct ARMGenericTimer {
>
>  #define GTIMER_PHYS 0
>  #define GTIMER_VIRT 1
> -#define NUM_GTIMERS 2
> +#define GTIMER_HYP  2
> +#define NUM_GTIMERS 3
>
>  typedef struct {
>      uint64_t raw_tcr;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 92dbb28..32df2f5 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1391,6 +1391,34 @@ static void gt_cntvoff_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      gt_recalc_timer(cpu, GTIMER_VIRT);
>  }
>
> +static void gt_hyp_cnt_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    gt_cnt_reset(env, ri, GTIMER_HYP);
> +}
> +    { .name = "CNTHP_TVAL_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 0,
> +      .type = ARM_CP_IO, .access = PL2_RW,
> +      .resetfn = gt_hyp_cnt_reset,
> +      .readfn = gt_hyp_tval_read, .writefn = gt_hyp_tval_write },

Something I just noticed while I was trying to add support
for the secure physical timer on top of this series: the
gt_*_cnt_reset functions are misnamed, because they're not
resetting the counters, they're resetting the timers.
(There are only two counters, physical and virtual, but there
are four timers, physical, secure-physical, virtual and hyp.
Since our reset function is deleting the underlying QEMU
timer it's a timer reset, not a counter reset.)
We should probably fix up the names and make sure they're
associated with the correct registers (the phys and virt
timer reset is currently hanging off a counter register).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 4/6] target-arm: Add the Hypervisor timer

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 3809 bytes --]

On 10/07/2015 7:58 pm, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 16 June 2015 at 02:51, 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-qom.h |  1 +
> >  target-arm/cpu.c     |  2 ++
> >  target-arm/cpu.h     |  3 ++-
> >  target-arm/helper.c  | 68
++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 73 insertions(+), 1 deletion(-)
> >
> > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> > index ed5a644..3aaa7b6 100644
> > --- a/target-arm/cpu-qom.h
> > +++ b/target-arm/cpu-qom.h
> > @@ -214,6 +214,7 @@ int arm_cpu_gdb_write_register(CPUState *cpu,
uint8_t *buf, int reg);
> >  /* Callback functions for the generic timer's timers. */
> >  void arm_gt_ptimer_cb(void *opaque);
> >  void arm_gt_vtimer_cb(void *opaque);
> > +void arm_gt_htimer_cb(void *opaque);
> >
> >  #ifdef TARGET_AARCH64
> >  int aarch64_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int
reg);
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 4a888ab..b631482 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -409,6 +409,8 @@ static void arm_cpu_initfn(Object *obj)
> >                                                  arm_gt_ptimer_cb, cpu);
> >      cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL,
GTIMER_SCALE,
> >                                                  arm_gt_vtimer_cb, cpu);
> > +    cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL,
GTIMER_SCALE,
> > +                                                arm_gt_htimer_cb, cpu);
> >      qdev_init_gpio_out(DEVICE(cpu), cpu->gt_timer_outputs,
> >                         ARRAY_SIZE(cpu->gt_timer_outputs));
> >  #endif
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index f39c32b..dfa9d77 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -113,7 +113,8 @@ typedef struct ARMGenericTimer {
> >
> >  #define GTIMER_PHYS 0
> >  #define GTIMER_VIRT 1
> > -#define NUM_GTIMERS 2
> > +#define GTIMER_HYP  2
> > +#define NUM_GTIMERS 3
> >
> >  typedef struct {
> >      uint64_t raw_tcr;
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 92dbb28..32df2f5 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -1391,6 +1391,34 @@ static void gt_cntvoff_write(CPUARMState *env,
const ARMCPRegInfo *ri,
> >      gt_recalc_timer(cpu, GTIMER_VIRT);
> >  }
> >
> > +static void gt_hyp_cnt_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> > +{
> > +    gt_cnt_reset(env, ri, GTIMER_HYP);
> > +}
> > +    { .name = "CNTHP_TVAL_EL2", .state = ARM_CP_STATE_BOTH,
> > +      .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 0,
> > +      .type = ARM_CP_IO, .access = PL2_RW,
> > +      .resetfn = gt_hyp_cnt_reset,
> > +      .readfn = gt_hyp_tval_read, .writefn = gt_hyp_tval_write },
>
> Something I just noticed while I was trying to add support
> for the secure physical timer on top of this series: the
> gt_*_cnt_reset functions are misnamed, because they're not
> resetting the counters, they're resetting the timers.
> (There are only two counters, physical and virtual, but there
> are four timers, physical, secure-physical, virtual and hyp.
> Since our reset function is deleting the underlying QEMU
> timer it's a timer reset, not a counter reset.)
> We should probably fix up the names and make sure they're
> associated with the correct registers (the phys and virt
> timer reset is currently hanging off a counter register).
>
> thanks
> -- PMM

Hi, yes that sounds good. Btw are you fixing this as you go or should I
send a new series fixing your comments? I've fixed the stuff you commented
on a few days ago in my tree...

Cheers,
Edgar

[-- Attachment #2: Type: text/html, Size: 5159 bytes --]

Re: [Qemu-devel] [PATCH v5 4/6] target-arm: Add the Hypervisor timer

[Peter Maydell]

On 10 July 2015 at 12:23, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
>
> On 10/07/2015 7:58 pm, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>> Something I just noticed while I was trying to add support
>> for the secure physical timer on top of this series: the
>> gt_*_cnt_reset functions are misnamed, because they're not
>> resetting the counters, they're resetting the timers.
>> (There are only two counters, physical and virtual, but there
>> are four timers, physical, secure-physical, virtual and hyp.
>> Since our reset function is deleting the underlying QEMU
>> timer it's a timer reset, not a counter reset.)
>> We should probably fix up the names and make sure they're
>> associated with the correct registers (the phys and virt
>> timer reset is currently hanging off a counter register)

> Hi, yes that sounds good. Btw are you fixing this as you go or should I send
> a new series fixing your comments? I've fixed the stuff you commented on a
> few days ago in my tree...

I rebased as I was reviewing it and am currently basing my
secure-timer patches on that. It would probably be good if
you fixed up the naming issue here and resent, and then I'll
rebase on top of that.

-- PMM

Re: [Qemu-devel] [PATCH v5 4/6] target-arm: Add the Hypervisor timer

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On 10/07/2015 9:26 pm, "Peter Maydell" <peter.maydell@linaro.org> wrote:
>
> On 10 July 2015 at 12:23, Edgar E. Iglesias <edgar.iglesias@gmail.com>
wrote:
> >
> > On 10/07/2015 7:58 pm, "Peter Maydell" <peter.maydell@linaro.org> wrote:
> >> Something I just noticed while I was trying to add support
> >> for the secure physical timer on top of this series: the
> >> gt_*_cnt_reset functions are misnamed, because they're not
> >> resetting the counters, they're resetting the timers.
> >> (There are only two counters, physical and virtual, but there
> >> are four timers, physical, secure-physical, virtual and hyp.
> >> Since our reset function is deleting the underlying QEMU
> >> timer it's a timer reset, not a counter reset.)
> >> We should probably fix up the names and make sure they're
> >> associated with the correct registers (the phys and virt
> >> timer reset is currently hanging off a counter register)
>
> > Hi, yes that sounds good. Btw are you fixing this as you go or should I
send
> > a new series fixing your comments? I've fixed the stuff you commented
on a
> > few days ago in my tree...
>
> I rebased as I was reviewing it and am currently basing my
> secure-timer patches on that. It would probably be good if
> you fixed up the naming issue here and resent, and then I'll
> rebase on top of that.
>
> -- PMM

Ok, sounds good.

Cheers,
Edgar

[-- Attachment #2: Type: text/html, Size: 1899 bytes --]

Re: [Qemu-devel] [PATCH v5 4/6] target-arm: Add the Hypervisor timer

[Edgar E. Iglesias]

On Fri, Jul 10, 2015 at 12:25:56PM +0100, Peter Maydell wrote:
> On 10 July 2015 at 12:23, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> >
> > On 10/07/2015 7:58 pm, "Peter Maydell" <peter.maydell@linaro.org> wrote:
> >> Something I just noticed while I was trying to add support
> >> for the secure physical timer on top of this series: the
> >> gt_*_cnt_reset functions are misnamed, because they're not
> >> resetting the counters, they're resetting the timers.
> >> (There are only two counters, physical and virtual, but there
> >> are four timers, physical, secure-physical, virtual and hyp.
> >> Since our reset function is deleting the underlying QEMU
> >> timer it's a timer reset, not a counter reset.)
> >> We should probably fix up the names and make sure they're
> >> associated with the correct registers (the phys and virt
> >> timer reset is currently hanging off a counter register)
> 
> > Hi, yes that sounds good. Btw are you fixing this as you go or should I send
> > a new series fixing your comments? I've fixed the stuff you commented on a
> > few days ago in my tree...
> 
> I rebased as I was reviewing it and am currently basing my
> secure-timer patches on that. It would probably be good if
> you fixed up the naming issue here and resent, and then I'll
> rebase on top of that.
>

Hi Peter,

I've just sent out a v6 hopefully addressing your comments.

I noticed that the naming is a bit incosistent with the timers but didn't
want to change too much in case you've got patches on top.

The timer and counter functions can be renamed to consistenly use

_phys_
_sec_phys_
_virt_
_hyp_ 

Or:
_p
_sp
_v
_h

Or any other reasonable combo.

We can patch into my series or do as followup

Cheers,
Edgar