Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find()

[Peter Maydell]

On Tue, 16 Jan 2024 at 16:43, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 16 Jan 2024 at 16:20, Philippe Mathieu-Daudé <philmd@linaro.org> 
> wrote:
> >
> > On 13/1/24 14:36, Peter Maydell wrote:
> > > On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <philmd@linaro.org> 
> > > wrote:
> > >>
> > >> Since v2 [2]:
> > >> - Dropped "Simplify checking A64_MTE bit in FEATURE_ID register"
> > >> - Correct object_property_get_bool() uses
> > >> - Update ARM_FEATURE_AARCH64 && aa64_mte
> > >>
> > >> Since RFC [1]:
> > >> - Split one patch per feature
> > >> - Addressed Peter's review comments
> > >>
> > >> [1] 
> > >> https://lore.kernel.org/qemu-devel/20231214171447.44025-1-philmd@linaro.org/
> > >> [2] 
> > >> 20240109180930.90793-1-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20240109180930.90793-1-philmd@linaro.org/
> > >>
> > >> Based-on: <20231123143813.42632-1-philmd@linaro.org>
> > >>    "hw: Simplify accesses to CPUState::'start-powered-off' property"
> > >>
> > >> Philippe Mathieu-Daudé (14):
> > >>    hw/arm/armv7m: Introduce cpudev variable in armv7m_realize()
> > >>    hw/arm/armv7m: Ensure requested CPU type implements ARM_FEATURE_M
> > >>    hw/arm/armv7m: Move code setting 'start-powered-off' property around
> > >>    hw/arm/armv7m: Always set 'init-nsvtor' property for Cortex-M CPUs
> >
> >
> > > The first part of this is fine and reasonable cleanup, but I
> > > continue to disagree about the later parts. What we want to do is
> > > "if this property is present, then set it", and that's what we do.
> > > Conversion to "if <some condition we know that the CPU is using to
> > > decide whether to define the property> then set it" is duplicating
> > > the condition logic in two places and opening the door for bugs
> > > where we change the condition in one place and not in the other.
> > > Where the <some condition> is a simple "feature X is set" it doesn't
> > > look too bad, but where it gets more complex it makes it IMHO more
> > > obvious that this is a bad idea, for example with:
> > >
> > > -        if (object_property_find(cpuobj, "reset-cbar")) {
> > > +        if (arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
> > > +            arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO)) {
> >
> > For that we could add helpers such
> >
> >    static inline bool arm_feature_cbar(CPUState *cpu) {
> >      return arm_feature(&cpu->env, ARM_FEATURE_CBAR) ||
> >             arm_feature(&cpu->env, ARM_FEATURE_CBAR_RO);
> >    }
> >
> > and use it in the target/ code where we register the property
> > and in the hw/ code where we set it.
>
> Well, we could, but why? The question we're trying to
> answer is "can we set this property?" and the simplest
> and most logical way to test that is "does the object
> have the property?". I really don't understand why we
> would want to change the code at all.
>
> > Do you mind taking the cleanup patches (1-4) meanwhile?
>
> Yes, I can take patches 1-4.

Hmm, this doesn't apply cleanly (probably due to the dependency
noted in the Based-on tag). Can you resend the relevant patches
in a form where they'll apply, or ping me once the dependency has
gone upstream), please?

thanks
-- PMM