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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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