On 2024/07/19 21:21, Cornelia Huck wrote:
On Fri, Jul 19 2024, Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
On 2024/07/18 21:07, Peter Maydell wrote:
On Tue, 16 Jul 2024 at 13:50, Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
kvm_arm_get_host_cpu_features() used to add the PMU feature
unconditionally, and kvm_arch_init_vcpu() removed it when it is actually
not available. Conditionally add the PMU feature in
kvm_arm_get_host_cpu_features() to save code.
Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
---
target/arm/kvm.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 70f79eda33cd..849e2e21b304 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -280,6 +280,7 @@ static bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
if (kvm_arm_pmu_supported()) {
init.features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
pmu_supported = true;
+ features |= 1ULL << ARM_FEATURE_PMU;
}
if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) {
@@ -448,7 +449,6 @@ static bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
features |= 1ULL << ARM_FEATURE_V8;
features |= 1ULL << ARM_FEATURE_NEON;
features |= 1ULL << ARM_FEATURE_AARCH64;
- features |= 1ULL << ARM_FEATURE_PMU;
features |= 1ULL << ARM_FEATURE_GENERIC_TIMER;
ahcf->features = features;
@@ -1888,13 +1888,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
if (!arm_feature(env, ARM_FEATURE_AARCH64)) {
cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT;
}
- if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) {
- cpu->has_pmu = false;
- }
if (cpu->has_pmu) {
cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3;
- } else {
- env->features &= ~(1ULL << ARM_FEATURE_PMU);
}
if (cpu_isar_feature(aa64_sve, cpu)) {
assert(kvm_arm_sve_supported());
Not every KVM CPU is necessarily the "host" CPU type.
The "cortex-a57" and "cortex-a53" CPU types will work if you
happen to be on a host of that CPU type, and they don't go
through kvm_arm_get_host_cpu_features().
kvm_arm_vcpu_init() will emit an error in such a situation and I think
it's better than silently removing a feature that the requested CPU type
has. A user can still disable the feature if desired.
OTOH, if we fail for the named cpu models if the kernel does not provide
the cap, but silently disable for the host cpu model in that case, that
also seems inconsistent. I'd rather keep it as it is now.
There are two perspectives of consistency:
1) The initial value of pmu
2) The behavior with the pmu value
This change introduces inconsistency for 1); the host cpu model will
have pmu=off by default and the other cpu models will keep default
pmu=on value on a system that does not support PMU. It still keeps
consistency for 2); it fails if the user sets pmu=on for any cpu model
on such a system.
We should align 1) for better consistency, but I don't think such a
change would be useful. It is likely that something is wrong with the
system when the system reports a cpu model but it doesn't support its
feature. I think that is the reason why we assert
kvm_arm_sve_supported() for SVE; however I don't think such an assertion
would help either because kvm_arm_vcpu_init() will fail anyway.
Regards,
Akihiko Odaki
