Re: [PATCH v2 2/4] target/arm: Abstract the generic timer frequency

["Andrew Jeffery" <andrew@aj.id.au>]

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