Re: [PATCH v3 2/5] target/arm/kvm: Fix PMU feature bit early

[Peter Maydell <peter.maydell@linaro.org>]

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