[PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[Peter Maydell]

In previous versions of the Arm architecture, the frequency of the
generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
In Armv8.6, the architecture standardized this frequency to 1GHz.

Because there is no ID register feature field that indicates whether a
CPU is v8.6 or that it ought to have this counter frequency, we
implement this by changing our default CNTFRQ value for all CPUs, with
exceptions for backwards compatibility:

 * CPU types which we already implement will retain the old
   default value. None of these are v8.6 CPUs, so this is
   architecturally OK.
 * CPUs used in versioned machine types with a version of 9.0
   or earlier will retain the old default value.

The upshot is that the only CPU type that changes is 'max'; but any
new type we add in future (whether v8.6 or not) will also get the new
1GHz default (assuming we spot in code review any attempts to set
the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
of cut-n-paste from an older CPU initfn ;-)).

It remains the case that the machine model can override the default
value via the 'cntfrq' QOM property (regardless of the CPU type).

Patch 1 is Paolo's "add the new versioned machine types" patch that
he sent out last month; patch 2 is some preliminary cleanup so that
we set the default cntfrq value in exactly one place, and patch 3
is the mechanics to set the default appropriately for the two
back-compat scenarios.

thanks
-- PMM

Paolo Bonzini (1):
  hw: Add compat machines for 9.1

Peter Maydell (2):
  target/arm: Refactor default generic timer frequency handling
  target/arm: Default to 1GHz cntfrq for 'max' and new CPUs

 include/hw/boards.h        |  3 +++
 include/hw/i386/pc.h       |  3 +++
 target/arm/cpu.h           | 11 +++++++++
 target/arm/internals.h     | 15 +++++++++---
 hw/arm/virt.c              | 11 +++++++--
 hw/core/machine.c          |  5 ++++
 hw/i386/pc.c               |  3 +++
 hw/i386/pc_piix.c          | 17 +++++++++++---
 hw/i386/pc_q35.c           | 14 ++++++++++--
 hw/m68k/virt.c             | 11 +++++++--
 hw/ppc/spapr.c             | 17 +++++++++++---
 hw/s390x/s390-virtio-ccw.c | 14 +++++++++++-
 target/arm/cpu.c           | 47 ++++++++++++++++++++++++++------------
 target/arm/cpu64.c         |  2 ++
 target/arm/helper.c        | 16 ++++++-------
 target/arm/tcg/cpu32.c     |  4 ++++
 target/arm/tcg/cpu64.c     | 18 +++++++++++++++
 17 files changed, 173 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH 1/3] hw: Add compat machines for 9.1

[Peter Maydell]

From: Paolo Bonzini <pbonzini@redhat.com>

Add 9.1 machine types for arm/i440fx/m68k/q35/s390x/spapr.

Cc: Cornelia Huck <cohuck@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Harsh Prateek Bora <harshpb@linux.ibm.com>
Cc: Gavin Shan <gshan@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
[PMM: fixed the typos Zhao Liu found in the s390 changes]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/boards.h        |  3 +++
 include/hw/i386/pc.h       |  3 +++
 hw/arm/virt.c              | 11 +++++++++--
 hw/core/machine.c          |  3 +++
 hw/i386/pc.c               |  3 +++
 hw/i386/pc_piix.c          | 17 ++++++++++++++---
 hw/i386/pc_q35.c           | 14 ++++++++++++--
 hw/m68k/virt.c             | 11 +++++++++--
 hw/ppc/spapr.c             | 17 ++++++++++++++---
 hw/s390x/s390-virtio-ccw.c | 14 +++++++++++++-
 10 files changed, 83 insertions(+), 13 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 8b8f6d5c00d..50e0cf4278e 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -425,6 +425,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_9_0[];
+extern const size_t hw_compat_9_0_len;
+
 extern GlobalProperty hw_compat_8_2[];
 extern const size_t hw_compat_8_2_len;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 27a68071d77..349f79df086 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -198,6 +198,9 @@ void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
 /* sgx.c */
 void pc_machine_init_sgx_epc(PCMachineState *pcms);
 
+extern GlobalProperty pc_compat_9_0[];
+extern const size_t pc_compat_9_0_len;
+
 extern GlobalProperty pc_compat_8_2[];
 extern const size_t pc_compat_8_2_len;
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a9a913aeadb..c9119ef3847 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -3223,10 +3223,17 @@ static void machvirt_machine_init(void)
 }
 type_init(machvirt_machine_init);
 
-static void virt_machine_9_0_options(MachineClass *mc)
+static void virt_machine_9_1_options(MachineClass *mc)
 {
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(9, 0)
+DEFINE_VIRT_MACHINE_AS_LATEST(9, 1)
+
+static void virt_machine_9_0_options(MachineClass *mc)
+{
+    virt_machine_9_1_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_9_0, hw_compat_9_0_len);
+}
+DEFINE_VIRT_MACHINE(9, 0)
 
 static void virt_machine_8_2_options(MachineClass *mc)
 {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 37ede0e7d4f..a92bec23147 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,6 +33,9 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "audio/audio.h"
 
+GlobalProperty hw_compat_9_0[] = {};
+const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
+
 GlobalProperty hw_compat_8_2[] = {
     { "migration", "zero-page-detection", "legacy"},
     { TYPE_VIRTIO_IOMMU_PCI, "granule", "4k" },
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 5c21b0c4dbf..b0d818b2094 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -78,6 +78,9 @@
     { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
     { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
 
+GlobalProperty pc_compat_9_0[] = {};
+const size_t pc_compat_9_0_len = G_N_ELEMENTS(pc_compat_9_0);
+
 GlobalProperty pc_compat_8_2[] = {};
 const size_t pc_compat_8_2_len = G_N_ELEMENTS(pc_compat_8_2);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 18ba0766092..8850c49c66a 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -513,13 +513,26 @@ static void pc_i440fx_machine_options(MachineClass *m)
                                      "Use a different south bridge than PIIX3");
 }
 
-static void pc_i440fx_9_0_machine_options(MachineClass *m)
+static void pc_i440fx_9_1_machine_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = true;
 }
 
+DEFINE_I440FX_MACHINE(v9_1, "pc-i440fx-9.1", NULL,
+                      pc_i440fx_9_1_machine_options);
+
+static void pc_i440fx_9_0_machine_options(MachineClass *m)
+{
+    pc_i440fx_9_1_machine_options(m);
+    m->alias = NULL;
+    m->is_default = false;
+
+    compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len);
+    compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
+}
+
 DEFINE_I440FX_MACHINE(v9_0, "pc-i440fx-9.0", NULL,
                       pc_i440fx_9_0_machine_options);
 
@@ -528,8 +541,6 @@ static void pc_i440fx_8_2_machine_options(MachineClass *m)
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
 
     pc_i440fx_9_0_machine_options(m);
-    m->alias = NULL;
-    m->is_default = false;
 
     compat_props_add(m->compat_props, hw_compat_8_2, hw_compat_8_2_len);
     compat_props_add(m->compat_props, pc_compat_8_2, pc_compat_8_2_len);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index c7bc8a2041f..6e1180d4b60 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -365,12 +365,23 @@ static void pc_q35_machine_options(MachineClass *m)
                      pc_q35_compat_defaults, pc_q35_compat_defaults_len);
 }
 
-static void pc_q35_9_0_machine_options(MachineClass *m)
+static void pc_q35_9_1_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->alias = "q35";
 }
 
+DEFINE_Q35_MACHINE(v9_1, "pc-q35-9.1", NULL,
+                   pc_q35_9_1_machine_options);
+
+static void pc_q35_9_0_machine_options(MachineClass *m)
+{
+    pc_q35_9_1_machine_options(m);
+    m->alias = NULL;
+    compat_props_add(m->compat_props, hw_compat_9_0, hw_compat_9_0_len);
+    compat_props_add(m->compat_props, pc_compat_9_0, pc_compat_9_0_len);
+}
+
 DEFINE_Q35_MACHINE(v9_0, "pc-q35-9.0", NULL,
                    pc_q35_9_0_machine_options);
 
@@ -378,7 +389,6 @@ static void pc_q35_8_2_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_9_0_machine_options(m);
-    m->alias = NULL;
     m->max_cpus = 1024;
     compat_props_add(m->compat_props, hw_compat_8_2, hw_compat_8_2_len);
     compat_props_add(m->compat_props, pc_compat_8_2, pc_compat_8_2_len);
diff --git a/hw/m68k/virt.c b/hw/m68k/virt.c
index b8e5e102e6b..09bc9bdfefb 100644
--- a/hw/m68k/virt.c
+++ b/hw/m68k/virt.c
@@ -357,10 +357,17 @@ type_init(virt_machine_register_types)
     } \
     type_init(machvirt_machine_##major##_##minor##_init);
 
-static void virt_machine_9_0_options(MachineClass *mc)
+static void virt_machine_9_1_options(MachineClass *mc)
 {
 }
-DEFINE_VIRT_MACHINE(9, 0, true)
+DEFINE_VIRT_MACHINE(9, 1, true)
+
+static void virt_machine_9_0_options(MachineClass *mc)
+{
+    virt_machine_9_1_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_9_0, hw_compat_9_0_len);
+}
+DEFINE_VIRT_MACHINE(9, 0, false)
 
 static void virt_machine_8_2_options(MachineClass *mc)
 {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e9bc97fee08..36ada4d0baf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4806,14 +4806,25 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
     type_init(spapr_machine_register_##suffix)
 
 /*
- * pseries-9.0
+ * pseries-9.1
  */
-static void spapr_machine_9_0_class_options(MachineClass *mc)
+static void spapr_machine_9_1_class_options(MachineClass *mc)
 {
     /* Defaults for the latest behaviour inherited from the base class */
 }
 
-DEFINE_SPAPR_MACHINE(9_0, "9.0", true);
+DEFINE_SPAPR_MACHINE(9_1, "9.1", true);
+
+/*
+ * pseries-9.0
+ */
+static void spapr_machine_9_0_class_options(MachineClass *mc)
+{
+    spapr_machine_9_1_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_9_0, hw_compat_9_0_len);
+}
+
+DEFINE_SPAPR_MACHINE(9_0, "9.0", false);
 
 /*
  * pseries-8.2
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index b1dcb3857f0..7cebbf6c02a 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -859,14 +859,26 @@ bool css_migration_enabled(void)
     }                                                                         \
     type_init(ccw_machine_register_##suffix)
 
+static void ccw_machine_9_1_instance_options(MachineState *machine)
+{
+}
+
+static void ccw_machine_9_1_class_options(MachineClass *mc)
+{
+}
+DEFINE_CCW_MACHINE(9_1, "9.1", true);
+
 static void ccw_machine_9_0_instance_options(MachineState *machine)
 {
+    ccw_machine_9_1_instance_options(machine);
 }
 
 static void ccw_machine_9_0_class_options(MachineClass *mc)
 {
+    ccw_machine_9_1_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_9_0, hw_compat_9_0_len);
 }
-DEFINE_CCW_MACHINE(9_0, "9.0", true);
+DEFINE_CCW_MACHINE(9_0, "9.0", false);
 
 static void ccw_machine_8_2_instance_options(MachineState *machine)
 {
-- 
2.34.1

[PATCH 2/3] target/arm: Refactor default generic timer frequency handling

[Peter Maydell]

The generic timer frequency is settable by board code via a QOM
property "cntfrq", but otherwise defaults to 62.5MHz.  The way this
is done includes some complication resulting from how this was
originally a fixed value with no QOM property.  Clean it up:

 * always set cpu->gt_cntfrq_hz to some sensible value, whether
   the CPU has the generic timer or not, and whether it's system
   or user-only emulation
 * this means we can always use gt_cntfrq_hz, and never need
   the old GTIMER_SCALE define
 * set the default value in exactly one place, in the realize fn

The aim here is to pave the way for handling the ARMv8.6 requirement
that the generic timer frequency is always 1GHz.  We're going to do
that by having old CPU types keep their legacy-in-QEMU behaviour and
having the default for any new CPU types be a 1GHz rather han 62.5MHz
cntfrq, so we want the point where the default is decided to be in
one place, and in code, not in a DEFINE_PROP_UINT64() initializer.

This commit should have no behavioural changes.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/internals.h |  7 ++++---
 target/arm/cpu.c       | 31 +++++++++++++++++--------------
 target/arm/helper.c    | 16 ++++++++--------
 3 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index dd3da211a3f..74d4b1b0990 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -59,10 +59,11 @@ static inline bool excp_is_internal(int excp)
         || excp == EXCP_SEMIHOST;
 }
 
-/* Scale factor for generic timers, ie number of ns per tick.
- * This gives a 62.5MHz timer.
+/*
+ * Default frequency for the generic timer, in Hz.
+ * This is 62.5MHz, which gives a 16 ns tick period.
  */
-#define GTIMER_SCALE 16
+#define GTIMER_DEFAULT_HZ 62500000
 
 /* Bit definitions for the v7M CONTROL register */
 FIELD(V7M_CONTROL, NPRIV, 0, 1)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ab8d007a86c..b248b283423 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1381,9 +1381,12 @@ static void arm_cpu_initfn(Object *obj)
     }
 }
 
+/*
+ * 0 means "unset, use the default value". That default might vary depending
+ * on the CPU type, and is set in the realize fn.
+ */
 static Property arm_cpu_gt_cntfrq_property =
-            DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq_hz,
-                               NANOSECONDS_PER_SECOND / GTIMER_SCALE);
+            DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq_hz, 0);
 
 static Property arm_cpu_reset_cbar_property =
             DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
@@ -1829,6 +1832,17 @@ 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".
+         * The default value of the generic timer frequency (as seen in
+         * CNTFRQ_EL0) is 62.5MHz, which corresponds to a period of 16ns.
+         * This is what you get (a) for a CONFIG_USER_ONLY CPU (b) if the
+         * board doesn't set it.
+         */
+        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
@@ -1877,18 +1891,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 
     {
-        uint64_t scale;
-
-        if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
-            if (!cpu->gt_cntfrq_hz) {
-                error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz",
-                           cpu->gt_cntfrq_hz);
-                return;
-            }
-            scale = gt_cntfrq_period_ns(cpu);
-        } else {
-            scale = GTIMER_SCALE;
-        }
+        uint64_t scale = gt_cntfrq_period_ns(cpu);
 
         cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
                                                arm_gt_ptimer_cb, cpu);
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 8bdbb404195..01cf231a861 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2461,6 +2461,13 @@ static const ARMCPRegInfo v6k_cp_reginfo[] = {
       .resetvalue = 0 },
 };
 
+static void arm_gt_cntfrq_reset(CPUARMState *env, const ARMCPRegInfo *opaque)
+{
+    ARMCPU *cpu = env_archcpu(env);
+
+    cpu->env.cp15.c14_cntfrq = cpu->gt_cntfrq_hz;
+}
+
 #ifndef CONFIG_USER_ONLY
 
 static CPAccessResult gt_cntfrq_access(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -3215,13 +3222,6 @@ void arm_gt_hvtimer_cb(void *opaque)
     gt_recalc_timer(cpu, GTIMER_HYPVIRT);
 }
 
-static void arm_gt_cntfrq_reset(CPUARMState *env, const ARMCPRegInfo *opaque)
-{
-    ARMCPU *cpu = env_archcpu(env);
-
-    cpu->env.cp15.c14_cntfrq = cpu->gt_cntfrq_hz;
-}
-
 static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
     /*
      * Note that CNTFRQ is purely reads-as-written for the benefit
@@ -3501,7 +3501,7 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 0,
       .type = ARM_CP_CONST, .access = PL0_R /* no PL1_RW in linux-user */,
       .fieldoffset = offsetof(CPUARMState, cp15.c14_cntfrq),
-      .resetvalue = NANOSECONDS_PER_SECOND / GTIMER_SCALE,
+      .resetfn = arm_gt_cntfrq_reset,
     },
     { .name = "CNTVCT_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 0, .opc2 = 2,
-- 
2.34.1

Re: [PATCH 2/3] target/arm: Refactor default generic timer frequency handling

[Richard Henderson]

On 4/19/24 11:46, Peter Maydell wrote:
> The generic timer frequency is settable by board code via a QOM
> property "cntfrq", but otherwise defaults to 62.5MHz.  The way this
> is done includes some complication resulting from how this was
> originally a fixed value with no QOM property.  Clean it up:
> 
>   * always set cpu->gt_cntfrq_hz to some sensible value, whether
>     the CPU has the generic timer or not, and whether it's system
>     or user-only emulation
>   * this means we can always use gt_cntfrq_hz, and never need
>     the old GTIMER_SCALE define
>   * set the default value in exactly one place, in the realize fn
> 
> The aim here is to pave the way for handling the ARMv8.6 requirement
> that the generic timer frequency is always 1GHz.  We're going to do
> that by having old CPU types keep their legacy-in-QEMU behaviour and
> having the default for any new CPU types be a 1GHz rather han 62.5MHz
> cntfrq, so we want the point where the default is decided to be in
> one place, and in code, not in a DEFINE_PROP_UINT64() initializer.
> 
> This commit should have no behavioural changes.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/internals.h |  7 ++++---
>   target/arm/cpu.c       | 31 +++++++++++++++++--------------
>   target/arm/helper.c    | 16 ++++++++--------
>   3 files changed, 29 insertions(+), 25 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/3] target/arm: Refactor default generic timer frequency handling

[Philippe Mathieu-Daudé]

On 19/4/24 20:46, Peter Maydell wrote:
> The generic timer frequency is settable by board code via a QOM
> property "cntfrq", but otherwise defaults to 62.5MHz.  The way this
> is done includes some complication resulting from how this was
> originally a fixed value with no QOM property.  Clean it up:
> 
>   * always set cpu->gt_cntfrq_hz to some sensible value, whether
>     the CPU has the generic timer or not, and whether it's system
>     or user-only emulation
>   * this means we can always use gt_cntfrq_hz, and never need
>     the old GTIMER_SCALE define
>   * set the default value in exactly one place, in the realize fn
> 
> The aim here is to pave the way for handling the ARMv8.6 requirement
> that the generic timer frequency is always 1GHz.  We're going to do
> that by having old CPU types keep their legacy-in-QEMU behaviour and
> having the default for any new CPU types be a 1GHz rather han 62.5MHz
> cntfrq, so we want the point where the default is decided to be in
> one place, and in code, not in a DEFINE_PROP_UINT64() initializer.
> 
> This commit should have no behavioural changes.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/internals.h |  7 ++++---
>   target/arm/cpu.c       | 31 +++++++++++++++++--------------
>   target/arm/helper.c    | 16 ++++++++--------
>   3 files changed, 29 insertions(+), 25 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/3] target/arm: Default to 1GHz cntfrq for 'max' and new CPUs

[Peter Maydell]

In previous versions of the Arm architecture, the frequency of the
generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
In Armv8.6, the architecture standardized this frequency to 1GHz.

Because there is no ID register feature field that indicates whether
a CPU is v8.6 or that it ought to have this counter frequency, we
implement this by changing our default CNTFRQ value for all CPUs,
with exceptions for backwards compatibility:

 * CPU types which we already implement will retain the old
   default value. None of these are v8.6 CPUs, so this is
   architecturally OK.
 * CPUs used in versioned machine types with a version of 9.0
   or earlier will retain the old default value.

The upshot is that the only CPU type that changes is 'max'; but any
new type we add in future (whether v8.6 or not) will also get the new
1GHz default.

It remains the case that the machine model can override the default
value via the 'cntfrq' QOM property (regardless of the CPU type).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       | 11 +++++++++++
 target/arm/internals.h | 12 ++++++++++--
 hw/core/machine.c      |  4 +++-
 target/arm/cpu.c       | 28 ++++++++++++++++++++++------
 target/arm/cpu64.c     |  2 ++
 target/arm/tcg/cpu32.c |  4 ++++
 target/arm/tcg/cpu64.c | 18 ++++++++++++++++++
 7 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 20d8257c853..4eeeac3fe94 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -953,6 +953,9 @@ struct ArchCPU {
      */
     bool host_cpu_probe_failed;
 
+    /* QOM property to indicate we should use the back-compat CNTFRQ default */
+    bool backcompat_cntfrq;
+
     /* Specify the number of cores in this CPU cluster. Used for the L2CTLR
      * register.
      */
@@ -2367,6 +2370,14 @@ enum arm_features {
     ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
     ARM_FEATURE_M_MAIN, /* M profile Main Extension */
     ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+    /*
+     * ARM_FEATURE_BACKCOMPAT_CNTFRQ makes the CPU default cntfrq be 62.5MHz
+     * if the board doesn't set a value, instead of 1GHz. It is for backwards
+     * compatibility and used only with CPU definitions that were already
+     * in QEMU before we changed the default. It should not be set on any
+     * CPU types added in future.
+     */
+    ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 74d4b1b0990..11d9ff0fc08 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -61,9 +61,17 @@ static inline bool excp_is_internal(int excp)
 
 /*
  * Default frequency for the generic timer, in Hz.
- * This is 62.5MHz, which gives a 16 ns tick period.
+ * 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
+ * In any case, the machine model may override via the cntfrq property.
  */
-#define GTIMER_DEFAULT_HZ 62500000
+#define GTIMER_DEFAULT_HZ 1000000000
+#define GTIMER_BACKCOMPAT_HZ 62500000
 
 /* Bit definitions for the v7M CONTROL register */
 FIELD(V7M_CONTROL, NPRIV, 0, 1)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index a92bec23147..bd40483d880 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,7 +33,9 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "audio/audio.h"
 
-GlobalProperty hw_compat_9_0[] = {};
+GlobalProperty hw_compat_9_0[] = {
+    {"arm-cpu", "backcompat-cntfrq", "true" },
+};
 const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
 
 GlobalProperty hw_compat_8_2[] = {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index b248b283423..2c8160d6b74 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1388,6 +1388,11 @@ static void arm_cpu_initfn(Object *obj)
 static Property arm_cpu_gt_cntfrq_property =
             DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq_hz, 0);
 
+/* True to default to the backwards-compatibility old CNTFRQ rather than 1Ghz */
+static Property arm_cpu_backcompat_cntfrq_property =
+            DEFINE_PROP_BOOL("backcompat-cntfrq", ARMCPU,
+                             backcompat_cntfrq, false);
+
 static Property arm_cpu_reset_cbar_property =
             DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
 
@@ -1709,6 +1714,8 @@ void arm_cpu_post_init(Object *obj)
         qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property);
     }
 
+    qdev_property_add_static(DEVICE(obj), &arm_cpu_backcompat_cntfrq_property);
+
     if (kvm_enabled()) {
         kvm_arm_add_vcpu_properties(cpu);
     }
@@ -1834,13 +1841,22 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
     if (!cpu->gt_cntfrq_hz) {
         /*
-         * 0 means "the board didn't set a value, use the default".
-         * The default value of the generic timer frequency (as seen in
-         * CNTFRQ_EL0) is 62.5MHz, which corresponds to a period of 16ns.
-         * This is what you get (a) for a CONFIG_USER_ONLY CPU (b) if the
-         * board doesn't set it.
+         * 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
          */
-        cpu->gt_cntfrq_hz = GTIMER_DEFAULT_HZ;
+        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
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 985b1efe160..c15d086049f 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -599,6 +599,7 @@ static void aarch64_a57_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -656,6 +657,7 @@ static void aarch64_a53_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
diff --git a/target/arm/tcg/cpu32.c b/target/arm/tcg/cpu32.c
index b5a60682fa6..bdd82d912a2 100644
--- a/target/arm/tcg/cpu32.c
+++ b/target/arm/tcg/cpu32.c
@@ -457,6 +457,7 @@ static void cortex_a7_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -505,6 +506,7 @@ static void cortex_a15_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -696,6 +698,7 @@ static void cortex_r52_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_PMSA);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_AUXCR);
     cpu->midr = 0x411fd133; /* r1p3 */
@@ -924,6 +927,7 @@ static void arm_max_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
index c3369f40824..b0eb7fbb385 100644
--- a/target/arm/tcg/cpu64.c
+++ b/target/arm/tcg/cpu64.c
@@ -63,6 +63,7 @@ static void aarch64_a35_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -231,6 +232,7 @@ static void aarch64_a55_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -299,6 +301,7 @@ static void aarch64_a72_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -354,6 +357,7 @@ static void aarch64_a76_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -423,6 +427,7 @@ static void aarch64_a64fx_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
@@ -592,6 +597,7 @@ static void aarch64_neoverse_n1_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -663,6 +669,7 @@ static void aarch64_neoverse_v1_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -885,6 +892,7 @@ static void aarch64_a710_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -982,6 +990,7 @@ static void aarch64_neoverse_n2_initfn(Object *obj)
     set_feature(&cpu->env, ARM_FEATURE_V8);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
+    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
     set_feature(&cpu->env, ARM_FEATURE_AARCH64);
     set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
     set_feature(&cpu->env, ARM_FEATURE_EL2);
@@ -1077,6 +1086,15 @@ void aarch64_max_tcg_initfn(Object *obj)
     uint64_t t;
     uint32_t u;
 
+    /*
+     * Unset ARM_FEATURE_BACKCOMPAT_CNTFRQ, which we would otherwise default
+     * to because we started with aarch64_a57_initfn(). A 'max' CPU might
+     * be a v8.6-or-later one, in which case the cntfrq must be 1GHz; and
+     * because it is our "may change" CPU type we are OK with it not being
+     * backwards-compatible with how it worked in old QEMU.
+     */
+    unset_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
+
     /*
      * Reset MIDR so the guest doesn't mistake our 'max' CPU type for a real
      * one and try to apply errata workarounds or use impdef features we
-- 
2.34.1

Re: [PATCH 3/3] target/arm: Default to 1GHz cntfrq for 'max' and new CPUs

[Richard Henderson]

On 4/19/24 11:46, Peter Maydell wrote:
> In previous versions of the Arm architecture, the frequency of the
> generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
> and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
> In Armv8.6, the architecture standardized this frequency to 1GHz.
> 
> Because there is no ID register feature field that indicates whether
> a CPU is v8.6 or that it ought to have this counter frequency, we
> implement this by changing our default CNTFRQ value for all CPUs,
> with exceptions for backwards compatibility:
> 
>   * CPU types which we already implement will retain the old
>     default value. None of these are v8.6 CPUs, so this is
>     architecturally OK.
>   * CPUs used in versioned machine types with a version of 9.0
>     or earlier will retain the old default value.
> 
> The upshot is that the only CPU type that changes is 'max'; but any
> new type we add in future (whether v8.6 or not) will also get the new
> 1GHz default.
> 
> It remains the case that the machine model can override the default
> value via the 'cntfrq' QOM property (regardless of the CPU type).
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h       | 11 +++++++++++
>   target/arm/internals.h | 12 ++++++++++--
>   hw/core/machine.c      |  4 +++-
>   target/arm/cpu.c       | 28 ++++++++++++++++++++++------
>   target/arm/cpu64.c     |  2 ++
>   target/arm/tcg/cpu32.c |  4 ++++
>   target/arm/tcg/cpu64.c | 18 ++++++++++++++++++
>   7 files changed, 70 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 3/3] target/arm: Default to 1GHz cntfrq for 'max' and new CPUs

[Philippe Mathieu-Daudé]

(+Markus for qdev properties; one inlined comment)

On 19/4/24 20:46, Peter Maydell wrote:
> In previous versions of the Arm architecture, the frequency of the
> generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
> and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
> In Armv8.6, the architecture standardized this frequency to 1GHz.
> 
> Because there is no ID register feature field that indicates whether
> a CPU is v8.6 or that it ought to have this counter frequency, we
> implement this by changing our default CNTFRQ value for all CPUs,
> with exceptions for backwards compatibility:
> 
>   * CPU types which we already implement will retain the old
>     default value. None of these are v8.6 CPUs, so this is
>     architecturally OK.
>   * CPUs used in versioned machine types with a version of 9.0
>     or earlier will retain the old default value.
> 
> The upshot is that the only CPU type that changes is 'max'; but any
> new type we add in future (whether v8.6 or not) will also get the new
> 1GHz default.
> 
> It remains the case that the machine model can override the default
> value via the 'cntfrq' QOM property (regardless of the CPU type).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h       | 11 +++++++++++
>   target/arm/internals.h | 12 ++++++++++--
>   hw/core/machine.c      |  4 +++-
>   target/arm/cpu.c       | 28 ++++++++++++++++++++++------
>   target/arm/cpu64.c     |  2 ++
>   target/arm/tcg/cpu32.c |  4 ++++
>   target/arm/tcg/cpu64.c | 18 ++++++++++++++++++
>   7 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 20d8257c853..4eeeac3fe94 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -953,6 +953,9 @@ struct ArchCPU {
>        */
>       bool host_cpu_probe_failed;
>   
> +    /* QOM property to indicate we should use the back-compat CNTFRQ default */
> +    bool backcompat_cntfrq;
> +
>       /* Specify the number of cores in this CPU cluster. Used for the L2CTLR
>        * register.
>        */
> @@ -2367,6 +2370,14 @@ enum arm_features {
>       ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>       ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>       ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
> +    /*
> +     * ARM_FEATURE_BACKCOMPAT_CNTFRQ makes the CPU default cntfrq be 62.5MHz
> +     * if the board doesn't set a value, instead of 1GHz. It is for backwards
> +     * compatibility and used only with CPU definitions that were already
> +     * in QEMU before we changed the default. It should not be set on any
> +     * CPU types added in future.
> +     */
> +    ARM_FEATURE_BACKCOMPAT_CNTFRQ, /* 62.5MHz timer default */
>   };
>   
>   static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 74d4b1b0990..11d9ff0fc08 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -61,9 +61,17 @@ static inline bool excp_is_internal(int excp)
>   
>   /*
>    * Default frequency for the generic timer, in Hz.
> - * This is 62.5MHz, which gives a 16 ns tick period.
> + * 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
> + * In any case, the machine model may override via the cntfrq property.
>    */
> -#define GTIMER_DEFAULT_HZ 62500000
> +#define GTIMER_DEFAULT_HZ 1000000000
> +#define GTIMER_BACKCOMPAT_HZ 62500000
>   
>   /* Bit definitions for the v7M CONTROL register */
>   FIELD(V7M_CONTROL, NPRIV, 0, 1)
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index a92bec23147..bd40483d880 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -33,7 +33,9 @@
>   #include "hw/virtio/virtio-iommu.h"
>   #include "audio/audio.h"
>   
> -GlobalProperty hw_compat_9_0[] = {};
> +GlobalProperty hw_compat_9_0[] = {
> +    {"arm-cpu", "backcompat-cntfrq", "true" },
> +};
>   const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
>   
>   GlobalProperty hw_compat_8_2[] = {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index b248b283423..2c8160d6b74 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1388,6 +1388,11 @@ static void arm_cpu_initfn(Object *obj)
>   static Property arm_cpu_gt_cntfrq_property =
>               DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq_hz, 0);
>   
> +/* True to default to the backwards-compatibility old CNTFRQ rather than 1Ghz */
> +static Property arm_cpu_backcompat_cntfrq_property =
> +            DEFINE_PROP_BOOL("backcompat-cntfrq", ARMCPU,
> +                             backcompat_cntfrq, false);
> +
>   static Property arm_cpu_reset_cbar_property =
>               DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
>   
> @@ -1709,6 +1714,8 @@ void arm_cpu_post_init(Object *obj)
>           qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property);
>       }
>   
> +    qdev_property_add_static(DEVICE(obj), &arm_cpu_backcompat_cntfrq_property);

I'd rather keep the qdev_property_add_static() for "dynamic"
properties (what a bad function name...) and add this one to
the static arm_cpu_properties[] array.

(Similar comment with arm_cpu_cfgend_property).

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>       if (kvm_enabled()) {
>           kvm_arm_add_vcpu_properties(cpu);
>       }
> @@ -1834,13 +1841,22 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>   
>       if (!cpu->gt_cntfrq_hz) {
>           /*
> -         * 0 means "the board didn't set a value, use the default".
> -         * The default value of the generic timer frequency (as seen in
> -         * CNTFRQ_EL0) is 62.5MHz, which corresponds to a period of 16ns.
> -         * This is what you get (a) for a CONFIG_USER_ONLY CPU (b) if the
> -         * board doesn't set it.
> +         * 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
>            */
> -        cpu->gt_cntfrq_hz = GTIMER_DEFAULT_HZ;
> +        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
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 985b1efe160..c15d086049f 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -599,6 +599,7 @@ static void aarch64_a57_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -656,6 +657,7 @@ static void aarch64_a53_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> diff --git a/target/arm/tcg/cpu32.c b/target/arm/tcg/cpu32.c
> index b5a60682fa6..bdd82d912a2 100644
> --- a/target/arm/tcg/cpu32.c
> +++ b/target/arm/tcg/cpu32.c
> @@ -457,6 +457,7 @@ static void cortex_a7_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -505,6 +506,7 @@ static void cortex_a15_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_DUMMY_C15_REGS);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -696,6 +698,7 @@ static void cortex_r52_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_PMSA);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_AUXCR);
>       cpu->midr = 0x411fd133; /* r1p3 */
> @@ -924,6 +927,7 @@ static void arm_max_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
>       set_feature(&cpu->env, ARM_FEATURE_EL3);
> diff --git a/target/arm/tcg/cpu64.c b/target/arm/tcg/cpu64.c
> index c3369f40824..b0eb7fbb385 100644
> --- a/target/arm/tcg/cpu64.c
> +++ b/target/arm/tcg/cpu64.c
> @@ -63,6 +63,7 @@ static void aarch64_a35_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -231,6 +232,7 @@ static void aarch64_a55_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -299,6 +301,7 @@ static void aarch64_a72_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -354,6 +357,7 @@ static void aarch64_a76_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -423,6 +427,7 @@ static void aarch64_a64fx_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
>       set_feature(&cpu->env, ARM_FEATURE_EL3);
> @@ -592,6 +597,7 @@ static void aarch64_neoverse_n1_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -663,6 +669,7 @@ static void aarch64_neoverse_v1_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -885,6 +892,7 @@ static void aarch64_a710_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -982,6 +990,7 @@ static void aarch64_neoverse_n2_initfn(Object *obj)
>       set_feature(&cpu->env, ARM_FEATURE_V8);
>       set_feature(&cpu->env, ARM_FEATURE_NEON);
>       set_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER);
> +    set_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
>       set_feature(&cpu->env, ARM_FEATURE_AARCH64);
>       set_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
>       set_feature(&cpu->env, ARM_FEATURE_EL2);
> @@ -1077,6 +1086,15 @@ void aarch64_max_tcg_initfn(Object *obj)
>       uint64_t t;
>       uint32_t u;
>   
> +    /*
> +     * Unset ARM_FEATURE_BACKCOMPAT_CNTFRQ, which we would otherwise default
> +     * to because we started with aarch64_a57_initfn(). A 'max' CPU might
> +     * be a v8.6-or-later one, in which case the cntfrq must be 1GHz; and
> +     * because it is our "may change" CPU type we are OK with it not being
> +     * backwards-compatible with how it worked in old QEMU.
> +     */
> +    unset_feature(&cpu->env, ARM_FEATURE_BACKCOMPAT_CNTFRQ);
> +
>       /*
>        * Reset MIDR so the guest doesn't mistake our 'max' CPU type for a real
>        * one and try to apply errata workarounds or use impdef features we

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[Peter Maydell]

On Fri, 19 Apr 2024 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In previous versions of the Arm architecture, the frequency of the
> generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
> and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
> In Armv8.6, the architecture standardized this frequency to 1GHz.
>
> Because there is no ID register feature field that indicates whether a
> CPU is v8.6 or that it ought to have this counter frequency, we
> implement this by changing our default CNTFRQ value for all CPUs, with
> exceptions for backwards compatibility:
>
>  * CPU types which we already implement will retain the old
>    default value. None of these are v8.6 CPUs, so this is
>    architecturally OK.
>  * CPUs used in versioned machine types with a version of 9.0
>    or earlier will retain the old default value.
>
> The upshot is that the only CPU type that changes is 'max'; but any
> new type we add in future (whether v8.6 or not) will also get the new
> 1GHz default (assuming we spot in code review any attempts to set
> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
> of cut-n-paste from an older CPU initfn ;-)).
>
> It remains the case that the machine model can override the default
> value via the 'cntfrq' QOM property (regardless of the CPU type).

This patchset turns out to break the sbsa-ref board: the
Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
avocado test both (a) takes rather longer to boot and (b) when
running thinks that time is advancing very fast.

I suspect this may be because the firmware hard-codes the
generic timer clock frequency it is expecting. I've cc'd the
sbsa-ref maintainers: is that correct?

If so, we can deal with it by making the sbsa-ref board set the
cntfrq QOM property on its CPUs to force them to the old
frequency. However this will produce a technically-out-of-spec
CPU when used with a v8.6-compliant CPU type, so probably we
should do something to be able to tell the firmware the clock
frequency (if it doesn't want to just read CNTFRQ_EL0 itself).

thanks
-- PMM

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[Marcin Juszkiewicz]

W dniu 22.04.2024 o 14:56, Peter Maydell pisze:
> On Fri, 19 Apr 2024 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote:

>> The upshot is that the only CPU type that changes is 'max'; but any
>> new type we add in future (whether v8.6 or not) will also get the new
>> 1GHz default (assuming we spot in code review any attempts to set
>> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
>> of cut-n-paste from an older CPU initfn ;-)).
>>
>> It remains the case that the machine model can override the default
>> value via the 'cntfrq' QOM property (regardless of the CPU type).
> 
> This patchset turns out to break the sbsa-ref board: the
> Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
> avocado test both (a) takes rather longer to boot and (b) when
> running thinks that time is advancing very fast.
> 
> I suspect this may be because the firmware hard-codes the
> generic timer clock frequency it is expecting. I've cc'd the
> sbsa-ref maintainers: is that correct?
> 
> If so, we can deal with it by making the sbsa-ref board set the
> cntfrq QOM property on its CPUs to force them to the old
> frequency. However this will produce a technically-out-of-spec
> CPU when used with a v8.6-compliant CPU type, so probably we
> should do something to be able to tell the firmware the clock
> frequency (if it doesn't want to just read CNTFRQ_EL0 itself).

 From what I see in EDK2 code we read CNTFREQ_EL0:

GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
ArmReadCntFrq() -> "mrs x0, cntfrq_el0"

I added debug output to firmware and it shows me:

HRW: GetPlatformTimerFreq TimerFreq = 62500000

Local tree:
ed0604e99c (HEAD -> test-counter) target/arm: Default to 1GHz cntfrq for 'max' and new CPUs
c0a8c341f5 target/arm: Refactor default generic timer frequency handling
592c01312b hw: Add compat machines for 9.1
62dbe54c24 (tag: v9.0.0-rc4, origin/master, origin/HEAD) Update version for v9.0.0-rc4 release
a12214d1c4 (origin/staging) usb-storage: Fix BlockConf defaults

sbsa-ref with "-cpu max" used. All cpu cores give me same value.

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[Peter Maydell]

On Mon, 22 Apr 2024 at 14:38, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> W dniu 22.04.2024 o 14:56, Peter Maydell pisze:
> > On Fri, 19 Apr 2024 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> >> The upshot is that the only CPU type that changes is 'max'; but any
> >> new type we add in future (whether v8.6 or not) will also get the new
> >> 1GHz default (assuming we spot in code review any attempts to set
> >> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
> >> of cut-n-paste from an older CPU initfn ;-)).
> >>
> >> It remains the case that the machine model can override the default
> >> value via the 'cntfrq' QOM property (regardless of the CPU type).
> >
> > This patchset turns out to break the sbsa-ref board: the
> > Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
> > avocado test both (a) takes rather longer to boot and (b) when
> > running thinks that time is advancing very fast.
> >
> > I suspect this may be because the firmware hard-codes the
> > generic timer clock frequency it is expecting. I've cc'd the
> > sbsa-ref maintainers: is that correct?
> >
> > If so, we can deal with it by making the sbsa-ref board set the
> > cntfrq QOM property on its CPUs to force them to the old
> > frequency. However this will produce a technically-out-of-spec
> > CPU when used with a v8.6-compliant CPU type, so probably we
> > should do something to be able to tell the firmware the clock
> > frequency (if it doesn't want to just read CNTFRQ_EL0 itself).
>
>  From what I see in EDK2 code we read CNTFREQ_EL0:
>
> GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
> sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
> ArmReadCntFrq() -> "mrs x0, cntfrq_el0"

Yeah, it looks like it's TF-A that hardcodes the frequency:
https://github.com/ARM-software/arm-trusted-firmware/blob/c8be7c08c3b3a2330d54b58651faa9438ff34f6e/plat/qemu/qemu_sbsa/include/platform_def.h#L269

I imagine that value gets written into CNTFRQ by TF-A somewhere
along the line (and then read by EDK2 later), though I haven't
quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
assumes that the generic-timer frequency and the watchdog
timer frequency are the same, which is a bit dubious IMHO.

It also seems to be hardcoded in TF-A's support for the virt
board too, annoyingly.

thanks
-- PMM

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[Marcin Juszkiewicz]

W dniu 22.04.2024 o 16:18, Peter Maydell pisze:
> On Mon, 22 Apr 2024 at 14:38, Marcin Juszkiewicz

>>   From what I see in EDK2 code we read CNTFREQ_EL0:
>>
>> GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
>> sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
>> ArmReadCntFrq() -> "mrs x0, cntfrq_el0"
> 
> Yeah, it looks like it's TF-A that hardcodes the frequency:
> https://github.com/ARM-software/arm-trusted-firmware/blob/c8be7c08c3b3a2330d54b58651faa9438ff34f6e/plat/qemu/qemu_sbsa/include/platform_def.h#L269
> 
> I imagine that value gets written into CNTFRQ by TF-A somewhere
> along the line (and then read by EDK2 later), though I haven't
> quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
> assumes that the generic-timer frequency and the watchdog
> timer frequency are the same, which is a bit dubious IMHO.
> 
> It also seems to be hardcoded in TF-A's support for the virt
> board too, annoyingly.

I looked at it and it seems that TF-A can just read what is in 
CNTFRQ_EL0 instead of hardcoding the value.

Submitted patch:

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/28313
refactor(qemu): do not hardcode counter frequency [NEW]

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[Marcin Juszkiewicz]

W dniu 22.04.2024 o 17:37, Marcin Juszkiewicz pisze:
>> It also seems to be hardcoded in TF-A's support for the virt
>> board too, annoyingly.
> 
> I looked at it and it seems that TF-A can just read what is in 
> CNTFRQ_EL0 instead of hardcoding the value.
> 
> Submitted patch:
> 
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/28313
> refactor(qemu): do not hardcode counter frequency [NEW]

Change is now merged.

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

[Peter Maydell]

On Mon, 22 Apr 2024 at 15:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> I imagine that value gets written into CNTFRQ by TF-A somewhere
> along the line (and then read by EDK2 later), though I haven't
> quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
> assumes that the generic-timer frequency and the watchdog
> timer frequency are the same, which is a bit dubious IMHO.

Checking the BSA spec, this is actually correct -- the system
watchdog is supposed to run at the generic counter frequency,
which will be the same as the one the CPU generic timers use.
So we need on the QEMU side to make the sbsa-watchdog device
be runtime configurable for frequency and arrange for it to
be set the same as the CPU.

(We could also arrange this by modelling the memory mapped
system counter properly; I have some slightly half-baked
patches to do that floating around somewhere. But I'm still
not quite sure it's worth the effort needed to try to get
them into a fully baked state :-))

thanks
-- PMM