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

[Edgar E. Iglesias]

On 10/07/2015 7:58 pm, "Peter Maydell" <address@hidden> wrote:
>
> 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

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