Re: [Qemu-devel] [PATCH v6 06/32] target-arm: A32: Emulate the SMC instruction

2014-10-13 Thread Peter Maydell 
On 10 October 2014 18:03, Greg Bellows greg.bell...@linaro.org wrote:
 From: Fabian Aggeler aggel...@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.fedo...@samsung.com
 Signed-off-by: Fabian Aggeler aggel...@ethz.ch
 Signed-off-by: Greg Bellows greg.bell...@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  0x);
 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

Re: [Qemu-devel] [PATCH v6 06/32] target-arm: A32: Emulate the SMC instruction

2014-10-13 Thread Greg Bellows 
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.mayd...@linaro.org wrote:

 On 10 October 2014 18:03, Greg Bellows greg.bell...@linaro.org wrote:
  From: Fabian Aggeler aggel...@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.fedo...@samsung.com
  Signed-off-by: Fabian Aggeler aggel...@ethz.ch
  Signed-off-by: Greg Bellows greg.bell...@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 
 0x);
  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

Re: [Qemu-devel] [PATCH v6 06/32] target-arm: A32: Emulate the SMC instruction

2014-10-13 Thread Peter Maydell 
On 13 October 2014 15:13, Greg Bellows greg.bell...@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

Re: [Qemu-devel] [PATCH v6 06/32] target-arm: A32: Emulate the SMC instruction

2014-10-13 Thread Greg Bellows 
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.mayd...@linaro.org wrote:

 On 13 October 2014 15:13, Greg Bellows greg.bell...@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

[Qemu-devel] [PATCH v6 06/32] target-arm: A32: Emulate the SMC instruction

2014-10-10 Thread Greg Bellows 
From: Fabian Aggeler aggel...@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.fedo...@samsung.com
Signed-off-by: Fabian Aggeler aggel...@ethz.ch
Signed-off-by: Greg Bellows greg.bell...@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  0x);
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);
+