| Message ID | 20231207111048.50568-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Restrict ARM_FEATURE_PMU to system emulation | expand |
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?
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
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 --
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(-)