Re: [PATCH v5] target/arm: Always add pmu property for Armv7-A/R+

[Peter Maydell]

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