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

[Andrew Jeffery]

On Thu, 29 Aug 2019, at 11:08, Andrew Jeffery wrote:
> 
> 
> 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

Ugh, I just looked at the subject and realised I hadn't added "RFC".

Too many things going on :/

Andrew