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

[Andrew Jeffery]

On Thu, 28 Nov 2019, at 19:16, Cédric Le Goater wrote:
> On 28/11/2019 06:45, 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>
> > ---
> >  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;
> > +}
> 
> Are you inlining this helper for performance reasons ? 

Originally I was going to do it as a macro in order to avoid redundantly 
scattering
the calculation around. My thought was to use a macro as it's a simple 
calculation,
but then figured it was a bit nicer as a function for type safety. I already 
had it as a
macro in the header, so it was the least effort to switch it to a static inline 
and leave
it where it was :) So that's the justification, mostly just evolution of 
thought process.
Performance was also a consideration but I've done no measurements.

Andrew