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

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

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