On 2025/01/28 23:48, Peter Maydell wrote:
On Sat, 4 Jan 2025 at 07:10, Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
kvm-steal-time and sve properties are added for KVM even if the
corresponding features are not available. Always add pmu property for
Armv7+. Note that the property is added only for Armv7-A/R+ as QEMU
currently emulates PMU only for such versions, and a different
version may have a different definition of PMU or may not have one at
all.
This isn't how we generally handle CPU properties corresponding
to features. The standard setup is:
* if the CPU can't have feature foo, no property
* if the CPU does have feature foo, define a property, so the
user can turn it off
Is that really standard? The patch message says kvm-steal-time and sve
properties are added even if the features are not available. Looking at
other architectures, I can confirm that IvyBridge, an x86_64 CPU, has a
property avx512f that can be set to true though the physical CPU model
does not have one. I believe the situation is no different for RISC-V too.
See also my longer explanation in reply to this patch in v4:
https://lore.kernel.org/all/CAFEAcA_HWfCU09NfZDf6EC=rpvHn148avySCztQ8PqPBMFx4_Q@xxxxxxxxxxxxxx/
It explains well why the PMU of ARMv7 is different from other features
like avx512f on x86_64 or RISC-V features; the architecture does not
allow feature detection. However, as I noted in an earlier email, it
also means explicitly disabling the PMU on ARMv7 is equally dangerous as
enabling the PMU. So I see two logical design options:
1. Forbid to explicitly disable or enable the PMU on ARMv7 at all to
avoid breaking the guest.
2. Allow explicitly disabling or enabling the PMU on ARMv7 under the
assumption that the property will be used only by experienced users.
Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
---
The "pmu" property is added only when the PMU is available. This makes
tests/qtest/arm-cpu-features.c fail as it reads the property to check
the availability. Always add the property when the architecture defines
the PMU even if it's not available to fix this.
This seems to me like a bug in the test.
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index dcedadc89eaf..e76d42398eb2 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1761,6 +1761,10 @@ void arm_cpu_post_init(Object *obj)
if (!arm_feature(&cpu->env, ARM_FEATURE_M)) {
qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_hivecs_property);
+
+ if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+ object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
+ }
}
if (arm_feature(&cpu->env, ARM_FEATURE_V8)) {
@@ -1790,7 +1794,6 @@ void arm_cpu_post_init(Object *obj)
if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
cpu->has_pmu = true;
- object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
}
/*
This would allow the user to enable the PMU on a CPU that
says it doesn't have one. We don't generally permit that.
thanks
-- PMM
