| Message ID | 1412957023-11105-7-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 10 October 2014 18:03, Greg Bellows <greg.bellows@linaro.org> wrote: > From: Fabian Aggeler <aggelerf@ethz.ch> > > Implements SMC instruction in AArch32 using the A32 syndrome. When executing > SMC instruction from monitor CPU mode SCR.NS bit is reset. > > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > ========== > > v5 -> v6 > - Fixed PC offsetting for presmc > - Removed extraneous semi-colon > - Fixed merge issue > > v4 -> v5 > - Merge pre_smc upstream changes and incorporated ss_advance > --- > target-arm/helper.c | 11 +++++++++++ > target-arm/internals.h | 5 +++++ > target-arm/op_helper.c | 3 +-- > target-arm/translate.c | 39 +++++++++++++++++++++++++++++---------- > 4 files changed, 46 insertions(+), 12 deletions(-) > > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 2381e6f..7f3f049 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -4090,6 +4090,12 @@ void arm_cpu_do_interrupt(CPUState *cs) > mask = CPSR_A | CPSR_I | CPSR_F; > offset = 4; > break; > + case EXCP_SMC: > + new_mode = ARM_CPU_MODE_MON; > + addr = 0x08; > + mask = CPSR_A | CPSR_I | CPSR_F; > + offset = 0; > + break; > default: > cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > return; /* Never happens. Keep compiler happy. */ > @@ -4108,6 +4114,11 @@ void arm_cpu_do_interrupt(CPUState *cs) > */ > addr += env->cp15.vbar_el[1]; > } > + > + if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) { > + env->cp15.scr_el3 &= ~SCR_NS; > + } > + > switch_mode (env, new_mode); > /* For exceptions taken to AArch32 we must clear the SS bit in both > * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it now. > diff --git a/target-arm/internals.h b/target-arm/internals.h > index fd69a83..544fb42 100644 > --- a/target-arm/internals.h > +++ b/target-arm/internals.h > @@ -236,6 +236,11 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb) > | (is_thumb ? 0 : ARM_EL_IL); > } > > +static inline uint32_t syn_aa32_smc(void) > +{ > + return (EC_AA32_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL; > +} > + > static inline uint32_t syn_aa64_bkpt(uint32_t imm16) > { > return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > index 0809d63..9e38f26 100644 > --- a/target-arm/op_helper.c > +++ b/target-arm/op_helper.c > @@ -419,8 +419,7 @@ void HELPER(pre_hvc)(CPUARMState *env) > void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome) > { > int cur_el = arm_current_el(env); > - /* FIXME: Use real secure state. */ > - bool secure = false; > + bool secure = arm_is_secure(env); > bool smd = env->cp15.scr_el3 & SCR_SMD; > /* On ARMv8 AArch32, SMD only applies to NS state. > * On ARMv7 SMD only applies to NS state and only if EL2 is available. > diff --git a/target-arm/translate.c b/target-arm/translate.c > index 617e6a9..60655e1 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -7872,15 +7872,27 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s) > case 7: > { > int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4); > - /* SMC instruction (op1 == 3) > - and undefined instructions (op1 == 0 || op1 == 2) > - will trap */ > - if (op1 != 1) { > + if (op1 == 1) { > + /* bkpt */ > + ARCH(5); > + gen_exception_insn(s, 4, EXCP_BKPT, > + syn_aa32_bkpt(imm16, false)); > + } else if (op1 == 3) { > + /* smi/smc */ > + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || > + s->current_el == 0) { > + goto illegal_op; > + } > + gen_set_pc_im(s, s->pc - 4); > + tmp = tcg_const_i32(syn_aa32_smc()); > + gen_helper_pre_smc(cpu_env, tmp); > + tcg_temp_free_i32(tmp); > + gen_ss_advance(s); > + gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc()); > + break; This is wrong; you should be basing this series on top of my PSCI series which will get you a correct implementation of the translate.c changes for SMC for A32/T32. (You'll still need the do_interrupt changes.) thanks -- PMM
I realize that, but I don't believe your changes were available yet and still sounded to be a bit in flux, so I was waiting to merge. As I mentioned previously, I had already merged on top of your initial changes. I'll recommit with your changes. Greg On 13 October 2014 08:06, Peter Maydell <peter.maydell@linaro.org> wrote: > On 10 October 2014 18:03, Greg Bellows <greg.bellows@linaro.org> wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > Implements SMC instruction in AArch32 using the A32 syndrome. When > executing > > SMC instruction from monitor CPU mode SCR.NS bit is reset. > > > > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > ========== > > > > v5 -> v6 > > - Fixed PC offsetting for presmc > > - Removed extraneous semi-colon > > - Fixed merge issue > > > > v4 -> v5 > > - Merge pre_smc upstream changes and incorporated ss_advance > > --- > > target-arm/helper.c | 11 +++++++++++ > > target-arm/internals.h | 5 +++++ > > target-arm/op_helper.c | 3 +-- > > target-arm/translate.c | 39 +++++++++++++++++++++++++++++---------- > > 4 files changed, 46 insertions(+), 12 deletions(-) > > > > diff --git a/target-arm/helper.c b/target-arm/helper.c > > index 2381e6f..7f3f049 100644 > > --- a/target-arm/helper.c > > +++ b/target-arm/helper.c > > @@ -4090,6 +4090,12 @@ void arm_cpu_do_interrupt(CPUState *cs) > > mask = CPSR_A | CPSR_I | CPSR_F; > > offset = 4; > > break; > > + case EXCP_SMC: > > + new_mode = ARM_CPU_MODE_MON; > > + addr = 0x08; > > + mask = CPSR_A | CPSR_I | CPSR_F; > > + offset = 0; > > + break; > > default: > > cpu_abort(cs, "Unhandled exception 0x%x\n", > cs->exception_index); > > return; /* Never happens. Keep compiler happy. */ > > @@ -4108,6 +4114,11 @@ void arm_cpu_do_interrupt(CPUState *cs) > > */ > > addr += env->cp15.vbar_el[1]; > > } > > + > > + if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) { > > + env->cp15.scr_el3 &= ~SCR_NS; > > + } > > + > > switch_mode (env, new_mode); > > /* For exceptions taken to AArch32 we must clear the SS bit in both > > * PSTATE and in the old-state value we save to SPSR_<mode>, so > zero it now. > > diff --git a/target-arm/internals.h b/target-arm/internals.h > > index fd69a83..544fb42 100644 > > --- a/target-arm/internals.h > > +++ b/target-arm/internals.h > > @@ -236,6 +236,11 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, > bool is_thumb) > > | (is_thumb ? 0 : ARM_EL_IL); > > } > > > > +static inline uint32_t syn_aa32_smc(void) > > +{ > > + return (EC_AA32_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL; > > +} > > + > > static inline uint32_t syn_aa64_bkpt(uint32_t imm16) > > { > > return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & > 0xffff); > > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c > > index 0809d63..9e38f26 100644 > > --- a/target-arm/op_helper.c > > +++ b/target-arm/op_helper.c > > @@ -419,8 +419,7 @@ void HELPER(pre_hvc)(CPUARMState *env) > > void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome) > > { > > int cur_el = arm_current_el(env); > > - /* FIXME: Use real secure state. */ > > - bool secure = false; > > + bool secure = arm_is_secure(env); > > bool smd = env->cp15.scr_el3 & SCR_SMD; > > /* On ARMv8 AArch32, SMD only applies to NS state. > > * On ARMv7 SMD only applies to NS state and only if EL2 is > available. > > diff --git a/target-arm/translate.c b/target-arm/translate.c > > index 617e6a9..60655e1 100644 > > --- a/target-arm/translate.c > > +++ b/target-arm/translate.c > > @@ -7872,15 +7872,27 @@ static void disas_arm_insn(CPUARMState * env, > DisasContext *s) > > case 7: > > { > > int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) > << 4); > > - /* SMC instruction (op1 == 3) > > - and undefined instructions (op1 == 0 || op1 == 2) > > - will trap */ > > - if (op1 != 1) { > > + if (op1 == 1) { > > + /* bkpt */ > > + ARCH(5); > > + gen_exception_insn(s, 4, EXCP_BKPT, > > + syn_aa32_bkpt(imm16, false)); > > + } else if (op1 == 3) { > > + /* smi/smc */ > > + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || > > + s->current_el == 0) { > > + goto illegal_op; > > + } > > + gen_set_pc_im(s, s->pc - 4); > > + tmp = tcg_const_i32(syn_aa32_smc()); > > + gen_helper_pre_smc(cpu_env, tmp); > > + tcg_temp_free_i32(tmp); > > + gen_ss_advance(s); > > + gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc()); > > + break; > > This is wrong; you should be basing this series on top of my PSCI > series which will get you a correct implementation of the translate.c > changes for SMC for A32/T32. (You'll still need the do_interrupt > changes.) > > thanks > -- PMM >
On 13 October 2014 15:13, Greg Bellows <greg.bellows@linaro.org> wrote: > I realize that, but I don't believe your changes were available yet and > still sounded to be a bit in flux, so I was waiting to merge. > > As I mentioned previously, I had already merged on top of your initial > changes. Ah. I thought when you said you'd merged your series on top of mine that you'd merged it on top of the relevant patches... > I'll recommit with your changes. Thanks. I'm going to finish reviewing the rest of this series, since the later patches are basically independent of this. I may be a day or two though as the next few require deeper thought than the minor nits in the first half dozen patches. So you don't need to push out a v7 just yet. -- PMM
I reapplied my changes on top of your v5 with the latest backing. It basically scraps most of my changes on this patch for yours, except for some slight updates here and there. I'll continue to make any v7 updates on your v5 set. Greg On 13 October 2014 08:36, Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 October 2014 15:13, Greg Bellows <greg.bellows@linaro.org> wrote: > > I realize that, but I don't believe your changes were available yet and > > still sounded to be a bit in flux, so I was waiting to merge. > > > > As I mentioned previously, I had already merged on top of your initial > > changes. > > Ah. I thought when you said you'd merged your series on top of mine > that you'd merged it on top of the relevant patches... > > > > I'll recommit with your changes. > > Thanks. > > I'm going to finish reviewing the rest of this series, since > the later patches are basically independent of this. I may > be a day or two though as the next few require deeper thought > than the minor nits in the first half dozen patches. So you > don't need to push out a v7 just yet. > > -- PMM >
========== v5 -> v6 - Fixed PC offsetting for presmc - Removed extraneous semi-colon - Fixed merge issue v4 -> v5 - Merge pre_smc upstream changes and incorporated ss_advance --- target-arm/helper.c | 11 +++++++++++ target-arm/internals.h | 5 +++++ target-arm/op_helper.c | 3 +-- target-arm/translate.c | 39 +++++++++++++++++++++++++++++---------- 4 files changed, 46 insertions(+), 12 deletions(-) diff --git a/target-arm/helper.c b/target-arm/helper.c index 2381e6f..7f3f049 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -4090,6 +4090,12 @@ void arm_cpu_do_interrupt(CPUState *cs) mask = CPSR_A | CPSR_I | CPSR_F; offset = 4; break; + case EXCP_SMC: + new_mode = ARM_CPU_MODE_MON; + addr = 0x08; + mask = CPSR_A | CPSR_I | CPSR_F; + offset = 0; + break; default: cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); return; /* Never happens. Keep compiler happy. */ @@ -4108,6 +4114,11 @@ void arm_cpu_do_interrupt(CPUState *cs) */ addr += env->cp15.vbar_el[1]; } + + if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_MON) { + env->cp15.scr_el3 &= ~SCR_NS; + } + switch_mode (env, new_mode); /* For exceptions taken to AArch32 we must clear the SS bit in both * PSTATE and in the old-state value we save to SPSR_<mode>, so zero it now. diff --git a/target-arm/internals.h b/target-arm/internals.h index fd69a83..544fb42 100644 --- a/target-arm/internals.h +++ b/target-arm/internals.h @@ -236,6 +236,11 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb) | (is_thumb ? 0 : ARM_EL_IL); } +static inline uint32_t syn_aa32_smc(void) +{ + return (EC_AA32_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL; +} + static inline uint32_t syn_aa64_bkpt(uint32_t imm16) { return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index 0809d63..9e38f26 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -419,8 +419,7 @@ void HELPER(pre_hvc)(CPUARMState *env) void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome) { int cur_el = arm_current_el(env); - /* FIXME: Use real secure state. */ - bool secure = false; + bool secure = arm_is_secure(env); bool smd = env->cp15.scr_el3 & SCR_SMD; /* On ARMv8 AArch32, SMD only applies to NS state. * On ARMv7 SMD only applies to NS state and only if EL2 is available. diff --git a/target-arm/translate.c b/target-arm/translate.c index 617e6a9..60655e1 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -7872,15 +7872,27 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s) case 7: { int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4); - /* SMC instruction (op1 == 3) - and undefined instructions (op1 == 0 || op1 == 2) - will trap */ - if (op1 != 1) { + if (op1 == 1) { + /* bkpt */ + ARCH(5); + gen_exception_insn(s, 4, EXCP_BKPT, + syn_aa32_bkpt(imm16, false)); + } else if (op1 == 3) { + /* smi/smc */ + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || + s->current_el == 0) { + goto illegal_op; + } + gen_set_pc_im(s, s->pc - 4); + tmp = tcg_const_i32(syn_aa32_smc()); + gen_helper_pre_smc(cpu_env, tmp); + tcg_temp_free_i32(tmp); + gen_ss_advance(s); + gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc()); + break; + } else { goto illegal_op; } - /* bkpt */ - ARCH(5); - gen_exception_insn(s, 4, EXCP_BKPT, syn_aa32_bkpt(imm16, false)); break; } case 0x8: /* signed multiply */ @@ -9711,9 +9723,16 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw if (insn & (1 << 26)) { /* Secure monitor call (v6Z) */ - qemu_log_mask(LOG_UNIMP, - "arm: unimplemented secure monitor call\n"); - goto illegal_op; /* not implemented. */ + if (!arm_dc_feature(s, ARM_FEATURE_EL3) || + s->current_el == 0) { + goto illegal_op; + } + gen_set_pc_im(s, s->pc - 4); + tmp = tcg_const_i32(syn_aa32_smc()); + gen_helper_pre_smc(cpu_env, tmp); + tcg_temp_free_i32(tmp); + gen_ss_advance(s); + gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc()); } else { op = (insn >> 20) & 7; switch (op) {