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

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

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
>