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

[Greg Bellows]

On 6 October 2014 10:46, Peter Maydell <address@hidden> wrote:

On 30 September 2014 22:49, 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>
>
> ----------
> v4 -> v5
> - Merge pre_smc upstream changes and incorporated ss_advance
> ---
>  target-arm/helper.c    | 11 +++++++++++
>  target-arm/internals.h |  7 ++++++-
>  target-arm/op_helper.c |  3 +--
>  target-arm/translate.c | 39 +++++++++++++++++++++++++++++----------
>  4 files changed, 47 insertions(+), 13 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..43a2e7d 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -236,7 +236,12 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
>          | (is_thumb ? 0 : ARM_EL_IL);
>  }
>
> -static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
> +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(uint16_t imm16)

Bogus change (probably introduced by accident in a merge
conflict fixup).

Fixed in v6.

 

>  {
>      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..8ed8ee9 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);;

Doubled semicolon.

Fixed in v6

 

>      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 f6404be..3f3ddfb 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_pl == 0) {
> +                    goto illegal_op;
> +                }
> +                gen_set_pc_im(s, s->pc);

This should be s->pc - 4, because if the pre_smc helper throws
an UNDEF exception you want the PC to point to the SMC insn,
not the insn after. (If the SMC executes as an SMC then the
PC for the EXCP_SMC will be correct because of the 0 offset
passed to gen_exception_insn below.)

Fixed in v6

 

> +                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;
> +            } else {
>                  goto illegal_op;
>              }

That said, I have a feeling we might want to put the SMC
handling into the end-of-loop processing for A32/T32,
the same way we do SVC. Ard has a patch onlist which does
that, though it is on my todo list to try to fix because
it has its own issues...

I'll leave it as-is for now because it appears to work.  In the meantime, I'll hunt for Ard's patch... unless you have a pointer and can save me the trouble.

 

-- PMM