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

[Edgar E. Iglesias]

On Thu, Sep 25, 2014 at 07:39:32PM +0100, Peter Maydell wrote:
> On 13 September 2014 05:29, Edgar E. Iglesias <address@hidden> wrote:
> 
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3652,7 +3652,33 @@ void switch_mode(CPUARMState *env, int mode)
> >   */
> >  unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
> >  {
> > -    return 1;
> > +    CPUARMState *env = cs->env_ptr;
> 
> Everywhere else that we start with a CPUState* and need
> a CPUARMState* we do it with:
> 
>     ARMCPU *cpu = ARM_CPU(cs);
>     CPUARMState *env = &cpu->env;
> 
> I think we should be consistent.
> 
> > +    unsigned int cur_el = arm_current_pl(env);
> > +    unsigned int target_el = 1;
> > +    bool route_to_el2 = false;
> > +    /* FIXME: Use actual secure state.  */
> > +    bool secure = false;
> > +
> > +    if (!env->aarch64) {
> > +        /* TODO: Add EL2 and 3 exception handling for AArch32.  */
> > +        return 1;
> > +    }
> > +
> > +    if (!secure
> > +        && arm_feature(env, ARM_FEATURE_EL2)
> > +        && cur_el < 2
> > +        && (env->cp15.hcr_el2 & HCR_TGE)) {
> > +        route_to_el2 = true;
> > +    }
> 
> HCR.TGE isn't actually relevant for some exceptions
> (eg SMC), and the HVC handling you have below
> effectively ends up ignoring the route_to_el2
> information. That suggests to me that you should put
> this code in a default: case in the switch below.

I don't really test nor support TGE so I'll just drop the TGE part.

> 
> > +    target_el = MAX(cur_el, route_to_el2 ? 2 : 1);
> > +
> > +    switch (excp_idx) {
> > +    case EXCP_HVC:
> > +        target_el = MAX(target_el, 2);
> > +        break;
> > +    }
> > +    return target_el;
> >  }
> >
> >  static void v7m_push(CPUARMState *env, uint32_t val)
> > diff --git a/target-arm/helper.h b/target-arm/helper.h
> > index 1d7003b..75fc1b3 100644
> > --- a/target-arm/helper.h
> > +++ b/target-arm/helper.h
> > @@ -50,6 +50,7 @@ DEF_HELPER_2(exception_internal, void, env, i32)
> >  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_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 64751a0..afcc4e0 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -53,6 +53,7 @@ static const char * const excnames[] = {
> >      [EXCP_EXCEPTION_EXIT] = "QEMU v7M exception exit",
> >      [EXCP_KERNEL_TRAP] = "QEMU intercept of kernel commpage",
> >      [EXCP_STREX] = "QEMU intercept of STREX",
> > +    [EXCP_HVC] = "Hypervisor Call",
> >  };
> >
> >  static inline void arm_log_exception(int idx)
> > @@ -215,6 +216,11 @@ static inline uint32_t syn_aa64_svc(uint32_t imm16)
> >      return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> >  }
> >
> > +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_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 b956216..8441c27 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -374,6 +374,38 @@ void HELPER(clear_pstate_ss)(CPUARMState *env)
> >      env->pstate &= ~PSTATE_SS;
> >  }
> >
> > +void HELPER(pre_hvc)(CPUARMState *env)
> > +{
> > +    int cur_el = arm_current_pl(env);
> > +    /* FIXME: Use actual secure state.  */
> > +    bool secure = false;
> > +    bool udef;
> 
> The standard spelling is "UNDEF" (except in the constant name
> "EXCP_UDEF", which we should probably change, though I won't
> actually do that now since it will cause a bunch of pointless
> conflicts for you. Another tempting rename is EXCP_SWI and
> DISAS_SWI to EXCP_SVC and DISAS_SVC, though in that case the
> old names have the virtue of having been right once.)
> 
> > +
> > +    /* We've already checked that EL2 exists at translation time.
> > +     * EL3.HCE has priority over EL2.HCD.
> > +     */
> > +    if (arm_feature(env, ARM_FEATURE_EL3)) {
> > +        udef = !(env->cp15.scr_el3 & SCR_HCE);
> > +    } else {
> > +        udef = env->cp15.hcr_el2 & HCR_HCD;
> > +    }
> > +
> > +    /* In ARMv7 and ARMv8/AArch32, HVC is udef in secure state.
> > +     * For ARMv8/AArch64, HVC is allowed in EL3.
> > +     * Note that we've already trapped HVC from EL0 at translation
> > +     * time.
> > +     */
> > +    if (secure && (!is_a64(env) || cur_el == 1)) {
> > +        udef = true;
> > +    }
> > +
> > +    if (udef) {
> > +        /* UDEFs trap on the HVC, roll back to the PC to the HVC insn.  */
> 
> "with the PC pointing to the HVC insn" ?  At any rate, nothing
> in this function is rolling anything back.

This is a hang-over from v5 prior to the singlestepping support
when the code was rolling back the pc.

HVCs that lead to undef are taken with the prefered exception
return pointing to the HVC, i.e before advancing the PC.

> 
> > +        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 8e66b6c..8eaa85f 100644
> > --- a/target-arm/translate-a64.c
> > +++ b/target-arm/translate-a64.c
> > @@ -1473,20 +1473,32 @@ static void disas_exc(DisasContext *s, uint32_t 
> > insn)
> >
> >      switch (opc) {
> >      case 0:
> > -        /* SVC, HVC, SMC; since we don't support the Virtualization
> > -         * or TrustZone extensions these all UNDEF except SVC.
> > -         */
> > -        if (op2_ll != 1) {
> > -            unallocated_encoding(s);
> > -            break;
> > -        }
> >          /* For SVC, HVC and SMC we advance the single-step state
> >           * machine before taking the exception. This is architecturally
> >           * mandated, to ensure that single-stepping a system call
> >           * instruction works properly.
> >           */
> > -        gen_ss_advance(s);
> > -        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> > +        switch (op2_ll) {
> > +        case 1:
> > +            gen_ss_advance(s);
> > +            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> > +            break;
> > +        case 2:
> > +            if (!arm_dc_feature(s, ARM_FEATURE_EL2) || s->current_pl == 0) 
> > {
> > +                unallocated_encoding(s);
> > +                break;
> > +            }
> > +            /* The pre HVC helper handles cases when HVC gets trapped
> > +             * as an undefined insn by runtime configuration.  */
> 
> "*/" on a line of its own, please.
> 
> > +            gen_a64_set_pc_im(s->pc - 4);
> > +            gen_helper_pre_hvc(cpu_env);
> > +            gen_ss_advance(s);
> > +            gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
> > +            break;
> > +        default:
> > +            unallocated_encoding(s);
> > +            break;
> > +        }
> >          break;
> >      case 1:
> >          if (op2_ll != 0) {
> > --
> > 1.9.1
> >
> 
> thanks
> -- PMM