[Qemu-devel] [[PATCH] 0/7] target-arm: EL3 trap support

[Qemu-devel] [[PATCH] 0/7] target-arm: EL3 trap support

[Greg Bellows]

Initial patchset adding support for trapping to an EL other than EL1.  Support
includes changes to interfaces to allow specification of the target EL.  Also
includes the addition of the ARMv8 CPTR system registers used for controlling
the trapping of features.

Greg Bellows (7):
  target-arm: Add exception target el infrastructure
  target-arm: Extend helpers to route exceptions
  target-arm: Update interrupt handling to use target EL
  target-arm: Add AArch64 CPTR registers
  target-arm: Add TTBR regime function and use
  target-arm: Add WFx syndrome function
  target-arm: Add WFx instruction trap support

 target-arm/cpu.c           |  61 ++++++++++++++-------
 target-arm/cpu.h           |  26 +++++++--
 target-arm/helper-a64.c    |   2 +-
 target-arm/helper.c        | 128 +++++++++++++++++++++++++++------------------
 target-arm/helper.h        |   2 +-
 target-arm/internals.h     |   6 +++
 target-arm/op_helper.c     |  83 +++++++++++++++++++++++++++--
 target-arm/translate-a64.c |  32 +++++++-----
 target-arm/translate.c     |  59 +++++++++++++--------
 9 files changed, 283 insertions(+), 116 deletions(-)

-- 
1.8.3.2

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

[Greg Bellows]

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(-)

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) {
-- 
1.8.3.2

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

[Peter Maydell]

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

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

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 2639 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 4465 bytes --]

[Qemu-devel] [[PATCH] 2/7] target-arm: Extend helpers to route exceptions

[Greg Bellows]

Updated the various helper routines to set the target EL as needed.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/op_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 72a973a..aa175b5 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -306,6 +306,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
     if (arm_feature(env, ARM_FEATURE_XSCALE) && ri->cp < 14
         && extract32(env->cp15.c15_cpar, ri->cp, 1) == 0) {
         env->exception.syndrome = syndrome;
+        env->exception.target_el = MAX(arm_current_el(env), 1);
         raise_exception(env, EXCP_UDEF);
     }
 
@@ -363,6 +364,7 @@ void HELPER(msr_i_pstate)(CPUARMState *env, uint32_t op, uint32_t imm)
      * to catch that case at translate time.
      */
     if (arm_current_el(env) == 0 && !(env->cp15.sctlr_el[1] & SCTLR_UMA)) {
+        env->exception.target_el = MAX(arm_current_el(env), 1);
         raise_exception(env, EXCP_UDEF);
     }
 
@@ -422,6 +424,7 @@ void HELPER(pre_hvc)(CPUARMState *env)
 
     if (undef) {
         env->exception.syndrome = syn_uncategorized();
+        env->exception.target_el = MAX(arm_current_el(env), 1);
         raise_exception(env, EXCP_UDEF);
     }
 }
@@ -452,11 +455,13 @@ void HELPER(pre_smc)(CPUARMState *env, uint32_t syndrome)
     } else if (!secure && cur_el == 1 && (env->cp15.hcr_el2 & HCR_TSC)) {
         /* In NS EL1, HCR controlled routing to EL2 has priority over SMD. */
         env->exception.syndrome = syndrome;
+        env->exception.target_el = 2;
         raise_exception(env, EXCP_HYP_TRAP);
     }
 
     if (undef) {
         env->exception.syndrome = syn_uncategorized();
+        env->exception.target_el = MAX(arm_current_el(env), 1);
         raise_exception(env, EXCP_UDEF);
     }
 }
-- 
1.8.3.2

Re: [Qemu-devel] [[PATCH] 2/7] target-arm: Extend helpers to route exceptions

[Peter Maydell]

On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> Updated the various helper routines to set the target EL as needed.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/op_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 72a973a..aa175b5 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -306,6 +306,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome)
>      if (arm_feature(env, ARM_FEATURE_XSCALE) && ri->cp < 14
>          && extract32(env->cp15.c15_cpar, ri->cp, 1) == 0) {
>          env->exception.syndrome = syndrome;
> +        env->exception.target_el = MAX(arm_current_el(env), 1);

Again, I don't think we should have all these MAX()es.

-- PMM

Re: [Qemu-devel] [[PATCH] 2/7] target-arm: Extend helpers to route exceptions

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Thu, Apr 16, 2015 at 12:51 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Updated the various helper routines to set the target EL as needed.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > ---
> >  target-arm/op_helper.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index 72a973a..aa175b5 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -306,6 +306,7 @@ void HELPER(access_check_cp_reg)(CPUARMState *env,
> void *rip, uint32_t syndrome)
> >      if (arm_feature(env, ARM_FEATURE_XSCALE) && ri->cp < 14
> >          && extract32(env->cp15.c15_cpar, ri->cp, 1) == 0) {
> >          env->exception.syndrome = syndrome;
> > +        env->exception.target_el = MAX(arm_current_el(env), 1);
>
> Again, I don't think we should have all these MAX()es.
>

​Added a dedicated function for determining the exception el.  Note this is
different than the one needed in translate as they take different input
criteria. I can create a common core function with two wrappers, but that
seems excessive.​


>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 2128 bytes --]

[Qemu-devel] [[PATCH] 3/7] target-arm: Update interrupt handling to use target EL

[Greg Bellows]

Updated the interrupt handling to utilize and report through the target EL
exception field.  This includes consolidating and cleaning up code where
needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and
do_interrupt was updated to use the target_el exception field.  The
necessary code from arm_excp_target_el() was merged in where needed and the
function removed.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/cpu.c        | 61 +++++++++++++++++++++++++++++++++----------------
 target-arm/cpu.h        |  7 +++---
 target-arm/helper-a64.c |  2 +-
 target-arm/helper.c     | 41 ++++-----------------------------
 4 files changed, 50 insertions(+), 61 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 986f04c..4aa71a7 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -206,31 +206,52 @@ static void arm_cpu_reset(CPUState *s)
 bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
+    CPUARMState *env = cs->env_ptr;
+    uint32_t cur_el = arm_current_el(env);
+    bool secure = arm_is_secure(env);
+    uint32_t target_el;
+    uint32_t excp_idx;
     bool ret = false;
 
-    if (interrupt_request & CPU_INTERRUPT_FIQ
-        && arm_excp_unmasked(cs, EXCP_FIQ)) {
-        cs->exception_index = EXCP_FIQ;
-        cc->do_interrupt(cs);
-        ret = true;
+    if (interrupt_request & CPU_INTERRUPT_FIQ) {
+        excp_idx = EXCP_FIQ;
+        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
+        if (arm_excp_unmasked(cs, excp_idx, target_el)) {
+            cs->exception_index = excp_idx;
+            env->exception.target_el = target_el;
+            cc->do_interrupt(cs);
+            ret = true;
+        }
     }
-    if (interrupt_request & CPU_INTERRUPT_HARD
-        && arm_excp_unmasked(cs, EXCP_IRQ)) {
-        cs->exception_index = EXCP_IRQ;
-        cc->do_interrupt(cs);
-        ret = true;
+    if (interrupt_request & CPU_INTERRUPT_HARD) {
+        excp_idx = EXCP_IRQ;
+        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
+        if (arm_excp_unmasked(cs, excp_idx, target_el)) {
+            cs->exception_index = excp_idx;
+            env->exception.target_el = target_el;
+            cc->do_interrupt(cs);
+            ret = true;
+        }
     }
-    if (interrupt_request & CPU_INTERRUPT_VIRQ
-        && arm_excp_unmasked(cs, EXCP_VIRQ)) {
-        cs->exception_index = EXCP_VIRQ;
-        cc->do_interrupt(cs);
-        ret = true;
+    if (interrupt_request & CPU_INTERRUPT_VIRQ) {
+        excp_idx = EXCP_VIRQ;
+        target_el = 1;
+        if (arm_excp_unmasked(cs, excp_idx, target_el)) {
+            cs->exception_index = excp_idx;
+            env->exception.target_el = target_el;
+            cc->do_interrupt(cs);
+            ret = true;
+        }
     }
-    if (interrupt_request & CPU_INTERRUPT_VFIQ
-        && arm_excp_unmasked(cs, EXCP_VFIQ)) {
-        cs->exception_index = EXCP_VFIQ;
-        cc->do_interrupt(cs);
-        ret = true;
+    if (interrupt_request & CPU_INTERRUPT_VFIQ) {
+        excp_idx = EXCP_VFIQ;
+        target_el = 1;
+        if (arm_excp_unmasked(cs, excp_idx, target_el)) {
+            cs->exception_index = excp_idx;
+            env->exception.target_el = target_el;
+            cc->do_interrupt(cs);
+            ret = true;
+        }
     }
 
     return ret;
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 0b232ba..2178a1f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1007,7 +1007,8 @@ static inline bool access_secure_reg(CPUARMState *env)
                        (_val))
 
 void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
-unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx);
+uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
+                                 uint32_t cur_el, bool secure);
 
 /* Interface between CPU and Interrupt controller.  */
 void armv7m_nvic_set_pending(void *opaque, int irq);
@@ -1489,11 +1490,11 @@ bool write_cpustate_to_list(ARMCPU *cpu);
 #  define TARGET_VIRT_ADDR_SPACE_BITS 32
 #endif
 
-static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx)
+static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
+                                     unsigned int target_el)
 {
     CPUARMState *env = cs->env_ptr;
     unsigned int cur_el = arm_current_el(env);
-    unsigned int target_el = arm_excp_target_el(cs, excp_idx);
     bool secure = arm_is_secure(env);
     uint32_t scr;
     uint32_t hcr;
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 7e0d038..07f9799 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
+    unsigned int new_el = MAX(env->exception.target_el, 1);
     target_ulong addr = env->cp15.vbar_el[new_el];
     unsigned int new_mode = aarch64_pstate_mode(new_el, true);
 
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 10886c5..95383d5 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4091,7 +4091,8 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
     return 0;
 }
 
-unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
+uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
+                                 uint32_t cur_el, bool secure)
 {
     return 1;
 }
@@ -4215,8 +4216,8 @@ const int8_t target_el_table[2][2][2][2][2][4] = {
 /*
  * Determine the target EL for physical exceptions
  */
-static inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
-                                        uint32_t cur_el, bool secure)
+uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
+                                 uint32_t cur_el, bool secure)
 {
     CPUARMState *env = cs->env_ptr;
     int rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
@@ -4251,40 +4252,6 @@ static inline uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
     return target_el;
 }
 
-/*
- * Determine the target EL for a given exception type.
- */
-unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx)
-{
-    ARMCPU *cpu = ARM_CPU(cs);
-    CPUARMState *env = &cpu->env;
-    unsigned int cur_el = arm_current_el(env);
-    unsigned int target_el;
-    bool secure = arm_is_secure(env);
-
-    switch (excp_idx) {
-    case EXCP_HVC:
-    case EXCP_HYP_TRAP:
-        target_el = 2;
-        break;
-    case EXCP_SMC:
-        target_el = 3;
-        break;
-    case EXCP_FIQ:
-    case EXCP_IRQ:
-        target_el = arm_phys_excp_target_el(cs, excp_idx, cur_el, secure);
-        break;
-    case EXCP_VIRQ:
-    case EXCP_VFIQ:
-        target_el = 1;
-        break;
-    default:
-        target_el = MAX(cur_el, 1);
-        break;
-    }
-    return target_el;
-}
-
 static void v7m_push(CPUARMState *env, uint32_t val)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
-- 
1.8.3.2

Re: [Qemu-devel] [[PATCH] 3/7] target-arm: Update interrupt handling to use target EL

[Peter Maydell]

On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> Updated the interrupt handling to utilize and report through the target EL
> exception field.  This includes consolidating and cleaning up code where
> needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and
> do_interrupt was updated to use the target_el exception field.  The
> necessary code from arm_excp_target_el() was merged in where needed and the
> function removed.

> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> -    unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
> +    unsigned int new_el = MAX(env->exception.target_el, 1);

Surely we should never be able to get here with target_el zero?

Rest of the patch looks OK.

-- PMM

Re: [Qemu-devel] [[PATCH] 3/7] target-arm: Update interrupt handling to use target EL

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On Thu, Apr 16, 2015 at 12:52 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Updated the interrupt handling to utilize and report through the target
> EL
> > exception field.  This includes consolidating and cleaning up code where
> > needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and
> > do_interrupt was updated to use the target_el exception field.  The
> > necessary code from arm_excp_target_el() was merged in where needed and
> the
> > function removed.
>
> > --- a/target-arm/helper-a64.c
> > +++ b/target-arm/helper-a64.c
> > @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
> >  {
> >      ARMCPU *cpu = ARM_CPU(cs);
> >      CPUARMState *env = &cpu->env;
> > -    unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
> > +    unsigned int new_el = MAX(env->exception.target_el, 1);
>
> Surely we should never be able to get here with target_el zero?
>

​Ideally that would be true and I wondered that myself so I took out the
the MAX safety net in arm_excp_target_el() and later hit the assert
in aarch64_banked_spsr_index() because new_el was 0.  This is why I
preserved the MAX behavior everywhere because just like the original code,
there are cases where current_el is 0.

​I figured this was the safest alternative as it would catch all the cases
where we were not specifying the target EL.


> Rest of the patch looks OK.
>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 2570 bytes --]

Re: [Qemu-devel] [[PATCH] 3/7] target-arm: Update interrupt handling to use target EL

[Peter Maydell]

On 16 April 2015 at 22:03, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Thu, Apr 16, 2015 at 12:52 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
>> > Updated the interrupt handling to utilize and report through the target
>> > EL
>> > exception field.  This includes consolidating and cleaning up code where
>> > needed. Target EL is now calculated once in arm_cpu_exec_interrupt() and
>> > do_interrupt was updated to use the target_el exception field.  The
>> > necessary code from arm_excp_target_el() was merged in where needed and
>> > the
>> > function removed.
>>
>> > --- a/target-arm/helper-a64.c
>> > +++ b/target-arm/helper-a64.c
>> > @@ -463,7 +463,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>> >  {
>> >      ARMCPU *cpu = ARM_CPU(cs);
>> >      CPUARMState *env = &cpu->env;
>> > -    unsigned int new_el = arm_excp_target_el(cs, cs->exception_index);
>> > +    unsigned int new_el = MAX(env->exception.target_el, 1);
>>
>> Surely we should never be able to get here with target_el zero?
>
>
> Ideally that would be true and I wondered that myself so I took out the the
> MAX safety net in arm_excp_target_el() and later hit the assert in
> aarch64_banked_spsr_index() because new_el was 0.  This is why I preserved
> the MAX behavior everywhere because just like the original code, there are
> cases where current_el is 0.

I guessed we might assert. But this is bad, because it suggests there's
somewhere that's not setting target_el at all, which in turn probably
means that after we've taken one exception, the next time around we'll
just go to whichever target_el the first one set up...

There should be a definite rule about who has to set up target_el,
so when we're reviewing or changing code we can tell whether it's
been done correctly. (Compare the rule for exception.syndrome, which
is "must be set up before the exception is raised, unless
excp_is_internal(excp)", which we have some assertions to try to
enforce. There's also a less-documented rule for exception.vaddress,
which is "must be set for EXCP_PREFETCH_ABORT and EXCP_DATA_ABORT".)

-- PMM

[Qemu-devel] [[PATCH] 4/7] target-arm: Add AArch64 CPTR registers

[Greg Bellows]

Adds CPTR_EL2/3 system registers definitions and access function.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/cpu.h    | 18 +++++++++++++++++-
 target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 2178a1f..a811369 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -202,6 +202,7 @@ typedef struct CPUARMState {
             uint64_t sctlr_el[4];
         };
         uint64_t c1_coproc; /* Coprocessor access register.  */
+        uint64_t cptr_el[4];  /* ARMv8 feature trap registers */
         uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
         uint64_t sder; /* Secure debug enable register. */
         uint32_t nsacr; /* Non-secure access control register. */
@@ -575,6 +576,10 @@ void pmccntr_sync(CPUARMState *env);
 #define SCTLR_AFE     (1U << 29)
 #define SCTLR_TE      (1U << 30)
 
+#define CPTR_TCPAC    (1U << 31)
+#define CPTR_TTA      (1U << 20)
+#define CPTR_TFP      (1U << 10)
+
 #define CPSR_M (0x1fU)
 #define CPSR_T (1U << 5)
 #define CPSR_F (1U << 6)
@@ -1813,9 +1818,20 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                                         target_ulong *cs_base, int *flags)
 {
     int fpen;
+    int cur_el = arm_current_el(env);
 
     if (arm_feature(env, ARM_FEATURE_V6)) {
-        fpen = extract32(env->cp15.c1_coproc, 20, 2);
+        /* In AArch64, FP can be enabled differently depending on our EL.
+         * If our EL is 2 or 3, we use the EL specific CPTR to determine if FP
+         * is enabled.  Otherwise, we fall back to using CPACR.
+         * CPTR.TFP is clear if FP is enabled whereas CPACR.FPEN is set to some
+         * degree.
+         */
+        if (is_a64(env) && cur_el >= 2) {
+            fpen = !extract32(env->cp15.cptr_el[cur_el], 10, 1);
+        } else {
+            fpen = extract32(env->cp15.c1_coproc, 20, 2);
+        }
     } else {
         /* CPACR doesn't exist before v6, so VFP is always accessible */
         fpen = 3;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 95383d5..00b457a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -592,6 +592,39 @@ static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c1_coproc = value;
 }
 
+static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    int cur_el = arm_current_el(env);
+    bool secure = arm_is_secure(env);
+
+    switch (ri->opc1) {
+    case 0:     /* CPACR and CPACR_EL1 */
+        if (arm_feature(env, ARM_FEATURE_V8) && cur_el == 1) {
+            /* Make sure we have EL2 before routine there */
+            if (arm_feature(env, ARM_FEATURE_EL2) && !secure &&
+                (env->cp15.cptr_el[2] & CPTR_TCPAC)) {
+                env->exception.target_el = 2;
+                return CP_ACCESS_TRAP;
+            /* Make sure we have EL3 before routine there */
+            } else if (arm_feature(env, ARM_FEATURE_EL3) &&
+                       env->cp15.cptr_el[3] & CPTR_TCPAC) {
+                env->exception.target_el = 3;
+                return CP_ACCESS_TRAP;
+            }
+        }
+        break;
+    case 4:     /* CPTR_EL2 */
+        /* It is safe to assume we have EL2 and ARMv8 if we get here */
+        if (cur_el == 2 && (env->cp15.cptr_el[3] & CPTR_TCPAC)) {
+            env->exception.target_el = 3;
+            return CP_ACCESS_TRAP;
+        }
+        break;
+    }
+
+    return CP_ACCESS_OK;
+}
+
 static const ARMCPRegInfo v6_cp_reginfo[] = {
     /* prefetch by MVA in v6, NOP in v7 */
     { .name = "MVA_prefetch",
@@ -614,7 +647,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
     { .name = "WFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0, },
     { .name = "CPACR", .state = ARM_CP_STATE_BOTH, .opc0 = 3,
-      .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2,
+      .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2, .accessfn = cptr_access,
       .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_coproc),
       .resetvalue = 0, .writefn = cpacr_write },
     REGINFO_SENTINEL
@@ -2537,6 +2570,10 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
       .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 1, .opc2 = 0,
       .access = PL3_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, sp_el[2]) },
+    { .name = "CPTR_EL2", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
+      .access = PL2_RW, .accessfn = cptr_access, .resetvalue = 0,
+      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[2]) },
     REGINFO_SENTINEL
 };
 
@@ -2598,6 +2635,10 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
       .access = PL3_RW, .writefn = vbar_write,
       .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[3]),
       .resetvalue = 0 },
+    { .name = "CPTR_EL3", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 2,
+      .access = PL3_RW, .accessfn = cptr_access, .resetvalue = 0,
+      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[3]) },
     REGINFO_SENTINEL
 };
 
-- 
1.8.3.2

Re: [Qemu-devel] [[PATCH] 4/7] target-arm: Add AArch64 CPTR registers

[Peter Maydell]

On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> Adds CPTR_EL2/3 system registers definitions and access function.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/cpu.h    | 18 +++++++++++++++++-
>  target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 2178a1f..a811369 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -202,6 +202,7 @@ typedef struct CPUARMState {
>              uint64_t sctlr_el[4];
>          };
>          uint64_t c1_coproc; /* Coprocessor access register.  */
> +        uint64_t cptr_el[4];  /* ARMv8 feature trap registers */
>          uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
>          uint64_t sder; /* Secure debug enable register. */
>          uint32_t nsacr; /* Non-secure access control register. */
> @@ -575,6 +576,10 @@ void pmccntr_sync(CPUARMState *env);
>  #define SCTLR_AFE     (1U << 29)
>  #define SCTLR_TE      (1U << 30)
>
> +#define CPTR_TCPAC    (1U << 31)
> +#define CPTR_TTA      (1U << 20)
> +#define CPTR_TFP      (1U << 10)
> +
>  #define CPSR_M (0x1fU)
>  #define CPSR_T (1U << 5)
>  #define CPSR_F (1U << 6)
> @@ -1813,9 +1818,20 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                                          target_ulong *cs_base, int *flags)
>  {
>      int fpen;
> +    int cur_el = arm_current_el(env);
>
>      if (arm_feature(env, ARM_FEATURE_V6)) {
> -        fpen = extract32(env->cp15.c1_coproc, 20, 2);
> +        /* In AArch64, FP can be enabled differently depending on our EL.
> +         * If our EL is 2 or 3, we use the EL specific CPTR to determine if FP
> +         * is enabled.  Otherwise, we fall back to using CPACR.
> +         * CPTR.TFP is clear if FP is enabled whereas CPACR.FPEN is set to some
> +         * degree.
> +         */
> +        if (is_a64(env) && cur_el >= 2) {
> +            fpen = !extract32(env->cp15.cptr_el[cur_el], 10, 1);
> +        } else {
> +            fpen = extract32(env->cp15.c1_coproc, 20, 2);
> +        }

See my comments on patch 1 about what else we need to do to
be able to merge fp enable bits like this.

Also, the logic here is wrong: if we're at NS-EL0 or NS-EL1
then fpen is not just the c1_coproc bit: if cptr_el[2].TFP
is set then FP is disabled and traps to EL2 (same as if we
were at NS-EL2).

>      } else {
>          /* CPACR doesn't exist before v6, so VFP is always accessible */
>          fpen = 3;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 95383d5..00b457a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -592,6 +592,39 @@ static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->cp15.c1_coproc = value;
>  }
>
> +static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    int cur_el = arm_current_el(env);
> +    bool secure = arm_is_secure(env);
> +
> +    switch (ri->opc1) {
> +    case 0:     /* CPACR and CPACR_EL1 */

We're not actually sharing any interesting code between these
two cases of ri->opc1, so you should just have two different
access functions.

> +        if (arm_feature(env, ARM_FEATURE_V8) && cur_el == 1) {
> +            /* Make sure we have EL2 before routine there */

"routing", but you don't need to -- if EL2 doesn't exist then
it's impossible for the trap bit to get set. Ditto EL3.

> +            if (arm_feature(env, ARM_FEATURE_EL2) && !secure &&
> +                (env->cp15.cptr_el[2] & CPTR_TCPAC)) {
> +                env->exception.target_el = 2;
> +                return CP_ACCESS_TRAP;
> +            /* Make sure we have EL3 before routine there */
> +            } else if (arm_feature(env, ARM_FEATURE_EL3) &&
> +                       env->cp15.cptr_el[3] & CPTR_TCPAC) {
> +                env->exception.target_el = 3;
> +                return CP_ACCESS_TRAP;
> +            }
> +        }
> +        break;
> +    case 4:     /* CPTR_EL2 */
> +        /* It is safe to assume we have EL2 and ARMv8 if we get here */

This register exists (as HCPTR) in ARMv7, so you can't assume
ARMv8, but on the other hand you don't have to, because again
the CPTR_EL3 bit can't get set unless we're ARMv8.

> +        if (cur_el == 2 && (env->cp15.cptr_el[3] & CPTR_TCPAC)) {
> +            env->exception.target_el = 3;
> +            return CP_ACCESS_TRAP;
> +        }
> +        break;
> +    }
> +
> +    return CP_ACCESS_OK;
> +}
> +
>  static const ARMCPRegInfo v6_cp_reginfo[] = {
>      /* prefetch by MVA in v6, NOP in v7 */
>      { .name = "MVA_prefetch",
> @@ -614,7 +647,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
>      { .name = "WFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 1,
>        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0, },
>      { .name = "CPACR", .state = ARM_CP_STATE_BOTH, .opc0 = 3,
> -      .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2,
> +      .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2, .accessfn = cptr_access,
>        .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.c1_coproc),
>        .resetvalue = 0, .writefn = cpacr_write },
>      REGINFO_SENTINEL
> @@ -2537,6 +2570,10 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
>        .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 1, .opc2 = 0,
>        .access = PL3_RW, .type = ARM_CP_ALIAS,
>        .fieldoffset = offsetof(CPUARMState, sp_el[2]) },
> +    { .name = "CPTR_EL2", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
> +      .access = PL2_RW, .accessfn = cptr_access, .resetvalue = 0,
> +      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[2]) },

This has an AArch32 view, as HCPTR.

Also, if EL2 isn't implemented then CPTR_EL2/HCPTR need to be RES0
if accessed from EL3, so you need an entry in v8_el3_no_el2_cp_reginfo.

>      REGINFO_SENTINEL
>  };
>
> @@ -2598,6 +2635,10 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>        .access = PL3_RW, .writefn = vbar_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[3]),
>        .resetvalue = 0 },
> +    { .name = "CPTR_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 2,
> +      .access = PL3_RW, .accessfn = cptr_access, .resetvalue = 0,
> +      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[3]) },
>      REGINFO_SENTINEL
>  };
>
> --
> 1.8.3.2

thanks
-- PMM

Re: [Qemu-devel] [[PATCH] 4/7] target-arm: Add AArch64 CPTR registers

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 7550 bytes --]

On Thu, Apr 16, 2015 at 1:00 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Adds CPTR_EL2/3 system registers definitions and access function.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > ---
> >  target-arm/cpu.h    | 18 +++++++++++++++++-
> >  target-arm/helper.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 59 insertions(+), 2 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 2178a1f..a811369 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -202,6 +202,7 @@ typedef struct CPUARMState {
> >              uint64_t sctlr_el[4];
> >          };
> >          uint64_t c1_coproc; /* Coprocessor access register.  */
> > +        uint64_t cptr_el[4];  /* ARMv8 feature trap registers */
> >          uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.
> */
> >          uint64_t sder; /* Secure debug enable register. */
> >          uint32_t nsacr; /* Non-secure access control register. */
> > @@ -575,6 +576,10 @@ void pmccntr_sync(CPUARMState *env);
> >  #define SCTLR_AFE     (1U << 29)
> >  #define SCTLR_TE      (1U << 30)
> >
> > +#define CPTR_TCPAC    (1U << 31)
> > +#define CPTR_TTA      (1U << 20)
> > +#define CPTR_TFP      (1U << 10)
> > +
> >  #define CPSR_M (0x1fU)
> >  #define CPSR_T (1U << 5)
> >  #define CPSR_F (1U << 6)
> > @@ -1813,9 +1818,20 @@ static inline void
> cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
> >                                          target_ulong *cs_base, int
> *flags)
> >  {
> >      int fpen;
> > +    int cur_el = arm_current_el(env);
> >
> >      if (arm_feature(env, ARM_FEATURE_V6)) {
> > -        fpen = extract32(env->cp15.c1_coproc, 20, 2);
> > +        /* In AArch64, FP can be enabled differently depending on our
> EL.
> > +         * If our EL is 2 or 3, we use the EL specific CPTR to
> determine if FP
> > +         * is enabled.  Otherwise, we fall back to using CPACR.
> > +         * CPTR.TFP is clear if FP is enabled whereas CPACR.FPEN is set
> to some
> > +         * degree.
> > +         */
> > +        if (is_a64(env) && cur_el >= 2) {
> > +            fpen = !extract32(env->cp15.cptr_el[cur_el], 10, 1);
> > +        } else {
> > +            fpen = extract32(env->cp15.c1_coproc, 20, 2);
> > +        }
>
> See my comments on patch 1 about what else we need to do to
> be able to merge fp enable bits like this.
>

​Broke out FP exception support into a separate patch for v2.  The fix is
as mentioned earlier: change DisasContext flag into EL where 0 means
enabled.​


>
> Also, the logic here is wrong: if we're at NS-EL0 or NS-EL1
> then fpen is not just the c1_coproc bit: if cptr_el[2].TFP
> is set then FP is disabled and traps to EL2 (same as if we
> were at NS-EL2).
>

​Yes, you are correct.  Fixed in the FP patch.​


>
> >      } else {
> >          /* CPACR doesn't exist before v6, so VFP is always accessible */
> >          fpen = 3;
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index 95383d5..00b457a 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -592,6 +592,39 @@ static void cpacr_write(CPUARMState *env, const
> ARMCPRegInfo *ri,
> >      env->cp15.c1_coproc = value;
> >  }
> >
> > +static CPAccessResult cptr_access(CPUARMState *env, const ARMCPRegInfo
> *ri)
> > +{
> > +    int cur_el = arm_current_el(env);
> > +    bool secure = arm_is_secure(env);
> > +
> > +    switch (ri->opc1) {
> > +    case 0:     /* CPACR and CPACR_EL1 */
>
> We're not actually sharing any interesting code between these
> two cases of ri->opc1, so you should just have two different
> access functions.
>

​Broke into to access functions in v2.​


>
> > +        if (arm_feature(env, ARM_FEATURE_V8) && cur_el == 1) {
> > +            /* Make sure we have EL2 before routine there */
>
> "routing", but you don't need to -- if EL2 doesn't exist then
> it's impossible for the trap bit to get set. Ditto EL3.
>

​Fixed in v2.
​


>
> > +            if (arm_feature(env, ARM_FEATURE_EL2) && !secure &&
> > +                (env->cp15.cptr_el[2] & CPTR_TCPAC)) {
> > +                env->exception.target_el = 2;
> > +                return CP_ACCESS_TRAP;
> > +            /* Make sure we have EL3 before routine there */
> > +            } else if (arm_feature(env, ARM_FEATURE_EL3) &&
> > +                       env->cp15.cptr_el[3] & CPTR_TCPAC) {
> > +                env->exception.target_el = 3;
> > +                return CP_ACCESS_TRAP;
> > +            }
> > +        }
> > +        break;
> > +    case 4:     /* CPTR_EL2 */
> > +        /* It is safe to assume we have EL2 and ARMv8 if we get here */
>
> This register exists (as HCPTR) in ARMv7, so you can't assume
> ARMv8, but on the other hand you don't have to, because again
> the CPTR_EL3 bit can't get set unless we're ARMv8.
>

​Fixed comment.
​


>
> > +        if (cur_el == 2 && (env->cp15.cptr_el[3] & CPTR_TCPAC)) {
> > +            env->exception.target_el = 3;
> > +            return CP_ACCESS_TRAP;
> > +        }
> > +        break;
> > +    }
> > +
> > +    return CP_ACCESS_OK;
> > +}
> > +
> >  static const ARMCPRegInfo v6_cp_reginfo[] = {
> >      /* prefetch by MVA in v6, NOP in v7 */
> >      { .name = "MVA_prefetch",
> > @@ -614,7 +647,7 @@ static const ARMCPRegInfo v6_cp_reginfo[] = {
> >      { .name = "WFAR", .cp = 15, .crn = 6, .crm = 0, .opc1 = 0, .opc2 =
> 1,
> >        .access = PL1_RW, .type = ARM_CP_CONST, .resetvalue = 0, },
> >      { .name = "CPACR", .state = ARM_CP_STATE_BOTH, .opc0 = 3,
> > -      .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2,
> > +      .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2, .accessfn = cptr_access,
> >        .access = PL1_RW, .fieldoffset = offsetof(CPUARMState,
> cp15.c1_coproc),
> >        .resetvalue = 0, .writefn = cpacr_write },
> >      REGINFO_SENTINEL
> > @@ -2537,6 +2570,10 @@ static const ARMCPRegInfo v8_el2_cp_reginfo[] = {
> >        .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 1, .opc2 = 0,
> >        .access = PL3_RW, .type = ARM_CP_ALIAS,
> >        .fieldoffset = offsetof(CPUARMState, sp_el[2]) },
> > +    { .name = "CPTR_EL2", .state = ARM_CP_STATE_AA64,
> > +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
> > +      .access = PL2_RW, .accessfn = cptr_access, .resetvalue = 0,
> > +      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[2]) },
>
> This has an AArch32 view, as HCPTR.
>
> Also, if EL2 isn't implemented then CPTR_EL2/HCPTR need to be RES0
> if accessed from EL3, so you need an entry in v8_el3_no_el2_cp_reginfo.
>

I was short-cutting the EL2 stuff, but for completeness I added all the
above in v2.
​


>
> >      REGINFO_SENTINEL
> >  };
> >
> > @@ -2598,6 +2635,10 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
> >        .access = PL3_RW, .writefn = vbar_write,
> >        .fieldoffset = offsetof(CPUARMState, cp15.vbar_el[3]),
> >        .resetvalue = 0 },
> > +    { .name = "CPTR_EL3", .state = ARM_CP_STATE_AA64,
> > +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 2,
> > +      .access = PL3_RW, .accessfn = cptr_access, .resetvalue = 0,
> > +      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[3]) },
> >      REGINFO_SENTINEL
> >  };
> >
> > --
> > 1.8.3.2
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 11204 bytes --]

[Qemu-devel] [[PATCH] 5/7] target-arm: Add TTBR regime function and use

[Greg Bellows]

Add a utility function for choosing the correct TTBR system register based on
the specified MMU index. Add use of function on physical address lookup.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/helper.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 00b457a..13fdf02 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4892,6 +4892,21 @@ static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
     return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
 }
 
+/* Return the TTBR associated with this translation regime */
+static inline uint32_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
+                                   int ttbrn)
+{
+    if (mmu_idx == ARMMMUIdx_S2NS) {
+        /* TODO: return VTTBR_EL2 */
+        g_assert_not_reached();
+    }
+    if (ttbrn == 0) {
+        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
+    } else {
+        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
+    }
+}
+
 /* Return true if the translation regime is using LPAE format page tables */
 static inline bool regime_using_lpae_format(CPUARMState *env,
                                             ARMMMUIdx mmu_idx)
@@ -5090,7 +5105,6 @@ static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
                                      uint32_t *table, uint32_t address)
 {
     /* Note that we can only get here for an AArch32 PL0/PL1 lookup */
-    int el = regime_el(env, mmu_idx);
     TCR *tcr = regime_tcr(env, mmu_idx);
 
     if (address & tcr->mask) {
@@ -5098,13 +5112,13 @@ static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
             /* Translation table walk disabled for TTBR1 */
             return false;
         }
-        *table = env->cp15.ttbr1_el[el] & 0xffffc000;
+        *table = regime_ttbr(env, mmu_idx, 1) & 0xffffc000;
     } else {
         if (tcr->raw_tcr & TTBCR_PD0) {
             /* Translation table walk disabled for TTBR0 */
             return false;
         }
-        *table = env->cp15.ttbr0_el[el] & tcr->base_mask;
+        *table = regime_ttbr(env, mmu_idx, 0) & tcr->base_mask;
     }
     *table |= (address >> 18) & 0x3ffc;
     return true;
@@ -5376,20 +5390,26 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     int32_t tbi = 0;
     TCR *tcr = regime_tcr(env, mmu_idx);
     int ap, ns, xn, pxn;
+    uint32_t el = regime_el(env, mmu_idx);
 
     /* TODO:
      * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
-     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
-     * and VTCR_EL2, or the fact that those regimes don't have a split
+     * it doesn't handle the different format TCR for and VTCR_EL2,
+     * or the fact that those regimes don't have a split
      * TTBR0/TTBR1. Attribute and permission bit handling should also
      * be checked when adding support for those page table walks.
      */
-    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
+    if (arm_el_is_aa64(env, el)) {
         va_size = 64;
-        if (extract64(address, 55, 1))
-            tbi = extract64(tcr->raw_tcr, 38, 1);
-        else
-            tbi = extract64(tcr->raw_tcr, 37, 1);
+        if (el == 3 || el == 2) {
+            tbi = extract64(tcr->raw_tcr, 20, 1);
+        } else {
+            if (extract64(address, 55, 1)) {
+                tbi = extract64(tcr->raw_tcr, 38, 1);
+            } else {
+                tbi = extract64(tcr->raw_tcr, 37, 1);
+            }
+        }
         tbi *= 8;
     }
 
@@ -5434,7 +5454,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
      * we will always flush the TLB any time the ASID is changed).
      */
     if (ttbr_select == 0) {
-        ttbr = A32_BANKED_CURRENT_REG_GET(env, ttbr0);
+        ttbr = regime_ttbr(env, mmu_idx, 0);
         epd = extract32(tcr->raw_tcr, 7, 1);
         tsz = t0sz;
 
@@ -5446,7 +5466,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
             granule_sz = 11;
         }
     } else {
-        ttbr = A32_BANKED_CURRENT_REG_GET(env, ttbr1);
+        ttbr = regime_ttbr(env, mmu_idx, 1);
         epd = extract32(tcr->raw_tcr, 23, 1);
         tsz = t1sz;
 
-- 
1.8.3.2

Re: [Qemu-devel] [[PATCH] 5/7] target-arm: Add TTBR regime function and use

[Sergey Fedorov]

On 27.03.2015 12:10, Greg Bellows wrote:
> Add a utility function for choosing the correct TTBR system register based on
> the specified MMU index. Add use of function on physical address lookup.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/helper.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 00b457a..13fdf02 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c

(snip)

> @@ -5376,20 +5390,26 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      int32_t tbi = 0;
>      TCR *tcr = regime_tcr(env, mmu_idx);
>      int ap, ns, xn, pxn;
> +    uint32_t el = regime_el(env, mmu_idx);
>  
>      /* TODO:
>       * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
> -     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
> -     * and VTCR_EL2, or the fact that those regimes don't have a split
> +     * it doesn't handle the different format TCR for and VTCR_EL2,

s/and VTCR_EL2/VTCR_EL2/

> +     * or the fact that those regimes don't have a split
>       * TTBR0/TTBR1. Attribute and permission bit handling should also
>       * be checked when adding support for those page table walks.
>       */
> -    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
> +    if (arm_el_is_aa64(env, el)) {
>          va_size = 64;
> -        if (extract64(address, 55, 1))
> -            tbi = extract64(tcr->raw_tcr, 38, 1);
> -        else
> -            tbi = extract64(tcr->raw_tcr, 37, 1);
> +        if (el == 3 || el == 2) {
> +            tbi = extract64(tcr->raw_tcr, 20, 1);
> +        } else {
> +            if (extract64(address, 55, 1)) {
> +                tbi = extract64(tcr->raw_tcr, 38, 1);
> +            } else {
> +                tbi = extract64(tcr->raw_tcr, 37, 1);
> +            }
> +        }
>          tbi *= 8;
>      }
>  

(snip)

Re: [Qemu-devel] [[PATCH] 5/7] target-arm: Add TTBR regime function and use

[Peter Maydell]

On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> Add a utility function for choosing the correct TTBR system register based on
> the specified MMU index. Add use of function on physical address lookup.

> @@ -5376,20 +5390,26 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      int32_t tbi = 0;
>      TCR *tcr = regime_tcr(env, mmu_idx);
>      int ap, ns, xn, pxn;
> +    uint32_t el = regime_el(env, mmu_idx);
>
>      /* TODO:
>       * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
> -     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
> -     * and VTCR_EL2, or the fact that those regimes don't have a split
> +     * it doesn't handle the different format TCR for and VTCR_EL2,
> +     * or the fact that those regimes don't have a split
>       * TTBR0/TTBR1. Attribute and permission bit handling should also
>       * be checked when adding support for those page table walks.
>       */
> -    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
> +    if (arm_el_is_aa64(env, el)) {
>          va_size = 64;
> -        if (extract64(address, 55, 1))
> -            tbi = extract64(tcr->raw_tcr, 38, 1);
> -        else
> -            tbi = extract64(tcr->raw_tcr, 37, 1);
> +        if (el == 3 || el == 2) {
> +            tbi = extract64(tcr->raw_tcr, 20, 1);
> +        } else {
> +            if (extract64(address, 55, 1)) {
> +                tbi = extract64(tcr->raw_tcr, 38, 1);
> +            } else {
> +                tbi = extract64(tcr->raw_tcr, 37, 1);
> +            }
> +        }
>          tbi *= 8;
>      }

The TTBR related parts of this patch are fine, but this section
feels a bit incomplete. The location of the TBI bit is not the
only thing about the TCR format that's different for TCR_EL2
and TCR_EL3. I think it would be better to move this hunk into
a separate patch that also fixes the other points the TODO comment
mentions (including the other parts of the function that access
raw_tcr bitfields).

-- PMM

Re: [Qemu-devel] [[PATCH] 5/7] target-arm: Add TTBR regime function and use

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

On Thu, Apr 16, 2015 at 1:03 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Add a utility function for choosing the correct TTBR system register
> based on
> > the specified MMU index. Add use of function on physical address lookup.
>
> > @@ -5376,20 +5390,26 @@ static int get_phys_addr_lpae(CPUARMState *env,
> target_ulong address,
> >      int32_t tbi = 0;
> >      TCR *tcr = regime_tcr(env, mmu_idx);
> >      int ap, ns, xn, pxn;
> > +    uint32_t el = regime_el(env, mmu_idx);
> >
> >      /* TODO:
> >       * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
> > -     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
> > -     * and VTCR_EL2, or the fact that those regimes don't have a split
> > +     * it doesn't handle the different format TCR for and VTCR_EL2,
> > +     * or the fact that those regimes don't have a split
> >       * TTBR0/TTBR1. Attribute and permission bit handling should also
> >       * be checked when adding support for those page table walks.
> >       */
> > -    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
> > +    if (arm_el_is_aa64(env, el)) {
> >          va_size = 64;
> > -        if (extract64(address, 55, 1))
> > -            tbi = extract64(tcr->raw_tcr, 38, 1);
> > -        else
> > -            tbi = extract64(tcr->raw_tcr, 37, 1);
> > +        if (el == 3 || el == 2) {
> > +            tbi = extract64(tcr->raw_tcr, 20, 1);
> > +        } else {
> > +            if (extract64(address, 55, 1)) {
> > +                tbi = extract64(tcr->raw_tcr, 38, 1);
> > +            } else {
> > +                tbi = extract64(tcr->raw_tcr, 37, 1);
> > +            }
> > +        }
> >          tbi *= 8;
> >      }
>
> The TTBR related parts of this patch are fine, but this section
> feels a bit incomplete. The location of the TBI bit is not the
> only thing about the TCR format that's different for TCR_EL2
> and TCR_EL3. I think it would be better to move this hunk into
> a separate patch that also fixes the other points the TODO comment
> mentions (including the other parts of the function that access
> raw_tcr bitfields).
>
> ​I broke up the TTBR and TCR changes into separate patches.

I'll add support for the lack of TTBR1 in EL2/3 but the rest (VTCR2 and
certain attributes (shareability & physical addr size) should be left as
separate patches as they are unrelated to EL3 and already unsupported.




> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 3810 bytes --]

Re: [Qemu-devel] [[PATCH] 5/7] target-arm: Add TTBR regime function and use

[Sergey Fedorov]

On 27.03.2015 12:10, Greg Bellows wrote:
> Add a utility function for choosing the correct TTBR system register based on
> the specified MMU index. Add use of function on physical address lookup.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/helper.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 00b457a..13fdf02 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4892,6 +4892,21 @@ static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
>      return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
>  }
>  
> +/* Return the TTBR associated with this translation regime */
> +static inline uint32_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
> +                                   int ttbrn)

Should return uint64_t.

> +{
> +    if (mmu_idx == ARMMMUIdx_S2NS) {
> +        /* TODO: return VTTBR_EL2 */
> +        g_assert_not_reached();
> +    }
> +    if (ttbrn == 0) {
> +        return env->cp15.ttbr0_el[regime_el(env, mmu_idx)];
> +    } else {
> +        return env->cp15.ttbr1_el[regime_el(env, mmu_idx)];
> +    }
> +}
> +
>  /* Return true if the translation regime is using LPAE format page tables */
>  static inline bool regime_using_lpae_format(CPUARMState *env,
>                                              ARMMMUIdx mmu_idx)
> @@ -5090,7 +5105,6 @@ static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
>                                       uint32_t *table, uint32_t address)
>  {
>      /* Note that we can only get here for an AArch32 PL0/PL1 lookup */
> -    int el = regime_el(env, mmu_idx);
>      TCR *tcr = regime_tcr(env, mmu_idx);
>  
>      if (address & tcr->mask) {
> @@ -5098,13 +5112,13 @@ static bool get_level1_table_address(CPUARMState *env, ARMMMUIdx mmu_idx,
>              /* Translation table walk disabled for TTBR1 */
>              return false;
>          }
> -        *table = env->cp15.ttbr1_el[el] & 0xffffc000;
> +        *table = regime_ttbr(env, mmu_idx, 1) & 0xffffc000;
>      } else {
>          if (tcr->raw_tcr & TTBCR_PD0) {
>              /* Translation table walk disabled for TTBR0 */
>              return false;
>          }
> -        *table = env->cp15.ttbr0_el[el] & tcr->base_mask;
> +        *table = regime_ttbr(env, mmu_idx, 0) & tcr->base_mask;
>      }
>      *table |= (address >> 18) & 0x3ffc;
>      return true;
> @@ -5376,20 +5390,26 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      int32_t tbi = 0;
>      TCR *tcr = regime_tcr(env, mmu_idx);
>      int ap, ns, xn, pxn;
> +    uint32_t el = regime_el(env, mmu_idx);
>  
>      /* TODO:
>       * This code assumes we're either a 64-bit EL1 or a 32-bit PL1;
> -     * it doesn't handle the different format TCR for TCR_EL2, TCR_EL3,
> -     * and VTCR_EL2, or the fact that those regimes don't have a split
> +     * it doesn't handle the different format TCR for and VTCR_EL2,
> +     * or the fact that those regimes don't have a split
>       * TTBR0/TTBR1. Attribute and permission bit handling should also
>       * be checked when adding support for those page table walks.
>       */
> -    if (arm_el_is_aa64(env, regime_el(env, mmu_idx))) {
> +    if (arm_el_is_aa64(env, el)) {
>          va_size = 64;
> -        if (extract64(address, 55, 1))
> -            tbi = extract64(tcr->raw_tcr, 38, 1);
> -        else
> -            tbi = extract64(tcr->raw_tcr, 37, 1);
> +        if (el == 3 || el == 2) {
> +            tbi = extract64(tcr->raw_tcr, 20, 1);
> +        } else {
> +            if (extract64(address, 55, 1)) {
> +                tbi = extract64(tcr->raw_tcr, 38, 1);
> +            } else {
> +                tbi = extract64(tcr->raw_tcr, 37, 1);
> +            }
> +        }
>          tbi *= 8;
>      }
>  
> @@ -5434,7 +5454,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>       * we will always flush the TLB any time the ASID is changed).
>       */
>      if (ttbr_select == 0) {
> -        ttbr = A32_BANKED_CURRENT_REG_GET(env, ttbr0);
> +        ttbr = regime_ttbr(env, mmu_idx, 0);
>          epd = extract32(tcr->raw_tcr, 7, 1);
>          tsz = t0sz;
>  
> @@ -5446,7 +5466,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>              granule_sz = 11;
>          }
>      } else {
> -        ttbr = A32_BANKED_CURRENT_REG_GET(env, ttbr1);
> +        ttbr = regime_ttbr(env, mmu_idx, 1);
>          epd = extract32(tcr->raw_tcr, 23, 1);
>          tsz = t1sz;
>  

[Qemu-devel] [[PATCH] 6/7] target-arm: Add WFx syndrome function

[Greg Bellows]

Adds a utility function for creating a WFx exception syndrome

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/internals.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index bb171a7..8dc2e2b 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -344,6 +344,12 @@ static inline uint32_t syn_breakpoint(int same_el)
         | ARM_EL_IL | 0x22;
 }
 
+static inline uint32_t syn_wfx(int cv, int cond, int ti)
+{
+    return (EC_WFX_TRAP << ARM_EL_EC_SHIFT) |
+           (cv << 24) | (cond << 20) | ti;
+}
+
 /* Update a QEMU watchpoint based on the information the guest has set in the
  * DBGWCR<n>_EL1 and DBGWVR<n>_EL1 registers.
  */
-- 
1.8.3.2

Re: [Qemu-devel] [[PATCH] 6/7] target-arm: Add WFx syndrome function

[Peter Maydell]

On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> Adds a utility function for creating a WFx exception syndrome
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [[PATCH] 7/7] target-arm: Add WFx instruction trap support

[Greg Bellows]

Add support for trapping WFI and WFE instructions to the proper EL when
SCTLR/SCR/HCR settings apply.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 target-arm/op_helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index aa175b5..d7e734d 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -209,23 +209,90 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t x, uint32_t shift)
     return res;
 }
 
+static inline uint32_t check_wfx_trap(CPUARMState *env, bool is_wfe)
+{
+    int cur_el = arm_current_el(env), el;
+    uint32_t target_el = 0;
+    uint64_t mask;
+
+    /* Check whether any EL controls above us trap WFx instructions */
+    for (el = cur_el + 1; el <= 3; el++) {
+        switch (el) {
+        case 1:
+            mask = (is_wfe) ? SCTLR_nTWE : SCTLR_nTWI;
+            /* Secure EL1 is only valid in AArch64.  If EL0 is AArch64 then so
+             * must be EL1.
+             */
+            if (arm_el_is_aa64(env, 1)) {
+                if ((env->cp15.sctlr_el[1] & mask) == 0) {
+                    target_el = el;
+                }
+            } else if (arm_feature(env, ARM_FEATURE_V8) &&
+                       !arm_is_secure(env)) {
+                /* SCTLR WFx SCTLR trap bits only exist in ARMv8 */
+                if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & mask) == 0) {
+                    target_el = el;
+                }
+            }
+            break;
+        case 2:
+            if (arm_feature(env, ARM_FEATURE_EL2) && !arm_is_secure(env)) {
+                mask = (is_wfe) ? HCR_TWE : HCR_TWI;
+                if ((env->cp15.hcr_el2 & mask) == mask) {
+                    target_el = el;
+                }
+            }
+            break;
+        case 3:
+            if (arm_feature(env, ARM_FEATURE_EL3) &&
+                arm_feature(env, ARM_FEATURE_V8)) {
+                mask = (is_wfe) ? SCR_TWE : SCR_TWI;
+                if ((env->cp15.scr_el3 & mask) == mask) {
+                    target_el = el;
+                }
+            }
+            break;
+        }
+    }
+
+    return target_el;
+}
+
 void HELPER(wfi)(CPUARMState *env)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
-
-    cs->exception_index = EXCP_HLT;
-    cs->halted = 1;
+    uint32_t target_el = 0;
+
+    target_el = check_wfx_trap(env, false);
+    if (target_el) {
+        cs->exception_index = EXCP_UDEF;
+        env->exception.syndrome = syn_wfx(1, 0xe, false);
+        env->exception.target_el = target_el;
+        env->pc -= 4;
+    } else {
+        cs->exception_index = EXCP_HLT;
+        cs->halted = 1;
+    }
     cpu_loop_exit(cs);
 }
 
 void HELPER(wfe)(CPUARMState *env)
 {
     CPUState *cs = CPU(arm_env_get_cpu(env));
+    uint32_t target_el = 0;
 
     /* Don't actually halt the CPU, just yield back to top
      * level loop
      */
-    cs->exception_index = EXCP_YIELD;
+    target_el = check_wfx_trap(env, true);
+    if (target_el) {
+        cs->exception_index = EXCP_UDEF;
+        env->exception.syndrome = syn_wfx(1, 0xe, true);
+        env->exception.target_el = target_el;
+        env->pc -= 4;
+    } else {
+        cs->exception_index = EXCP_YIELD;
+    }
     cpu_loop_exit(cs);
 }
 
-- 
1.8.3.2

Re: [Qemu-devel] [[PATCH] 7/7] target-arm: Add WFx instruction trap support

[Peter Maydell]

On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> Add support for trapping WFI and WFE instructions to the proper EL when
> SCTLR/SCR/HCR settings apply.
>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  target-arm/op_helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 71 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index aa175b5..d7e734d 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -209,23 +209,90 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t x, uint32_t shift)
>      return res;
>  }
>
> +static inline uint32_t check_wfx_trap(CPUARMState *env, bool is_wfe)

Why uint32_t rather than int?

A comment that gave the semantics of the return value would be nice.

> +{
> +    int cur_el = arm_current_el(env), el;
> +    uint32_t target_el = 0;
> +    uint64_t mask;
> +
> +    /* Check whether any EL controls above us trap WFx instructions */
> +    for (el = cur_el + 1; el <= 3; el++) {
> +        switch (el) {
> +        case 1:
> +            mask = (is_wfe) ? SCTLR_nTWE : SCTLR_nTWI;

I don't think the brackets round is_wfe are needed.

> +            /* Secure EL1 is only valid in AArch64.  If EL0 is AArch64 then so
> +             * must be EL1.
> +             */

I'm not clear how this comment relates to the code...

> +            if (arm_el_is_aa64(env, 1)) {
> +                if ((env->cp15.sctlr_el[1] & mask) == 0) {
> +                    target_el = el;
> +                }
> +            } else if (arm_feature(env, ARM_FEATURE_V8) &&
> +                       !arm_is_secure(env)) {

Why the !arm_is_secure() check? WFI/WFE at Secure-PL0 should trap
to Secure-PL1 if these bits are clear, I think.

> +                /* SCTLR WFx SCTLR trap bits only exist in ARMv8 */
> +                if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & mask) == 0) {
> +                    target_el = el;
> +                }
> +            }
> +            break;
> +        case 2:
> +            if (arm_feature(env, ARM_FEATURE_EL2) && !arm_is_secure(env)) {
> +                mask = (is_wfe) ? HCR_TWE : HCR_TWI;
> +                if ((env->cp15.hcr_el2 & mask) == mask) {

Just "if (val & mask)" is fine.

> +                    target_el = el;
> +                }
> +            }
> +            break;
> +        case 3:
> +            if (arm_feature(env, ARM_FEATURE_EL3) &&
> +                arm_feature(env, ARM_FEATURE_V8)) {
> +                mask = (is_wfe) ? SCR_TWE : SCR_TWI;
> +                if ((env->cp15.scr_el3 & mask) == mask) {
> +                    target_el = el;
> +                }
> +            }
> +            break;

The loop-and-switch structure here seems a bit odd; why
not just have three successive if() statements?
Also, I think you have the exception priority wrong -- taking
a trap to EL1 should take priority over trapping to EL2, which
in turn is prioritised over trapping to EL3. So you want
something like

    if (el < 1) {
        if (conditions for trap to EL1) {
            return 1;
        }
    }
    if (el <= 2 && conditions for trap to EL2) {
        return 2;
    }
    if (el <= 3 && conditions for trap to EL3) {
        return 3;
    }
    return 0;


> +        }
> +    }
> +
> +    return target_el;
> +}
> +
>  void HELPER(wfi)(CPUARMState *env)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
> -
> -    cs->exception_index = EXCP_HLT;
> -    cs->halted = 1;
> +    uint32_t target_el = 0;

No point zero-initializing this variable, we set it
in the following line anyway.

> +
> +    target_el = check_wfx_trap(env, false);
> +    if (target_el) {
> +        cs->exception_index = EXCP_UDEF;
> +        env->exception.syndrome = syn_wfx(1, 0xe, false);

The third argument here is an int, not a bool.

> +        env->exception.target_el = target_el;
> +        env->pc -= 4;
> +    } else {
> +        cs->exception_index = EXCP_HLT;
> +        cs->halted = 1;
> +    }
>      cpu_loop_exit(cs);
>  }
>
>  void HELPER(wfe)(CPUARMState *env)
>  {
>      CPUState *cs = CPU(arm_env_get_cpu(env));
> +    uint32_t target_el = 0;

Ditto.

>
>      /* Don't actually halt the CPU, just yield back to top
>       * level loop
>       */
> -    cs->exception_index = EXCP_YIELD;
> +    target_el = check_wfx_trap(env, true);
> +    if (target_el) {
> +        cs->exception_index = EXCP_UDEF;
> +        env->exception.syndrome = syn_wfx(1, 0xe, true);
> +        env->exception.target_el = target_el;
> +        env->pc -= 4;
> +    } else {
> +        cs->exception_index = EXCP_YIELD;
> +    }
>      cpu_loop_exit(cs);
>  }
>
> --
> 1.8.3.2
>

thanks
-- PMM

Re: [Qemu-devel] [[PATCH] 7/7] target-arm: Add WFx instruction trap support

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 5803 bytes --]

On Thu, Apr 16, 2015 at 1:22 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
> > Add support for trapping WFI and WFE instructions to the proper EL when
> > SCTLR/SCR/HCR settings apply.
> >
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> > ---
> >  target-arm/op_helper.c | 75
> +++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 71 insertions(+), 4 deletions(-)
> >
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index aa175b5..d7e734d 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -209,23 +209,90 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t
> x, uint32_t shift)
> >      return res;
> >  }
> >
> > +static inline uint32_t check_wfx_trap(CPUARMState *env, bool is_wfe)
>
> Why uint32_t rather than int?
>

​EL can't be negative so this made sense.​


>
> A comment that gave the semantics of the return value would be nice.
>

​Yeah, may have helped with the above?
​


>
> > +{
> > +    int cur_el = arm_current_el(env), el;
> > +    uint32_t target_el = 0;
> > +    uint64_t mask;
> > +
> > +    /* Check whether any EL controls above us trap WFx instructions */
> > +    for (el = cur_el + 1; el <= 3; el++) {
> > +        switch (el) {
> > +        case 1:
> > +            mask = (is_wfe) ? SCTLR_nTWE : SCTLR_nTWI;
>
> I don't think the brackets round is_wfe are needed.
>

​I'll remove
​


>
> > +            /* Secure EL1 is only valid in AArch64.  If EL0 is AArch64
> then so
> > +             * must be EL1.
> > +             */
>
> I'm not clear how this comment relates to the code...
>

​Me either :-)  Probably made sense when I wrote it.​


>
> > +            if (arm_el_is_aa64(env, 1)) {
> > +                if ((env->cp15.sctlr_el[1] & mask) == 0) {
> > +                    target_el = el;
> > +                }
> > +            } else if (arm_feature(env, ARM_FEATURE_V8) &&
> > +                       !arm_is_secure(env)) {
>
> Why the !arm_is_secure() check? WFI/WFE at Secure-PL0 should trap
> to Secure-PL1 if these bits are clear, I think.
>

It must have been left over from when I simplified but you are right it
should not be there.  Removed in v2


>
> > +                /* SCTLR WFx SCTLR trap bits only exist in ARMv8 */
> > +                if ((A32_BANKED_CURRENT_REG_GET(env, sctlr) & mask) ==
> 0) {
> > +                    target_el = el;
> > +                }
> > +            }
> > +            break;
> > +        case 2:
> > +            if (arm_feature(env, ARM_FEATURE_EL2) &&
> !arm_is_secure(env)) {
> > +                mask = (is_wfe) ? HCR_TWE : HCR_TWI;
> > +                if ((env->cp15.hcr_el2 & mask) == mask) {
>
> Just "if (val & mask)" is fine.
>

​Fixed
​


>
> > +                    target_el = el;
> > +                }
> > +            }
> > +            break;
> > +        case 3:
> > +            if (arm_feature(env, ARM_FEATURE_EL3) &&
> > +                arm_feature(env, ARM_FEATURE_V8)) {
> > +                mask = (is_wfe) ? SCR_TWE : SCR_TWI;
> > +                if ((env->cp15.scr_el3 & mask) == mask) {
> > +                    target_el = el;
> > +                }
> > +            }
> > +            break;
>
> The loop-and-switch structure here seems a bit odd; why
> not just have three successive if() statements?
>

Yeah, the loop came out of me simplifying the spec pseudo code, but it is
excessive.  Will fix in v2.


> Also, I think you have the exception priority wrong -- taking
> a trap to EL1 should take priority over trapping to EL2, which
> in turn is prioritised over trapping to EL3. So you want
> something like
>
>     if (el < 1) {
>         if (conditions for trap to EL1) {
>             return 1;
>         }
>     }
>     if (el <= 2 && conditions for trap to EL2) {
>         return 2;
>     }
>     if (el <= 3 && conditions for trap to EL3) {
>         return 3;
>     }
>     return 0;
>

> > +        }
> > +    }
> > +
> > +    return target_el;
> > +}
> > +
> >  void HELPER(wfi)(CPUARMState *env)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> > -
> > -    cs->exception_index = EXCP_HLT;
> > -    cs->halted = 1;
> > +    uint32_t target_el = 0;
>
> No point zero-initializing this variable, we set it
> in the following line anyway.
>
> > +
> > +    target_el = check_wfx_trap(env, false);
> > +    if (target_el) {
> > +        cs->exception_index = EXCP_UDEF;
> > +        env->exception.syndrome = syn_wfx(1, 0xe, false);
>
> The third argument here is an int, not a bool.
>

​Changed function to take bool.​


>
> > +        env->exception.target_el = target_el;
> > +        env->pc -= 4;
> > +    } else {
> > +        cs->exception_index = EXCP_HLT;
> > +        cs->halted = 1;
> > +    }
> >      cpu_loop_exit(cs);
> >  }
> >
> >  void HELPER(wfe)(CPUARMState *env)
> >  {
> >      CPUState *cs = CPU(arm_env_get_cpu(env));
> > +    uint32_t target_el = 0;
>
> Ditto.
>
> >
> >      /* Don't actually halt the CPU, just yield back to top
> >       * level loop
> >       */
> > -    cs->exception_index = EXCP_YIELD;
> > +    target_el = check_wfx_trap(env, true);
> > +    if (target_el) {
> > +        cs->exception_index = EXCP_UDEF;
> > +        env->exception.syndrome = syn_wfx(1, 0xe, true);
> > +        env->exception.target_el = target_el;
> > +        env->pc -= 4;
> > +    } else {
> > +        cs->exception_index = EXCP_YIELD;
> > +    }
> >      cpu_loop_exit(cs);
> >  }
> >
> > --
> > 1.8.3.2
> >
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 9905 bytes --]

Re: [Qemu-devel] [[PATCH] 7/7] target-arm: Add WFx instruction trap support

[Peter Maydell]

On 17 April 2015 at 16:47, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On Thu, Apr 16, 2015 at 1:22 PM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 27 March 2015 at 19:10, Greg Bellows <greg.bellows@linaro.org> wrote:
>> > Add support for trapping WFI and WFE instructions to the proper EL when
>> > SCTLR/SCR/HCR settings apply.
>> >
>> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>> > ---
>> >  target-arm/op_helper.c | 75
>> > +++++++++++++++++++++++++++++++++++++++++++++++---
>> >  1 file changed, 71 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
>> > index aa175b5..d7e734d 100644
>> > --- a/target-arm/op_helper.c
>> > +++ b/target-arm/op_helper.c
>> > @@ -209,23 +209,90 @@ uint32_t HELPER(usat16)(CPUARMState *env, uint32_t
>> > x, uint32_t shift)
>> >      return res;
>> >  }
>> >
>> > +static inline uint32_t check_wfx_trap(CPUARMState *env, bool is_wfe)
>>
>> Why uint32_t rather than int?
>
>
> EL can't be negative so this made sense.

Mostly our existing functions that return an EL use 'int'
(eg arm_current_el()), so my thinking was that 'int' would be
more in line with those.

-- PMM