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

[Philippe Mathieu-Daudé]

Hi,

On 18/12/23 10:48, Peter Maydell wrote:
> On Mon, 18 Dec 2023 at 07:26, Markus Armbruster <armbru@redhat.com> wrote:
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > 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"
> > >
> > > I'm not a super-fan of board-level code looking inside
> > > the QOM object with direct use of arm_feature() when
> > > it doesn't have to. What's wrong with asking whether
> > > the property exists before trying to set it?
> >
> > I'm not a fan of using QOM instead of the native C interface.
> >
> > The native C interface is CPUArmState method arm_feature().
>
> But we don't in most of these cases really want to know "is this
> a CPU with feature foo?". What we're asking is "does this
> QOM property exist so it won't blow up if I set/get it?".

[More analysis on this topic.]

ARMV7M (hw/arm/armv7m.c) is an interesting QOM use case.

ARMV7M is a ARMCPU container, with few more things. (We have
more complex cases with containers of array of vCPUs, so this
single-vCPU case is a good start).

We'd like to apply properties on ARMV7M which get forwarded
to the embedded ARMCPU.
Usually we create the ARMCPU in armv7m_instance_init(), call
object_property_add_alias() to alias some ARMCPU to ARMV7M,
so these properties can be set externally before ARMV7M is
realized, being directly forwarded to the embedded ARMCPU [*].

The problem with ARMV7M is it the ARMCPU QOM type is variable,
so we don't know it in armv7m_instance_init() but only later
in armv7m_realize(), thus we can not call QOM _add_alias() to
alias them. One way to resolve this is to duplicate all possible
ARMCPU properties we want to set on ARMV7M, and set them in
armv7m_realize() after the ARMCPU is created and before it is
realized (the current implementation):

static void armv7m_realize(DeviceState *dev, Error **errp)
{
    ...
    s->cpu = ARM_CPU(object_new_with_props(s->cpu_type,
                                           OBJECT(s), "cpu",
                                           &err, NULL));
    ...

    if (object_property_find(OBJECT(s->cpu), "vfp")) {
        if (!object_property_set_bool(OBJECT(s->cpu), "vfp",
                                      s->vfp, errp)) {
            return;
        }
    }
    ...

    if (!qdev_realize(cpudev, NULL, errp)) {
        return;
    }
    ...
}

static Property armv7m_properties[] = {
    DEFINE_PROP_STRING("cpu-type", ARMv7MState, cpu_type),
    ...
    DEFINE_PROP_BOOL("vfp", ARMv7MState, vfp, true),
    ...
};

Note ARMV7M "vfp" is a /static/ QOM property, so can not be
unregistered if the ARMCPU doesn't expose it.

* If ARMCPU doesn't provide "vfp", ARMV7M properties introspection
  still shows 'vfp=true'.

* If ARMCPU doesn't provide "vfp", requesting 'vfp=true' on ARMV7M
  is silently ignored.

* If ARMCPU doesn't provide "vfp", even if we unregister ARMV7M "vfp"
  property in realize() for cleaner introspection, we can not check
  whether user requested an explicit value before realize().
  Possibly we could use a tri-state {unset/false/true} dynamic property
  to check that.

[*] object_property_add_alias() is a 1-1 static aliasing. In the
    case of cluster of objects we don't have API to do a 1-N static
    aliasing; the current way to do that is similar to dynamic
    properties setters iterating on the array (getter usually return
    the container property, ignoring the cluster values).

Regards,

Phil.