diff mbox

[v3,13/16] target-arm: A64: Emulate the HVC insn

Message ID 1402994746-8328-14-git-send-email-edgar.iglesias@gmail.com
State New
Headers show

Commit Message

Edgar E. Iglesias June 17, 2014, 8:45 a.m. UTC
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(-)

Comments

Peter Maydell Aug. 1, 2014, 2:21 p.m. UTC | #1
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?)

> +{
> +    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.

> +            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
Edgar E. Iglesias Aug. 4, 2014, 4:12 a.m. UTC | #2
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
Peter Maydell Aug. 4, 2014, 2:24 p.m. UTC | #3
On 4 August 2014 05:12, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 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:
>> > +        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.

Sorry, yes; you're right and I was wrong -- we only retranslate
where we call cpu_restore_state(), which is done only where
tlb_fill() is going to raise an exception.

(I think I need to think a bit about how I'm currently implementing
architectural debug singlestep, since at the moment I assume
that you can tell at translate time whether something is going
to be a valid SMC/HVC/SVC or not, and so whether or not to
advance the singlestep state machine. Maybe I can defer that
to exception entry...)

thanks
-- PMM
Edgar E. Iglesias Aug. 4, 2014, 3:15 p.m. UTC | #4
On Mon, Aug 04, 2014 at 03:24:42PM +0100, Peter Maydell wrote:
> On 4 August 2014 05:12, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > 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:
> >> > +        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.
> 
> Sorry, yes; you're right and I was wrong -- we only retranslate
> where we call cpu_restore_state(), which is done only where
> tlb_fill() is going to raise an exception.
> 
> (I think I need to think a bit about how I'm currently implementing
> architectural debug singlestep, since at the moment I assume
> that you can tell at translate time whether something is going
> to be a valid SMC/HVC/SVC or not, and so whether or not to
> advance the singlestep state machine. Maybe I can defer that
> to exception entry...)

Aha, I see. THere is actually another twist to this code that I
found while testing more. The UDEFs and SMC route to EL2 case should
raise the exception on the SMC/HVC itself, while the success case should
raise it with ELR pointing ahead of the SMC/HVC insn.

My first thought was to fixup the PC in the helper but a split helper
approach might be OK aswell if it helps your debug implementation.

I'll look more at it tomorrow.

Thanks,
Edgar

> 
> thanks
> -- PMM
diff mbox

Patch

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;
 }
 
 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)
+{
+    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);
+            gen_helper_hvc(cpu_env, tmp);
+            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) {