| Message ID | 1434419515-3572-5-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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 };