1. Patchew
  2. QEMU
  3. [PATCH v3 0/5] target/arm/kvm: Report PMU unavailability
  4. View patch

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

Akihiko Odaki posted 5 patches 2 months ago
Maintainers: Peter Maydell <peter.maydell@linaro.org>, Alexander Graf <agraf@csgraf.de>, Paolo Bonzini <pbonzini@redhat.com>, Thomas Huth <thuth@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v3 2/5] target/arm/kvm: Fix PMU feature bit early
Posted by Akihiko Odaki 2 months ago 
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());

-- 
2.45.2
Re: [PATCH v3 2/5] target/arm/kvm: Fix PMU feature bit early
Posted by Peter Maydell 2 months ago 
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
Re: [PATCH v3 2/5] target/arm/kvm: Fix PMU feature bit early
Posted by Akihiko Odaki 1 month, 4 weeks ago 
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
Re: [PATCH v3 2/5] target/arm/kvm: Fix PMU feature bit early
Posted by Cornelia Huck 1 month, 4 weeks ago 
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.
Re: [PATCH v3 2/5] target/arm/kvm: Fix PMU feature bit early
Posted by Akihiko Odaki 1 month, 4 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.