| Message ID | 20240720-pmu-v4-3-2a2b28f6b08f@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm/kvm: Report PMU unavailability | expand |
On Sat, 20 Jul 2024 at 10:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > kvm-steal-time and sve properties are added for KVM even if the > corresponding features are not available. Always add pmu property for > Armv8. 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. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/arm/cpu.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 19191c239181..c1955a82fb3c 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1741,6 +1741,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); > + } Not every V7 CPU has a PMU[*]. Unfortunately for PMUv1 the architecture did not define an ID register field for it, so there's no ID field you can look at to distinguish "has PMUv1" from "has no PMU". (For PMUv2 and later you can look at ID_DFR0 bits [27:24]; or for AArch64 ID_AA64DFR0_EL1.PMUVer.) This is why we have the ARM_FEATURE_PMU feature bit. So the correct way to determine "does this CPU have a PMU and so it's OK to add the 'pmu' property" is to look at ARM_FEATURE_PMU. Which is what we already do. Alternatively, if you want to make the property always present even on CPUs where it can't be set, you need to have some mechanism for having the user's attempt to enable it fail. But mostly for Arm at the moment we have properties which are only present when they're meaningful. (I'm not opposed to changing this -- it would arguably be cleaner to have properties be per-class, not per-object, to aid in introspection. But it's a big task and probably not easy.) [*] It happens that all the v7 CPUs that QEMU currently models do have at least a PMUv1, but that's not an architectural requirement. thanks -- PMM
On 2024/07/30 0:13, Peter Maydell wrote: > On Sat, 20 Jul 2024 at 10:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> kvm-steal-time and sve properties are added for KVM even if the >> corresponding features are not available. Always add pmu property for >> Armv8. 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. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> target/arm/cpu.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >> index 19191c239181..c1955a82fb3c 100644 >> --- a/target/arm/cpu.c >> +++ b/target/arm/cpu.c >> @@ -1741,6 +1741,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); >> + } > > Not every V7 CPU has a PMU[*]. Unfortunately for PMUv1 the > architecture did not define an ID register field for it, > so there's no ID field you can look at to distinguish > "has PMUv1" from "has no PMU". (For PMUv2 and later you > can look at ID_DFR0 bits [27:24]; or for AArch64 > ID_AA64DFR0_EL1.PMUVer.) This is why we have the > ARM_FEATURE_PMU feature bit. So the correct way to determine > "does this CPU have a PMU and so it's OK to add the 'pmu' > property" is to look at ARM_FEATURE_PMU. Which is what > we already do. > > Alternatively, if you want to make the property always > present even on CPUs where it can't be set, you need > to have some mechanism for having the user's attempt to > enable it fail. But mostly for Arm at the moment we > have properties which are only present when they're > meaningful. (I'm not opposed to changing this -- it would > arguably be cleaner to have properties be per-class, > not per-object, to aid in introspection. But it's a big > task and probably not easy.) Why not disabling PMU fail for V7 then? If the guest cannot know the presence or the lack of PMUv1, disabling PMUv1 for a V7 CPU that has one is as wrong as enabling PMUv1 for a V7 CPU lacking PMUv1. Regards, Akihiko Odaki
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 19191c239181..c1955a82fb3c 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1741,6 +1741,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)) { @@ -1770,7 +1774,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); } /*
kvm-steal-time and sve properties are added for KVM even if the corresponding features are not available. Always add pmu property for Armv8. 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. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- target/arm/cpu.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)