| Message ID | 1410582564-27687-8-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
Reviewed-by: Greg Bellows <greg.bellows@linaro.org> On 12 September 2014 21:29, 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 | 32 ++++++++++++++++++++++++++++++++ > target-arm/translate-a64.c | 30 +++++++++++++++++++++--------- > 7 files changed, 89 insertions(+), 10 deletions(-) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 7f8a410..b553f3d 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 c6ef8e9..4e6ca26 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_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 4431fbb..504ff42 100644 > --- 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; > + 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 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; > + > + /* 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. */ > + 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. */ > + 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 > >
On 13 September 2014 05:29, Edgar E. Iglesias <edgar.iglesias@gmail.com> 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. > + 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. > + 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
On Thu, Sep 25, 2014 at 07:39:32PM +0100, Peter Maydell wrote: > On 13 September 2014 05:29, Edgar E. Iglesias <edgar.iglesias@gmail.com> 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
On 25 September 2014 23:20, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Thu, Sep 25, 2014 at 07:39:32PM +0100, Peter Maydell wrote: >> 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. I'd rather we just implemented it properly... -- PMM
On Fri, Sep 26, 2014 at 12:01:11AM +0100, Peter Maydell wrote: > On 25 September 2014 23:20, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Thu, Sep 25, 2014 at 07:39:32PM +0100, Peter Maydell wrote: > >> 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. > > I'd rather we just implemented it properly... IMO, It's not about implementing it properly. It's about implementing the EL3/2 support incrementally. The spec is too big to add all the features at once. Cheers, Edgar
On 26 September 2014 00:06, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Fri, Sep 26, 2014 at 12:01:11AM +0100, Peter Maydell wrote: >> On 25 September 2014 23:20, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: >> > On Thu, Sep 25, 2014 at 07:39:32PM +0100, Peter Maydell wrote: >> >> 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. >> >> I'd rather we just implemented it properly... > > IMO, It's not about implementing it properly. It's about implementing the > EL3/2 support incrementally. The spec is too big to add all the features > at once. Well, maybe. My point still stands that the code you have there to figure out route_to_el2 is not going to be implementable as a generic code fragment that doesn't care about the exception type, and so you should stick to having the code inside each switch case. -- PMM
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 7f8a410..b553f3d 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 c6ef8e9..4e6ca26 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_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 4431fbb..504ff42 100644 --- 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; + 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 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; + + /* 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. */ + 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. */ + 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) {