| Message ID | 1410582564-27687-9-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 | 6 ++++++ > target-arm/helper.h | 1 + > target-arm/internals.h | 6 ++++++ > target-arm/op_helper.c | 26 ++++++++++++++++++++++++++ > target-arm/translate-a64.c | 13 +++++++++++++ > 7 files changed, 54 insertions(+) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index b553f3d..c24af40 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 4e6ca26..996dfea 100644 > --- a/target-arm/helper-a64.c > +++ b/target-arm/helper-a64.c > @@ -477,6 +477,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 504ff42..719c95d 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -3677,6 +3677,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 (!secure && cur_el == 1 && (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 75fc1b3..dec3728 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_1(pre_hvc, void, env) > +DEF_HELPER_2(pre_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 afcc4e0..e15ae57 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) > @@ -221,6 +222,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 8441c27..e6a0656 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -406,6 +406,32 @@ void HELPER(pre_hvc)(CPUARMState *env) > } > } > > +void HELPER(pre_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 ARMv8 AArch32, SMD only applies to NS mode. > + * On ARMv7 SMD only applies to NS mode and only if EL2 is available. > + * For ARMv7 non EL2, we force SMD to zero so we don't need to > re-check > + * the EL2 condition here. > + */ > + bool udef = is_a64(env) ? smd : !secure && smd; > + > + /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. */ > + if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { > + env->exception.syndrome = syndrome; > + raise_exception(env, EXCP_SMC); > + } > + > + /* We've already checked that EL3 exists at translation time. */ > + if (udef) { > + 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 8eaa85f..9a58de8 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -1470,6 +1470,7 @@ 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: > @@ -1495,6 +1496,18 @@ static void disas_exc(DisasContext *s, uint32_t > insn) > gen_ss_advance(s); > gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16)); > break; > + case 3: > + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl == > 0) { > + unallocated_encoding(s); > + break; > + } > + gen_a64_set_pc_im(s->pc - 4); > + tmp = tcg_const_i32(syn_aa64_smc(imm16)); > + gen_helper_pre_smc(cpu_env, tmp); > + tcg_temp_free_i32(tmp); > + gen_ss_advance(s); > + gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16)); > + break; > default: > unallocated_encoding(s); > break; > -- > 1.9.1 > >
On 13 September 2014 05: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 | 6 ++++++ > target-arm/helper.h | 1 + > target-arm/internals.h | 6 ++++++ > target-arm/op_helper.c | 26 ++++++++++++++++++++++++++ > target-arm/translate-a64.c | 13 +++++++++++++ > 7 files changed, 54 insertions(+) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index b553f3d..c24af40 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 4e6ca26..996dfea 100644 > --- a/target-arm/helper-a64.c > +++ b/target-arm/helper-a64.c > @@ -477,6 +477,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 504ff42..719c95d 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -3677,6 +3677,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 (!secure && cur_el == 1 && (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 75fc1b3..dec3728 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_1(pre_hvc, void, env) > +DEF_HELPER_2(pre_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 afcc4e0..e15ae57 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) > @@ -221,6 +222,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 8441c27..e6a0656 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -406,6 +406,32 @@ void HELPER(pre_hvc)(CPUARMState *env) > } > } > > +void HELPER(pre_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 ARMv8 AArch32, SMD only applies to NS mode. > + * On ARMv7 SMD only applies to NS mode and only if EL2 is available. > + * For ARMv7 non EL2, we force SMD to zero so we don't need to re-check > + * the EL2 condition here. > + */ NS isn't a mode, it's a state. (S vs NS state is orthogonal to exception levels and mostly orthogonal to AArch32 modes.) > + bool udef = is_a64(env) ? smd : !secure && smd; What precedence did you intend here between the ?: and the && operators? More brackets would be nice... > + /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. */ > + if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { > + env->exception.syndrome = syndrome; > + raise_exception(env, EXCP_SMC); Shouldn't this just be returning so that the generated code immediately following can raise the SMC exception with the correct syndrome, PC and singlestep state? (would also save you passing in the syndrome argument to this fn). > + } > + > + /* We've already checked that EL3 exists at translation time. */ > + if (udef) { > + 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 8eaa85f..9a58de8 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -1470,6 +1470,7 @@ 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: > @@ -1495,6 +1496,18 @@ static void disas_exc(DisasContext *s, uint32_t insn) > gen_ss_advance(s); > gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16)); > break; > + case 3: > + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl == 0) { > + unallocated_encoding(s); > + break; > + } > + gen_a64_set_pc_im(s->pc - 4); > + tmp = tcg_const_i32(syn_aa64_smc(imm16)); > + gen_helper_pre_smc(cpu_env, tmp); > + tcg_temp_free_i32(tmp); > + gen_ss_advance(s); > + gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16)); > + break; > default: > unallocated_encoding(s); > break; > -- > 1.9.1 thanks -- PMM
On Thu, Sep 25, 2014 at 07:47:16PM +0100, Peter Maydell wrote: > On 13 September 2014 05: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 | 6 ++++++ > > target-arm/helper.h | 1 + > > target-arm/internals.h | 6 ++++++ > > target-arm/op_helper.c | 26 ++++++++++++++++++++++++++ > > target-arm/translate-a64.c | 13 +++++++++++++ > > 7 files changed, 54 insertions(+) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index b553f3d..c24af40 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 4e6ca26..996dfea 100644 > > --- a/target-arm/helper-a64.c > > +++ b/target-arm/helper-a64.c > > @@ -477,6 +477,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 504ff42..719c95d 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -3677,6 +3677,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 (!secure && cur_el == 1 && (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 75fc1b3..dec3728 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_1(pre_hvc, void, env) > > +DEF_HELPER_2(pre_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 afcc4e0..e15ae57 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) > > @@ -221,6 +222,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 8441c27..e6a0656 100644 > > --- a/target-arm/op_helper.c > > +++ b/target-arm/op_helper.c > > @@ -406,6 +406,32 @@ void HELPER(pre_hvc)(CPUARMState *env) > > } > > } > > > > +void HELPER(pre_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 ARMv8 AArch32, SMD only applies to NS mode. > > + * On ARMv7 SMD only applies to NS mode and only if EL2 is available. > > + * For ARMv7 non EL2, we force SMD to zero so we don't need to re-check > > + * the EL2 condition here. > > + */ > > NS isn't a mode, it's a state. (S vs NS state is orthogonal > to exception levels and mostly orthogonal to AArch32 modes.) > > > + bool udef = is_a64(env) ? smd : !secure && smd; > > What precedence did you intend here between the ?: and > the && operators? More brackets would be nice... Will change it to: is_a64(env) ? smd : (!secure && smd); > > > + /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. */ > > + if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { > > + env->exception.syndrome = syndrome; > > + raise_exception(env, EXCP_SMC); > > Shouldn't this just be returning so that the generated > code immediately following can raise the SMC exception > with the correct syndrome, PC and singlestep state? > (would also save you passing in the syndrome argument > to this fn). When routing SMCs to EL2, the exception happens before advancing the PC. It's similar to the undef cases for HVC (and SMC). > > > + } > > + > > + /* We've already checked that EL3 exists at translation time. */ > > + if (udef) { > > + 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 8eaa85f..9a58de8 100644 > > --- a/target-arm/translate-a64.c > > +++ b/target-arm/translate-a64.c > > @@ -1470,6 +1470,7 @@ 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: > > @@ -1495,6 +1496,18 @@ static void disas_exc(DisasContext *s, uint32_t insn) > > gen_ss_advance(s); > > gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16)); > > break; > > + case 3: > > + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl == 0) { > > + unallocated_encoding(s); > > + break; > > + } > > + gen_a64_set_pc_im(s->pc - 4); > > + tmp = tcg_const_i32(syn_aa64_smc(imm16)); > > + gen_helper_pre_smc(cpu_env, tmp); > > + tcg_temp_free_i32(tmp); > > + gen_ss_advance(s); > > + gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16)); > > + break; > > default: > > unallocated_encoding(s); > > break; > > -- > > 1.9.1 > > thanks > -- PMM
On 25 September 2014 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Thu, Sep 25, 2014 at 07:47:16PM +0100, Peter Maydell wrote: >> > + /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. */ >> > + if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { >> > + env->exception.syndrome = syndrome; >> > + raise_exception(env, EXCP_SMC); >> >> Shouldn't this just be returning so that the generated >> code immediately following can raise the SMC exception >> with the correct syndrome, PC and singlestep state? >> (would also save you passing in the syndrome argument >> to this fn). > > When routing SMCs to EL2, the exception happens before advancing the > PC. It's similar to the undef cases for HVC (and SMC). Oh, yes, that's the trap enable bit. In that case we shouldn't be using EXCP_SMC: this isn't routing the SMC exception, it's taking a Hyp trap exception, and in AArch32 the vector entry point is different. (Granted, you can't get to AArch32 by taking an exception from AArch64, but we should use the right EXCP_ value to avoid the code looking gratuitously different for the two cases.) thanks -- PMM
On Fri, Sep 26, 2014 at 12:17:59AM +0100, Peter Maydell wrote: > On 25 September 2014 23:55, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Thu, Sep 25, 2014 at 07:47:16PM +0100, Peter Maydell wrote: > >> > + /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. */ > >> > + if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { > >> > + env->exception.syndrome = syndrome; > >> > + raise_exception(env, EXCP_SMC); > >> > >> Shouldn't this just be returning so that the generated > >> code immediately following can raise the SMC exception > >> with the correct syndrome, PC and singlestep state? > >> (would also save you passing in the syndrome argument > >> to this fn). > > > > When routing SMCs to EL2, the exception happens before advancing the > > PC. It's similar to the undef cases for HVC (and SMC). > > Oh, yes, that's the trap enable bit. In that case we shouldn't > be using EXCP_SMC: this isn't routing the SMC exception, it's > taking a Hyp trap exception, and in AArch32 the vector > entry point is different. (Granted, you can't get to AArch32 > by taking an exception from AArch64, but we should use the > right EXCP_ value to avoid the code looking gratuitously > different for the two cases.) I see. I hadn't thought much about the AArch32 case here. For AArch64, the pseudo code referes to this as route_to_el2. Anyway, your comment makes sense to avoid diff between a32/a64 and I think it actually makes the AArch64 code a bit cleaner aswell. I'll add EXCP_HYP_TRAP. Thanks, Edgar
On 26 September 2014 00:31, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Fri, Sep 26, 2014 at 12:17:59AM +0100, Peter Maydell wrote: >> Oh, yes, that's the trap enable bit. In that case we shouldn't >> be using EXCP_SMC: this isn't routing the SMC exception, it's >> taking a Hyp trap exception, and in AArch32 the vector >> entry point is different. (Granted, you can't get to AArch32 >> by taking an exception from AArch64, but we should use the >> right EXCP_ value to avoid the code looking gratuitously >> different for the two cases.) > > I see. I hadn't thought much about the AArch32 case here. For > AArch64, the pseudo code referes to this as route_to_el2. Mmm, but the pseudocode keeps AArch64 and AArch32 exception paths a lot more separate than we do, and so it doesn't need any information about the AArch32 exception when it's dealing with a from-AArch64 exception. Our implementation routes both paths in common, and so it's slightly clearer to always retain the correct info for both cases (if nothing else, it's slightly more accurate when we print out debug log about what exceptions we're taking). > Anyway, your comment makes sense to avoid diff between a32/a64 > and I think it actually makes the AArch64 code a bit cleaner > aswell. > > I'll add EXCP_HYP_TRAP. Thanks. -- PMM
On Fri, Sep 26, 2014 at 12:43:40AM +0100, Peter Maydell wrote: > On 26 September 2014 00:31, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Fri, Sep 26, 2014 at 12:17:59AM +0100, Peter Maydell wrote: > >> Oh, yes, that's the trap enable bit. In that case we shouldn't > >> be using EXCP_SMC: this isn't routing the SMC exception, it's > >> taking a Hyp trap exception, and in AArch32 the vector > >> entry point is different. (Granted, you can't get to AArch32 > >> by taking an exception from AArch64, but we should use the > >> right EXCP_ value to avoid the code looking gratuitously > >> different for the two cases.) > > > > I see. I hadn't thought much about the AArch32 case here. For > > AArch64, the pseudo code referes to this as route_to_el2. > > Mmm, but the pseudocode keeps AArch64 and AArch32 exception > paths a lot more separate than we do, and so it doesn't need > any information about the AArch32 exception when it's dealing > with a from-AArch64 exception. Our implementation routes > both paths in common, and so it's slightly clearer to always > retain the correct info for both cases (if nothing else, it's > slightly more accurate when we print out debug log about > what exceptions we're taking). Agreed, your suggestion makes sense and will save some pain for the A32 case. Good catch, thanks. > > > Anyway, your comment makes sense to avoid diff between a32/a64 > > and I think it actually makes the AArch64 code a bit cleaner > > aswell. > > > > I'll add EXCP_HYP_TRAP. > > Thanks. > > -- PMM
On Fri, Sep 26, 2014 at 12:43:40AM +0100, Peter Maydell wrote: > On 26 September 2014 00:31, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Fri, Sep 26, 2014 at 12:17:59AM +0100, Peter Maydell wrote: > >> Oh, yes, that's the trap enable bit. In that case we shouldn't > >> be using EXCP_SMC: this isn't routing the SMC exception, it's > >> taking a Hyp trap exception, and in AArch32 the vector > >> entry point is different. (Granted, you can't get to AArch32 > >> by taking an exception from AArch64, but we should use the > >> right EXCP_ value to avoid the code looking gratuitously > >> different for the two cases.) > > > > I see. I hadn't thought much about the AArch32 case here. For > > AArch64, the pseudo code referes to this as route_to_el2. > > Mmm, but the pseudocode keeps AArch64 and AArch32 exception > paths a lot more separate than we do, and so it doesn't need > any information about the AArch32 exception when it's dealing > with a from-AArch64 exception. Our implementation routes > both paths in common, and so it's slightly clearer to always > retain the correct info for both cases (if nothing else, it's > slightly more accurate when we print out debug log about > what exceptions we're taking). > > > Anyway, your comment makes sense to avoid diff between a32/a64 > > and I think it actually makes the AArch64 code a bit cleaner > > aswell. > > > > I'll add EXCP_HYP_TRAP. > > Thanks. Hi, I've just sent a v7. I think I've addressed your comments. I'll see if I get some HCR.TGE test-cases going and send follow-up patches on that later. Cheers, Edgar
diff --git a/target-arm/cpu.h b/target-arm/cpu.h index b553f3d..c24af40 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 4e6ca26..996dfea 100644 --- a/target-arm/helper-a64.c +++ b/target-arm/helper-a64.c @@ -477,6 +477,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 504ff42..719c95d 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -3677,6 +3677,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 (!secure && cur_el == 1 && (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 75fc1b3..dec3728 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_1(pre_hvc, void, env) +DEF_HELPER_2(pre_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 afcc4e0..e15ae57 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) @@ -221,6 +222,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 8441c27..e6a0656 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -406,6 +406,32 @@ void HELPER(pre_hvc)(CPUARMState *env) } } +void HELPER(pre_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 ARMv8 AArch32, SMD only applies to NS mode. + * On ARMv7 SMD only applies to NS mode and only if EL2 is available. + * For ARMv7 non EL2, we force SMD to zero so we don't need to re-check + * the EL2 condition here. + */ + bool udef = is_a64(env) ? smd : !secure && smd; + + /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. */ + if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) { + env->exception.syndrome = syndrome; + raise_exception(env, EXCP_SMC); + } + + /* We've already checked that EL3 exists at translation time. */ + if (udef) { + 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 8eaa85f..9a58de8 100644 --- a/target-arm/translate-a64.c +++ b/target-arm/translate-a64.c @@ -1470,6 +1470,7 @@ 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: @@ -1495,6 +1496,18 @@ static void disas_exc(DisasContext *s, uint32_t insn) gen_ss_advance(s); gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16)); break; + case 3: + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || s->current_pl == 0) { + unallocated_encoding(s); + break; + } + gen_a64_set_pc_im(s->pc - 4); + tmp = tcg_const_i32(syn_aa64_smc(imm16)); + gen_helper_pre_smc(cpu_env, tmp); + tcg_temp_free_i32(tmp); + gen_ss_advance(s); + gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16)); + break; default: unallocated_encoding(s); break;