| Message ID | 20191203041440.6275-3-andrew@aj.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Expose GT CNTFRQ as a CPU property to support AST2600 | expand |
On 12/3/19 5:14 AM, Andrew Jeffery wrote: > Prepare for SoCs such as the ASPEED AST2600 whose firmware configures > CNTFRQ to values significantly larger than the static 62.5MHz value > currently derived from GTIMER_SCALE. As the OS potentially derives its > timer periods from the CNTFRQ value the lack of support for running > QEMUTimers at the appropriate rate leads to sticky behaviour in the > guest. > > Substitute the GTIMER_SCALE constant with use of a helper to derive the > period from gt_cntfrq stored in struct ARMCPU. Initially set gt_cntfrq > to the frequency associated with GTIMER_SCALE so current behaviour is > maintained. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.c | 2 ++ > target/arm/cpu.h | 10 ++++++++++ > target/arm/helper.c | 10 +++++++--- > 3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 7a4ac9339bf9..5698a74061bb 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -974,6 +974,8 @@ static void arm_cpu_initfn(Object *obj) > if (tcg_enabled()) { > cpu->psci_version = 2; /* TCG implements PSCI 0.2 */ > } > + > + cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE; > } > > static Property arm_cpu_reset_cbar_property = > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 83a809d4bac4..666c03871fdf 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -932,8 +932,18 @@ struct ARMCPU { > */ > DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ); > DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ); > + > + /* Generic timer counter frequency, in Hz */ > + uint64_t gt_cntfrq; You can also explicit the unit by calling it 'gt_cntfrq_hz'. > }; > > +static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > +{ > + /* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */ Why inline this call? I doubt there is a significant performance gain. > + const unsigned int ns_per_s = 1000 * 1000 * 1000; > + return ns_per_s > cpu->gt_cntfrq ? ns_per_s / cpu->gt_cntfrq : 1; > +} > + > void arm_cpu_post_init(Object *obj); > > uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz); > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 65c4441a3896..2622a9a8d02f 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -2409,7 +2409,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env, > > static uint64_t gt_get_countervalue(CPUARMState *env) > { > - return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / GTIMER_SCALE; > + ARMCPU *cpu = env_archcpu(env); > + > + return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(cpu); > } > > static void gt_recalc_timer(ARMCPU *cpu, int timeridx) > @@ -2445,7 +2447,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) > * set the timer for as far in the future as possible. When the > * timer expires we will reset the timer for any remaining period. > */ > - if (nexttick > INT64_MAX / GTIMER_SCALE) { > + if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) { > timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX); > } else { > timer_mod(cpu->gt_timer[timeridx], nexttick); > @@ -2874,11 +2876,13 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = { > > static uint64_t gt_virt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri) > { > + ARMCPU *cpu = env_archcpu(env); > + > /* Currently we have no support for QEMUTimer in linux-user so we > * can't call gt_get_countervalue(env), instead we directly > * call the lower level functions. > */ > - return cpu_get_clock() / GTIMER_SCALE; > + return cpu_get_clock() / gt_cntfrq_period_ns(cpu); > } > > static const ARMCPRegInfo generic_timer_cp_reginfo[] = { >
On Tue, 3 Dec 2019, at 16:39, Philippe Mathieu-Daudé wrote: > On 12/3/19 5:14 AM, Andrew Jeffery wrote: > > Prepare for SoCs such as the ASPEED AST2600 whose firmware configures > > CNTFRQ to values significantly larger than the static 62.5MHz value > > currently derived from GTIMER_SCALE. As the OS potentially derives its > > timer periods from the CNTFRQ value the lack of support for running > > QEMUTimers at the appropriate rate leads to sticky behaviour in the > > guest. > > > > Substitute the GTIMER_SCALE constant with use of a helper to derive the > > period from gt_cntfrq stored in struct ARMCPU. Initially set gt_cntfrq > > to the frequency associated with GTIMER_SCALE so current behaviour is > > maintained. > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > --- > > target/arm/cpu.c | 2 ++ > > target/arm/cpu.h | 10 ++++++++++ > > target/arm/helper.c | 10 +++++++--- > > 3 files changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 7a4ac9339bf9..5698a74061bb 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -974,6 +974,8 @@ static void arm_cpu_initfn(Object *obj) > > if (tcg_enabled()) { > > cpu->psci_version = 2; /* TCG implements PSCI 0.2 */ > > } > > + > > + cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE; > > } > > > > static Property arm_cpu_reset_cbar_property = > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index 83a809d4bac4..666c03871fdf 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -932,8 +932,18 @@ struct ARMCPU { > > */ > > DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ); > > DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ); > > + > > + /* Generic timer counter frequency, in Hz */ > > + uint64_t gt_cntfrq; > > You can also explicit the unit by calling it 'gt_cntfrq_hz'. Fair call, I'll fix that. > > > }; > > > > +static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > > +{ > > + /* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */ > > Why inline this call? I doubt there is a significant performance gain. It wasn't so much performance. It started out as a macro for a simple calculation because I didn't want to duplicate it across a number of places, then I wanted type safety for the pointer so I switched the macro in the header to an inline function. So it is an evolution of the patch rather than something that came from an explicit goal of e.g. performance. Andrew
On 12/3/19 1:48 PM, Andrew Jeffery wrote: > On Tue, 3 Dec 2019, at 16:39, Philippe Mathieu-Daudé wrote: >> On 12/3/19 5:14 AM, Andrew Jeffery wrote: >>> Prepare for SoCs such as the ASPEED AST2600 whose firmware configures >>> CNTFRQ to values significantly larger than the static 62.5MHz value >>> currently derived from GTIMER_SCALE. As the OS potentially derives its >>> timer periods from the CNTFRQ value the lack of support for running >>> QEMUTimers at the appropriate rate leads to sticky behaviour in the >>> guest. >>> >>> Substitute the GTIMER_SCALE constant with use of a helper to derive the >>> period from gt_cntfrq stored in struct ARMCPU. Initially set gt_cntfrq >>> to the frequency associated with GTIMER_SCALE so current behaviour is >>> maintained. >>> >>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> target/arm/cpu.c | 2 ++ >>> target/arm/cpu.h | 10 ++++++++++ >>> target/arm/helper.c | 10 +++++++--- >>> 3 files changed, 19 insertions(+), 3 deletions(-) >>> >>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >>> index 7a4ac9339bf9..5698a74061bb 100644 >>> --- a/target/arm/cpu.c >>> +++ b/target/arm/cpu.c >>> @@ -974,6 +974,8 @@ static void arm_cpu_initfn(Object *obj) >>> if (tcg_enabled()) { >>> cpu->psci_version = 2; /* TCG implements PSCI 0.2 */ >>> } >>> + >>> + cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE; >>> } >>> >>> static Property arm_cpu_reset_cbar_property = >>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >>> index 83a809d4bac4..666c03871fdf 100644 >>> --- a/target/arm/cpu.h >>> +++ b/target/arm/cpu.h >>> @@ -932,8 +932,18 @@ struct ARMCPU { >>> */ >>> DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ); >>> DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ); >>> + >>> + /* Generic timer counter frequency, in Hz */ >>> + uint64_t gt_cntfrq; >> >> You can also explicit the unit by calling it 'gt_cntfrq_hz'. > > Fair call, I'll fix that. > >> >>> }; >>> >>> +static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) >>> +{ >>> + /* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */ >> >> Why inline this call? I doubt there is a significant performance gain. > > It wasn't so much performance. It started out as a macro for a simple calculation > because I didn't want to duplicate it across a number of places, then I wanted type > safety for the pointer so I switched the macro in the header to an inline function. So > it is an evolution of the patch rather than something that came from an explicit goal > of e.g. performance. OK. Eventually NANOSECONDS_PER_SECOND will move to "qemu/units.h". Should the XXX comment stay? I'm not sure, it is confusing. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Wed, 4 Dec 2019, at 03:57, Philippe Mathieu-Daudé wrote: > On 12/3/19 1:48 PM, Andrew Jeffery wrote: > > On Tue, 3 Dec 2019, at 16:39, Philippe Mathieu-Daudé wrote: > >> On 12/3/19 5:14 AM, Andrew Jeffery wrote: > >>> Prepare for SoCs such as the ASPEED AST2600 whose firmware configures > >>> CNTFRQ to values significantly larger than the static 62.5MHz value > >>> currently derived from GTIMER_SCALE. As the OS potentially derives its > >>> timer periods from the CNTFRQ value the lack of support for running > >>> QEMUTimers at the appropriate rate leads to sticky behaviour in the > >>> guest. > >>> > >>> Substitute the GTIMER_SCALE constant with use of a helper to derive the > >>> period from gt_cntfrq stored in struct ARMCPU. Initially set gt_cntfrq > >>> to the frequency associated with GTIMER_SCALE so current behaviour is > >>> maintained. > >>> > >>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > >>> --- > >>> target/arm/cpu.c | 2 ++ > >>> target/arm/cpu.h | 10 ++++++++++ > >>> target/arm/helper.c | 10 +++++++--- > >>> 3 files changed, 19 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c > >>> index 7a4ac9339bf9..5698a74061bb 100644 > >>> --- a/target/arm/cpu.c > >>> +++ b/target/arm/cpu.c > >>> @@ -974,6 +974,8 @@ static void arm_cpu_initfn(Object *obj) > >>> if (tcg_enabled()) { > >>> cpu->psci_version = 2; /* TCG implements PSCI 0.2 */ > >>> } > >>> + > >>> + cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE; > >>> } > >>> > >>> static Property arm_cpu_reset_cbar_property = > >>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h > >>> index 83a809d4bac4..666c03871fdf 100644 > >>> --- a/target/arm/cpu.h > >>> +++ b/target/arm/cpu.h > >>> @@ -932,8 +932,18 @@ struct ARMCPU { > >>> */ > >>> DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ); > >>> DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ); > >>> + > >>> + /* Generic timer counter frequency, in Hz */ > >>> + uint64_t gt_cntfrq; > >> > >> You can also explicit the unit by calling it 'gt_cntfrq_hz'. > > > > Fair call, I'll fix that. > > > >> > >>> }; > >>> > >>> +static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > >>> +{ > >>> + /* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */ > >> > >> Why inline this call? I doubt there is a significant performance gain. > > > > It wasn't so much performance. It started out as a macro for a simple calculation > > because I didn't want to duplicate it across a number of places, then I wanted type > > safety for the pointer so I switched the macro in the header to an inline function. So > > it is an evolution of the patch rather than something that came from an explicit goal > > of e.g. performance. > > OK. Eventually NANOSECONDS_PER_SECOND will move to "qemu/units.h". > > Should the XXX comment stay? I'm not sure, it is confusing. I'll remove that. > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks. However, did you still want your comment on 4/4 addressed (move the comment to this patch)? Andrew
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 7a4ac9339bf9..5698a74061bb 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -974,6 +974,8 @@ static void arm_cpu_initfn(Object *obj) if (tcg_enabled()) { cpu->psci_version = 2; /* TCG implements PSCI 0.2 */ } + + cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE; } static Property arm_cpu_reset_cbar_property = diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 83a809d4bac4..666c03871fdf 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -932,8 +932,18 @@ struct ARMCPU { */ DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ); DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ); + + /* Generic timer counter frequency, in Hz */ + uint64_t gt_cntfrq; }; +static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) +{ + /* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */ + const unsigned int ns_per_s = 1000 * 1000 * 1000; + return ns_per_s > cpu->gt_cntfrq ? ns_per_s / cpu->gt_cntfrq : 1; +} + void arm_cpu_post_init(Object *obj); uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz); diff --git a/target/arm/helper.c b/target/arm/helper.c index 65c4441a3896..2622a9a8d02f 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2409,7 +2409,9 @@ static CPAccessResult gt_stimer_access(CPUARMState *env, static uint64_t gt_get_countervalue(CPUARMState *env) { - return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / GTIMER_SCALE; + ARMCPU *cpu = env_archcpu(env); + + return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(cpu); } static void gt_recalc_timer(ARMCPU *cpu, int timeridx) @@ -2445,7 +2447,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) * set the timer for as far in the future as possible. When the * timer expires we will reset the timer for any remaining period. */ - if (nexttick > INT64_MAX / GTIMER_SCALE) { + if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) { timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX); } else { timer_mod(cpu->gt_timer[timeridx], nexttick); @@ -2874,11 +2876,13 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = { static uint64_t gt_virt_cnt_read(CPUARMState *env, const ARMCPRegInfo *ri) { + ARMCPU *cpu = env_archcpu(env); + /* Currently we have no support for QEMUTimer in linux-user so we * can't call gt_get_countervalue(env), instead we directly * call the lower level functions. */ - return cpu_get_clock() / GTIMER_SCALE; + return cpu_get_clock() / gt_cntfrq_period_ns(cpu); } static const ARMCPRegInfo generic_timer_cp_reginfo[] = {