Re: [Qemu-devel] [PATCH v3 13/16] target-arm: A64: Emulate the HVC insn

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Fri, Aug 01, 2014 at 03:21:08PM +0100, Peter Maydell wrote:
> On 17 June 2014 09:45, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.h           |  1 +
> >  target-arm/helper-a64.c    |  1 +
> >  target-arm/helper.c        | 28 +++++++++++++++++++++++++++-
> >  target-arm/helper.h        |  1 +
> >  target-arm/internals.h     |  6 ++++++
> >  target-arm/op_helper.c     | 33 +++++++++++++++++++++++++++++++++
> >  target-arm/translate-a64.c | 21 ++++++++++++++++-----
> >  7 files changed, 85 insertions(+), 6 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 4f273ac..258bc09 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -51,6 +51,7 @@
> >  #define EXCP_EXCEPTION_EXIT  8   /* Return from v7M exception.  */
> >  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
> >  #define EXCP_STREX          10
> > +#define EXCP_HVC            11   /* HyperVisor 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 cf8ce1e..7382d0a 100644
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -475,6 +475,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >      case EXCP_BKPT:
> >      case EXCP_UDEF:
> >      case EXCP_SWI:
> > +    case EXCP_HVC:
> >          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 972e91c..44e8d47 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3281,7 +3281,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;
> > +    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;
> > +    }
> > +
> > +    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;
> 
> I was wondering if this was going to get a bit heavyweight to
> call twice on each trip round the cpu-exec.c main loop, but
> we'll only do that in the case that an interrupt is pending
> but currently masked I guess so it should be OK.
> 
> >  }
> >
> >  static void v7m_push(CPUARMState *env, uint32_t val)
> > diff --git a/target-arm/helper.h b/target-arm/helper.h
> > index facfcd2..70cfd28 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_2(hvc, 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 08fa697..b68e6f9 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)
> > @@ -204,6 +205,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(uint16_t imm16)
> 
> This isn't consistent with the other syn_ functions which take a
> uint32_t imm16. (Didn't we do this before?)

OK, I thought we were not going to change the existing calls in this
series but the ones I add would not use the explicit masking. Some
future series would eventually change the others.

It's got no importance at all for this series so I'm happy to change it
allthough I personally prefer the uint16_t code.


> 
> > +{
> > +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | imm16;
> > +}
> > +
> >  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 25ad902..c1fa797 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -369,6 +369,39 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
> >      }
> >  }
> >
> > +void HELPER(hvc)(CPUARMState *env, uint32_t syndrome)
> > +{
> > +    int cur_el = arm_current_pl(env);
> > +    /* FIXME: Use actual secure state.  */
> > +    bool secure = false;
> > +    bool udef;
> > +
> > +    /* 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) {
> > +        env->exception.syndrome = syn_uncategorized();
> > +        raise_exception(env, EXCP_UDEF);
> > +    }
> > +    env->exception.syndrome = syndrome;
> > +    raise_exception(env, EXCP_HVC);
> > +}
> > +
> >  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 63ad787..5692dff 100644
> > --- a/target-arm/translate-a64.c
> > +++ b/target-arm/translate-a64.c
> > @@ -1436,17 +1436,28 @@ 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:
> > -        /* SVC, HVC, SMC; since we don't support the Virtualization
> > -         * or TrustZone extensions these all UNDEF except SVC.
> > -         */
> > -        if (op2_ll != 1) {
> > +        switch (op2_ll) {
> > +        case 1:
> > +            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;
> > +            }
> > +            tmp = tcg_const_i32(syn_aa64_hvc(imm16));
> > +            gen_a64_set_pc_im(s->pc);
> 
> (This set_pc_im is unnecessary.)
> 
> > +            gen_helper_hvc(cpu_env, tmp);
> 
> This means that exceptions due to HVC are going to be
> runtime-detected and cause us to go through and retranslate
> the TB to determine the guest PC. Maybe we should do:
> 
>     /* This helper will raise EXCP_UDEF if HVC is not permitted */
>     gen_helper_hvc_access_check(cpu_env);
>     /* Common case: HVC causes EXCP_HVC */
>     gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
> 
> Then you only get the overhead of retranslating the TB in the
> unexpected case where the guest does something dumb and
> executes an HVC that UNDEFs.

That doesn't match my understanding of what will happen with this kind
of exception raising. I think the set_pc_im matters and there won't
be any retranslation of TBs to figure out the guest PC.

Can you double check your thinking here or elaborate a little bit more
so we are on the same page?

Thanks,
Edgar



> 
> > +            tcg_temp_free_i32(tmp);
> > +            break;
> > +        default:
> >              unallocated_encoding(s);
> >              break;
> >          }
> > -        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> >          break;
> >      case 1:
> >          if (op2_ll != 0) {
> 
> General comment, this is likely to clash with the PSCI support; I
> guess we'll see which gets in first.
> 
> thanks
> -- PMM