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

[PATCH v2 0/5] 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>
---
Changes in v2:
- Restricted writes to 'pmu' to host and max.
- Prohibited writes to 'pmu' for hvf.
- Link to v1: https://lore.kernel.org/r/20240629-pmu-v1-0-7269123b88a4@daynix.com

---
Akihiko Odaki (5):
      tests/arm-cpu-features: Do not assume PMU availability
      target/arm: Allow setting 'pmu' only for host and max
      target/arm: Do not allow setting 'pmu' for hvf
      target/arm: Always add pmu property
      target/arm/kvm: Report PMU unavailability

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

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

[PATCH v2 1/5] 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

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

[Philippe Mathieu-Daudé]

On 16/7/24 10:28, Akihiko Odaki wrote:
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 2/5] target/arm: Allow setting 'pmu' only for host and max

[Akihiko Odaki]

Setting 'pmu' does not make sense for CPU types emulating physical
CPUs.

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

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 14d4eca12740..8c180c679ce2 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1594,6 +1594,13 @@ static bool arm_get_pmu(Object *obj, Error **errp)
 static void arm_set_pmu(Object *obj, bool value, Error **errp)
 {
     ARMCPU *cpu = ARM_CPU(obj);
+    const char *typename = object_get_typename(obj);
+
+    if (strcmp(typename, ARM_CPU_TYPE_NAME("host")) &&
+        strcmp(typename, ARM_CPU_TYPE_NAME("max"))) {
+        error_setg(errp, "Setting 'pmu' is only supported by host and max");
+        return;
+    }
 
     if (value) {
         if (kvm_enabled() && !kvm_arm_pmu_supported()) {

-- 
2.45.2

Re: [PATCH v2 2/5] target/arm: Allow setting 'pmu' only for host and max

[Peter Maydell]

On Tue, 16 Jul 2024 at 09:28, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Setting 'pmu' does not make sense for CPU types emulating physical
> CPUs.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  target/arm/cpu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 14d4eca12740..8c180c679ce2 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1594,6 +1594,13 @@ static bool arm_get_pmu(Object *obj, Error **errp)
>  static void arm_set_pmu(Object *obj, bool value, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> +    const char *typename = object_get_typename(obj);
> +
> +    if (strcmp(typename, ARM_CPU_TYPE_NAME("host")) &&
> +        strcmp(typename, ARM_CPU_TYPE_NAME("max"))) {
> +        error_setg(errp, "Setting 'pmu' is only supported by host and max");
> +        return;
> +    }

This doesn't seem right. In general where we provide a
user-facing -cpu foo,bar=off option we allow the user to
use it to disable the bar feature on named CPUs too.
So you can use it to say "give me a neoverse-v1 without the PMU".

thanks
-- PMM

[PATCH v2 3/5] target/arm: Do not allow setting 'pmu' for hvf

[Akihiko Odaki]

hvf currently does not support PMU.

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

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 8c180c679ce2..9e1d15701468 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1603,6 +1603,10 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
     }
 
     if (value) {
+        if (hvf_enabled()) {
+            error_setg(errp, "'pmu' feature not suported by hvf");
+            return;
+        }
         if (kvm_enabled() && !kvm_arm_pmu_supported()) {
             error_setg(errp, "'pmu' feature not supported by KVM on this host");
             return;

-- 
2.45.2

Re: [PATCH v2 3/5] target/arm: Do not allow setting 'pmu' for hvf

[Peter Maydell]

On Tue, 16 Jul 2024 at 09:28, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> hvf currently does not support PMU.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  target/arm/cpu.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 8c180c679ce2..9e1d15701468 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1603,6 +1603,10 @@ static void arm_set_pmu(Object *obj, bool value, Error **errp)
>      }
>
>      if (value) {
> +        if (hvf_enabled()) {
> +            error_setg(errp, "'pmu' feature not suported by hvf");
> +            return;
> +        }
>          if (kvm_enabled() && !kvm_arm_pmu_supported()) {
>              error_setg(errp, "'pmu' feature not supported by KVM on this host");
>              return;

Typo (should be "supported") but otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 4/5] 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 9e1d15701468..32508644aee7 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1781,9 +1781,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 v2 4/5] target/arm: Always add pmu property

[Philippe Mathieu-Daudé]

On 16/7/24 10:28, Akihiko Odaki 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 4/5] target/arm: Always add pmu property

[Peter Maydell]

On Tue, 16 Jul 2024 at 09:28, 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 9e1d15701468..32508644aee7 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1781,9 +1781,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

Before we do this we need to do something to forbid setting
the pmu property to true on CPUs which don't have it. That is:

 * for CPUs which do have a PMU, we should default to present, and
   allow the user to turn it on and off with pmu=on/off
 * for CPUs which do not have a PMU, we should not let the user
   turn it on and off (either by not providing the property, or
   else by making the property-set method raise an error, or by
   having realize detect the discrepancy and raise an error)

thanks
-- PMM

Re: [PATCH v2 4/5] target/arm: Always add pmu property

[Akihiko Odaki]

On 2024/07/16 20:32, Peter Maydell wrote:
> On Tue, 16 Jul 2024 at 09:28, 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 9e1d15701468..32508644aee7 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1781,9 +1781,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
> 
> Before we do this we need to do something to forbid setting
> the pmu property to true on CPUs which don't have it. That is:
> 
>   * for CPUs which do have a PMU, we should default to present, and
>     allow the user to turn it on and off with pmu=on/off
>   * for CPUs which do not have a PMU, we should not let the user
>     turn it on and off (either by not providing the property, or
>     else by making the property-set method raise an error, or by
>     having realize detect the discrepancy and raise an error)

I don't think there is any reason to prohibit adding a PMU to a CPU that 
doesn't have when you allow to remove one. For example, neoverse-v1 
should always have PMU in the real world.

Perhaps it may make sense to prohibit adding a PMU when the CPU is not 
Armv8 as the PMU we emulate is apparently PMUv3, which is part of Armv8.

Regards,
Akihiko Odaki

Re: [PATCH v2 4/5] target/arm: Always add pmu property

[Peter Maydell]

On Tue, 16 Jul 2024 at 12:36, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/07/16 20:32, Peter Maydell wrote:
> > On Tue, 16 Jul 2024 at 09:28, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > Before we do this we need to do something to forbid setting
> > the pmu property to true on CPUs which don't have it. That is:
> >
> >   * for CPUs which do have a PMU, we should default to present, and
> >     allow the user to turn it on and off with pmu=on/off
> >   * for CPUs which do not have a PMU, we should not let the user
> >     turn it on and off (either by not providing the property, or
> >     else by making the property-set method raise an error, or by
> >     having realize detect the discrepancy and raise an error)
>
> I don't think there is any reason to prohibit adding a PMU to a CPU that
> doesn't have when you allow to remove one. For example, neoverse-v1
> should always have PMU in the real world.

For example, the Cortex-M3 doesn't have a PMU anything like the
A-profile one, so we shouldn't allow the user to set pmu=on.
The Arm1176 doesn't have a PMU like the one we emulate, so we
shouldn't allow the user to turn it on. All the CPUs where it
is reasonable and architecturally valid to have a PMU set the
ARM_FEATURE_PMU bit, so there (by design) is no CPU where that
bit isn't set by default but could reasonably be enabled by
the user.

Conversely, the PMUv3 is architecturally optional, so it's not
unreasonable to allow the user to disable it even if the
real-hardware Neoverse-V1 doesn't provide that as a config
option in the RTL.

thanks
-- PMM

[PATCH v2 5/5] 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 70f79eda33cd..b20a35052f41 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 v2 5/5] target/arm/kvm: Report PMU unavailability

[Philippe Mathieu-Daudé]

On 16/7/24 10:28, Akihiko Odaki wrote:
> 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")

Commit dc40d45ebd8e only moves the code around. I suppose you meant:

Fixes: 929e754d5a ("arm: Add an option to turn on/off vPMU support")

> 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 70f79eda33cd..b20a35052f41 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;
>