From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>,
qemu-arm@nongnu.org, qemu-devel@nongnu.org
Cc: Markus Armbruster <armbru@redhat.com>
Subject: Re: [PATCH 3/3] target/arm: Default to 1GHz cntfrq for 'max' and new CPUs
Date: Tue, 23 Apr 2024 22:47:30 +0200 [thread overview]
Message-ID: <482edb42-95cf-402f-9124-4e0b1f76b91f@linaro.org> (raw)
In-Reply-To: <20240419184608.2675213-4-peter.maydell@linaro.org>
(+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
next prev parent reply other threads:[~2024-04-23 20:48 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-19 18:46 [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines Peter Maydell
2024-04-19 18:46 ` [PATCH 1/3] hw: Add compat machines for 9.1 Peter Maydell
2024-04-19 18:46 ` [PATCH 2/3] target/arm: Refactor default generic timer frequency handling Peter Maydell
2024-04-20 16:58 ` Richard Henderson
2024-04-23 12:12 ` Philippe Mathieu-Daudé
2024-04-19 18:46 ` [PATCH 3/3] target/arm: Default to 1GHz cntfrq for 'max' and new CPUs Peter Maydell
2024-04-20 17:04 ` Richard Henderson
2024-04-23 20:47 ` Philippe Mathieu-Daudé [this message]
2024-04-22 12:56 ` [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines Peter Maydell
2024-04-22 13:38 ` Marcin Juszkiewicz
2024-04-22 14:18 ` Peter Maydell
2024-04-22 15:37 ` Marcin Juszkiewicz
2024-04-23 7:26 ` Marcin Juszkiewicz
2024-04-22 15:57 ` Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=482edb42-95cf-402f-9124-4e0b1f76b91f@linaro.org \
--to=philmd@linaro.org \
--cc=armbru@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).