| Message ID | 20240716-pmu-v2-4-f3e3e4b2d3d5@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm/kvm: Report PMU unavailability | expand |
On 16/7/24 10:28, Akihiko Odaki wrote: > kvm-steal-time and sve properties are added for KVM even if the > corresponding features are not available. Always add pmu property too. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/arm/cpu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Tue, 16 Jul 2024 at 09:28, 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 too. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/arm/cpu.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 9e1d15701468..32508644aee7 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1781,9 +1781,10 @@ 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); > } > > + object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); > + > /* > * Allow user to turn off VFP and Neon support, but only for TCG -- > * KVM does not currently allow us to lie to the guest about its Before we do this we need to do something to forbid setting the pmu property to true on CPUs which don't have it. That is: * for CPUs which do have a PMU, we should default to present, and allow the user to turn it on and off with pmu=on/off * for CPUs which do not have a PMU, we should not let the user turn it on and off (either by not providing the property, or else by making the property-set method raise an error, or by having realize detect the discrepancy and raise an error) thanks -- PMM
On 2024/07/16 20:32, Peter Maydell wrote: > On Tue, 16 Jul 2024 at 09:28, 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 too. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> target/arm/cpu.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c >> index 9e1d15701468..32508644aee7 100644 >> --- a/target/arm/cpu.c >> +++ b/target/arm/cpu.c >> @@ -1781,9 +1781,10 @@ 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); >> } >> >> + object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); >> + >> /* >> * Allow user to turn off VFP and Neon support, but only for TCG -- >> * KVM does not currently allow us to lie to the guest about its > > Before we do this we need to do something to forbid setting > the pmu property to true on CPUs which don't have it. That is: > > * for CPUs which do have a PMU, we should default to present, and > allow the user to turn it on and off with pmu=on/off > * for CPUs which do not have a PMU, we should not let the user > turn it on and off (either by not providing the property, or > else by making the property-set method raise an error, or by > having realize detect the discrepancy and raise an error) I don't think there is any reason to prohibit adding a PMU to a CPU that doesn't have when you allow to remove one. For example, neoverse-v1 should always have PMU in the real world. Perhaps it may make sense to prohibit adding a PMU when the CPU is not Armv8 as the PMU we emulate is apparently PMUv3, which is part of Armv8. Regards, Akihiko Odaki
On Tue, 16 Jul 2024 at 12:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2024/07/16 20:32, Peter Maydell wrote: > > On Tue, 16 Jul 2024 at 09:28, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > Before we do this we need to do something to forbid setting > > the pmu property to true on CPUs which don't have it. That is: > > > > * for CPUs which do have a PMU, we should default to present, and > > allow the user to turn it on and off with pmu=on/off > > * for CPUs which do not have a PMU, we should not let the user > > turn it on and off (either by not providing the property, or > > else by making the property-set method raise an error, or by > > having realize detect the discrepancy and raise an error) > > I don't think there is any reason to prohibit adding a PMU to a CPU that > doesn't have when you allow to remove one. For example, neoverse-v1 > should always have PMU in the real world. For example, the Cortex-M3 doesn't have a PMU anything like the A-profile one, so we shouldn't allow the user to set pmu=on. The Arm1176 doesn't have a PMU like the one we emulate, so we shouldn't allow the user to turn it on. All the CPUs where it is reasonable and architecturally valid to have a PMU set the ARM_FEATURE_PMU bit, so there (by design) is no CPU where that bit isn't set by default but could reasonably be enabled by the user. Conversely, the PMUv3 is architecturally optional, so it's not unreasonable to allow the user to disable it even if the real-hardware Neoverse-V1 doesn't provide that as a config option in the RTL. thanks -- PMM
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 9e1d15701468..32508644aee7 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1781,9 +1781,10 @@ 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); } + object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu); + /* * Allow user to turn off VFP and Neon support, but only for TCG -- * KVM does not currently allow us to lie to the guest about its
kvm-steal-time and sve properties are added for KVM even if the corresponding features are not available. Always add pmu property too. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- target/arm/cpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)