On Wed, 23 Jul 2025 at 14:52, Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
>
> Setting ARMCPU::gt_cntfrq_hz in hvf_arch_init_vcpu() is
> not correct because the timers have already be initialized
> with the default frequency.
>
> Set it earlier in the AccelOpsClass::cpu_target_realize()
> handler instead, and assert the value is correct when
> reaching hvf_arch_init_vcpu().
Could we say here what the user-visible consequences of
getting this wrong were ?
> Fixes: a1477da3dde ("hvf: Add Apple Silicon support")
> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> target/arm/hvf/hvf.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 7de770da4f3..ea9e6b1c0c6 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1007,6 +1007,13 @@ cleanup:
> return ret;
> }
>
> +static uint64_t get_cntfrq_el0(void)
> +{
> + uint64_t freq_hz = 0;
> + asm volatile("mrs %0, cntfrq_el0" : "=r"(freq_hz));
> + return freq_hz;
> +}
> +
> int hvf_arch_init_vcpu(CPUState *cpu)
> {
> ARMCPU *arm_cpu = ARM_CPU(cpu);
> @@ -1018,7 +1025,9 @@ int hvf_arch_init_vcpu(CPUState *cpu)
> int i;
>
> env->aarch64 = true;
> - asm volatile("mrs %0, cntfrq_el0" : "=r"(arm_cpu->gt_cntfrq_hz));
> +
> + /* system count frequency sanity check */
> + assert(arm_cpu->gt_cntfrq_hz == get_cntfrq_el0());
>
> /* Allocate enough space for our sysreg sync */
> arm_cpu->cpreg_indexes = g_renew(uint64_t, arm_cpu->cpreg_indexes,
> @@ -1083,6 +1092,10 @@ int hvf_arch_init_vcpu(CPUState *cpu)
>
> bool hvf_arch_cpu_realize(CPUState *cs, Error **errp)
> {
> + ARMCPU *cpu = ARM_CPU(cs);
> +
> + cpu->gt_cntfrq_hz = get_cntfrq_el0();
Maybe worth a short comment
/*
* We must set the counter frequency hvf will be using
* early, before arm_cpu_realizefn initializes the timers
* with it.
*/
What happens in the case where we're doing "try hvf, fall
back to tcg if hvf not possible" ? I guess we must figure out
that hvf won't work quite early, and well before we get to the
hvf_arch_cpu_realize() hook?
-- PMM
