[PATCH-for-10.1 v4 0/4] target/arm/hvf: Correctly set Generic Timer frequency

[PATCH-for-10.1 v4 0/4] target/arm/hvf: Correctly set Generic Timer frequency

[Philippe Mathieu-Daudé]

(Series fully reviewed)

When using HVF, the accelerator gets the host Generic Timer
frequency and update cpu->gt_cntfrq_hz; it is however too
late, as the timers have been created in arm_cpu_realizefn()
using the default frequency. Then guest virtualization code
depending on timers ends very slow (experienced on Silicon M1,
timer running at 24MHz but QEMU timer a 1GHz, ~70x slower).

This series fixes that by introducing a cpu_target_realize()
callback for accelerators, creating ARM timers *after* calling
this callback, and correctly setting the per-cpu timer freq
there.

Philippe Mathieu-Daudé (4):
  accel: Introduce AccelOpsClass::cpu_target_realize() hook
  accel/hvf: Add hvf_arch_cpu_realize() stubs
  target/arm: Create GTimers *after* features finalized / accel realized
  target/arm/hvf: Really set Generic Timer counter frequency

 include/accel/accel-cpu-ops.h |  1 +
 include/system/hvf_int.h      |  2 ++
 accel/accel-common.c          |  5 +++
 accel/hvf/hvf-accel-ops.c     |  2 ++
 target/arm/cpu.c              | 65 ++++++++++++++++++-----------------
 target/arm/hvf/hvf.c          | 20 ++++++++++-
 target/i386/hvf/hvf.c         |  5 +++
 7 files changed, 67 insertions(+), 33 deletions(-)

-- 
2.49.0

[PATCH-for-10.1 v4 1/4] accel: Introduce AccelOpsClass::cpu_target_realize() hook

[Philippe Mathieu-Daudé]

Allow accelerators to set vCPU properties before its realization.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/accel/accel-cpu-ops.h | 1 +
 accel/accel-common.c          | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/accel/accel-cpu-ops.h b/include/accel/accel-cpu-ops.h
index 0674764914f..9c07a903ea0 100644
--- a/include/accel/accel-cpu-ops.h
+++ b/include/accel/accel-cpu-ops.h
@@ -34,6 +34,7 @@ struct AccelOpsClass {
     /* initialization function called when accel is chosen */
     void (*ops_init)(AccelClass *ac);
 
+    bool (*cpu_target_realize)(CPUState *cpu, Error **errp);
     bool (*cpus_are_resettable)(void);
     void (*cpu_reset_hold)(CPUState *cpu);
 
diff --git a/accel/accel-common.c b/accel/accel-common.c
index 850c5ab4b8e..eecb2a292af 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -106,6 +106,11 @@ bool accel_cpu_common_realize(CPUState *cpu, Error **errp)
     if (acc->cpu_common_realize && !acc->cpu_common_realize(cpu, errp)) {
         return false;
     }
+    if (acc->ops
+        && acc->ops->cpu_target_realize
+        && !acc->ops->cpu_target_realize(cpu, errp)) {
+        return false;
+    }
 
     return true;
 }
-- 
2.49.0

[PATCH-for-10.1 v4 2/4] accel/hvf: Add hvf_arch_cpu_realize() stubs

[Philippe Mathieu-Daudé]

Implement HVF AccelOpsClass::cpu_target_realize() hook as
empty stubs. Target implementations will come separately.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/system/hvf_int.h  | 2 ++
 accel/hvf/hvf-accel-ops.c | 2 ++
 target/arm/hvf/hvf.c      | 5 +++++
 target/i386/hvf/hvf.c     | 5 +++++
 4 files changed, 14 insertions(+)

diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h
index a3b06a3e75b..cbd59525e87 100644
--- a/include/system/hvf_int.h
+++ b/include/system/hvf_int.h
@@ -111,4 +111,6 @@ void hvf_arch_update_guest_debug(CPUState *cpu);
  */
 bool hvf_arch_supports_guest_debug(void);
 
+bool hvf_arch_cpu_realize(CPUState *cpu, Error **errp);
+
 #endif
diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index d488d6afbac..6c9369388b8 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -373,6 +373,8 @@ static void hvf_accel_ops_class_init(ObjectClass *oc, const void *data)
 {
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
+    ops->cpu_target_realize = hvf_arch_cpu_realize;
+
     ops->create_vcpu_thread = hvf_start_vcpu_thread;
     ops->kick_vcpu_thread = hvf_kick_vcpu_thread;
     ops->handle_interrupt = generic_handle_interrupt;
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index bd6b5d11de8..7de770da4f3 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1081,6 +1081,11 @@ int hvf_arch_init_vcpu(CPUState *cpu)
     return 0;
 }
 
+bool hvf_arch_cpu_realize(CPUState *cs, Error **errp)
+{
+    return true;
+}
+
 void hvf_kick_vcpu_thread(CPUState *cpu)
 {
     cpus_kick_thread(cpu);
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 818b50419f4..a24786c8c35 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -367,6 +367,11 @@ int hvf_arch_init_vcpu(CPUState *cpu)
     return 0;
 }
 
+bool hvf_arch_cpu_realize(CPUState *cs, Error **errp)
+{
+    return true;
+}
+
 static void hvf_store_events(CPUState *cpu, uint32_t ins_len, uint64_t idtvec_info)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);
-- 
2.49.0

[PATCH-for-10.1 v4 3/4] target/arm: Create GTimers *after* features finalized / accel realized

[Philippe Mathieu-Daudé]

Call generic (including accelerator) cpu_realize() handlers
*before* setting @gt_cntfrq_hz default

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.c | 65 ++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index e2b2337399c..70755fc7e87 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1992,26 +1992,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (!cpu->gt_cntfrq_hz) {
-        /*
-         * 0 means "the board didn't set a value, use the default". (We also
-         * get here for the CONFIG_USER_ONLY case.)
-         * ARMv8.6 and later CPUs architecturally must use a 1GHz timer; before
-         * that it was an IMPDEF choice, and QEMU initially picked 62.5MHz,
-         * which gives a 16ns tick period.
-         *
-         * We will use the back-compat value:
-         *  - for QEMU CPU types added before we standardized on 1GHz
-         *  - for versioned machine types with a version of 9.0 or earlier
-         */
-        if (arm_feature(env, ARM_FEATURE_BACKCOMPAT_CNTFRQ) ||
-            cpu->backcompat_cntfrq) {
-            cpu->gt_cntfrq_hz = GTIMER_BACKCOMPAT_HZ;
-        } else {
-            cpu->gt_cntfrq_hz = GTIMER_DEFAULT_HZ;
-        }
-    }
-
 #ifndef CONFIG_USER_ONLY
     /* The NVIC and M-profile CPU are two halves of a single piece of
      * hardware; trying to use one without the other is a command line
@@ -2058,7 +2038,40 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
             return;
         }
     }
+#endif
 
+    cpu_exec_realizefn(cs, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    arm_cpu_finalize_features(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    if (!cpu->gt_cntfrq_hz) {
+        /*
+         * 0 means "the board didn't set a value, use the default". (We also
+         * get here for the CONFIG_USER_ONLY case.)
+         * ARMv8.6 and later CPUs architecturally must use a 1GHz timer; before
+         * that it was an IMPDEF choice, and QEMU initially picked 62.5MHz,
+         * which gives a 16ns tick period.
+         *
+         * We will use the back-compat value:
+         *  - for QEMU CPU types added before we standardized on 1GHz
+         *  - for versioned machine types with a version of 9.0 or earlier
+         */
+        if (arm_feature(env, ARM_FEATURE_BACKCOMPAT_CNTFRQ) ||
+            cpu->backcompat_cntfrq) {
+            cpu->gt_cntfrq_hz = GTIMER_BACKCOMPAT_HZ;
+        } else {
+            cpu->gt_cntfrq_hz = GTIMER_DEFAULT_HZ;
+        }
+    }
+#ifndef CONFIG_USER_ONLY
     {
         uint64_t scale = gt_cntfrq_period_ns(cpu);
 
@@ -2079,18 +2092,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #endif
 
-    cpu_exec_realizefn(cs, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    arm_cpu_finalize_features(cpu, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
 #ifdef CONFIG_USER_ONLY
     /*
      * User mode relies on IC IVAU instructions to catch modification of
-- 
2.49.0

[PATCH-for-10.1 v4 4/4] target/arm/hvf: Really set Generic Timer counter frequency

[Philippe Mathieu-Daudé]

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().

Fixes: a1477da3dde ("hvf: Add Apple Silicon support")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@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();
+
     return true;
 }
 
-- 
2.49.0

Re: [PATCH-for-10.1 v4 1/4] accel: Introduce AccelOpsClass::cpu_target_realize() hook

[Peter Maydell]

On Wed, 23 Jul 2025 at 14:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Allow accelerators to set vCPU properties before its realization.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/accel/accel-cpu-ops.h | 1 +
>  accel/accel-common.c          | 5 +++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/include/accel/accel-cpu-ops.h b/include/accel/accel-cpu-ops.h
> index 0674764914f..9c07a903ea0 100644
> --- a/include/accel/accel-cpu-ops.h
> +++ b/include/accel/accel-cpu-ops.h
> @@ -34,6 +34,7 @@ struct AccelOpsClass {
>      /* initialization function called when accel is chosen */
>      void (*ops_init)(AccelClass *ac);
>
> +    bool (*cpu_target_realize)(CPUState *cpu, Error **errp);
>      bool (*cpus_are_resettable)(void);
>      void (*cpu_reset_hold)(CPUState *cpu);
>
> diff --git a/accel/accel-common.c b/accel/accel-common.c
> index 850c5ab4b8e..eecb2a292af 100644
> --- a/accel/accel-common.c
> +++ b/accel/accel-common.c
> @@ -106,6 +106,11 @@ bool accel_cpu_common_realize(CPUState *cpu, Error **errp)
>      if (acc->cpu_common_realize && !acc->cpu_common_realize(cpu, errp)) {
>          return false;
>      }
> +    if (acc->ops
> +        && acc->ops->cpu_target_realize
> +        && !acc->ops->cpu_target_realize(cpu, errp)) {
> +        return false;
> +    }

You don't need to check if acc->ops is NULL here: per the
comment in accel_init_ops_interfaces(), all accelerators
need to define ops, and we would have already crashed if
it was NULL.

thanks
-- PMM

Re: [PATCH-for-10.1 v4 3/4] target/arm: Create GTimers *after* features finalized / accel realized

[Peter Maydell]

On Wed, 23 Jul 2025 at 14:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Call generic (including accelerator) cpu_realize() handlers
> *before* setting @gt_cntfrq_hz default
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.c | 65 ++++++++++++++++++++++++------------------------
>  1 file changed, 33 insertions(+), 32 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(I think my previous misgivings about this patch were due to
not looking closely enough at it -- it looks at first glance
as if it might be moving the cpu_exec_realizefn calls above
more code than just the timer stuff, but it does not.)

thanks
-- PMM

Re: [PATCH-for-10.1 v4 4/4] target/arm/hvf: Really set Generic Timer counter frequency

[Peter Maydell]

On Wed, 23 Jul 2025 at 14:52, Philippe Mathieu-Daudé <philmd@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é <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@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

Re: [PATCH-for-10.1 v4 2/4] accel/hvf: Add hvf_arch_cpu_realize() stubs

[Peter Maydell]

On Wed, 23 Jul 2025 at 14:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Implement HVF AccelOpsClass::cpu_target_realize() hook as
> empty stubs. Target implementations will come separately.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH-for-10.1 v4 1/4] accel: Introduce AccelOpsClass::cpu_target_realize() hook

[Philippe Mathieu-Daudé]

On 23/7/25 17:49, Peter Maydell wrote:
> On Wed, 23 Jul 2025 at 14:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Allow accelerators to set vCPU properties before its realization.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   include/accel/accel-cpu-ops.h | 1 +
>>   accel/accel-common.c          | 5 +++++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/include/accel/accel-cpu-ops.h b/include/accel/accel-cpu-ops.h
>> index 0674764914f..9c07a903ea0 100644
>> --- a/include/accel/accel-cpu-ops.h
>> +++ b/include/accel/accel-cpu-ops.h
>> @@ -34,6 +34,7 @@ struct AccelOpsClass {
>>       /* initialization function called when accel is chosen */
>>       void (*ops_init)(AccelClass *ac);
>>
>> +    bool (*cpu_target_realize)(CPUState *cpu, Error **errp);
>>       bool (*cpus_are_resettable)(void);
>>       void (*cpu_reset_hold)(CPUState *cpu);
>>
>> diff --git a/accel/accel-common.c b/accel/accel-common.c
>> index 850c5ab4b8e..eecb2a292af 100644
>> --- a/accel/accel-common.c
>> +++ b/accel/accel-common.c
>> @@ -106,6 +106,11 @@ bool accel_cpu_common_realize(CPUState *cpu, Error **errp)
>>       if (acc->cpu_common_realize && !acc->cpu_common_realize(cpu, errp)) {
>>           return false;
>>       }
>> +    if (acc->ops
>> +        && acc->ops->cpu_target_realize
>> +        && !acc->ops->cpu_target_realize(cpu, errp)) {
>> +        return false;
>> +    }
> 
> You don't need to check if acc->ops is NULL here: per the
> comment in accel_init_ops_interfaces(), all accelerators
> need to define ops, and we would have already crashed if
> it was NULL.

Only system-mode registers @ops, so unfortunately we need this check
for user-mode emulation... Richard and myself are aware of this and
plan to work on exposing AccelOpsClass to user code (after v10.1).

Re: [PATCH-for-10.1 v4 3/4] target/arm: Create GTimers *after* features finalized / accel realized

[Philippe Mathieu-Daudé]

On 23/7/25 18:00, Peter Maydell wrote:
> On Wed, 23 Jul 2025 at 14:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Call generic (including accelerator) cpu_realize() handlers
>> *before* setting @gt_cntfrq_hz default
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/arm/cpu.c | 65 ++++++++++++++++++++++++------------------------
>>   1 file changed, 33 insertions(+), 32 deletions(-)
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> (I think my previous misgivings about this patch were due to
> not looking closely enough at it -- it looks at first glance
> as if it might be moving the cpu_exec_realizefn calls above
> more code than just the timer stuff, but it does not.)

I agree the default git diff output is misleading.