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 <address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  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