kvm-steal-time and sve properties are added for KVM even if the
corresponding features are not available. Always add pmu property for
Armv7+. Note that the property is added only for Armv7-A/R+ as QEMU
currently emulates PMU only for such versions, and a different
version may have a different definition of PMU or may not have one at
all.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
The "pmu" property is added only when the PMU is available. This makes
tests/qtest/arm-cpu-features.c fail as it reads the property to check
the availability. Always add the property when the architecture defines
the PMU even if it's not available to fix this.
---
Changes in v5:
- Rebased.
- Link to v4: https://lore.kernel.org/r/20240720-pmu-v4-0-2a2b28f6b08f@daynix.com

Changes in v4:
- Split patch "target/arm/kvm: Fix PMU feature bit early" into
  "target/arm/kvm: Set PMU for host only when available" and
  "target/arm/kvm: Do not silently remove PMU".
- Changed to define PMU also for Armv7.
- Changed not to define PMU for M.
- Extracted patch "hvf: arm: Raise an exception for sysreg by default"
  from "hvf: arm: Properly disable PMU".
- Rebased.
- Link to v3: https://lore.kernel.org/r/20240716-pmu-v3-0-8c7c1858a227@daynix.com

Changes in v3:
- Dropped patch "target/arm: Do not allow setting 'pmu' for hvf".
- Dropped patch "target/arm: Allow setting 'pmu' only for host and max".
- Dropped patch "target/arm/kvm: Report PMU unavailability".
- Added patch "target/arm/kvm: Fix PMU feature bit early".
- Added patch "hvf: arm: Do not advance PC when raising an exception".
- Added patch "hvf: arm: Properly disable PMU".
- Changed to check for Armv8 before adding PMU property.
- Link to v2: https://lore.kernel.org/r/20240716-pmu-v2-0-f3e3e4b2d3d5@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
---
 target/arm/cpu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index dcedadc89eaf..e76d42398eb2 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1761,6 +1761,10 @@ void arm_cpu_post_init(Object *obj)
 
     if (!arm_feature(&cpu->env, ARM_FEATURE_M)) {
         qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_hivecs_property);
+
+        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
+            object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
+        }
     }
 
     if (arm_feature(&cpu->env, ARM_FEATURE_V8)) {
@@ -1790,7 +1794,6 @@ 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);
     }
 
     /*

---
base-commit: 38d0939b86e2eef6f6a622c6f1f7befda0146595
change-id: 20240629-pmu-ad5f67e2c5d0

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

On Sat, 4 Jan 2025 at 07:10, 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 for
> Armv7+. Note that the property is added only for Armv7-A/R+ as QEMU
> currently emulates PMU only for such versions, and a different
> version may have a different definition of PMU or may not have one at
> all.

This isn't how we generally handle CPU properties corresponding
to features. The standard setup is:
 * if the CPU can't have feature foo, no property
 * if the CPU does have feature foo, define a property, so the
   user can turn it off

See also my longer explanation in reply to this patch in v4:

https://lore.kernel.org/all/CAFEAcA_HWfCU09NfZDf6EC=rpvHn148avySCztQ8PqPBMFx4_Q@mail.gmail.com/

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> The "pmu" property is added only when the PMU is available. This makes
> tests/qtest/arm-cpu-features.c fail as it reads the property to check
> the availability. Always add the property when the architecture defines
> the PMU even if it's not available to fix this.

This seems to me like a bug in the test.

> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index dcedadc89eaf..e76d42398eb2 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1761,6 +1761,10 @@ void arm_cpu_post_init(Object *obj)
>
>      if (!arm_feature(&cpu->env, ARM_FEATURE_M)) {
>          qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_hivecs_property);
> +
> +        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
> +            object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
> +        }
>      }
>
>      if (arm_feature(&cpu->env, ARM_FEATURE_V8)) {
> @@ -1790,7 +1794,6 @@ 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);
>      }
>
>      /*

This would allow the user to enable the PMU on a CPU that
says it doesn't have one. We don't generally permit that.

thanks
-- PMM

On 2025/01/28 23:48, Peter Maydell wrote:
> On Sat, 4 Jan 2025 at 07:10, 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 for
>> Armv7+. Note that the property is added only for Armv7-A/R+ as QEMU
>> currently emulates PMU only for such versions, and a different
>> version may have a different definition of PMU or may not have one at
>> all.
> 
> This isn't how we generally handle CPU properties corresponding
> to features. The standard setup is:
>   * if the CPU can't have feature foo, no property
>   * if the CPU does have feature foo, define a property, so the
>     user can turn it off

Is that really standard? The patch message says kvm-steal-time and sve 
properties are added even if the features are not available. Looking at 
other architectures, I can confirm that IvyBridge, an x86_64 CPU, has a 
property avx512f that can be set to true though the physical CPU model 
does not have one. I believe the situation is no different for RISC-V too.

> 
> See also my longer explanation in reply to this patch in v4:
> 
> https://lore.kernel.org/all/CAFEAcA_HWfCU09NfZDf6EC=rpvHn148avySCztQ8PqPBMFx4_Q@mail.gmail.com/

It explains well why the PMU of ARMv7 is different from other features 
like avx512f on x86_64 or RISC-V features; the architecture does not 
allow feature detection. However, as I noted in an earlier email, it 
also means explicitly disabling the PMU on ARMv7 is equally dangerous as 
enabling the PMU. So I see two logical design options:

1. Forbid to explicitly disable or enable the PMU on ARMv7 at all to 
avoid breaking the guest.
2. Allow explicitly disabling or enabling the PMU on ARMv7 under the 
assumption that the property will be used only by experienced users.

> 
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> The "pmu" property is added only when the PMU is available. This makes
>> tests/qtest/arm-cpu-features.c fail as it reads the property to check
>> the availability. Always add the property when the architecture defines
>> the PMU even if it's not available to fix this.
> 
> This seems to me like a bug in the test.
> 
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index dcedadc89eaf..e76d42398eb2 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1761,6 +1761,10 @@ void arm_cpu_post_init(Object *obj)
>>
>>       if (!arm_feature(&cpu->env, ARM_FEATURE_M)) {
>>           qdev_property_add_static(DEVICE(obj), &arm_cpu_reset_hivecs_property);
>> +
>> +        if (arm_feature(&cpu->env, ARM_FEATURE_V7)) {
>> +            object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
>> +        }
>>       }
>>
>>       if (arm_feature(&cpu->env, ARM_FEATURE_V8)) {
>> @@ -1790,7 +1794,6 @@ 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);
>>       }
>>
>>       /*
> 
> This would allow the user to enable the PMU on a CPU that
> says it doesn't have one. We don't generally permit that.
> 
> thanks
> -- PMM