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

[Peter Maydell]

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

> Attempts to use it on anything but a CPUArmState * will be caught by the
> compiler.  object_property_find() will happily take any Object.
>
> Likewise, typos in its second argument will be caught by the compiler.
> object_property_find() will happily return NULL then.
>
>
> I also don't like adding QOM properties to instances.
> arm_cpu_post_init() seems to do that.  I feel it's best to stick to
> class properties whenever practical.

I agree, and the Arm CPU is a bit of an outlier in what it's
doing, for reasons that are largely I think historical. I'd
be happy to review patches that change these to class properties
where applicable, but I suspect that might be tricky...

thanks
-- PMM