Re: [PATCH for-8.2 1/3] target/arm: Do all "ARM_FEATURE_X implies Y" checks in post_init

[Richard Henderson <richard.henderson@linaro.org>]

On 7/24/23 10:43, Peter Maydell wrote:
> Where architecturally one ARM_FEATURE_X flag implies another
> ARM_FEATURE_Y, we allow the CPU init function to only set X, and then
> set Y for it.  Currently we do this in two places -- we set a few
> flags in arm_cpu_post_init() because we need them to decide which
> properties to create on the CPU object, and then we do the rest in
> arm_cpu_realizefn().  However, this is fragile, because it's easy to
> add a new property and not notice that this means that an X-implies-Y
> check now has to move from realize to post-init.
> 
> As a specific example, the pmsav7-dregion property is conditional
> on ARM_FEATURE_PMSA && ARM_FEATURE_V7, which means it won't appear
> on the Cortex-M33 and -M55, because they set ARM_FEATURE_V8 and
> rely on V8-implies-V7, which doesn't happen until the realizefn.
> 
> Move all of these X-implies-Y checks into a new function, which
> we call at the top of arm_cpu_post_init(), so the feature bits
> are available at that point.
> 
> This does now give us the reverse issue, that if there's a feature
> bit which is enabled or disabled by the setting of a property then
> then X-implies-Y features that are dependent on that property need to
> be in realize, not in this new function.  But the only one of those
> is the "EL3 implies VBAR" which is already in the right place, so
> putting things this way round seems better to me.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu.c | 176 +++++++++++++++++++++++++----------------------
>   1 file changed, 94 insertions(+), 82 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 69e2bde3c2d..58301c4b7d8 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1356,17 +1356,105 @@ unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
>         NANOSECONDS_PER_SECOND / cpu->gt_cntfrq_hz : 1;
>   }
>   
> +static void arm_cpu_propagate_feature_implications(ARMCPU *cpu)
> +{
> +    CPUARMState *env = &cpu->env;
> +    bool no_aa32 = false;
> +    /*
> +     * Some features automatically imply others: set the feature

Spacing after local vars.

> +    if (arm_feature(env, ARM_FEATURE_V7VE)) {
> +        /* v7 Virtualization Extensions. In real hardware this implies

Should fix the comment formatting.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I thought I had tried this myself at some point, and ran into a problem.  But I can't 
recall the specifics now.


r~