| Message ID | 20190809054341.24728-1-andrew@aj.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] target-arm: Make the counter tick relative to cntfrq | expand |
On Fri, 9 Aug 2019 at 06:43, Andrew Jeffery <andrew@aj.id.au> 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 <andrew@aj.id.au> > --- > 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. 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. 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. > + gt_recalc_timer(cpu, i); > + } > +} thanks -- PMM
On Wed, 28 Aug 2019, at 01:39, Peter Maydell wrote: > On Fri, 9 Aug 2019 at 06:43, Andrew Jeffery <andrew@aj.id.au> 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 <andrew@aj.id.au> > > --- > > 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
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 <andrew@aj.id.au> 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 <andrew@aj.id.au> > > > --- > > > 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
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; + gt_recalc_timer(cpu, i); + } +} + +static void gt_cntfrq_reset(CPUARMState *env, const ARMCPRegInfo *opaque) +{ + gt_cntfrq_write(env, opaque, opaque->resetvalue); +} + static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri, bool isread) { @@ -2407,7 +2435,21 @@ static CPAccessResult gt_stimer_access(CPUARMState *env, static uint64_t gt_get_countervalue(CPUARMState *env) { - return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / GTIMER_SCALE; + uint64_t effective; + + /* + * Deal with quantized clock scaling by calculating the effective frequency + * in terms of the timer scale. + */ + if (env->cp15.c14_cntfrq <= NANOSECONDS_PER_SECOND) { + uint32_t scale = NANOSECONDS_PER_SECOND / env->cp15.c14_cntfrq; + effective = NANOSECONDS_PER_SECOND / scale; + } else { + effective = NANOSECONDS_PER_SECOND; + } + + return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), effective, + NANOSECONDS_PER_SECOND); } static void gt_recalc_timer(ARMCPU *cpu, int timeridx) @@ -2443,8 +2485,8 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx) * set the timer for as far in the future as possible. When the * timer expires we will reset the timer for any remaining period. */ - if (nexttick > INT64_MAX / GTIMER_SCALE) { - nexttick = INT64_MAX / GTIMER_SCALE; + if (nexttick > INT64_MAX / cpu->gt_timer[timeridx]->scale) { + nexttick = INT64_MAX / cpu->gt_timer[timeridx]->scale; } timer_mod(cpu->gt_timer[timeridx], nexttick); trace_arm_gt_recalc(timeridx, irqstate, nexttick); @@ -2686,13 +2728,16 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = { { .name = "CNTFRQ", .cp = 15, .crn = 14, .crm = 0, .opc1 = 0, .opc2 = 0, .type = ARM_CP_ALIAS, .access = PL1_RW | PL0_R, .accessfn = gt_cntfrq_access, + .writefn = gt_cntfrq_write, .fieldoffset = offsetoflow32(CPUARMState, cp15.c14_cntfrq), }, { .name = "CNTFRQ_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 0, .access = PL1_RW | PL0_R, .accessfn = gt_cntfrq_access, + .writefn = gt_cntfrq_write, .fieldoffset = offsetof(CPUARMState, cp15.c14_cntfrq), .resetvalue = (1000 * 1000 * 1000) / GTIMER_SCALE, + .resetfn = gt_cntfrq_reset, }, /* overall control: mostly access permissions */ { .name = "CNTKCTL", .state = ARM_CP_STATE_BOTH,
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 <andrew@aj.id.au> --- 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(-)