On Tue, 16 Jan 2024 at 16:43, Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Tue, 16 Jan 2024 at 16:20, Philippe Mathieu-Daudé <phi...@linaro.org> > wrote: > > > > On 13/1/24 14:36, Peter Maydell wrote: > > > On Wed, 10 Jan 2024 at 19:53, Philippe Mathieu-Daudé <phi...@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-phi...@linaro.org/ > > >> [2] > > >> https://lore.kernel.org/qemu-devel/20240109180930.90793-1-phi...@linaro.org/ > > >> > > >> Based-on: <20231123143813.42632-1-phi...@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
