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

[Akihiko Odaki]

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