[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_f
From: |
Peter Maydell |
Subject: |
Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find() |
Date: |
Sat, 13 Jan 2024 13:36:53 +0000 |
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
> 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
- [PATCH v3 09/14] hw/arm: Prefer arm_feature(EL2) over object_property_find(has_el2), (continued)
- [PATCH v3 09/14] hw/arm: Prefer arm_feature(EL2) over object_property_find(has_el2), Philippe Mathieu-Daudé, 2024/01/10
- [PATCH v3 10/14] hw/arm: Prefer arm_feature(CBAR*) over object_property_find(reset-cbar), Philippe Mathieu-Daudé, 2024/01/10
- [PATCH v3 11/14] hw/arm: Prefer arm_feature(PMU) over object_property_find(pmu), Philippe Mathieu-Daudé, 2024/01/10
- [PATCH v3 12/14] hw/arm: Prefer arm_feature(GENERIC_TMR) over 'kvm-no-adjvtime' property, Philippe Mathieu-Daudé, 2024/01/10
- [PATCH v3 13/14] hw/arm: Prefer arm_feature(AARCH64) over object_property_find(aarch64), Philippe Mathieu-Daudé, 2024/01/10
[PATCH v3 14/14] hw/arm: Prefer cpu_isar_feature(aa64_mte) over property_find(tag-memory), Philippe Mathieu-Daudé, 2024/01/10
Re: [PATCH v3 00/14] hw/arm: Prefer arm_feature() over object_property_find(),
Peter Maydell <=