| Message ID | 20240716-pmu-v3-2-8c7c1858a227@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm/kvm: Report PMU unavailability | expand |
On Tue, 16 Jul 2024 at 13:50, Akihiko Odaki <akihiko.odaki@daynix.com> 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@daynix.com> > --- > 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(). (Also, at some point in the future we're probably going to want to support "tell the guest it has CPU type X via the ID registers even when the host is CPU type Y". It seems plausible that in that case also we'll end up wanting this there too. But I don't put much weight on this because there's probably a bunch of things we'll need to fix up if and when we eventually try to implement this.) thanks -- PMM
On 2024/07/18 21:07, Peter Maydell wrote: > On Tue, 16 Jul 2024 at 13:50, Akihiko Odaki <akihiko.odaki@daynix.com> 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@daynix.com> >> --- >> 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. Regards, Akihiko Odaki
On Fri, Jul 19 2024, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > On 2024/07/18 21:07, Peter Maydell wrote: >> On Tue, 16 Jul 2024 at 13:50, Akihiko Odaki <akihiko.odaki@daynix.com> 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@daynix.com> >>> --- >>> 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.
On 2024/07/19 21:21, Cornelia Huck wrote: > On Fri, Jul 19 2024, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> On 2024/07/18 21:07, Peter Maydell wrote: >>> On Tue, 16 Jul 2024 at 13:50, Akihiko Odaki <akihiko.odaki@daynix.com> 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@daynix.com> >>>> --- >>>> 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
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());
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@daynix.com> --- target/arm/kvm.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)