Re: [PATCH 06/10] target/arm: Move arm_current_el() and arm_el_is_aa64() to internals.h

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

On 3/6/25 08:39, Peter Maydell wrote:
> The functions arm_current_el() and arm_el_is_aa64() are used only in
> target/arm and in hw/intc/arm_gicv3_cpuif.c.  They're functions that
> query internal state of the CPU.  Move them out of cpu.h and into
> internals.h.
> 
> This means we need to include internals.h in arm_gicv3_cpuif.c, but
> this is justifiable because that file is implementing the GICv3 CPU
> interface, which really is part of the CPU proper; we just ended up
> implementing it in code in hw/intc/ for historical reasons.
> 
> The motivation for this move is that we'd like to change
> arm_el_is_aa64() to add a condition that uses cpu_isar_feature();
> but we don't want to include cpu-features.h in cpu.h.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   target/arm/cpu.h          | 66 --------------------------------------
>   target/arm/internals.h    | 67 +++++++++++++++++++++++++++++++++++++++
>   hw/intc/arm_gicv3_cpuif.c |  1 +
>   target/arm/arch_dump.c    |  1 +
>   4 files changed, 69 insertions(+), 66 deletions(-)

Likewise, is there a good reason to keep arm_current_el inline?

I can see that a fair fraction of arm_el_is_aa64 calls have a constant second argument 
(and most of those check el3).  Therefore I can see that keeping that function inline can 
eliminate quite a lot of tests.

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


> +/* Return true if the specified exception level is running in AArch64 state. */
> +static inline bool arm_el_is_aa64(CPUARMState *env, int el)
> +{
> +    /*
> +     * This isn't valid for EL0 (if we're in EL0, is_a64() is what you want,
> +     * and if we're not in EL0 then the state of EL0 isn't well defined.)
> +     */
> +    assert(el >= 1 && el <= 3);
> +    bool aa64 = arm_feature(env, ARM_FEATURE_AARCH64);
> +
> +    /*
> +     * The highest exception level is always at the maximum supported
> +     * register width, and then lower levels have a register width controlled
> +     * by bits in the SCR or HCR registers.
> +     */
> +    if (el == 3) {
> +        return aa64;
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_EL3) &&
> +        ((env->cp15.scr_el3 & SCR_NS) || !(env->cp15.scr_el3 & SCR_EEL2))) {
> +        aa64 = aa64 && (env->cp15.scr_el3 & SCR_RW);
> +    }
> +
> +    if (el == 2) {
> +        return aa64;
> +    }
> +
> +    if (arm_is_el2_enabled(env)) {
> +        aa64 = aa64 && (env->cp15.hcr_el2 & HCR_RW);
> +    }
> +
> +    return aa64;
> +}

I'll note that this would be clearer with early returns instead of &&.
E.g.

     if (!arm_feature(env, ARM_FEATURE_AARCH64)) {
         return false;
     }
     if (el == 3) {
         return true;
     }

     if (arm_feature(env, ARM_FEATURE_EL3)
         && (env->cp15.scr_el3 & SCR_RW)
         && ((env->cp15.scr_el3 & SCR_NS)
             || !(env->cp15.scr_el3 & SCR_EEL2))) {
         return false;
     }
     if (el == 2) {
         return true;
     }
     ...

But of course not changed with code movement.


r~