Re: [Qemu-devel] [PATCH v6 08/10] target-arm: A64: Emulate the SMC insn

[Peter Maydell]

On 13 September 2014 05:29, Edgar E. Iglesias <address@hidden> wrote:
> From: "Edgar E. Iglesias" <address@hidden>
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>  target-arm/cpu.h           |  1 +
>  target-arm/helper-a64.c    |  1 +
>  target-arm/helper.c        |  6 ++++++
>  target-arm/helper.h        |  1 +
>  target-arm/internals.h     |  6 ++++++
>  target-arm/op_helper.c     | 26 ++++++++++++++++++++++++++
>  target-arm/translate-a64.c | 13 +++++++++++++
>  7 files changed, 54 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b553f3d..c24af40 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -52,6 +52,7 @@
>  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
>  #define EXCP_STREX          10
>  #define EXCP_HVC            11   /* HyperVisor Call */
> +#define EXCP_SMC            12   /* Secure Monitor Call */
>
>  #define ARMV7M_EXCP_RESET   1
>  #define ARMV7M_EXCP_NMI     2
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 4e6ca26..996dfea 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -477,6 +477,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      case EXCP_UDEF:
>      case EXCP_SWI:
>      case EXCP_HVC:
> +    case EXCP_SMC:
>          env->cp15.esr_el[new_el] = env->exception.syndrome;
>          break;
>      case EXCP_IRQ:
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 504ff42..719c95d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3677,6 +3677,12 @@ unsigned int arm_excp_target_el(CPUState *cs, unsigned 
> int excp_idx)
>      case EXCP_HVC:
>          target_el = MAX(target_el, 2);
>          break;
> +    case EXCP_SMC:
> +        target_el = 3;
> +        if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
> +            target_el = 2;
> +        }
> +        break;
>      }
>      return target_el;
>  }
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 75fc1b3..dec3728 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -51,6 +51,7 @@ DEF_HELPER_3(exception_with_syndrome, void, env, i32, i32)
>  DEF_HELPER_1(wfi, void, env)
>  DEF_HELPER_1(wfe, void, env)
>  DEF_HELPER_1(pre_hvc, void, env)
> +DEF_HELPER_2(pre_smc, void, env, i32)
>
>  DEF_HELPER_3(cpsr_write, void, env, i32, i32)
>  DEF_HELPER_1(cpsr_read, i32, env)
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index afcc4e0..e15ae57 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -54,6 +54,7 @@ static const char * const excnames[] = {
>      [EXCP_KERNEL_TRAP] = "QEMU intercept of kernel commpage",
>      [EXCP_STREX] = "QEMU intercept of STREX",
>      [EXCP_HVC] = "Hypervisor Call",
> +    [EXCP_SMC] = "Secure Monitor Call",
>  };
>
>  static inline void arm_log_exception(int idx)
> @@ -221,6 +222,11 @@ static inline uint32_t syn_aa64_hvc(uint32_t imm16)
>      return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>  }
>
> +static inline uint32_t syn_aa64_smc(uint32_t imm16)
> +{
> +    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> +}
> +
>  static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
>  {
>      return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 8441c27..e6a0656 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -406,6 +406,32 @@ void HELPER(pre_hvc)(CPUARMState *env)
>      }
>  }
>
> +void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
> +{
> +    int cur_el = arm_current_pl(env);
> +    /* FIXME: Use real secure state.  */
> +    bool secure = false;
> +    bool smd = env->cp15.scr_el3 & SCR_SMD;
> +    /* On ARMv8 AArch32, SMD only applies to NS mode.
> +     * On ARMv7 SMD only applies to NS mode and only if EL2 is available.
> +     * For ARMv7 non EL2, we force SMD to zero so we don't need to re-check
> +     * the EL2 condition here.
> +     */

NS isn't a mode, it's a state. (S vs NS state is orthogonal
to exception levels and mostly orthogonal to AArch32 modes.)

> +    bool udef = is_a64(env) ? smd : !secure && smd;

What precedence did you intend here between the ?: and
the && operators? More brackets would be nice...

> +    /* In NS EL1, HCR controlled routing to EL2 has priority over SMD.  */
> +    if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
> +        env->exception.syndrome = syndrome;
> +        raise_exception(env, EXCP_SMC);

Shouldn't this just be returning so that the generated
code immediately following can raise the SMC exception
with the correct syndrome, PC and singlestep state?
(would also save you passing in the syndrome argument
to this fn).

> +    }
> +
> +    /* We've already checked that EL3 exists at translation time.  */
> +    if (udef) {
> +        env->exception.syndrome = syn_uncategorized();
> +        raise_exception(env, EXCP_UDEF);
> +    }
> +}
> +
>  void HELPER(exception_return)(CPUARMState *env)
>  {
>      int cur_el = arm_current_pl(env);
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 8eaa85f..9a58de8 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1470,6 +1470,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>      int opc = extract32(insn, 21, 3);
>      int op2_ll = extract32(insn, 0, 5);
>      int imm16 = extract32(insn, 5, 16);
> +    TCGv_i32 tmp;
>
>      switch (opc) {
>      case 0:
> @@ -1495,6 +1496,18 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>              gen_ss_advance(s);
>              gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>              break;
> +        case 3:
> +            if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl == 0) {
> +                unallocated_encoding(s);
> +                break;
> +            }
> +            gen_a64_set_pc_im(s->pc - 4);
> +            tmp = tcg_const_i32(syn_aa64_smc(imm16));
> +            gen_helper_pre_smc(cpu_env, tmp);
> +            tcg_temp_free_i32(tmp);
> +            gen_ss_advance(s);
> +            gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16));
> +            break;
>          default:
>              unallocated_encoding(s);
>              break;
> --
> 1.9.1

thanks
-- PMM