Re: [RFC PATCH v2 04/22] target/arm: Implement ALLINT MSR (immediate)

[Richard Henderson]

On 2/21/24 03:08, Jinjie Ruan via wrote:
> Add ALLINT MSR (immediate) to decodetree. And the EL0 check is necessary
> to ALLINT. Avoid the unconditional write to pc and use raise_exception_ra
> to unwind.
>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   target/arm/tcg/a64.decode      |  1 +
>   target/arm/tcg/helper-a64.c    | 24 ++++++++++++++++++++++++
>   target/arm/tcg/helper-a64.h    |  1 +
>   target/arm/tcg/translate-a64.c | 10 ++++++++++
>   4 files changed, 36 insertions(+)
>
> diff --git a/target/arm/tcg/a64.decode b/target/arm/tcg/a64.decode
> index 8a20dce3c8..3588080024 100644
> --- a/target/arm/tcg/a64.decode
> +++ b/target/arm/tcg/a64.decode
> @@ -207,6 +207,7 @@ MSR_i_DIT       1101 0101 0000 0 011 0100 .... 010 11111 
> @msr_i
>   MSR_i_TCO       1101 0101 0000 0 011 0100 .... 100 11111 @msr_i
>   MSR_i_DAIFSET   1101 0101 0000 0 011 0100 .... 110 11111 @msr_i
>   MSR_i_DAIFCLEAR 1101 0101 0000 0 011 0100 .... 111 11111 @msr_i
> +MSR_i_ALLINT    1101 0101 0000 0 001 0100 .... 000 11111 @msr_i
>   MSR_i_SVCR      1101 0101 0000 0 011 0100 0 mask:2 imm:1 011 11111
>   
>   # MRS, MSR (register), SYS, SYSL. These are all essentially the
> diff --git a/target/arm/tcg/helper-a64.c b/target/arm/tcg/helper-a64.c
> index ebaa7f00df..3686926ada 100644
> --- a/target/arm/tcg/helper-a64.c
> +++ b/target/arm/tcg/helper-a64.c
> @@ -66,6 +66,30 @@ void HELPER(msr_i_spsel)(CPUARMState *env, uint32_t imm)
>       update_spsel(env, imm);
>   }
>   
> +static void allint_check(CPUARMState *env, uint32_t op,
> +                       uint32_t imm, uintptr_t ra)
> +{
> +    /* ALLINT update to PSTATE. */
> +    if (arm_current_el(env) == 0) {
> +        raise_exception_ra(env, EXCP_UDEF,
> +                           syn_aa64_sysregtrap(0, extract32(op, 0, 3),
> +                                               extract32(op, 3, 3), 4,
> +                                               imm, 0x1f, 0),
> +                           exception_target_el(env), ra);
> +    }
> +}

A runtime check for EL0 is not necessary; you've already handled that in 
trans_MSR_i_ALLINT().  However, what *is* missing here is the test against TALLINT for EL1.

> +
> +void HELPER(msr_i_allint)(CPUARMState *env, uint32_t imm)
> +{
> +    allint_check(env, 0x8, imm, GETPC());
> +    if (imm == 1) {
> +        env->allint |= PSTATE_ALLINT;
> +    } else {
> +        env->allint &= ~PSTATE_ALLINT;
> +    }

I think you should not write an immediate-specific helper, but one which can also handle 
the variable "MSR allint, <xt>".  This is no more difficult than

void HELPER(msr_allint)(CPUARMState *env, target_ulong val)
{
    ... check ...
    env->pstate = (env->pstate & ~PSTATE_ALLINT) | (val & PSTATE_ALLINT);
}

> +    arm_rebuild_hflags(env);
> +}

allint does not affect hflags; no rebuild required.

> +static bool trans_MSR_i_ALLINT(DisasContext *s, arg_i *a)
> +{
> +    if (!dc_isar_feature(aa64_nmi, s) || s->current_el == 0) {
> +        return false;
> +    }
> +    gen_helper_msr_i_allint(tcg_env, tcg_constant_i32(a->imm));

You're passing all of #imm4, not #imm1, which meant the test in your msr_i_allint helper 
was wrong.

To work with the generalized helper above, this would be

    tcg_constant_tl((a->imm & 1) * PSTATE_ALLINT);


r~