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

[Andrew Jeffery]

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 <address@hidden>
> > Reviewed-by: Richard Henderson <address@hidden>
> > ---
> >   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