Re: [Qemu-devel] [PATCH v1 14/16] target-arm: A64: Emulate the SMC insn

[Greg Bellows]

On 30 May 2014 22:49, Edgar E. Iglesias <address@hidden> wrote:

On Fri, May 30, 2014 at 11:50:23AM -0500, Greg Bellows wrote:
> On 30 May 2014 02:28, 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     | 11 +++++++++++
> >  target-arm/translate-a64.c | 10 ++++++++++
> >  7 files changed, 36 insertions(+)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 1a26ed4..b3631f2 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 974fa66..3894a6f 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -476,6 +476,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 5b2070c..35091ea 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3298,6 +3298,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 (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 fb711be..6c3d84d 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_2(hvc, void, env, i32)
> > +DEF_HELPER_2(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 b08381c..e50a68e 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)
> > @@ -210,6 +211,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 6bf34b0..6840828 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -405,6 +405,17 @@ void HELPER(hvc)(CPUARMState *env, uint32_t syndrome)
> >      raise_exception(env, EXCP_HVC);
> >  }
> >
> > +void HELPER(smc)(CPUARMState *env, uint32_t syndrome)
> > +{
> > +    /* We've already checked that EL3 exists at translation time.  */
> > +    if (env->cp15.scr_el3 & SCR_SMD) {
> >
>
> In ARMv7 isn't this is this only a valid check if the virtualization
> extension is present?  If so, should we be checking for the virt extension
> on AArch32 prior to raising undefined?

My understanding is that on AArch32 SCR.SMD only applies
to NS mode and only if HCR.TSC=0 (always the case if EL2 does not exist but
can also be zero even if EL2 exists).
So it's not dependant on virtual extensions/EL2 non-existance? Am I missing
something?

I'm not sure how we handle this as the ARMv7 spec states the following for the SCD bit:

"SCD, bit[7], when implementation includes the Virtualization Extensions"

This is not worded the same in the ARMv8 spec, possibly because the extensions are assumed to always be present?  The QQMU code still supports the notion of features in which case sometimes are extensions.

 

I'm not adding AArch32 SMC here but am happy to try to get it right
in preparation for the AArch32 support.

Sure, I suspect that Fabian's commits on top of yours will close any AArch32 gaps.

 

The patch misses that the HCR.TSC routing of SMC to EL2 takes priority over
SMD for NS EL1. For AArch32, I need to check for non-secure state
aswell. I'll update those for v2.

Need to run a few tests but I'm considering something along these lines:

void HELPER(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 AArch32, SMD only applies to NS mode.  */
    bool udef = is_a64(env) ? smd : !secure && smd;

Is it possible to get in here at EL0?  A quick check, did not turn up a check for this in the code calling this helper, but I may have missed it.

 

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

    }

    /* We've already checked that EL3 exists at translation time.  */

    if (udef) {
        env->exception.syndrome = syn_uncategorized();
        raise_exception(env, EXCP_UDEF);
    }
    env->exception.syndrome = syndrome;
    raise_exception(env, EXCP_SMC);
}

Thanks for the comments,
Edgar

>
>
> > +        env->exception.syndrome = syn_uncategorized();
> > +        raise_exception(env, EXCP_UDEF);
> > +    }
> > +    env->exception.syndrome = syndrome;
> > +    raise_exception(env, EXCP_SMC);
> > +}
> > +
> >  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 3981ee1..a02fe06 100644
> > --- a/target-arm/translate-a64.c
> > +++ b/target-arm/translate-a64.c
> > @@ -1451,6 +1451,16 @@ static void disas_exc(DisasContext *s, uint32_t
> > insn)
> >              gen_helper_hvc(cpu_env, tmp);
> >              tcg_temp_free_i32(tmp);
> >              break;
> > +        case 3:
> > +            if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl ==
> > 0) {
> > +                unallocated_encoding(s);
> > +                break;
> > +            }
> > +            tmp = tcg_const_i32(syn_aa64_smc(imm16));
> > +            gen_a64_set_pc_im(s->pc);
> > +            gen_helper_smc(cpu_env, tmp);
> > +            tcg_temp_free_i32(tmp);
> > +            break;
> >          default:
> >              unallocated_encoding(s);
> >              break;
> > --
> > 1.8.3.2
> >
> >