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

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

[Philippe Mathieu-Daudé]

ARM Performance Monitor Unit is not reachable from user
emulation, restrict it to system emulation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 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
 
     /*
      * Allow user to turn off VFP and Neon support, but only for TCG --
-- 
2.41.0

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

[Philippe Mathieu-Daudé]

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>
> ---
>   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?

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