[PATCH 0/3] target/arm/kvm: Report PMU unavailability

[PATCH 0/3] target/arm/kvm: Report PMU unavailability

[Akihiko Odaki]

target/arm/kvm.c checked PMU availability but claimed PMU is
available even if it is not. In fact, Asahi Linux supports KVM but lacks
PMU support. Only advertise PMU availability only when it is really
available.

Fixes: dc40d45ebd8e ("target/arm/kvm: Move kvm_arm_get_host_cpu_features and unexport")

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (3):
      tests/arm-cpu-features: Do not assume PMU availability
      target/arm: Always add pmu property
      target/arm/kvm: Report PMU unavailability

 target/arm/cpu.c               |  3 ++-
 target/arm/kvm.c               |  2 +-
 tests/qtest/arm-cpu-features.c | 13 ++++++++-----
 3 files changed, 11 insertions(+), 7 deletions(-)
---
base-commit: 046a64b9801343e2e89eef10c7a48eec8d8c0d4f
change-id: 20240629-pmu-ad5f67e2c5d0

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH 1/3] tests/arm-cpu-features: Do not assume PMU availability

[Akihiko Odaki]

Asahi Linux supports KVM but lacks PMU support.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 tests/qtest/arm-cpu-features.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 966c65d5c3e4..cfd6f7735354 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -509,6 +509,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
     assert_set_feature(qts, "host", "kvm-no-adjvtime", false);
 
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
+        bool kvm_supports_pmu;
         bool kvm_supports_steal_time;
         bool kvm_supports_sve;
         char max_name[8], name[8];
@@ -537,11 +538,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
 
         assert_has_feature_enabled(qts, "host", "aarch64");
 
-        /* Enabling and disabling pmu should always work. */
-        assert_has_feature_enabled(qts, "host", "pmu");
-        assert_set_feature(qts, "host", "pmu", false);
-        assert_set_feature(qts, "host", "pmu", true);
-
         /*
          * Some features would be enabled by default, but they're disabled
          * because this instance of KVM doesn't support them. Test that the
@@ -551,11 +547,18 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         assert_has_feature(qts, "host", "sve");
 
         resp = do_query_no_props(qts, "host");
+        kvm_supports_pmu = resp_get_feature(resp, "pmu");
         kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
         kvm_supports_sve = resp_get_feature(resp, "sve");
         vls = resp_get_sve_vls(resp);
         qobject_unref(resp);
 
+        if (kvm_supports_pmu) {
+            /* If we have pmu then we should be able to toggle it. */
+            assert_set_feature(qts, "host", "pmu", false);
+            assert_set_feature(qts, "host", "pmu", true);
+        }
+
         if (kvm_supports_steal_time) {
             /* If we have steal-time then we should be able to toggle it. */
             assert_set_feature(qts, "host", "kvm-steal-time", false);

-- 
2.45.2

[PATCH 2/3] target/arm: Always add pmu property

[Akihiko Odaki]

kvm-steal-time and sve properties are added for KVM even if the
corresponding features are not available. Always add pmu property too.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/arm/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 35fa281f1b98..0da72c12a5bd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1770,9 +1770,10 @@ void arm_cpu_post_init(Object *obj)
 
     if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
         cpu->has_pmu = true;
-        object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
     }
 
+    object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
+
     /*
      * Allow user to turn off VFP and Neon support, but only for TCG --
      * KVM does not currently allow us to lie to the guest about its

-- 
2.45.2

Re: [PATCH 2/3] target/arm: Always add pmu property

[Peter Maydell]

On Sat, 29 Jun 2024 at 13:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> kvm-steal-time and sve properties are added for KVM even if the
> corresponding features are not available. Always add pmu property too.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  target/arm/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 35fa281f1b98..0da72c12a5bd 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1770,9 +1770,10 @@ void arm_cpu_post_init(Object *obj)
>
>      if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>          cpu->has_pmu = true;
> -        object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
>      }
>
> +    object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);

This will allow the user to set the ARM_FEATURE_PMU feature
bit on TCG CPUs where that doesn't make sense. If we want to
make the property visible on all CPUs, we need to make it
be an error to set it when it's not valid to set it (probably
by adding some TCG/hvf equivalent to the "raise an error
in arm_set_pmu()" code branch we already have for KVM).

thanks
-- PMM

Re: [PATCH 2/3] target/arm: Always add pmu property

[Akihiko Odaki]

On 2024/07/01 20:54, Peter Maydell wrote:
> On Sat, 29 Jun 2024 at 13:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> kvm-steal-time and sve properties are added for KVM even if the
>> corresponding features are not available. Always add pmu property too.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   target/arm/cpu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 35fa281f1b98..0da72c12a5bd 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1770,9 +1770,10 @@ void arm_cpu_post_init(Object *obj)
>>
>>       if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
>>           cpu->has_pmu = true;
>> -        object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
>>       }
>>
>> +    object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
> 
> This will allow the user to set the ARM_FEATURE_PMU feature
> bit on TCG CPUs where that doesn't make sense. If we want to
> make the property visible on all CPUs, we need to make it
> be an error to set it when it's not valid to set it (probably
> by adding some TCG/hvf equivalent to the "raise an error
> in arm_set_pmu()" code branch we already have for KVM).

Doesn't TCG support PMU though?
Certainly hvf needs some care on the other hand.

Regards,
Akihiko Odaki

Re: [PATCH 2/3] target/arm: Always add pmu property

[Peter Maydell]

On Mon, 1 Jul 2024 at 13:17, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/07/01 20:54, Peter Maydell wrote:
> > On Sat, 29 Jun 2024 at 13:51, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> kvm-steal-time and sve properties are added for KVM even if the
> >> corresponding features are not available. Always add pmu property too.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   target/arm/cpu.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> >> index 35fa281f1b98..0da72c12a5bd 100644
> >> --- a/target/arm/cpu.c
> >> +++ b/target/arm/cpu.c
> >> @@ -1770,9 +1770,10 @@ void arm_cpu_post_init(Object *obj)
> >>
> >>       if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
> >>           cpu->has_pmu = true;
> >> -        object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
> >>       }
> >>
> >> +    object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
> >
> > This will allow the user to set the ARM_FEATURE_PMU feature
> > bit on TCG CPUs where that doesn't make sense. If we want to
> > make the property visible on all CPUs, we need to make it
> > be an error to set it when it's not valid to set it (probably
> > by adding some TCG/hvf equivalent to the "raise an error
> > in arm_set_pmu()" code branch we already have for KVM).
>
> Doesn't TCG support PMU though?

Not for every CPU. If the CPU is, say, an ARM1176, then it's
too old to have the PMUv3 that our TCG code emulates. And
that kind of PMU doesn't exist on the M-profile CPUs either.

thanks
-- PMM

[PATCH 3/3] target/arm/kvm: Report PMU unavailability

[Akihiko Odaki]

target/arm/kvm.c checked PMU availability but claimed PMU is
available even if it is not. In fact, Asahi Linux supports KVM but lacks
PMU support. Only advertise PMU availability only when it is really
available.

Fixes: dc40d45ebd8e ("target/arm/kvm: Move kvm_arm_get_host_cpu_features and unexport")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/arm/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 7cf5cf31dec4..6bb72c09be10 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;

-- 
2.45.2

Re: [PATCH 0/3] target/arm/kvm: Report PMU unavailability

[Richard Henderson]

On 6/29/24 05:50, Akihiko Odaki wrote:
> Akihiko Odaki (3):
>        tests/arm-cpu-features: Do not assume PMU availability
>        target/arm: Always add pmu property
>        target/arm/kvm: Report PMU unavailability

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~