diff mbox

[[PATCH] 1/7] target-arm: Add exception target el infrastructure

Message ID 1427483446-31900-2-git-send-email-greg.bellows@linaro.org
State New
Headers show

Commit Message

Greg Bellows March 27, 2015, 7:10 p.m. UTC 
Add a CPU state exception target EL field that will be used for communicating
the EL to which an exception should be routed.

Add a target EL argument to the generic exception helper for callers to specify
the EL to which the exception should be routed.  Extended the helper to set
the newly added CPU state exception target el.  Updated calls to helpers to
include target EL, minimally the current el, which gets upgraded as needed.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/cpu.h           |  1 +
 target-arm/helper.h        |  2 +-
 target-arm/op_helper.c     |  3 ++-
 target-arm/translate-a64.c | 32 +++++++++++++++----------
 target-arm/translate.c     | 59 ++++++++++++++++++++++++++++------------------
 5 files changed, 60 insertions(+), 37 deletions(-)

Comments

Peter Maydell April 16, 2015, 5:50 p.m. UTC | #1 
On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> Add a CPU state exception target EL field that will be used for communicating
> the EL to which an exception should be routed.
>
> Add a target EL argument to the generic exception helper for callers to specify
> the EL to which the exception should be routed.  Extended the helper to set
> the newly added CPU state exception target el.  Updated calls to helpers to
> include target EL, minimally the current el, which gets upgraded as needed.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

This is generally OK, but I don't like the way we directly use
s->current_el as the target EL in many places:

(1) putting the MAX(target_el, 1) in gen_exception() feels
like the wrong place.
(2) if we're executing with a 32-bit EL3 and we're in Secure
EL0, surely the target EL should be 3, not 1?

Maybe a default_target_el() function would help (returning
2 if current EL is EL2, 3 if EL3, and 1 or 3 as appropriate
if current EL is EL0 or EL1) ?

Also, what's the plan for handling exceptions which can
trap to EL1, 2 or 3 configurably (eg the various floating
point disables)? Do we put all those trap bits into the TB
flags? If we want to be able to combine them into a single
"FP is disabled" bit in the TB flags, then we need to generate
the same code for any kind of FP-disabled case, which means we
can't encode the target-EL in the generated code like this:
we would need to do something like have a new EXCP_FPDIS
which we then sorted out into an (EXCP_UDEF, target_el, syndrome)
at runtime based on the trap register bits.

thanks
-- PMM
Greg Bellows April 16, 2015, 9:39 p.m. UTC | #2 
On Thu, Apr 16, 2015 at 12:50 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Add a CPU state exception target EL field that will be used for
> communicating
> > the EL to which an exception should be routed.
> >
> > Add a target EL argument to the generic exception helper for callers to
> specify
> > the EL to which the exception should be routed.  Extended the helper to
> set
> > the newly added CPU state exception target el.  Updated calls to helpers
> to
> > include target EL, minimally the current el, which gets upgraded as
> needed.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>
> This is generally OK, but I don't like the way we directly use
> s->current_el as the target EL in many places:
>
> (1) putting the MAX(target_el, 1) in gen_exception() feels
> like the wrong place.
>

​This was simply a safety net as we have to catch the case eventually and
since the target_el originates here it seemed like the most logical place.
Perhaps we just need to pass 1 here and anything that is not 1 needs to be
addressed at this point.


> (2) if we're executing with a 32-bit EL3 and we're in Secure
> EL0, surely the target EL should be 3, not 1?
>
​

> Maybe a default_target_el() function would help (returning
> 2 if current EL is EL2, 3 if EL3, and 1 or 3 as appropriate
> if current EL is EL0 or EL1) ?
>

​Hmmm... yes that is true in the case of secure EL0 it should be routed to
EL3. Which differs from the case of nonsecure EL0.  Would this function
make sense in place of the MAX above?


> Also, what's the plan for handling exceptions which can
> trap to EL1, 2 or 3 configurably (eg the various floating
> point disables)? Do we put all those trap bits into the TB
> flags? If we want to be able to combine them into a single
> "FP is disabled" bit in the TB flags, then we need to generate
> the same code for any kind of FP-disabled case, which means we
> can't encode the target-EL in the generated code like this:
> we would need to do something like have a new EXCP_FPDIS
> which we then sorted out into an (EXCP_UDEF, target_el, syndrome)
> at runtime based on the trap register bits.
>
> I was thinking that we could just turn the fpen into an exception level
(int instead of bool) that if not 0 meant that an exception should be
generated.  We would catch this in fp_access_check().  The value of fpen
(or whatever a better name would be) would get passed into the
gen_exception_insn() call as the target el.



> thanks
> -- PMM
>
diff mbox

Patch

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 083211c..0b232ba 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -401,6 +401,7 @@  typedef struct CPUARMState {
         uint32_t syndrome; /* AArch64 format syndrome register */
         uint32_t fsr; /* AArch32 format fault status register info */
         uint64_t vaddress; /* virtual addr associated with exception, if any */
+        uint32_t target_el; /* EL the exception should be targeted for */
         /* If we implement EL2 we will also need to store information
          * about the intermediate physical address for stage 2 faults.
          */
diff --git a/target-arm/helper.h b/target-arm/helper.h
index dec3728..fc885de 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -47,7 +47,7 @@  DEF_HELPER_FLAGS_2(usad8, TCG_CALL_NO_RWG_SE, i32, i32, i32)
 DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE,
                    i32, i32, i32, i32)
 DEF_HELPER_2(exception_internal, void, env, i32)
-DEF_HELPER_3(exception_with_syndrome, void, env, i32, i32)
+DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32)
 DEF_HELPER_1(wfi, void, env)
 DEF_HELPER_1(wfe, void, env)
 DEF_HELPER_1(pre_hvc, void, env)
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 7713022..72a973a 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -246,13 +246,14 @@  void HELPER(exception_internal)(CPUARMState *env, uint32_t excp)
 
 /* Raise an exception with the specified syndrome register value */
 void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp,
-                                     uint32_t syndrome)
+                                     uint32_t syndrome, uint32_t target_el)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
 
     assert(!excp_is_internal(excp));
     cs->exception_index = excp;
     env->exception.syndrome = syndrome;
+    env->exception.target_el = target_el;
     cpu_loop_exit(cs);
 }
 
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 0b192a1..488eeb5 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -197,12 +197,15 @@  static void gen_exception_internal(int excp)
     tcg_temp_free_i32(tcg_excp);
 }
 
-static void gen_exception(int excp, uint32_t syndrome)
+static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
 {
     TCGv_i32 tcg_excp = tcg_const_i32(excp);
     TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
+    TCGv_i32 tcg_el = tcg_const_i32(MAX(target_el, 1));
 
-    gen_helper_exception_with_syndrome(cpu_env, tcg_excp, tcg_syn);
+    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
+                                       tcg_syn, tcg_el);
+    tcg_temp_free_i32(tcg_el);
     tcg_temp_free_i32(tcg_syn);
     tcg_temp_free_i32(tcg_excp);
 }
@@ -215,10 +218,10 @@  static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
 }
 
 static void gen_exception_insn(DisasContext *s, int offset, int excp,
-                               uint32_t syndrome)
+                               uint32_t syndrome, uint32_t target_el)
 {
     gen_a64_set_pc_im(s->pc - offset);
-    gen_exception(excp, syndrome);
+    gen_exception(excp, syndrome, target_el);
     s->is_jmp = DISAS_EXC;
 }
 
@@ -245,7 +248,8 @@  static void gen_step_complete_exception(DisasContext *s)
      * of the exception, and our syndrome information is always correct.
      */
     gen_ss_advance(s);
-    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex));
+    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
+                  s->current_el);
     s->is_jmp = DISAS_EXC;
 }
 
@@ -292,7 +296,7 @@  static inline void gen_goto_tb(DisasContext *s, int n, uint64_t dest)
 static void unallocated_encoding(DisasContext *s)
 {
     /* Unallocated and reserved encodings are uncategorized */
-    gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized());
+    gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(), s->current_el);
 }
 
 #define unsupported_encoding(s, insn)                                    \
@@ -971,7 +975,8 @@  static inline bool fp_access_check(DisasContext *s)
         return true;
     }
 
-    gen_exception_insn(s, 4, EXCP_UDEF, syn_fp_access_trap(1, 0xe, false));
+    gen_exception_insn(s, 4, EXCP_UDEF, syn_fp_access_trap(1, 0xe, false),
+                       s->current_el);
     return false;
 }
 
@@ -1498,7 +1503,8 @@  static void disas_exc(DisasContext *s, uint32_t insn)
         switch (op2_ll) {
         case 1:
             gen_ss_advance(s);
-            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
+            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
+                               s->current_el);
             break;
         case 2:
             if (s->current_el == 0) {
@@ -1511,7 +1517,7 @@  static void disas_exc(DisasContext *s, uint32_t insn)
             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));
+            gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
             break;
         case 3:
             if (s->current_el == 0) {
@@ -1523,7 +1529,7 @@  static void disas_exc(DisasContext *s, uint32_t insn)
             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));
+            gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16), 3);
             break;
         default:
             unallocated_encoding(s);
@@ -1536,7 +1542,8 @@  static void disas_exc(DisasContext *s, uint32_t insn)
             break;
         }
         /* BRK */
-        gen_exception_insn(s, 4, EXCP_BKPT, syn_aa64_bkpt(imm16));
+        gen_exception_insn(s, 4, EXCP_BKPT, syn_aa64_bkpt(imm16),
+                           s->current_el);
         break;
     case 2:
         if (op2_ll != 0) {
@@ -11031,7 +11038,8 @@  void gen_intermediate_code_internal_a64(ARMCPU *cpu,
              * bits should be zero.
              */
             assert(num_insns == 0);
-            gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0));
+            gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
+                          dc->current_el);
             dc->is_jmp = DISAS_EXC;
             break;
         }
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 9116529..06c711c 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -217,12 +217,16 @@  static void gen_exception_internal(int excp)
     tcg_temp_free_i32(tcg_excp);
 }
 
-static void gen_exception(int excp, uint32_t syndrome)
+static void gen_exception(int excp, uint32_t syndrome, uint32_t target_el)
 {
     TCGv_i32 tcg_excp = tcg_const_i32(excp);
     TCGv_i32 tcg_syn = tcg_const_i32(syndrome);
+    TCGv_i32 tcg_el = tcg_const_i32(MAX(target_el, 1));
 
-    gen_helper_exception_with_syndrome(cpu_env, tcg_excp, tcg_syn);
+    gen_helper_exception_with_syndrome(cpu_env, tcg_excp,
+                                       tcg_syn, tcg_el);
+
+    tcg_temp_free_i32(tcg_el);
     tcg_temp_free_i32(tcg_syn);
     tcg_temp_free_i32(tcg_excp);
 }
@@ -250,7 +254,8 @@  static void gen_step_complete_exception(DisasContext *s)
      * of the exception, and our syndrome information is always correct.
      */
     gen_ss_advance(s);
-    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex));
+    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
+                  s->current_el);
     s->is_jmp = DISAS_EXC;
 }
 
@@ -1013,11 +1018,12 @@  static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
     s->is_jmp = DISAS_JUMP;
 }
 
-static void gen_exception_insn(DisasContext *s, int offset, int excp, int syn)
+static void gen_exception_insn(DisasContext *s, int offset, int excp,
+                               int syn, uint32_t target_el)
 {
     gen_set_condexec(s);
     gen_set_pc_im(s, s->pc - offset);
-    gen_exception(excp, syn);
+    gen_exception(excp, syn, target_el);
     s->is_jmp = DISAS_JUMP;
 }
 
@@ -3040,7 +3046,7 @@  static int disas_vfp_insn(DisasContext *s, uint32_t insn)
      */
     if (!s->cpacr_fpen) {
         gen_exception_insn(s, 4, EXCP_UDEF,
-                           syn_fp_access_trap(1, 0xe, s->thumb));
+                           syn_fp_access_trap(1, 0xe, s->thumb), s->current_el);
         return 0;
     }
 
@@ -4358,7 +4364,7 @@  static int disas_neon_ls_insn(DisasContext *s, uint32_t insn)
      */
     if (!s->cpacr_fpen) {
         gen_exception_insn(s, 4, EXCP_UDEF,
-                           syn_fp_access_trap(1, 0xe, s->thumb));
+                           syn_fp_access_trap(1, 0xe, s->thumb), s->current_el);
         return 0;
     }
 
@@ -5096,7 +5102,7 @@  static int disas_neon_data_insn(DisasContext *s, uint32_t insn)
      */
     if (!s->cpacr_fpen) {
         gen_exception_insn(s, 4, EXCP_UDEF,
-                           syn_fp_access_trap(1, 0xe, s->thumb));
+                           syn_fp_access_trap(1, 0xe, s->thumb), s->current_el);
         return 0;
     }
 
@@ -7960,7 +7966,7 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                 /* bkpt */
                 ARCH(5);
                 gen_exception_insn(s, 4, EXCP_BKPT,
-                                   syn_aa32_bkpt(imm16, false));
+                                   syn_aa32_bkpt(imm16, false), s->current_el);
                 break;
             case 2:
                 /* Hypervisor call (v7) */
@@ -9021,7 +9027,8 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
             break;
         default:
         illegal_op:
-            gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized());
+            gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+                               s->current_el);
             break;
         }
     }
@@ -10858,7 +10865,8 @@  static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
         {
             int imm8 = extract32(insn, 0, 8);
             ARCH(5);
-            gen_exception_insn(s, 2, EXCP_BKPT, syn_aa32_bkpt(imm8, true));
+            gen_exception_insn(s, 2, EXCP_BKPT, syn_aa32_bkpt(imm8, true),
+                               s->current_el);
             break;
         }
 
@@ -11013,11 +11021,12 @@  static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
     }
     return;
 undef32:
-    gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized());
+    gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+                       s->current_el);
     return;
 illegal_op:
 undef:
-    gen_exception_insn(s, 2, EXCP_UDEF, syn_uncategorized());
+    gen_exception_insn(s, 2, EXCP_UDEF, syn_uncategorized(), s->current_el);
 }
 
 /* generate intermediate code in gen_opc_buf and gen_opparam_buf for
@@ -11216,7 +11225,8 @@  static inline void gen_intermediate_code_internal(ARMCPU *cpu,
              * bits should be zero.
              */
             assert(num_insns == 0);
-            gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0));
+            gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
+                          dc->current_el);
             goto done_generating;
         }
 
@@ -11276,13 +11286,14 @@  static inline void gen_intermediate_code_internal(ARMCPU *cpu,
             gen_set_condexec(dc);
             if (dc->is_jmp == DISAS_SWI) {
                 gen_ss_advance(dc);
-                gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb));
+                gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb),
+                              dc->current_el);
             } else if (dc->is_jmp == DISAS_HVC) {
                 gen_ss_advance(dc);
-                gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm));
+                gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm), 2);
             } else if (dc->is_jmp == DISAS_SMC) {
                 gen_ss_advance(dc);
-                gen_exception(EXCP_SMC, syn_aa32_smc());
+                gen_exception(EXCP_SMC, syn_aa32_smc(), 3);
             } else if (dc->ss_active) {
                 gen_step_complete_exception(dc);
             } else {
@@ -11297,13 +11308,14 @@  static inline void gen_intermediate_code_internal(ARMCPU *cpu,
         gen_set_condexec(dc);
         if (dc->is_jmp == DISAS_SWI && !dc->condjmp) {
             gen_ss_advance(dc);
-            gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb));
+            gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb),
+                          dc->current_el);
         } else if (dc->is_jmp == DISAS_HVC && !dc->condjmp) {
             gen_ss_advance(dc);
-            gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm));
+            gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm), 2);
         } else if (dc->is_jmp == DISAS_SMC && !dc->condjmp) {
             gen_ss_advance(dc);
-            gen_exception(EXCP_SMC, syn_aa32_smc());
+            gen_exception(EXCP_SMC, syn_aa32_smc(), 3);
         } else if (dc->ss_active) {
             gen_step_complete_exception(dc);
         } else {
@@ -11341,13 +11353,14 @@  static inline void gen_intermediate_code_internal(ARMCPU *cpu,
             gen_helper_wfe(cpu_env);
             break;
         case DISAS_SWI:
-            gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb));
+            gen_exception(EXCP_SWI, syn_aa32_svc(dc->svc_imm, dc->thumb),
+                          dc->current_el);
             break;
         case DISAS_HVC:
-            gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm));
+            gen_exception(EXCP_HVC, syn_aa32_hvc(dc->svc_imm), 2);
             break;
         case DISAS_SMC:
-            gen_exception(EXCP_SMC, syn_aa32_smc());
+            gen_exception(EXCP_SMC, syn_aa32_smc(), 3);
             break;
         }
         if (dc->condjmp) {