Re: [PATCH] target/arm: Restrict ARM_FEATURE_PMU to system emulation

[Peter Maydell]

On Thu, 7 Dec 2023 at 13:19, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 7/12/23 12:10, Philippe Mathieu-Daudé wrote:
> > ARM Performance Monitor Unit is not reachable from user
> > emulation, restrict it to system emulation.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

What's the aim of this patch? In general if we can avoid
ifdefs then I prefer to avoid ifdefs, because they clutter
up the code. So they should come with a justification of
why they're necessary (eg "the QOM property is visible to
the end-user but pointless" or "we want to be able to
change this file to be compiled a different way and this is
a necessary substep" or whatever the reason is).


> > ---
> >   target/arm/cpu.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 51f57fd5b4..60cf747fd6 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1410,6 +1410,7 @@ static Property arm_cpu_pmsav7_dregion_property =
> >                                              pmsav7_dregion,
> >                                              qdev_prop_uint32, uint32_t);
> >
> > +#ifndef CONFIG_USER_ONLY
> >   static bool arm_get_pmu(Object *obj, Error **errp)
> >   {
> >       ARMCPU *cpu = ARM_CPU(obj);
> > @@ -1432,6 +1433,7 @@ static void arm_set_pmu(Object *obj, bool value, 
> > Error **errp)
> >       }
> >       cpu->has_pmu = value;
> >   }
> > +#endif
> >
> >   unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
> >   {
> > @@ -1592,12 +1594,12 @@ void arm_cpu_post_init(Object *obj)
> >       if (arm_feature(&cpu->env, ARM_FEATURE_EL2)) {
> >           qdev_property_add_static(DEVICE(obj), &arm_cpu_has_el2_property);
> >       }
> > -#endif
> >
> >       if (arm_feature(&cpu->env, ARM_FEATURE_PMU)) {
> >           cpu->has_pmu = true;
> >           object_property_add_bool(obj, "pmu", arm_get_pmu, arm_set_pmu);
> >       }
> > +#endif
>
> I think this patch is incomplete: should the PMU registers in
> v7_cp_reginfo[] be restricted to TCG?

I can't remember if we had this conversation on IRC, but
in general it's preferable not to restrict a regdef to
TCG only because that means that gdb won't be able to read
the register value if you're using KVM.

thanks
-- PMM