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

["Cédric Le Goater" <clg@kaod.org>]

On 12/09/2019 05:25, Andrew Jeffery wrote:
> Allow machines to configure CNTFRQ via a property if the ARM core
> supports the generic timer. This is necessary on e.g. the ASPEED AST2600
> SoC where the generic timer clock is run at 800MHz or above. The default
> value for CNTFRQ remains at 62.50MHz (based on GTIMER_SCALE).
> 
> CNTFRQ is a read-as-written co-processor register; the property sets the
> register's initial value which is used during realize() to configure the
> QEMUTimers that back the generic timers. Beyond that the firmware can to
> do whatever it sees fit with the CNTFRQ register though changes to the
> value will not be reflected in the timers' rate.
> 
> I've tested this using an out-of-tree AST2600 SoC model (Cortex-A7) with
> the SDK u-boot that sets CNTFRQ as appropriate, and by starting/running
> machines with assorted ARM CPUs (palmetto-bmc with the ARM926EJ-S,
> romulus-bmc with the ARM1176 and raspi2 with the Cortex-A53).
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> v4: Fix configuration for cores without a generic timer
> 
> v3: https://patchwork.ozlabs.org/patch/1160634/
> Peter - I think this addresses most of your feedback. I still reach into
> the QEMUTimer to fetch out scale when adjusting the nexttick
> calculation, but we no longer need to update the scale member and force
> a recalculation of the period.
> 
> v2: https://patchwork.ozlabs.org/patch/1144389/
> ---
>  roms/SLOF           |  2 +-
>  roms/skiboot        |  2 +-
>  target/arm/cpu.c    | 43 +++++++++++++++++++++++++++++++++++--------
>  target/arm/cpu.h    |  3 +++
>  target/arm/helper.c | 30 ++++++++++++++++++++++++++----
>  5 files changed, 66 insertions(+), 14 deletions(-)
> 
> diff --git a/roms/SLOF b/roms/SLOF
> index 7bfe584e3219..ea221600a116 160000
> --- a/roms/SLOF
> +++ b/roms/SLOF
> @@ -1 +1 @@
> -Subproject commit 7bfe584e321946771692711ff83ad2b5850daca7
> +Subproject commit ea221600a116883137ef90b2b7ab7d2472bc4f10
> diff --git a/roms/skiboot b/roms/skiboot
> index 261ca8e779e5..3a6fdede6ce1 160000
> --- a/roms/skiboot
> +++ b/roms/skiboot
> @@ -1 +1 @@
> -Subproject commit 261ca8e779e5138869a45f174caa49be6a274501
> +Subproject commit 3a6fdede6ce117facec0108afe716cf5d0472c3f


The changes above seem not related.

C. 


> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 2399c144718d..8b63a27761bb 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -40,6 +40,8 @@
>  #include "disas/capstone.h"
>  #include "fpu/softfloat.h"
>  
> +#include <inttypes.h>
> +
>  static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -976,6 +978,10 @@ static void arm_cpu_initfn(Object *obj)
>      }
>  }
>  
> +static Property arm_cpu_gt_cntfrq_property =
> +            DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq,
> +                               (1000 * 1000 * 1000) / GTIMER_SCALE);
> +
>  static Property arm_cpu_reset_cbar_property =
>              DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
>  
> @@ -1172,6 +1178,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)
> @@ -1238,14 +1249,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;
> +
> +        if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
> +            if (!cpu->gt_cntfrq) {
> +                error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz",
> +                           cpu->gt_cntfrq);
> +                return;
> +            }
> +            scale = MAX(1, NANOSECONDS_PER_SECOND / cpu->gt_cntfrq);
> +        } 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 297ad5e47ad8..8bd576f834ba 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -915,6 +915,9 @@ struct ARMCPU {
>  
>      /* Used to set the maximum vector length the cpu will support.  */
>      uint32_t sve_max_vq;
> +
> +    /* Used to configure the generic timer input clock */
> +    uint64_t gt_cntfrq;
>  };
>  
>  void arm_cpu_post_init(Object *obj);
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 507026c9154b..09975704d47f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2409,7 +2409,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)
> @@ -2445,8 +2459,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);
> @@ -2680,6 +2694,14 @@ 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 ?: (1000 * 1000 * 1000) / GTIMER_SCALE;
> +}
> +
>  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.
> @@ -2694,7 +2716,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,
>