[Qemu-devel] [PATCH 0/4] target-arm: Implement Secure physical timer

[Qemu-devel] [PATCH 0/4] target-arm: Implement Secure physical timer

[Peter Maydell]

We managed to forget to implement the Secure physical timer when
we added TZ support for AArch32. This patchset fixes the omission,
and wires up its interrupt.

This patchset sits on top of
https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm-post-2.4
which is where I'm putting patches which I've reviewed but which aren't
2.4 material. Currently that's just Edgar's hyp timer series. (Warning,
branch will rebase.)

Since we're now wiring up four timer interrupts in virt and
a15mpcore, I switched them to a data-driven loop for neatness.

Incidentally, the reason that kernels worked and continue to work
even if they're booting Secure is that they cope with timer interrupts
coming in via either the NS timer irq line or the S timer irq line.

The next step is to put the secure-GIC patchset on top of this.
I think we're going to need to make the virt board default to
non-secure at that point, because the QEMU UEFI blob doesn't cope
with being booted Secure. That's probably for the best anyway, since
then it will be the same for TCG and KVM.

thanks
-- PMM

Peter Maydell (4):
  target-arm: Add the AArch64 view of the Secure physical timer
  target-arm: Add AArch32 banked register access to secure physical
    timer
  hw/arm/virt: Wire up secure timer interrupt
  hw/cpu/a15mpcore: Wire up hyp and secure physical timer interrupts

 hw/arm/virt.c        |  28 +++++++------
 hw/cpu/a15mpcore.c   |  21 ++++++----
 target-arm/cpu-qom.h |   1 +
 target-arm/cpu.c     |   2 +
 target-arm/cpu.h     |   3 +-
 target-arm/helper.c  | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 148 insertions(+), 21 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] target-arm: Add the AArch64 view of the Secure physical timer

[Peter Maydell]

On CPUs with EL3, there are two physical timers, one for Secure and one
for Non-secure. Implement this extra timer and the AArch64 registers
which access it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu-qom.h |  1 +
 target-arm/cpu.c     |  2 ++
 target-arm/cpu.h     |  3 +-
 target-arm/helper.c  | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 54db337..00c0716 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -225,6 +225,7 @@ int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 void arm_gt_ptimer_cb(void *opaque);
 void arm_gt_vtimer_cb(void *opaque);
 void arm_gt_htimer_cb(void *opaque);
+void arm_gt_stimer_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 3525348..fafc3ed 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -455,6 +455,8 @@ static void arm_cpu_initfn(Object *obj)
                                                 arm_gt_vtimer_cb, cpu);
     cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
                                                 arm_gt_htimer_cb, cpu);
+    cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
+                                                arm_gt_stimer_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 7346c5f..4a6cd51 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -114,7 +114,8 @@ typedef struct ARMGenericTimer {
 #define GTIMER_PHYS 0
 #define GTIMER_VIRT 1
 #define GTIMER_HYP  2
-#define NUM_GTIMERS 3
+#define GTIMER_SEC  3
+#define NUM_GTIMERS 4
 
 typedef struct {
     uint64_t raw_tcr;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 4a7dd24..d31b946 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1214,6 +1214,32 @@ static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
     return gt_timer_access(env, GTIMER_VIRT);
 }
 
+static CPAccessResult gt_stimer_access(CPUARMState *env,
+                                       const ARMCPRegInfo *ri)
+{
+    /* The AArch64 register view of the secure physical timer is
+     * always accessible from EL3, and configurably accessible from
+     * Secure EL1.
+     */
+    switch (arm_current_el(env)) {
+    case 1:
+        if (!arm_is_secure(env)) {
+            return CP_ACCESS_TRAP;
+        }
+        if (!(env->cp15.scr_el3 & SCR_ST)) {
+            return CP_ACCESS_TRAP_EL3;
+        }
+        return CP_ACCESS_OK;
+    case 0:
+    case 2:
+        return CP_ACCESS_TRAP;
+    case 3:
+        return CP_ACCESS_OK;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static uint64_t gt_get_countervalue(CPUARMState *env)
 {
     return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / GTIMER_SCALE;
@@ -1420,6 +1446,34 @@ static void gt_hyp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
     gt_ctl_write(env, ri, GTIMER_HYP, value);
 }
 
+static void gt_sec_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    gt_timer_reset(env, ri, GTIMER_SEC);
+}
+
+static void gt_sec_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_cval_write(env, ri, GTIMER_SEC, value);
+}
+
+static uint64_t gt_sec_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return gt_tval_read(env, ri, GTIMER_SEC);
+}
+
+static void gt_sec_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_tval_write(env, ri, GTIMER_SEC, value);
+}
+
+static void gt_sec_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                              uint64_t value)
+{
+    gt_ctl_write(env, ri, GTIMER_SEC, value);
+}
+
 void arm_gt_ptimer_cb(void *opaque)
 {
     ARMCPU *cpu = opaque;
@@ -1441,6 +1495,13 @@ void arm_gt_htimer_cb(void *opaque)
     gt_recalc_timer(cpu, GTIMER_HYP);
 }
 
+void arm_gt_stimer_cb(void *opaque)
+{
+    ARMCPU *cpu = opaque;
+
+    gt_recalc_timer(cpu, GTIMER_SEC);
+}
+
 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.
@@ -1570,6 +1631,32 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .resetvalue = 0, .accessfn = gt_vtimer_access,
       .writefn = gt_virt_cval_write, .raw_writefn = raw_write,
     },
+    /* Secure timer -- this is actually restricted to only EL3
+     * and configurably Secure-EL1 via the accessfn.
+     */
+    { .name = "CNTPS_TVAL_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 0,
+      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW,
+      .accessfn = gt_stimer_access,
+      .readfn = gt_sec_tval_read,
+      .writefn = gt_sec_tval_write,
+      .resetfn = gt_sec_timer_reset,
+    },
+    { .name = "CNTPS_CTL_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 1,
+      .type = ARM_CP_IO, .access = PL1_RW,
+      .accessfn = gt_stimer_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].ctl),
+      .resetvalue = 0,
+      .writefn = gt_sec_ctl_write, .raw_writefn = raw_write,
+    },
+    { .name = "CNTPS_CVAL_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 2,
+      .type = ARM_CP_IO,
+      .accessfn = gt_stimer_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
+      .writefn = gt_sec_cval_write, .raw_writefn = raw_write,
+    },
     REGINFO_SENTINEL
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] target-arm: Add AArch32 banked register access to secure physical timer

[Peter Maydell]

If EL3 is AArch32, then the secure physical timer is accessed via
banking of the registers used for the non-secure physical timer.
Implement this banking.

Note that the access controls for the AArch32 banked registers
remain the same as the physical-timer checks; they are not the
same as the controls on the AArch64 secure timer registers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index d31b946..6fd5d93 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1527,12 +1527,22 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
     },
     /* per-timer control */
     { .name = "CNTP_CTL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1,
+      .secure = ARM_CP_SECSTATE_NS,
       .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
       .accessfn = gt_ptimer_access,
       .fieldoffset = offsetoflow32(CPUARMState,
                                    cp15.c14_timer[GTIMER_PHYS].ctl),
       .writefn = gt_phys_ctl_write, .raw_writefn = raw_write,
     },
+    { .name = "CNTP_CTL(S)",
+      .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 1,
+      .secure = ARM_CP_SECSTATE_S,
+      .type = ARM_CP_IO | ARM_CP_ALIAS, .access = PL1_RW | PL0_R,
+      .accessfn = gt_ptimer_access,
+      .fieldoffset = offsetoflow32(CPUARMState,
+                                   cp15.c14_timer[GTIMER_SEC].ctl),
+      .writefn = gt_sec_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,
       .type = ARM_CP_IO, .access = PL1_RW | PL0_R,
@@ -1558,10 +1568,18 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
     },
     /* TimerValue views: a 32 bit downcounting view of the underlying state */
     { .name = "CNTP_TVAL", .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0,
+      .secure = ARM_CP_SECSTATE_NS,
       .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
       .accessfn = gt_ptimer_access,
       .readfn = gt_phys_tval_read, .writefn = gt_phys_tval_write,
     },
+    { .name = "CNTP_TVAL(S)",
+      .cp = 15, .crn = 14, .crm = 2, .opc1 = 0, .opc2 = 0,
+      .secure = ARM_CP_SECSTATE_S,
+      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW | PL0_R,
+      .accessfn = gt_ptimer_access,
+      .readfn = gt_sec_tval_read, .writefn = gt_sec_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,
@@ -1602,12 +1620,21 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
     },
     /* Comparison value, indicating when the timer goes off */
     { .name = "CNTP_CVAL", .cp = 15, .crm = 14, .opc1 = 2,
+      .secure = ARM_CP_SECSTATE_NS,
       .access = PL1_RW | PL0_R,
       .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_PHYS].cval),
       .accessfn = gt_ptimer_access,
       .writefn = gt_phys_cval_write, .raw_writefn = raw_write,
     },
+    { .name = "CNTP_CVAL(S)", .cp = 15, .crm = 14, .opc1 = 2,
+      .secure = ARM_CP_SECSTATE_S,
+      .access = PL1_RW | PL0_R,
+      .type = ARM_CP_64BIT | ARM_CP_IO | ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
+      .accessfn = gt_ptimer_access,
+      .writefn = gt_sec_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,
       .access = PL1_RW | PL0_R,
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] hw/arm/virt: Wire up secure timer interrupt

[Peter Maydell]

Wire up the secure timer interrupt. Since we've defined
that the plain old physical timer is the NS timer, we can
drop the now-out-of-date comment about QEMU not having TZ.

Use a data-driven loop to wire up the timer interrupts, since
we now have four of them and the code is the same for each.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index aab99f7..95b1a9a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -392,20 +392,22 @@ 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 * 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.
+        int irq;
+        /* Mapping from the output timer irq lines from the CPU to the
+         * GIC PPI inputs we use for the virt board.
          */
-        qdev_connect_gpio_out(cpudev, 0,
-                              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 + ARCH_TIMER_VIRT_IRQ));
-        /* Hypervisor timer.  */
-        qdev_connect_gpio_out(cpudev, 2,
-                              qdev_get_gpio_in(gicdev,
-                                             ppibase + ARCH_TIMER_NS_EL2_IRQ));
+        const int timer_irq[] = {
+            [GTIMER_PHYS] = ARCH_TIMER_NS_EL1_IRQ,
+            [GTIMER_VIRT] = ARCH_TIMER_VIRT_IRQ,
+            [GTIMER_HYP]  = ARCH_TIMER_NS_EL2_IRQ,
+            [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
+        };
+
+        for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
+            qdev_connect_gpio_out(cpudev, irq,
+                                  qdev_get_gpio_in(gicdev,
+                                                   ppibase + timer_irq[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 4/4] hw/cpu/a15mpcore: Wire up hyp and secure physical timer interrupts

[Peter Maydell]

Since we now support both the hypervisor and the secure physical timer, wire
their interrupt lines up in the a15mpcore wrapper object.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/cpu/a15mpcore.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index acc419e..49727d0 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -79,14 +79,21 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < s->num_cpu; i++) {
         DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
         int ppibase = s->num_irq - 32 + i * 32;
-        /* physical timer; we wire it up to the non-secure timer's ID,
-         * since a real A15 always has TrustZone but QEMU doesn't.
+        int irq;
+        /* Mapping from the output timer irq lines from the CPU to the
+         * GIC PPI inputs used on the A15:
          */
-        qdev_connect_gpio_out(cpudev, 0,
-                              qdev_get_gpio_in(gicdev, ppibase + 30));
-        /* virtual timer */
-        qdev_connect_gpio_out(cpudev, 1,
-                              qdev_get_gpio_in(gicdev, ppibase + 27));
+        const int timer_irq[] = {
+            [GTIMER_PHYS] = 30,
+            [GTIMER_VIRT] = 27,
+            [GTIMER_HYP]  = 26,
+            [GTIMER_SEC]  = 29,
+        };
+        for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
+            qdev_connect_gpio_out(cpudev, irq,
+                                  qdev_get_gpio_in(gicdev,
+                                                   ppibase + timer_irq[irq]));
+        }
     }
 
     /* Memory map (addresses are offsets from PERIPHBASE):
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/4] target-arm: Add the AArch64 view of the Secure physical timer

[Edgar E. Iglesias]

On Thu, Jul 16, 2015 at 12:47:26PM +0100, Peter Maydell wrote:
> On CPUs with EL3, there are two physical timers, one for Secure and one
> for Non-secure. Implement this extra timer and the AArch64 registers
> which access it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu-qom.h |  1 +
>  target-arm/cpu.c     |  2 ++
>  target-arm/cpu.h     |  3 +-
>  target-arm/helper.c  | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 54db337..00c0716 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -225,6 +225,7 @@ int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  void arm_gt_ptimer_cb(void *opaque);
>  void arm_gt_vtimer_cb(void *opaque);
>  void arm_gt_htimer_cb(void *opaque);
> +void arm_gt_stimer_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 3525348..fafc3ed 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -455,6 +455,8 @@ static void arm_cpu_initfn(Object *obj)
>                                                  arm_gt_vtimer_cb, cpu);
>      cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
>                                                  arm_gt_htimer_cb, cpu);
> +    cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> +                                                arm_gt_stimer_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 7346c5f..4a6cd51 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -114,7 +114,8 @@ typedef struct ARMGenericTimer {
>  #define GTIMER_PHYS 0
>  #define GTIMER_VIRT 1
>  #define GTIMER_HYP  2
> -#define NUM_GTIMERS 3
> +#define GTIMER_SEC  3
> +#define NUM_GTIMERS 4
>  
>  typedef struct {
>      uint64_t raw_tcr;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4a7dd24..d31b946 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1214,6 +1214,32 @@ static CPAccessResult gt_vtimer_access(CPUARMState *env, const ARMCPRegInfo *ri)
>      return gt_timer_access(env, GTIMER_VIRT);
>  }
>  
> +static CPAccessResult gt_stimer_access(CPUARMState *env,
> +                                       const ARMCPRegInfo *ri)
> +{
> +    /* The AArch64 register view of the secure physical timer is
> +     * always accessible from EL3, and configurably accessible from
> +     * Secure EL1.
> +     */
> +    switch (arm_current_el(env)) {
> +    case 1:
> +        if (!arm_is_secure(env)) {
> +            return CP_ACCESS_TRAP;
> +        }
> +        if (!(env->cp15.scr_el3 & SCR_ST)) {
> +            return CP_ACCESS_TRAP_EL3;
> +        }
> +        return CP_ACCESS_OK;
> +    case 0:
> +    case 2:
> +        return CP_ACCESS_TRAP;
> +    case 3:
> +        return CP_ACCESS_OK;
> +    default:
> +        g_assert_not_reached();
> +    }
> +}
> +
>  static uint64_t gt_get_countervalue(CPUARMState *env)
>  {
>      return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / GTIMER_SCALE;
> @@ -1420,6 +1446,34 @@ static void gt_hyp_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      gt_ctl_write(env, ri, GTIMER_HYP, value);
>  }
>  
> +static void gt_sec_timer_reset(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    gt_timer_reset(env, ri, GTIMER_SEC);
> +}
> +
> +static void gt_sec_cval_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                              uint64_t value)
> +{
> +    gt_cval_write(env, ri, GTIMER_SEC, value);
> +}
> +
> +static uint64_t gt_sec_tval_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return gt_tval_read(env, ri, GTIMER_SEC);
> +}
> +
> +static void gt_sec_tval_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                              uint64_t value)
> +{
> +    gt_tval_write(env, ri, GTIMER_SEC, value);
> +}
> +
> +static void gt_sec_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                              uint64_t value)
> +{
> +    gt_ctl_write(env, ri, GTIMER_SEC, value);
> +}
> +
>  void arm_gt_ptimer_cb(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
> @@ -1441,6 +1495,13 @@ void arm_gt_htimer_cb(void *opaque)
>      gt_recalc_timer(cpu, GTIMER_HYP);
>  }
>  
> +void arm_gt_stimer_cb(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +
> +    gt_recalc_timer(cpu, GTIMER_SEC);
> +}
> +
>  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.
> @@ -1570,6 +1631,32 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>        .resetvalue = 0, .accessfn = gt_vtimer_access,
>        .writefn = gt_virt_cval_write, .raw_writefn = raw_write,
>      },
> +    /* Secure timer -- this is actually restricted to only EL3
> +     * and configurably Secure-EL1 via the accessfn.
> +     */
> +    { .name = "CNTPS_TVAL_EL1", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 0,
> +      .type = ARM_CP_NO_RAW | ARM_CP_IO, .access = PL1_RW,
> +      .accessfn = gt_stimer_access,
> +      .readfn = gt_sec_tval_read,
> +      .writefn = gt_sec_tval_write,
> +      .resetfn = gt_sec_timer_reset,
> +    },
> +    { .name = "CNTPS_CTL_EL1", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 1,
> +      .type = ARM_CP_IO, .access = PL1_RW,
> +      .accessfn = gt_stimer_access,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].ctl),
> +      .resetvalue = 0,
> +      .writefn = gt_sec_ctl_write, .raw_writefn = raw_write,
> +    },
> +    { .name = "CNTPS_CVAL_EL1", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 2,
> +      .type = ARM_CP_IO,
> +      .accessfn = gt_stimer_access,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
> +      .writefn = gt_sec_cval_write, .raw_writefn = raw_write,

Hi Peter,

I think you've missed a .access = PL1_RW here. With that change the series passes my sectimer tests.

Cheers,
Edgar


> +    },
>      REGINFO_SENTINEL
>  };
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 1/4] target-arm: Add the AArch64 view of the Secure physical timer

[Peter Maydell]

On 24 July 2015 at 10:48, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Jul 16, 2015 at 12:47:26PM +0100, Peter Maydell wrote:
>> +    { .name = "CNTPS_CVAL_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 2,
>> +      .type = ARM_CP_IO,
>> +      .accessfn = gt_stimer_access,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
>> +      .writefn = gt_sec_cval_write, .raw_writefn = raw_write,

> I think you've missed a .access = PL1_RW here. With that change the series passes my sectimer tests.

Yep, you're right, this needs to be folded into this patch:

--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1679,7 +1679,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
     },
     { .name = "CNTPS_CVAL_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 2,
-      .type = ARM_CP_IO,
+      .type = ARM_CP_IO, .access = PL1_RW,
       .accessfn = gt_stimer_access,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
       .writefn = gt_sec_cval_write, .raw_writefn = raw_write,

(I won't bother resending unless there are other fixes that need
to be made too.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/4] target-arm: Add the AArch64 view of the Secure physical timer

[Edgar E. Iglesias]

On Fri, Jul 24, 2015 at 11:06:01AM +0100, Peter Maydell wrote:
> On 24 July 2015 at 10:48, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Thu, Jul 16, 2015 at 12:47:26PM +0100, Peter Maydell wrote:
> >> +    { .name = "CNTPS_CVAL_EL1", .state = ARM_CP_STATE_AA64,
> >> +      .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 2,
> >> +      .type = ARM_CP_IO,
> >> +      .accessfn = gt_stimer_access,
> >> +      .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
> >> +      .writefn = gt_sec_cval_write, .raw_writefn = raw_write,
> 
> > I think you've missed a .access = PL1_RW here. With that change the series passes my sectimer tests.
> 
> Yep, you're right, this needs to be folded into this patch:
> 
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1679,7 +1679,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>      },
>      { .name = "CNTPS_CVAL_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 7, .crn = 14, .crm = 2, .opc2 = 2,
> -      .type = ARM_CP_IO,
> +      .type = ARM_CP_IO, .access = PL1_RW,
>        .accessfn = gt_stimer_access,
>        .fieldoffset = offsetof(CPUARMState, cp15.c14_timer[GTIMER_SEC].cval),
>        .writefn = gt_sec_cval_write, .raw_writefn = raw_write,
> 
> (I won't bother resending unless there are other fixes that need
> to be made too.)

Sounds good, the rest looks good to me, feel free to add my RB on the entire series.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar