Re: [Qemu-devel] [PATCH v2] target-arm: Make the counter tick relative to cntfrq

[Andrew Jeffery]

On Wed, 28 Aug 2019, at 01:39, Peter Maydell wrote:
> On Fri, 9 Aug 2019 at 06:43, Andrew Jeffery <address@hidden> wrote:
> >
> > The use of GTIMER_SCALE assumes the clock feeding the generic timer is
> > 62.5MHz for all platforms. This is untrue in general, for example the
> > ASPEED AST2600 feeds the counter with either an 800 or 1200MHz clock,
> > and CNTFRQ is configured appropriately by u-boot.
> >
> > To cope with these values we need to take care of the quantization
> > caused by the clock scaling in terms of nanoseconds per clock by
> > calculating an effective frequency such that NANOSECONDS_PER_SECOND is
> > an integer multiple of the effective frequency. Failing to account for
> > the quantisation leads to sticky behaviour in the VM as the guest timer
> > subsystems account for the difference between delay time and the counter
> > value.
> >
> > Signed-off-by: Andrew Jeffery <address@hidden>
> > ---
> > v2:
> > 1. Removed the user-mode change that broke v1
> > 2. Rearranged the implementation as a consequence of 1.
> >
> >  target/arm/helper.c | 51 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 48 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index b74c23a9bc08..166a54daf278 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2277,6 +2277,34 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
> >
> >  #ifndef CONFIG_USER_ONLY
> >
> > +static void gt_recalc_timer(ARMCPU *cpu, int timeridx);
> > +static void gt_cntfrq_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > +                            uint64_t value)
> > +{
> > +    uint64_t scale;
> > +    ARMCPU *cpu;
> > +    int i;
> > +
> > +    raw_write(env, ri, value);
> > +
> > +    /* Fix up the timer scaling */
> > +    cpu = env_archcpu(env);
> > +    scale = MAX(1, NANOSECONDS_PER_SECOND / value);
> > +    for (i = 0; i < NUM_GTIMERS; i++) {
> > +        if (!cpu->gt_timer[i]) {
> > +            continue;
> > +        }
> > +
> > +        cpu->gt_timer[i]->scale = scale;
> 
> Reaching into the internals of a QEMUTimer and changing
> the 'scale' value seems like a bad idea. If QEMUTimer doesn't
> have a facility for changing the frequency and we need one
> then we should add one at that API layer.

Yeah, fair point. It is an RFC patch for these kinds of reasons -
I solved the problem but wasn't at all convinced about the
shape of the solution.

> 
> Also, this isn't how the hardware works, I'm pretty sure.
> In hardware the timers tick at whatever frequency they're
> set up to tick, and CNTFRQ is just a reads-as-written register
> that firmware can fill in with an appropriate value that it's
> determined from somewhere for the benefit of other software.

Yes, I think you're right. Again, as above this got rid of the
problem, but needed some massaging. The write just was a
handy hook point to inject the change of frequency.

> 
> If on ASPEED SoCs the timer frequency is different, then we
> should model that by having CPU properties for timer frequency
> and having the SoC set those properties, I think.

Sounds good, I'll work on that approach.

Thanks for the feedback.

Andrew