Re: [PATCH] target/arm: Get HVC UNDEF cases for Secure mode right

[Jim MacArthur <jim.macarthur@linaro.org>]

On Fri, Oct 30, 2026 at 09:28:15AM +0000, Peter Maydell wrote:
> In the pre_hvc helper, we have a "bool secure = false" and a FIXME
> comment that we should use the actual Secure state. This is a
> signpost to the fact that we're not getting the UNDEF cases right.
> Specifically, we get most of them correct but are missing the ones
> related to Secure state:
> 
>  * For A64: HVC must UNDEF at EL1 if EL2 is not enabled in the
>    current Security state
> 
>  * For v8 A32/T32, HVC must UNDEF in Secure state, if EL2 is not
>    enabled in the current Security state
> 
>  * For v7, HVC must UNDEF in Secure state
> 
> The v7 requirement is identical to the v8 A32/T32 one, since
> there is no Secure EL2 in v7.
> 
> This fixes a bug where HVC should have UNDEFed but instead trapped
> when in Secure state with Secure EL2 disabled.  The bug is unlikely
> to affect real-world code, because only test code puts itself in a
> situation where HVC should UNDEF and then executes HVC and requires
> it to UNDEF.
> 
> This was noticed by Coverity, because the "if (secure && ...)"
> check was dead code.
> 
> CID: 1689834
> Cc: qemu-stable@nongnu.org
> Fixes: 35979d71c401 ("target-arm: A64: Emulate the HVC insn")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/tcg/op_helper.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
> index 6a673e5ae6e..3d3c850a198 100644
> --- a/target/arm/tcg/op_helper.c
> +++ b/target/arm/tcg/op_helper.c
> @@ -1295,8 +1295,6 @@ void HELPER(pre_hvc)(CPUARMState *env)
>  {
>      ARMCPU *cpu = env_archcpu(env);
>      int cur_el = arm_current_el(env);
> -    /* FIXME: Use actual secure state.  */
> -    bool secure = false;
>      bool undef;
>  
>      if (arm_is_psci_call(cpu, EXCP_HVC)) {
> @@ -1306,6 +1304,8 @@ void HELPER(pre_hvc)(CPUARMState *env)
>          return;
>      }
>  
> +    /* Note that we have already UNDEFed HVC from EL0 at translation time */
> +
>      if (!arm_feature(env, ARM_FEATURE_EL2)) {
>          /* If EL2 doesn't exist, HVC always UNDEFs */
>          undef = true;
> @@ -1316,12 +1316,19 @@ void HELPER(pre_hvc)(CPUARMState *env)
>          undef = env->cp15.hcr_el2 & HCR_HCD;
>      }
>  
> -    /* In ARMv7 and ARMv8/AArch32, HVC is undef in secure state.
> -     * For ARMv8/AArch64, HVC is allowed in EL3.
> -     * Note that we've already trapped HVC from EL0 at translation
> -     * time.
> +    /*
> +     * In ARMv7 and ARMv8/AArch32, HVC is undef in Secure state if
> +     * EL2 is not enabled in the current security state (which is
> +     * always for Secure in v7, as Secure EL2 doesn't exist there).
> +     *
> +     * For ARMv8/AArch64, HVC is undef at EL1 if EL2 is not enabled in
> +     * the current security state.
> +     *
> +     * Because we've already dealt with "EL2 not implemented" above,
> +     * arm_is_el2_enabled() is effectively always true here in NonSecure,
> +     * and we don't need to explicitly also check for secure/non-secure.
>       */
> -    if (secure && (!is_a64(env) || cur_el == 1)) {
> +    if (!arm_is_el2_enabled(env) && (!is_a64(env) || cur_el == 1)) {
>          undef = true;
>      }
>  
> -- 
> 2.43.0

Looks good to me

Reviewed-by: Jim MacArthur <jim.macarthur@linaro.org>