Re: [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 12/3/19 5:14 AM, Andrew Jeffery wrote:
> The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On
> recent firmwares this is at 1125MHz, which is considerably quicker than
> the assumed 62.5MHz of the current generic timer implementation. The
> delta between the value as read from CNTFRQ and the true rate of the
> underlying QEMUTimer leads to sticky behaviour in AST2600 guests.
> 
> Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the
> generic timer. This allows platforms to configure CNTFRQ (and the
> associated QEMUTimer) to the appropriate frequency prior to starting the
> guest.
> 
> As the platform can now determine the rate of CNTFRQ we're exposed to
> limitations of QEMUTimer that didn't previously materialise: In the
> course of emulation we need to arbitrarily and accurately convert
> between guest ticks and time, but we're constrained by QEMUTimer's use
> of an integer scaling factor. The effect is QEMUTimer cannot exactly
> capture the period of frequencies that do not cleanly divide
> NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an
> equally inaccurate scaling factor for scaling time to ticks so at least
> a self-consistent inverse relationship holds.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/cpu.c    | 43 +++++++++++++++++++++++++++++++++----------
>   target/arm/cpu.h    | 18 ++++++++++++++++++
>   target/arm/helper.c |  9 ++++++++-
>   3 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 5698a74061bb..f186019a77fd 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -974,10 +974,12 @@ 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_gt_cntfrq_property =
> +            DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq,
> +                               NANOSECONDS_PER_SECOND / GTIMER_SCALE);
> +
>   static Property arm_cpu_reset_cbar_property =
>               DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
>   
> @@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj)
>   
>       qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property,
>                                &error_abort);
> +
> +    if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) {
> +        qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property,
> +                                 &error_abort);
> +    }
>   }
>   
>   static void arm_cpu_finalizefn(Object *obj)
> @@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>           }
>       }
>   
> -    cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> -                                           arm_gt_ptimer_cb, cpu);
> -    cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> -                                           arm_gt_vtimer_cb, cpu);
> -    cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> -                                          arm_gt_htimer_cb, cpu);
> -    cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> -                                          arm_gt_stimer_cb, cpu);
> +
> +    {
> +        uint64_t scale;

Apparently you have to use this odd indent due to the '#ifndef 
CONFIG_USER_ONLY'. Well, acceptable.

> +
> +        if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
> +            if (!cpu->gt_cntfrq) {
> +                error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz",
> +                           cpu->gt_cntfrq);
> +                return;
> +            }
> +            scale = gt_cntfrq_period_ns(cpu);
> +        } else {
> +            scale = GTIMER_SCALE;
> +        }
> +
> +        cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> +                                               arm_gt_ptimer_cb, cpu);
> +        cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> +                                               arm_gt_vtimer_cb, cpu);
> +        cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> +                                              arm_gt_htimer_cb, cpu);
> +        cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> +                                              arm_gt_stimer_cb, cpu);
> +    }
>   #endif
>   
>       cpu_exec_realizefn(cs, &local_err);
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 666c03871fdf..0bcd13dcac81 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -939,6 +939,24 @@ struct ARMCPU {
>   
>   static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
>   {
> +    /*
> +     * The exact approach to calculating guest ticks is:
> +     *
> +     *     muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), cpu->gt_cntfrq,
> +     *              NANOSECONDS_PER_SECOND);
> +     *
> +     * We don't do that. Rather we intentionally use integer division
> +     * truncation below and in the caller for the conversion of host monotonic
> +     * time to guest ticks to provide the exact inverse for the semantics of
> +     * the QEMUTimer scale factor. QEMUTimer's scale facter is an integer, so
> +     * it loses precision when representing frequencies where
> +     * `(NANOSECONDS_PER_SECOND % cpu->gt_cntfrq) > 0` holds. Failing to
> +     * provide an exact inverse leads to scheduling timers with negative
> +     * periods, which in turn leads to sticky behaviour in the guest.
> +     *
> +     * Finally, CNTFRQ is effectively capped at 1GHz to ensure our scale factor
> +     * cannot become zero.
> +     */

This comment belong to the previous patch. I'd rather see this function 
+ big comment in target/arm/cpu.c.

With comment moved (and if possible function uninlined):
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>       /* 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;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2622a9a8d02f..da960d17040b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2683,6 +2683,13 @@ void arm_gt_stimer_cb(void *opaque)
>       gt_recalc_timer(cpu, GTIMER_SEC);
>   }
>   
> +static void arm_gt_cntfrq_reset(CPUARMState *env, const ARMCPRegInfo *opaque)
> +{
> +    ARMCPU *cpu = env_archcpu(env);
> +
> +    cpu->env.cp15.c14_cntfrq = cpu->gt_cntfrq;
> +}
> +
>   static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>       /* Note that CNTFRQ is purely reads-as-written for the benefit
>        * of software; writing it doesn't actually change the timer frequency.
> @@ -2697,7 +2704,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
>         .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 0,
>         .access = PL1_RW | PL0_R, .accessfn = gt_cntfrq_access,
>         .fieldoffset = offsetof(CPUARMState, cp15.c14_cntfrq),
> -      .resetvalue = (1000 * 1000 * 1000) / GTIMER_SCALE,
> +      .resetfn = arm_gt_cntfrq_reset,
>       },
>       /* overall control: mostly access permissions */
>       { .name = "CNTKCTL", .state = ARM_CP_STATE_BOTH,
>