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

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

[Peter Maydell]

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

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

[Jim MacArthur]

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>