Re: [Qemu-devel] [PATCH v6 06/32] target-arm: A32: Emulate the SMC instruction

[Greg Bellows]

I realize that, but I don't believe your changes were available yet and still sounded to be a bit in flux, so I was waiting to merge.

As I mentioned previously, I had already merged on top of your initial changes.  

I'll recommit with your changes.

Greg

On 13 October 2014 08:06, Peter Maydell <address@hidden> wrote:

On 10 October 2014 18:03, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> Implements SMC instruction in AArch32 using the A32 syndrome. When executing
> SMC instruction from monitor CPU mode SCR.NS bit is reset.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ==========
>
> v5 -> v6
> - Fixed PC offsetting for presmc
> - Removed extraneous semi-colon
> - Fixed merge issue
>
> v4 -> v5
> - Merge pre_smc upstream changes and incorporated ss_advance
> ---
>  target-arm/helper.c    | 11 +++++++++++
>  target-arm/internals.h |  5 +++++
>  target-arm/op_helper.c |  3 +--
>  target-arm/translate.c | 39 +++++++++++++++++++++++++++++----------
>  4 files changed, 46 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 2381e6f..7f3f049 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4090,6 +4090,12 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          mask = CPSR_A | CPSR_I | CPSR_F;
>          offset = 4;
>          break;
> +    case EXCP_SMC:
> +        new_mode = ARM_CPU_MODE_MON;
> +        addr = 0x08;
> +        mask = CPSR_A | CPSR_I | CPSR_F;
> +        offset = 0;
> +        break;
>      default:
>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>          return; /* Never happens.  Keep compiler happy.  */
> @@ -4108,6 +4114,11 @@ void arm_cpu_do_interrupt(CPUState *cs)
>           */
>          addr += env->cp15.vbar_el[1];
>      }
> +
> +    if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) {
> +        env->cp15.scr_el3 &= ~SCR_NS;
> +    }
> +
>      switch_mode (env, new_mode);
>      /* For exceptions taken to AArch32 we must clear the SS bit in both
>       * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it now.
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index fd69a83..544fb42 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -236,6 +236,11 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
>          | (is_thumb ? 0 : ARM_EL_IL);
>  }
>
> +static inline uint32_t syn_aa32_smc(void)
> +{
> +    return (EC_AA32_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL;
> +}
> +
>  static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
>  {
>      return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 0809d63..9e38f26 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -419,8 +419,7 @@ void HELPER(pre_hvc)(CPUARMState *env)
>  void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
>  {
>      int cur_el = arm_current_el(env);
> -    /* FIXME: Use real secure state.  */
> -    bool secure = false;
> +    bool secure = arm_is_secure(env);
>      bool smd = env->cp15.scr_el3 & SCR_SMD;
>      /* On ARMv8 AArch32, SMD only applies to NS state.
>       * On ARMv7 SMD only applies to NS state and only if EL2 is available.
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 617e6a9..60655e1 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7872,15 +7872,27 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>          case 7:
>          {
>              int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
> -            /* SMC instruction (op1 == 3)
> -               and undefined instructions (op1 == 0 || op1 == 2)
> -               will trap */
> -            if (op1 != 1) {
> +            if (op1 == 1) {
> +                /* bkpt */
> +                ARCH(5);
> +                gen_exception_insn(s, 4, EXCP_BKPT,
> +                        syn_aa32_bkpt(imm16, false));
> +            } else if (op1 == 3) {
> +                /* smi/smc */
> +                if (!arm_dc_feature(s, ARM_FEATURE_EL3) ||
> +                        s->current_el == 0) {
> +                    goto illegal_op;
> +                }
> +                gen_set_pc_im(s, s->pc - 4);
> +                tmp = tcg_const_i32(syn_aa32_smc());
> +                gen_helper_pre_smc(cpu_env, tmp);
> +                tcg_temp_free_i32(tmp);
> +                gen_ss_advance(s);
> +                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
> +                break;

This is wrong; you should be basing this series on top of my PSCI
series which will get you a correct implementation of the translate.c
changes for SMC for A32/T32. (You'll still need the do_interrupt
changes.)

thanks
-- PMM