* [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 @ 2019-12-03 4:14 Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 1/4] target/arm: Remove redundant scaling of nexttick Andrew Jeffery ` (4 more replies) 0 siblings, 5 replies; 13+ messages in thread From: Andrew Jeffery @ 2019-12-03 4:14 UTC (permalink / raw) To: qemu-arm; +Cc: qemu-devel, peter.maydell, clg, joel Hello, This is a v2 of the belated follow-up from a few of my earlier attempts to fix up the ARM generic timer for correct behaviour on the ASPEED AST2600 SoC. The AST2600 clocks the generic timer at the rate of HPLL, which is configured to 1125MHz. This is significantly quicker than the currently hard-coded generic timer rate of 62.5MHz and so we see "sticky" behaviour in the guest. v1 can be found here: https://patchwork.ozlabs.org/cover/1201887/ Changes since v1: * Fix a user mode build failure from partial renaming of gt_cntfrq_period_ns() * Add tags from Cedric and Richard Please review. Andrew Andrew Jeffery (4): target/arm: Remove redundant scaling of nexttick target/arm: Abstract the generic timer frequency target/arm: Prepare generic timer for per-platform CNTFRQ ast2600: Configure CNTFRQ at 1125MHz hw/arm/aspeed_ast2600.c | 3 +++ target/arm/cpu.c | 41 +++++++++++++++++++++++++++++++++-------- target/arm/cpu.h | 28 ++++++++++++++++++++++++++++ target/arm/helper.c | 24 ++++++++++++++++++------ 4 files changed, 82 insertions(+), 14 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/4] target/arm: Remove redundant scaling of nexttick 2019-12-03 4:14 [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Andrew Jeffery @ 2019-12-03 4:14 ` Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 2/4] target/arm: Abstract the generic timer frequency Andrew Jeffery ` (3 subsequent siblings) 4 siblings, 0 replies; 13+ messages in thread From: Andrew Jeffery @ 2019-12-03 4:14 UTC (permalink / raw) To: qemu-arm; +Cc: qemu-devel, peter.maydell, Richard Henderson, clg, joel The corner-case codepath was adjusting nexttick such that overflow wouldn't occur when timer_mod() scaled the value back up. Remove a use of GTIMER_SCALE and avoid unnecessary operations by calling timer_mod_ns() directly. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> --- target/arm/helper.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index a089fb5a6909..65c4441a3896 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2446,9 +2446,10 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) * timer expires we will reset the timer for any remaining period. */ if (nexttick > INT64_MAX / GTIMER_SCALE) { - nexttick = INT64_MAX / GTIMER_SCALE; + timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX); + } else { + timer_mod(cpu->gt_timer[timeridx], nexttick); } - timer_mod(cpu->gt_timer[timeridx], nexttick); trace_arm_gt_recalc(timeridx, irqstate, nexttick); } else { /* Timer disabled: ISTATUS and timer output always clear */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] target/arm: Abstract the generic timer frequency 2019-12-03 4:14 [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 1/4] target/arm: Remove redundant scaling of nexttick Andrew Jeffery @ 2019-12-03 4:14 ` Andrew Jeffery 2019-12-03 6:09 ` Philippe Mathieu-Daudé 2019-12-03 4:14 ` [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ Andrew Jeffery ` (2 subsequent siblings) 4 siblings, 1 reply; 13+ messages in thread From: Andrew Jeffery @ 2019-12-03 4:14 UTC (permalink / raw) To: qemu-arm; +Cc: qemu-devel, peter.maydell, Richard Henderson, clg, joel 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; }; +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[] = { -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency 2019-12-03 4:14 ` [PATCH v2 2/4] target/arm: Abstract the generic timer frequency Andrew Jeffery @ 2019-12-03 6:09 ` Philippe Mathieu-Daudé 2019-12-03 12:48 ` Andrew Jeffery 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-03 6:09 UTC (permalink / raw) To: Andrew Jeffery, qemu-arm Cc: joel, peter.maydell, Richard Henderson, qemu-devel, clg 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[] = { > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency 2019-12-03 6:09 ` Philippe Mathieu-Daudé @ 2019-12-03 12:48 ` Andrew Jeffery 2019-12-03 17:27 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Andrew Jeffery @ 2019-12-03 12:48 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-arm Cc: Joel Stanley, Peter Maydell, Richard Henderson, qemu-devel, Cédric Le Goater 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency 2019-12-03 12:48 ` Andrew Jeffery @ 2019-12-03 17:27 ` Philippe Mathieu-Daudé 2019-12-05 5:04 ` Andrew Jeffery 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-03 17:27 UTC (permalink / raw) To: Andrew Jeffery, qemu-arm Cc: Joel Stanley, Peter Maydell, Richard Henderson, qemu-devel, Cédric Le Goater 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> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency 2019-12-03 17:27 ` Philippe Mathieu-Daudé @ 2019-12-05 5:04 ` Andrew Jeffery 0 siblings, 0 replies; 13+ messages in thread From: Andrew Jeffery @ 2019-12-05 5:04 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-arm Cc: Joel Stanley, Peter Maydell, Richard Henderson, qemu-devel, Cédric Le Goater 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ 2019-12-03 4:14 [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 1/4] target/arm: Remove redundant scaling of nexttick Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 2/4] target/arm: Abstract the generic timer frequency Andrew Jeffery @ 2019-12-03 4:14 ` Andrew Jeffery 2019-12-03 6:19 ` Philippe Mathieu-Daudé 2019-12-03 4:14 ` [PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz Andrew Jeffery 2019-12-03 6:05 ` [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Philippe Mathieu-Daudé 4 siblings, 1 reply; 13+ messages in thread From: Andrew Jeffery @ 2019-12-03 4:14 UTC (permalink / raw) To: qemu-arm; +Cc: qemu-devel, peter.maydell, Richard Henderson, clg, joel The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On recent firmwares this is at 1125MHz, which is considerably quicker than the assumed 62.5MHz of the current generic timer implementation. The delta between the value as read from CNTFRQ and the true rate of the underlying QEMUTimer leads to sticky behaviour in AST2600 guests. Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the generic timer. This allows platforms to configure CNTFRQ (and the associated QEMUTimer) to the appropriate frequency prior to starting the guest. As the platform can now determine the rate of CNTFRQ we're exposed to limitations of QEMUTimer that didn't previously materialise: In the course of emulation we need to arbitrarily and accurately convert between guest ticks and time, but we're constrained by QEMUTimer's use of an integer scaling factor. The effect is QEMUTimer cannot exactly capture the period of frequencies that do not cleanly divide NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an equally inaccurate scaling factor for scaling time to ticks so at least a self-consistent inverse relationship holds. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.c | 43 +++++++++++++++++++++++++++++++++---------- target/arm/cpu.h | 18 ++++++++++++++++++ target/arm/helper.c | 9 ++++++++- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 5698a74061bb..f186019a77fd 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -974,10 +974,12 @@ 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_gt_cntfrq_property = + DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq, + NANOSECONDS_PER_SECOND / GTIMER_SCALE); + static Property arm_cpu_reset_cbar_property = DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0); @@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj) qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property, &error_abort); + + if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) { + qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property, + &error_abort); + } } static void arm_cpu_finalizefn(Object *obj) @@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } } - cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, - 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); - cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, - arm_gt_stimer_cb, cpu); + + { + uint64_t scale; + + if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { + if (!cpu->gt_cntfrq) { + error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz", + cpu->gt_cntfrq); + return; + } + scale = gt_cntfrq_period_ns(cpu); + } else { + scale = GTIMER_SCALE; + } + + cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale, + arm_gt_ptimer_cb, cpu); + cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale, + arm_gt_vtimer_cb, cpu); + cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale, + arm_gt_htimer_cb, cpu); + cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale, + arm_gt_stimer_cb, cpu); + } #endif cpu_exec_realizefn(cs, &local_err); diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 666c03871fdf..0bcd13dcac81 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -939,6 +939,24 @@ struct ARMCPU { static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) { + /* + * The exact approach to calculating guest ticks is: + * + * muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), cpu->gt_cntfrq, + * NANOSECONDS_PER_SECOND); + * + * We don't do that. Rather we intentionally use integer division + * truncation below and in the caller for the conversion of host monotonic + * time to guest ticks to provide the exact inverse for the semantics of + * the QEMUTimer scale factor. QEMUTimer's scale facter is an integer, so + * it loses precision when representing frequencies where + * `(NANOSECONDS_PER_SECOND % cpu->gt_cntfrq) > 0` holds. Failing to + * provide an exact inverse leads to scheduling timers with negative + * periods, which in turn leads to sticky behaviour in the guest. + * + * Finally, CNTFRQ is effectively capped at 1GHz to ensure our scale factor + * cannot become zero. + */ /* 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; diff --git a/target/arm/helper.c b/target/arm/helper.c index 2622a9a8d02f..da960d17040b 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -2683,6 +2683,13 @@ void arm_gt_stimer_cb(void *opaque) gt_recalc_timer(cpu, GTIMER_SEC); } +static void arm_gt_cntfrq_reset(CPUARMState *env, const ARMCPRegInfo *opaque) +{ + ARMCPU *cpu = env_archcpu(env); + + cpu->env.cp15.c14_cntfrq = cpu->gt_cntfrq; +} + 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. @@ -2697,7 +2704,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = { .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 0, .access = PL1_RW | PL0_R, .accessfn = gt_cntfrq_access, .fieldoffset = offsetof(CPUARMState, cp15.c14_cntfrq), - .resetvalue = (1000 * 1000 * 1000) / GTIMER_SCALE, + .resetfn = arm_gt_cntfrq_reset, }, /* overall control: mostly access permissions */ { .name = "CNTKCTL", .state = ARM_CP_STATE_BOTH, -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ 2019-12-03 4:14 ` [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ Andrew Jeffery @ 2019-12-03 6:19 ` Philippe Mathieu-Daudé 2019-12-03 12:59 ` Andrew Jeffery 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-03 6:19 UTC (permalink / raw) To: Andrew Jeffery, qemu-arm Cc: joel, peter.maydell, Richard Henderson, qemu-devel, clg On 12/3/19 5:14 AM, Andrew Jeffery wrote: > The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On > recent firmwares this is at 1125MHz, which is considerably quicker than > the assumed 62.5MHz of the current generic timer implementation. The > delta between the value as read from CNTFRQ and the true rate of the > underlying QEMUTimer leads to sticky behaviour in AST2600 guests. > > Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the > generic timer. This allows platforms to configure CNTFRQ (and the > associated QEMUTimer) to the appropriate frequency prior to starting the > guest. > > As the platform can now determine the rate of CNTFRQ we're exposed to > limitations of QEMUTimer that didn't previously materialise: In the > course of emulation we need to arbitrarily and accurately convert > between guest ticks and time, but we're constrained by QEMUTimer's use > of an integer scaling factor. The effect is QEMUTimer cannot exactly > capture the period of frequencies that do not cleanly divide > NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an > equally inaccurate scaling factor for scaling time to ticks so at least > a self-consistent inverse relationship holds. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.c | 43 +++++++++++++++++++++++++++++++++---------- > target/arm/cpu.h | 18 ++++++++++++++++++ > target/arm/helper.c | 9 ++++++++- > 3 files changed, 59 insertions(+), 11 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 5698a74061bb..f186019a77fd 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -974,10 +974,12 @@ 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_gt_cntfrq_property = > + DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq, > + NANOSECONDS_PER_SECOND / GTIMER_SCALE); > + > static Property arm_cpu_reset_cbar_property = > DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0); > > @@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj) > > qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property, > &error_abort); > + > + if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) { > + qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property, > + &error_abort); > + } > } > > static void arm_cpu_finalizefn(Object *obj) > @@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > } > } > > - cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, > - 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); > - cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, > - arm_gt_stimer_cb, cpu); > + > + { > + uint64_t scale; Apparently you have to use this odd indent due to the '#ifndef CONFIG_USER_ONLY'. Well, acceptable. > + > + if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { > + if (!cpu->gt_cntfrq) { > + error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz", > + cpu->gt_cntfrq); > + return; > + } > + scale = gt_cntfrq_period_ns(cpu); > + } else { > + scale = GTIMER_SCALE; > + } > + > + cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > + arm_gt_ptimer_cb, cpu); > + cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > + arm_gt_vtimer_cb, cpu); > + cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > + arm_gt_htimer_cb, cpu); > + cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > + arm_gt_stimer_cb, cpu); > + } > #endif > > cpu_exec_realizefn(cs, &local_err); > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 666c03871fdf..0bcd13dcac81 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -939,6 +939,24 @@ struct ARMCPU { > > static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > { > + /* > + * The exact approach to calculating guest ticks is: > + * > + * muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), cpu->gt_cntfrq, > + * NANOSECONDS_PER_SECOND); > + * > + * We don't do that. Rather we intentionally use integer division > + * truncation below and in the caller for the conversion of host monotonic > + * time to guest ticks to provide the exact inverse for the semantics of > + * the QEMUTimer scale factor. QEMUTimer's scale facter is an integer, so > + * it loses precision when representing frequencies where > + * `(NANOSECONDS_PER_SECOND % cpu->gt_cntfrq) > 0` holds. Failing to > + * provide an exact inverse leads to scheduling timers with negative > + * periods, which in turn leads to sticky behaviour in the guest. > + * > + * Finally, CNTFRQ is effectively capped at 1GHz to ensure our scale factor > + * cannot become zero. > + */ This comment belong to the previous patch. I'd rather see this function + big comment in target/arm/cpu.c. With comment moved (and if possible function uninlined): Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > /* 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; > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 2622a9a8d02f..da960d17040b 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -2683,6 +2683,13 @@ void arm_gt_stimer_cb(void *opaque) > gt_recalc_timer(cpu, GTIMER_SEC); > } > > +static void arm_gt_cntfrq_reset(CPUARMState *env, const ARMCPRegInfo *opaque) > +{ > + ARMCPU *cpu = env_archcpu(env); > + > + cpu->env.cp15.c14_cntfrq = cpu->gt_cntfrq; > +} > + > 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. > @@ -2697,7 +2704,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = { > .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 0, > .access = PL1_RW | PL0_R, .accessfn = gt_cntfrq_access, > .fieldoffset = offsetof(CPUARMState, cp15.c14_cntfrq), > - .resetvalue = (1000 * 1000 * 1000) / GTIMER_SCALE, > + .resetfn = arm_gt_cntfrq_reset, > }, > /* overall control: mostly access permissions */ > { .name = "CNTKCTL", .state = ARM_CP_STATE_BOTH, > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ 2019-12-03 6:19 ` Philippe Mathieu-Daudé @ 2019-12-03 12:59 ` Andrew Jeffery 0 siblings, 0 replies; 13+ messages in thread From: Andrew Jeffery @ 2019-12-03 12:59 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-arm Cc: Joel Stanley, Peter Maydell, Richard Henderson, qemu-devel, Cédric Le Goater On Tue, 3 Dec 2019, at 16:49, Philippe Mathieu-Daudé wrote: > On 12/3/19 5:14 AM, Andrew Jeffery wrote: > > The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On > > recent firmwares this is at 1125MHz, which is considerably quicker than > > the assumed 62.5MHz of the current generic timer implementation. The > > delta between the value as read from CNTFRQ and the true rate of the > > underlying QEMUTimer leads to sticky behaviour in AST2600 guests. > > > > Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the > > generic timer. This allows platforms to configure CNTFRQ (and the > > associated QEMUTimer) to the appropriate frequency prior to starting the > > guest. > > > > As the platform can now determine the rate of CNTFRQ we're exposed to > > limitations of QEMUTimer that didn't previously materialise: In the > > course of emulation we need to arbitrarily and accurately convert > > between guest ticks and time, but we're constrained by QEMUTimer's use > > of an integer scaling factor. The effect is QEMUTimer cannot exactly > > capture the period of frequencies that do not cleanly divide > > NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an > > equally inaccurate scaling factor for scaling time to ticks so at least > > a self-consistent inverse relationship holds. > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > --- > > target/arm/cpu.c | 43 +++++++++++++++++++++++++++++++++---------- > > target/arm/cpu.h | 18 ++++++++++++++++++ > > target/arm/helper.c | 9 ++++++++- > > 3 files changed, 59 insertions(+), 11 deletions(-) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 5698a74061bb..f186019a77fd 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -974,10 +974,12 @@ 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_gt_cntfrq_property = > > + DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq, > > + NANOSECONDS_PER_SECOND / GTIMER_SCALE); > > + > > static Property arm_cpu_reset_cbar_property = > > DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0); > > > > @@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj) > > > > qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property, > > &error_abort); > > + > > + if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) { > > + qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property, > > + &error_abort); > > + } > > } > > > > static void arm_cpu_finalizefn(Object *obj) > > @@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > > } > > } > > > > - cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, > > - 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); > > - cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE, > > - arm_gt_stimer_cb, cpu); > > + > > + { > > + uint64_t scale; > > Apparently you have to use this odd indent due to the '#ifndef > CONFIG_USER_ONLY'. Well, acceptable. It's the indent associated with the block scope for the scale variable to limit its lifetime to where I needed it. > > > + > > + if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) { > > + if (!cpu->gt_cntfrq) { > > + error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz", > > + cpu->gt_cntfrq); > > + return; > > + } > > + scale = gt_cntfrq_period_ns(cpu); > > + } else { > > + scale = GTIMER_SCALE; > > + } > > + > > + cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > > + arm_gt_ptimer_cb, cpu); > > + cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > > + arm_gt_vtimer_cb, cpu); > > + cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > > + arm_gt_htimer_cb, cpu); > > + cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale, > > + arm_gt_stimer_cb, cpu); > > + } > > #endif > > > > cpu_exec_realizefn(cs, &local_err); > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index 666c03871fdf..0bcd13dcac81 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -939,6 +939,24 @@ struct ARMCPU { > > > > static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu) > > { > > + /* > > + * The exact approach to calculating guest ticks is: > > + * > > + * muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), cpu->gt_cntfrq, > > + * NANOSECONDS_PER_SECOND); > > + * > > + * We don't do that. Rather we intentionally use integer division > > + * truncation below and in the caller for the conversion of host monotonic > > + * time to guest ticks to provide the exact inverse for the semantics of > > + * the QEMUTimer scale factor. QEMUTimer's scale facter is an integer, so > > + * it loses precision when representing frequencies where > > + * `(NANOSECONDS_PER_SECOND % cpu->gt_cntfrq) > 0` holds. Failing to > > + * provide an exact inverse leads to scheduling timers with negative > > + * periods, which in turn leads to sticky behaviour in the guest. > > + * > > + * Finally, CNTFRQ is effectively capped at 1GHz to ensure our scale factor > > + * cannot become zero. > > + */ > > This comment belong to the previous patch. Sort of, but also sort of not. We don't expose the limitation until this patch as NANOSECONDS_PER_SECOND is an integer multiple of GTIMER_SCALE, which is what gt_cntfrq is set to until we add the property to configure it to arbitrary values in this patch. So I added the comment in this patch rather than the previous one which adds the code. Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz 2019-12-03 4:14 [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Andrew Jeffery ` (2 preceding siblings ...) 2019-12-03 4:14 ` [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ Andrew Jeffery @ 2019-12-03 4:14 ` Andrew Jeffery 2019-12-03 6:20 ` Philippe Mathieu-Daudé 2019-12-03 6:05 ` [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Philippe Mathieu-Daudé 4 siblings, 1 reply; 13+ messages in thread From: Andrew Jeffery @ 2019-12-03 4:14 UTC (permalink / raw) To: qemu-arm; +Cc: qemu-devel, peter.maydell, Richard Henderson, clg, joel This matches the configuration set by u-boot on the AST2600. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> --- hw/arm/aspeed_ast2600.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index 931887ac681f..5aecc3b3caec 100644 --- a/hw/arm/aspeed_ast2600.c +++ b/hw/arm/aspeed_ast2600.c @@ -259,6 +259,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) object_property_set_int(OBJECT(&s->cpu[i]), aspeed_calc_affinity(i), "mp-affinity", &error_abort); + object_property_set_int(OBJECT(&s->cpu[i]), 1125000000, "cntfrq", + &error_abort); + /* * TODO: the secondary CPUs are started and a boot helper * is needed when using -kernel -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz 2019-12-03 4:14 ` [PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz Andrew Jeffery @ 2019-12-03 6:20 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-03 6:20 UTC (permalink / raw) To: Andrew Jeffery, qemu-arm Cc: joel, peter.maydell, Richard Henderson, qemu-devel, clg On 12/3/19 5:14 AM, Andrew Jeffery wrote: > This matches the configuration set by u-boot on the AST2600. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/arm/aspeed_ast2600.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c > index 931887ac681f..5aecc3b3caec 100644 > --- a/hw/arm/aspeed_ast2600.c > +++ b/hw/arm/aspeed_ast2600.c > @@ -259,6 +259,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) > object_property_set_int(OBJECT(&s->cpu[i]), aspeed_calc_affinity(i), > "mp-affinity", &error_abort); > > + object_property_set_int(OBJECT(&s->cpu[i]), 1125000000, "cntfrq", > + &error_abort); > + > /* > * TODO: the secondary CPUs are started and a boot helper > * is needed when using -kernel > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 2019-12-03 4:14 [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Andrew Jeffery ` (3 preceding siblings ...) 2019-12-03 4:14 ` [PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz Andrew Jeffery @ 2019-12-03 6:05 ` Philippe Mathieu-Daudé 4 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-03 6:05 UTC (permalink / raw) To: Andrew Jeffery, qemu-arm; +Cc: peter.maydell, joel, qemu-devel, clg On 12/3/19 5:14 AM, Andrew Jeffery wrote: > Hello, > > This is a v2 of the belated follow-up from a few of my earlier attempts to fix > up the ARM generic timer for correct behaviour on the ASPEED AST2600 SoC. The > AST2600 clocks the generic timer at the rate of HPLL, which is configured to > 1125MHz. This is significantly quicker than the currently hard-coded generic > timer rate of 62.5MHz and so we see "sticky" behaviour in the guest. Glad you fixed this! I hit the same problem with the Raspi4. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-12-05 5:04 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-03 4:14 [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 1/4] target/arm: Remove redundant scaling of nexttick Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 2/4] target/arm: Abstract the generic timer frequency Andrew Jeffery 2019-12-03 6:09 ` Philippe Mathieu-Daudé 2019-12-03 12:48 ` Andrew Jeffery 2019-12-03 17:27 ` Philippe Mathieu-Daudé 2019-12-05 5:04 ` Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ Andrew Jeffery 2019-12-03 6:19 ` Philippe Mathieu-Daudé 2019-12-03 12:59 ` Andrew Jeffery 2019-12-03 4:14 ` [PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz Andrew Jeffery 2019-12-03 6:20 ` Philippe Mathieu-Daudé 2019-12-03 6:05 ` [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).