Re: [RFC PATCH] hw/arm: Prefer arm_feature() over object_property_find()

[Peter Maydell]

On Thu, 14 Dec 2023 at 17:14, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> QOM properties are added on the ARM vCPU object when a
> feature is present. Rather than checking the property
> is present, check the feature.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: If there is no objection on this patch, I can split
>      as a per-feature series if necessary.
>
> Based-on: <20231123143813.42632-1-philmd@linaro.org>
>   "hw: Simplify accesses to CPUState::'start-powered-off' property"
> ---
>  hw/arm/armv7m.c       | 21 ++++++++++++---------
>  hw/arm/exynos4210.c   |  4 ++--
>  hw/arm/highbank.c     |  3 ++-
>  hw/arm/integratorcp.c |  5 ++---
>  hw/arm/realview.c     |  2 +-
>  hw/arm/sbsa-ref.c     |  3 ++-
>  hw/arm/versatilepb.c  |  5 ++---
>  hw/arm/vexpress.c     |  6 ++++--
>  hw/arm/virt.c         | 27 +++++++++++++++------------
>  hw/arm/xilinx_zynq.c  |  2 +-
>  hw/cpu/a15mpcore.c    | 17 +++++++++++------
>  hw/cpu/a9mpcore.c     |  6 +++---
>  12 files changed, 57 insertions(+), 44 deletions(-)
>
> diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
> index 3a6d72b0f3..932061c11a 100644
> --- a/hw/arm/armv7m.c
> +++ b/hw/arm/armv7m.c
> @@ -302,28 +302,29 @@ static void armv7m_realize(DeviceState *dev, Error 
> **errp)
>
>      object_property_set_link(OBJECT(s->cpu), "memory", OBJECT(&s->container),
>                               &error_abort);
> -    if (object_property_find(OBJECT(s->cpu), "idau")) {
> +    if (arm_feature(&s->cpu->env, ARM_FEATURE_M_SECURITY)) {
>          object_property_set_link(OBJECT(s->cpu), "idau", s->idau,
>                                   &error_abort);
> -    }
> -    if (object_property_find(OBJECT(s->cpu), "init-svtor")) {
>          if (!object_property_set_uint(OBJECT(s->cpu), "init-svtor",
>                                        s->init_svtor, errp)) {
>              return;
>          }
>      }
> -    if (object_property_find(OBJECT(s->cpu), "init-nsvtor")) {
> +    if (arm_feature(&s->cpu->env, ARM_FEATURE_M)) {

This doesn't make sense as a check -- we shouldn't be able to get
here if the CPU isn't M-profile.

>          if (!object_property_set_uint(OBJECT(s->cpu), "init-nsvtor",
>                                        s->init_nsvtor, errp)) {
>              return;
>          }
>      }
> -    if (object_property_find(OBJECT(s->cpu), "vfp")) {
> -        if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, errp)) {
> -            return;
> +    if (arm_feature(&s->cpu->env, ARM_FEATURE_AARCH64)) {

Similarly this can't possibly be an AArch64 CPU, so this is
not the correct condition to check.

> +        if (cpu_isar_feature(aa64_fp_simd, s->cpu)) {
> +            if (!object_property_set_bool(OBJECT(s->cpu), "vfp", s->vfp, 
> errp)) {
> +                return;
> +            }
>          }
>      }
> -    if (object_property_find(OBJECT(s->cpu), "dsp")) {
> +    if (arm_feature(&s->cpu->env, ARM_FEATURE_M) &&
> +        arm_feature(&s->cpu->env, ARM_FEATURE_THUMB_DSP)) {
>          if (!object_property_set_bool(OBJECT(s->cpu), "dsp", s->dsp, errp)) {

Another unnecessary "is this M-profile?" check. This also is
introducing a point of potential future failure because now
we have the condition for "do we have a dsp property" in two
places: in the CPU object where we add it, and then again here
when we set it. Now they can get out of sync.

Most of the others are similar. There might be places where we're
using the "does property X check" to do something more than just
guard "now set property X"; those are probably worth looking at
to see if they should be checking something else.

thanks
-- PMM