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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.