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

[Cornelia Huck <cohuck@redhat.com>]

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.