| Message ID | 20240110195329.3995-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/arm: Prefer arm_feature() over object_property_find() | expand |
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] 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 > hw/arm: Prefer arm_feature(M_SECURITY) over object_property_find() > hw/arm: Prefer arm_feature(THUMB_DSP) over object_property_find(dsp) > hw/arm: Prefer arm_feature(V7) over > object_property_find(pmsav7-dregion) > hw/arm: Prefer arm_feature(EL3) over object_property_find(has_el3) > hw/arm: Prefer arm_feature(EL2) over object_property_find(has_el2) > hw/arm: Prefer arm_feature(CBAR*) over > object_property_find(reset-cbar) > hw/arm: Prefer arm_feature(PMU) over object_property_find(pmu) > hw/arm: Prefer arm_feature(GENERIC_TMR) over 'kvm-no-adjvtime' > property > hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64) > hw/arm: Prefer cpu_isar_feature(aa64_mte) over > property_find(tag-memory) 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)) { thanks -- PMM
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] 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. Anyway I'll wait to see how Kevin's effort is going before insisting with this series. Do you mind taking the cleanup patches (1-4) meanwhile? Thanks, Phil.
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] 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. thanks -- PMM
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] 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
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] 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 hw/arm: Prefer arm_feature(M_SECURITY) over object_property_find() hw/arm: Prefer arm_feature(THUMB_DSP) over object_property_find(dsp) hw/arm: Prefer arm_feature(V7) over object_property_find(pmsav7-dregion) hw/arm: Prefer arm_feature(EL3) over object_property_find(has_el3) hw/arm: Prefer arm_feature(EL2) over object_property_find(has_el2) hw/arm: Prefer arm_feature(CBAR*) over object_property_find(reset-cbar) hw/arm: Prefer arm_feature(PMU) over object_property_find(pmu) hw/arm: Prefer arm_feature(GENERIC_TMR) over 'kvm-no-adjvtime' property hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64) hw/arm: Prefer cpu_isar_feature(aa64_mte) over property_find(tag-memory) hw/arm/armv7m.c | 36 ++++++++++++++++-------------------- 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 | 26 +++++++++++++------------- hw/arm/xilinx_zynq.c | 2 +- hw/cpu/a15mpcore.c | 23 +++++++++++++++-------- hw/cpu/a9mpcore.c | 9 +++++---- 12 files changed, 65 insertions(+), 59 deletions(-)