[Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[Peter Maydell]

This patchset corrects a number of places in the ARM translation code
which were generating code which was dependent on values in the CPUState
structure which might change at runtime. This is a bad idea for two
reasons. Firstly, we might try to reuse the generated code later when
the assumptions baked into the generated code were no longer valid.
Secondly, we might try to retranslate the same TB (eg when an exception
results in our calling cpu_restore_state()) but get different generated
code, which could result in qemu crashing.

Bug https://bugs.launchpad.net/bugs/604872 is a particular example
of the latter case involving the IT bits; this patchset fixes that bug.

I believe that this patchset deals with all the problems. Remaining
CPUState fields referred to in translate.c are either constant after
system init or trigger flushing of affected TBs when they are changed.

Peter Maydell (7):
  target-arm: Don't generate code specific to current CPU mode for SRS
  target-arm: Translate with VFP-enabled from TB flags, not CPUState
  target-arm: Translate with VFP len/stride from TB flags, not CPUState
  target-arm: Translate with Thumb state from TB flags, not CPUState
  target-arm: Translate with condexec bits from TB flags, not CPUState
  target-arm: Set privileged bit in TB flags correctly for M profile
  target-arm: Translate with user-state from TB flags, not CPUState

 target-arm/cpu.h       |   17 +++++++++-
 target-arm/helper.c    |   12 +++++-
 target-arm/translate.c |   88 ++++++++++++++++++-----------------------------
 3 files changed, 60 insertions(+), 57 deletions(-)

[Qemu-devel] [PATCH 1/7] target-arm: Don't generate code specific to current CPU mode for SRS

[Peter Maydell]

When translating the SRS instruction, handle the "store registers
to stack of current mode" case in the helper function rather than
inline. This means the generated code does not make assumptions
about the current CPU mode which might not be valid when the TB
is executed later.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c    |   12 ++++++++++--
 target-arm/translate.c |   46 +++++++++++++++-------------------------------
 2 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 705b99f..f08e09e 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1846,12 +1846,20 @@ bad_reg:
 
 void HELPER(set_r13_banked)(CPUState *env, uint32_t mode, uint32_t val)
 {
-    env->banked_r13[bank_number(mode)] = val;
+    if ((env->uncached_cpsr & CPSR_M) == mode) {
+        env->regs[13] = val;
+    } else {
+        env->banked_r13[bank_number(mode)] = val;
+    }
 }
 
 uint32_t HELPER(get_r13_banked)(CPUState *env, uint32_t mode)
 {
-    return env->banked_r13[bank_number(mode)];
+    if ((env->uncached_cpsr & CPSR_M) == mode) {
+        return env->regs[13];
+    } else {
+        return env->banked_r13[bank_number(mode)];
+    }
 }
 
 uint32_t HELPER(v7m_mrs)(CPUState *env, uint32_t reg)
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 2ce82f3..c391398 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -6101,14 +6101,10 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
                 goto illegal_op;
             ARCH(6);
             op1 = (insn & 0x1f);
-            if (op1 == (env->uncached_cpsr & CPSR_M)) {
-                addr = load_reg(s, 13);
-            } else {
-                addr = new_tmp();
-                tmp = tcg_const_i32(op1);
-                gen_helper_get_r13_banked(addr, cpu_env, tmp);
-                tcg_temp_free_i32(tmp);
-            }
+            addr = new_tmp();
+            tmp = tcg_const_i32(op1);
+            gen_helper_get_r13_banked(addr, cpu_env, tmp);
+            tcg_temp_free_i32(tmp);
             i = (insn >> 23) & 3;
             switch (i) {
             case 0: offset = -4; break; /* DA */
@@ -6135,14 +6131,10 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
                 }
                 if (offset)
                     tcg_gen_addi_i32(addr, addr, offset);
-                if (op1 == (env->uncached_cpsr & CPSR_M)) {
-                    store_reg(s, 13, addr);
-                } else {
-                    tmp = tcg_const_i32(op1);
-                    gen_helper_set_r13_banked(cpu_env, tmp, addr);
-                    tcg_temp_free_i32(tmp);
-                    dead_tmp(addr);
-                }
+                tmp = tcg_const_i32(op1);
+                gen_helper_set_r13_banked(cpu_env, tmp, addr);
+                tcg_temp_free_i32(tmp);
+                dead_tmp(addr);
             } else {
                 dead_tmp(addr);
             }
@@ -7554,14 +7546,10 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
                 } else {
                     /* srs */
                     op = (insn & 0x1f);
-                    if (op == (env->uncached_cpsr & CPSR_M)) {
-                        addr = load_reg(s, 13);
-                    } else {
-                        addr = new_tmp();
-                        tmp = tcg_const_i32(op);
-                        gen_helper_get_r13_banked(addr, cpu_env, tmp);
-                        tcg_temp_free_i32(tmp);
-                    }
+                    addr = new_tmp();
+                    tmp = tcg_const_i32(op);
+                    gen_helper_get_r13_banked(addr, cpu_env, tmp);
+                    tcg_temp_free_i32(tmp);
                     if ((insn & (1 << 24)) == 0) {
                         tcg_gen_addi_i32(addr, addr, -8);
                     }
@@ -7577,13 +7565,9 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
                         } else {
                             tcg_gen_addi_i32(addr, addr, 4);
                         }
-                        if (op == (env->uncached_cpsr & CPSR_M)) {
-                            store_reg(s, 13, addr);
-                        } else {
-                            tmp = tcg_const_i32(op);
-                            gen_helper_set_r13_banked(cpu_env, tmp, addr);
-                            tcg_temp_free_i32(tmp);
-                        }
+                        tmp = tcg_const_i32(op);
+                        gen_helper_set_r13_banked(cpu_env, tmp, addr);
+                        tcg_temp_free_i32(tmp);
                     } else {
                         dead_tmp(addr);
                     }
-- 
1.6.3.3

[Qemu-devel] [PATCH 2/7] target-arm: Translate with VFP-enabled from TB flags, not CPUState

[Peter Maydell]

When translating code, whether the VFP unit is enabled for this TB
is stored in a bit in the TB flags. Use this rather than incorrectly
reading the FPEXC from the CPUState passed to translation.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index c391398..d10b484 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -59,6 +59,7 @@ typedef struct DisasContext {
 #if !defined(CONFIG_USER_ONLY)
     int user;
 #endif
+    int vfp_enabled;
 } DisasContext;
 
 #if defined(CONFIG_USER_ONLY)
@@ -2603,12 +2604,6 @@ static void gen_vfp_msr(TCGv tmp)
     dead_tmp(tmp);
 }
 
-static inline int
-vfp_enabled(CPUState * env)
-{
-    return ((env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)) != 0);
-}
-
 static void gen_neon_dup_u8(TCGv var, int shift)
 {
     TCGv tmp = new_tmp();
@@ -2653,7 +2648,7 @@ static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
     if (!arm_feature(env, ARM_FEATURE_VFP))
         return 1;
 
-    if (!vfp_enabled(env)) {
+    if (!s->vfp_enabled) {
         /* VFP disabled.  Only allow fmxr/fmrx to/from some control regs.  */
         if ((insn & 0x0fe00fff) != 0x0ee00a10)
             return 1;
@@ -3804,7 +3799,7 @@ static int disas_neon_ls_insn(CPUState * env, DisasContext *s, uint32_t insn)
     TCGv tmp2;
     TCGv_i64 tmp64;
 
-    if (!vfp_enabled(env))
+    if (!s->vfp_enabled)
       return 1;
     VFP_DREG_D(rd, insn);
     rn = (insn >> 16) & 0xf;
@@ -4199,7 +4194,7 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
     TCGv tmp, tmp2, tmp3, tmp4, tmp5;
     TCGv_i64 tmp64;
 
-    if (!vfp_enabled(env))
+    if (!s->vfp_enabled)
       return 1;
     q = (insn & (1 << 6)) != 0;
     u = (insn >> 24) & 1;
@@ -9087,6 +9082,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
         dc->user = (env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR;
     }
 #endif
+    dc->vfp_enabled = ((tb->flags & (1 << 7)) != 0);
     cpu_F0s = tcg_temp_new_i32();
     cpu_F1s = tcg_temp_new_i32();
     cpu_F0d = tcg_temp_new_i64();
-- 
1.6.3.3

[Qemu-devel] [PATCH 3/7] target-arm: Translate with VFP len/stride from TB flags, not CPUState

[Peter Maydell]

When translating, the VFP vector length and stride for this TB are encoded
in the TB flags; the CPUState copies may be different and must not be used.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index d10b484..10419bf 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -60,6 +60,8 @@ typedef struct DisasContext {
     int user;
 #endif
     int vfp_enabled;
+    int vec_len;
+    int vec_stride;
 } DisasContext;
 
 #if defined(CONFIG_USER_ONLY)
@@ -2895,7 +2897,7 @@ static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 rm = VFP_SREG_M(insn);
             }
 
-            veclen = env->vfp.vec_len;
+            veclen = s->vec_len;
             if (op == 15 && rn > 3)
                 veclen = 0;
 
@@ -2916,9 +2918,9 @@ static int disas_vfp_insn(CPUState * env, DisasContext *s, uint32_t insn)
                     veclen = 0;
                 } else {
                     if (dp)
-                        delta_d = (env->vfp.vec_stride >> 1) + 1;
+                        delta_d = (s->vec_stride >> 1) + 1;
                     else
-                        delta_d = env->vfp.vec_stride + 1;
+                        delta_d = s->vec_stride + 1;
 
                     if ((rm & bank_mask) == 0) {
                         /* mixed scalar/vector */
@@ -9083,6 +9085,8 @@ static inline void gen_intermediate_code_internal(CPUState *env,
     }
 #endif
     dc->vfp_enabled = ((tb->flags & (1 << 7)) != 0);
+    dc->vec_len = (tb->flags >> 1) & 7;
+    dc->vec_stride = (tb->flags >> 4) & 3;
     cpu_F0s = tcg_temp_new_i32();
     cpu_F1s = tcg_temp_new_i32();
     cpu_F0d = tcg_temp_new_i64();
-- 
1.6.3.3

[Qemu-devel] [PATCH 4/7] target-arm: Translate with Thumb state from TB flags, not CPUState

[Peter Maydell]

The Thumb/ARM state for the TB being translated should come from
the TB flags, not the CPUState.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 10419bf..723961a 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9074,7 +9074,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
     dc->pc = pc_start;
     dc->singlestep_enabled = env->singlestep_enabled;
     dc->condjmp = 0;
-    dc->thumb = env->thumb;
+    dc->thumb = tb->flags & 1;
     dc->condexec_mask = (env->condexec_bits & 0xf) << 1;
     dc->condexec_cond = env->condexec_bits >> 4;
 #if !defined(CONFIG_USER_ONLY)
@@ -9161,7 +9161,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
         if (num_insns + 1 == max_insns && (tb->cflags & CF_LAST_IO))
             gen_io_start();
 
-        if (env->thumb) {
+        if (dc->thumb) {
             disas_thumb_insn(env, dc);
             if (dc->condexec_mask) {
                 dc->condexec_cond = (dc->condexec_cond & 0xe)
@@ -9275,7 +9275,7 @@ done_generating:
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
         qemu_log("----------------\n");
         qemu_log("IN: %s\n", lookup_symbol(pc_start));
-        log_target_disas(pc_start, dc->pc - pc_start, env->thumb);
+        log_target_disas(pc_start, dc->pc - pc_start, dc->thumb);
         qemu_log("\n");
     }
 #endif
-- 
1.6.3.3

[Qemu-devel] [PATCH 5/7] target-arm: Translate with condexec bits from TB flags, not CPUState

[Peter Maydell]

When translating, the condexec bits for the TB are in the TB flags;
the CPUState condexec bits may be different.

This patch fixes https://bugs.launchpad.net/bugs/604872 where we might
segfault if we took an exception in the middle of a TB with an IT
block, because when we came to retranslate in cpu_restore_state()
the CPUState condexec bits would have advanced compared to the start
of the TB and we would generate different (wrong) code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 723961a..7d042ee 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9075,8 +9075,8 @@ static inline void gen_intermediate_code_internal(CPUState *env,
     dc->singlestep_enabled = env->singlestep_enabled;
     dc->condjmp = 0;
     dc->thumb = tb->flags & 1;
-    dc->condexec_mask = (env->condexec_bits & 0xf) << 1;
-    dc->condexec_cond = env->condexec_bits >> 4;
+    dc->condexec_mask = (tb->flags & 0xf00) >> 7;
+    dc->condexec_cond = (tb->flags & 0xf000) >> 12;
 #if !defined(CONFIG_USER_ONLY)
     if (IS_M(env)) {
         dc->user = ((env->v7m.exception == 0) && (env->v7m.control & 1));
@@ -9105,7 +9105,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
     gen_icount_start();
     /* Reset the conditional execution bits immediately. This avoids
        complications trying to do it at the end of the block.  */
-    if (env->condexec_bits)
+    if (dc->condexec_mask || dc->condexec_cond)
       {
         TCGv tmp = new_tmp();
         tcg_gen_movi_i32(tmp, 0);
-- 
1.6.3.3

[Qemu-devel] [PATCH 6/7] target-arm: Set privileged bit in TB flags correctly for M profile

[Peter Maydell]

M profile ARM cores don't have a CPSR mode field. Set the bit in the
TB flags that indicates non-user mode correctly for these cores.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 340933e..3a2d141 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -443,12 +443,27 @@ static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
 static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
                                         target_ulong *cs_base, int *flags)
 {
+    int privmode;
     *pc = env->regs[15];
     *cs_base = 0;
+    /* flags word usage:
+     * [0] thumbstate
+     * [3..1] vec_len
+     * [5..4] vec_stride
+     * [6] privileged (ie not user) mode
+     * [7] VFP enable bit
+     * [15..8] condexec bits
+     */
     *flags = env->thumb | (env->vfp.vec_len << 1)
             | (env->vfp.vec_stride << 4) | (env->condexec_bits << 8);
-    if ((env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR)
+    if (arm_feature(env, ARM_FEATURE_M)) {
+        privmode = !((env->v7m.exception == 0) && (env->v7m.control & 1));
+    } else {
+        privmode = (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR;
+    }
+    if (privmode) {
         *flags |= (1 << 6);
+    }
     if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30))
         *flags |= (1 << 7);
 }
-- 
1.6.3.3

[Qemu-devel] [PATCH 7/7] target-arm: Translate with user-state from TB flags, not CPUState

[Peter Maydell]

When translating, get the user/priv state from the TB flags, not
the CPUState.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 7d042ee..289501d 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9078,11 +9078,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
     dc->condexec_mask = (tb->flags & 0xf00) >> 7;
     dc->condexec_cond = (tb->flags & 0xf000) >> 12;
 #if !defined(CONFIG_USER_ONLY)
-    if (IS_M(env)) {
-        dc->user = ((env->v7m.exception == 0) && (env->v7m.control & 1));
-    } else {
-        dc->user = (env->uncached_cpsr & 0x1f) == ARM_CPU_MODE_USR;
-    }
+    dc->user = ((tb->flags & (1 << 6)) == 0);
 #endif
     dc->vfp_enabled = ((tb->flags & (1 << 7)) != 0);
     dc->vec_len = (tb->flags >> 1) & 7;
-- 
1.6.3.3

Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[Aurelien Jarno]

Hi,

On Fri, Jan 07, 2011 at 03:06:27PM +0000, Peter Maydell wrote:
> This patchset corrects a number of places in the ARM translation code
> which were generating code which was dependent on values in the CPUState
> structure which might change at runtime. This is a bad idea for two
> reasons. Firstly, we might try to reuse the generated code later when
> the assumptions baked into the generated code were no longer valid.
> Secondly, we might try to retranslate the same TB (eg when an exception
> results in our calling cpu_restore_state()) but get different generated
> code, which could result in qemu crashing.
> 
> Bug https://bugs.launchpad.net/bugs/604872 is a particular example
> of the latter case involving the IT bits; this patchset fixes that bug.
> 
> I believe that this patchset deals with all the problems. Remaining
> CPUState fields referred to in translate.c are either constant after
> system init or trigger flushing of affected TBs when they are changed.
> 
> Peter Maydell (7):
>   target-arm: Don't generate code specific to current CPU mode for SRS
>   target-arm: Translate with VFP-enabled from TB flags, not CPUState
>   target-arm: Translate with VFP len/stride from TB flags, not CPUState
>   target-arm: Translate with Thumb state from TB flags, not CPUState
>   target-arm: Translate with condexec bits from TB flags, not CPUState
>   target-arm: Set privileged bit in TB flags correctly for M profile
>   target-arm: Translate with user-state from TB flags, not CPUState
> 
>  target-arm/cpu.h       |   17 +++++++++-
>  target-arm/helper.c    |   12 +++++-
>  target-arm/translate.c |   88 ++++++++++++++++++-----------------------------
>  3 files changed, 60 insertions(+), 57 deletions(-)
> 

Commenting here as it concerns all patches.

In overall I think it's the correct approach to fix the issue, this is
a really good cleanup. I have tested this patch series, and it clearly
improve armv7 support. However I am surprised it doesn't fix the issue 
mentioned in https://bugs.launchpad.net/qemu/+bug/581335 , which seems
to be the same issue. Executing the testcase still returns 1 instead of
0 on QEMU.

My other concern is about the definition of the individual bits in the 
flags. I have seen that you have tried to summarize the usage in the 
patch 6, but the masks and shifts are still duplicated in different 
files, which may leads to mistakes if the flags definition are changed.

Have you considered using #define as for example in the MIPS target?

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 1/7] target-arm: Don't generate code specific to current CPU mode for SRS

[Aurelien Jarno]

On Fri, Jan 07, 2011 at 03:06:28PM +0000, Peter Maydell wrote:
> When translating the SRS instruction, handle the "store registers
> to stack of current mode" case in the helper function rather than
> inline. This means the generated code does not make assumptions
> about the current CPU mode which might not be valid when the TB
> is executed later.

Have you considered using tb flags instead? On the other hand I am not
sure it will make a real difference.

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/helper.c    |   12 ++++++++++--
>  target-arm/translate.c |   46 +++++++++++++++-------------------------------
>  2 files changed, 25 insertions(+), 33 deletions(-)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 705b99f..f08e09e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1846,12 +1846,20 @@ bad_reg:
>  
>  void HELPER(set_r13_banked)(CPUState *env, uint32_t mode, uint32_t val)
>  {
> -    env->banked_r13[bank_number(mode)] = val;
> +    if ((env->uncached_cpsr & CPSR_M) == mode) {
> +        env->regs[13] = val;
> +    } else {
> +        env->banked_r13[bank_number(mode)] = val;
> +    }
>  }
>  
>  uint32_t HELPER(get_r13_banked)(CPUState *env, uint32_t mode)
>  {
> -    return env->banked_r13[bank_number(mode)];
> +    if ((env->uncached_cpsr & CPSR_M) == mode) {
> +        return env->regs[13];
> +    } else {
> +        return env->banked_r13[bank_number(mode)];
> +    }
>  }
>  
>  uint32_t HELPER(v7m_mrs)(CPUState *env, uint32_t reg)
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 2ce82f3..c391398 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -6101,14 +6101,10 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
>                  goto illegal_op;
>              ARCH(6);
>              op1 = (insn & 0x1f);
> -            if (op1 == (env->uncached_cpsr & CPSR_M)) {
> -                addr = load_reg(s, 13);
> -            } else {
> -                addr = new_tmp();
> -                tmp = tcg_const_i32(op1);
> -                gen_helper_get_r13_banked(addr, cpu_env, tmp);
> -                tcg_temp_free_i32(tmp);
> -            }
> +            addr = new_tmp();
> +            tmp = tcg_const_i32(op1);
> +            gen_helper_get_r13_banked(addr, cpu_env, tmp);
> +            tcg_temp_free_i32(tmp);
>              i = (insn >> 23) & 3;
>              switch (i) {
>              case 0: offset = -4; break; /* DA */
> @@ -6135,14 +6131,10 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
>                  }
>                  if (offset)
>                      tcg_gen_addi_i32(addr, addr, offset);
> -                if (op1 == (env->uncached_cpsr & CPSR_M)) {
> -                    store_reg(s, 13, addr);
> -                } else {
> -                    tmp = tcg_const_i32(op1);
> -                    gen_helper_set_r13_banked(cpu_env, tmp, addr);
> -                    tcg_temp_free_i32(tmp);
> -                    dead_tmp(addr);
> -                }
> +                tmp = tcg_const_i32(op1);
> +                gen_helper_set_r13_banked(cpu_env, tmp, addr);
> +                tcg_temp_free_i32(tmp);
> +                dead_tmp(addr);
>              } else {
>                  dead_tmp(addr);
>              }
> @@ -7554,14 +7546,10 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
>                  } else {
>                      /* srs */
>                      op = (insn & 0x1f);
> -                    if (op == (env->uncached_cpsr & CPSR_M)) {
> -                        addr = load_reg(s, 13);
> -                    } else {
> -                        addr = new_tmp();
> -                        tmp = tcg_const_i32(op);
> -                        gen_helper_get_r13_banked(addr, cpu_env, tmp);
> -                        tcg_temp_free_i32(tmp);
> -                    }
> +                    addr = new_tmp();
> +                    tmp = tcg_const_i32(op);
> +                    gen_helper_get_r13_banked(addr, cpu_env, tmp);
> +                    tcg_temp_free_i32(tmp);
>                      if ((insn & (1 << 24)) == 0) {
>                          tcg_gen_addi_i32(addr, addr, -8);
>                      }
> @@ -7577,13 +7565,9 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
>                          } else {
>                              tcg_gen_addi_i32(addr, addr, 4);
>                          }
> -                        if (op == (env->uncached_cpsr & CPSR_M)) {
> -                            store_reg(s, 13, addr);
> -                        } else {
> -                            tmp = tcg_const_i32(op);
> -                            gen_helper_set_r13_banked(cpu_env, tmp, addr);
> -                            tcg_temp_free_i32(tmp);
> -                        }
> +                        tmp = tcg_const_i32(op);
> +                        gen_helper_set_r13_banked(cpu_env, tmp, addr);
> +                        tcg_temp_free_i32(tmp);
>                      } else {
>                          dead_tmp(addr);
>                      }
> -- 
> 1.6.3.3
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[Peter Maydell]

On 7 January 2011 16:01, Aurelien Jarno <aurelien@aurel32.net> wrote:
> In overall I think it's the correct approach to fix the issue, this is
> a really good cleanup. I have tested this patch series, and it clearly
> improve armv7 support. However I am surprised it doesn't fix the issue
> mentioned in https://bugs.launchpad.net/qemu/+bug/581335 , which seems
> to be the same issue. Executing the testcase still returns 1 instead of
> 0 on QEMU.

I think that bug is a different problem, which is that when we take
an unexpected exception we don't correctly reset the condexec bits
when restarting. I think that the right way to fix that is for gen_pc_load()
to restore not just the PC but also the condexec bits (in the same way
that on MIPS we restore env->hflags).

I haven't quite managed to work that up into a patch yet, though
(figuring out exactly where we do and don't need to output code
to update the condexec bits is a bit tricky). I have a long plane flight
on Sunday though so might have another hack at it then :-)

-- PMM

Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[Peter Maydell]

On 7 January 2011 16:01, Aurelien Jarno <aurelien@aurel32.net> wrote:
> My other concern is about the definition of the individual bits in the
> flags. I have seen that you have tried to summarize the usage in the
> patch 6, but the masks and shifts are still duplicated in different
> files, which may leads to mistakes if the flags definition are changed.
>
> Have you considered using #define as for example in the MIPS target?

That might be a good plan. I'll put that into v2.

-- PMM

Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[David Turner]

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

For what is worth, this was fixed in the Android emulator with the following
patch:

http://android.git.kernel.org/?p=platform/external/qemu.git;a=commit;h=01e9608cb62901d13b330f851a260a2082e81a06

<http://android.git.kernel.org/?p=platform/external/qemu.git;a=commit;h=01e9608cb62901d13b330f851a260a2082e81a06>And
sorry, we don't have any time to adapt this to the upstream QEMU sources
right now.

Hope this helps

On Fri, Jan 7, 2011 at 5:12 PM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 7 January 2011 16:01, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > In overall I think it's the correct approach to fix the issue, this is
> > a really good cleanup. I have tested this patch series, and it clearly
> > improve armv7 support. However I am surprised it doesn't fix the issue
> > mentioned in https://bugs.launchpad.net/qemu/+bug/581335 , which seems
> > to be the same issue. Executing the testcase still returns 1 instead of
> > 0 on QEMU.
>
> I think that bug is a different problem, which is that when we take
> an unexpected exception we don't correctly reset the condexec bits
> when restarting. I think that the right way to fix that is for
> gen_pc_load()
> to restore not just the PC but also the condexec bits (in the same way
> that on MIPS we restore env->hflags).
>
> I haven't quite managed to work that up into a patch yet, though
> (figuring out exactly where we do and don't need to output code
> to update the condexec bits is a bit tricky). I have a long plane flight
> on Sunday though so might have another hack at it then :-)
>
> -- PMM
>
>

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

Re: [Qemu-devel] [PATCH 1/7] target-arm: Don't generate code specific to current CPU mode for SRS

[Peter Maydell]

On 7 January 2011 16:01, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Fri, Jan 07, 2011 at 03:06:28PM +0000, Peter Maydell wrote:
>> When translating the SRS instruction, handle the "store registers
>> to stack of current mode" case in the helper function rather than
>> inline. This means the generated code does not make assumptions
>> about the current CPU mode which might not be valid when the TB
>> is executed later.
>
> Have you considered using tb flags instead? On the other hand I am not
> sure it will make a real difference.

I thought about that, but:
(a) if we put the current mode into the tb->flags then TBs which
could previously have been shared between several modes now
have to be translated as separate TBs for each mode
(b) it would eat 5 bits of tb->flags and we only have 16 left
(c) SRS isn't a very commonly used instruction anyway (and
the overhead of taking the exception will dwarf the call out
to the helper for SRS)

-- PMM

Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[Peter Maydell]

On 7 January 2011 16:01, Aurelien Jarno <aurelien@aurel32.net> wrote:
> My other concern is about the definition of the individual bits in the
> flags. I have seen that you have tried to summarize the usage in the
> patch 6, but the masks and shifts are still duplicated in different
> files, which may leads to mistakes if the flags definition are changed.
>
> Have you considered using #define as for example in the MIPS target?

I'll put out a proper v2 patchset in a bit but to save a round,
are you happy with the following set of #defines?
(I'm going to drop the comment since the #defines give the
same info.)

/* Bit usage in the TB flags field: */
#define ARM_TBFLAG_THUMB_SHIFT 0
#define ARM_TBFLAG_THUMB_MASK (1 << ARM_TBFLAG_THUMB_SHIFT)
#define ARM_TBFLAG_VECLEN_SHIFT 1
#define ARM_TBFLAG_VECLEN_MASK (0x7 << ARM_TBFLAG_VECLEN_SHIFT)
#define ARM_TBFLAG_VECSTRIDE_SHIFT 4
#define ARM_TBFLAG_VECSTRIDE_MASK (0x3 << ARM_TBFLAG_VECSTRIDE_SHIFT)
#define ARM_TBFLAG_PRIV_SHIFT 6
#define ARM_TBFLAG_PRIV_MASK (1 << ARM_TBFLAG_PRIV_SHIFT)
#define ARM_TBFLAG_VFPEN_SHIFT 7
#define ARM_TBFLAG_VFPEN_MASK (1 << ARM_TBFLAG_VFPEN_SHIFT)
#define ARM_TBFLAG_CONDEXEC_SHIFT 8
#define ARM_TBFLAG_CONDEXEC_MASK (0xff << ARM_TBFLAG_CONDEXEC_SHIFT)

/* some convenience accessor macros */
#define ARM_TBFLAG_THUMB(F) \
    (((F) & ARM_TBFLAG_THUMB_MASK) >> ARM_TBFLAG_THUMB_SHIFT)
#define ARM_TBFLAG_VECLEN(F) \
    (((F) & ARM_TBFLAG_VECLEN_MASK) >> ARM_TBFLAG_VECLEN_SHIFT)
#define ARM_TBFLAG_VECSTRIDE(F) \
    (((F) & ARM_TBFLAG_VECSTRIDE_MASK) >> ARM_TBFLAG_VECSTRIDE_SHIFT)
#define ARM_TBFLAG_PRIV(F) \
    (((F) & ARM_TBFLAG_PRIV_MASK) >> ARM_TBFLAG_PRIV_SHIFT)
#define ARM_TBFLAG_VFPEN(F) \
    (((F) & ARM_TBFLAG_VFPEN_MASK) >> ARM_TBFLAG_VFPEN_SHIFT)
#define ARM_TBFLAG_CONDEXEC(F) \
    (((F) & ARM_TBFLAG_CONDEXEC_MASK) >> ARM_TBFLAG_CONDEXEC_SHIFT)

-- PMM

Re: [Qemu-devel] [PATCH 0/7] target-arm: Translate based on TB flags, not CPUState

[Aurelien Jarno]

On Fri, Jan 07, 2011 at 05:50:51PM +0000, Peter Maydell wrote:
> On 7 January 2011 16:01, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > My other concern is about the definition of the individual bits in the
> > flags. I have seen that you have tried to summarize the usage in the
> > patch 6, but the masks and shifts are still duplicated in different
> > files, which may leads to mistakes if the flags definition are changed.
> >
> > Have you considered using #define as for example in the MIPS target?
> 
> I'll put out a proper v2 patchset in a bit but to save a round,
> are you happy with the following set of #defines?
> (I'm going to drop the comment since the #defines give the
> same info.)
> 
> /* Bit usage in the TB flags field: */
> #define ARM_TBFLAG_THUMB_SHIFT 0
> #define ARM_TBFLAG_THUMB_MASK (1 << ARM_TBFLAG_THUMB_SHIFT)
> #define ARM_TBFLAG_VECLEN_SHIFT 1
> #define ARM_TBFLAG_VECLEN_MASK (0x7 << ARM_TBFLAG_VECLEN_SHIFT)
> #define ARM_TBFLAG_VECSTRIDE_SHIFT 4
> #define ARM_TBFLAG_VECSTRIDE_MASK (0x3 << ARM_TBFLAG_VECSTRIDE_SHIFT)
> #define ARM_TBFLAG_PRIV_SHIFT 6
> #define ARM_TBFLAG_PRIV_MASK (1 << ARM_TBFLAG_PRIV_SHIFT)
> #define ARM_TBFLAG_VFPEN_SHIFT 7
> #define ARM_TBFLAG_VFPEN_MASK (1 << ARM_TBFLAG_VFPEN_SHIFT)
> #define ARM_TBFLAG_CONDEXEC_SHIFT 8
> #define ARM_TBFLAG_CONDEXEC_MASK (0xff << ARM_TBFLAG_CONDEXEC_SHIFT)

I am find with the names, maybe you can align the values for easier
readability, but that's details.

> /* some convenience accessor macros */
> #define ARM_TBFLAG_THUMB(F) \
>     (((F) & ARM_TBFLAG_THUMB_MASK) >> ARM_TBFLAG_THUMB_SHIFT)
> #define ARM_TBFLAG_VECLEN(F) \
>     (((F) & ARM_TBFLAG_VECLEN_MASK) >> ARM_TBFLAG_VECLEN_SHIFT)
> #define ARM_TBFLAG_VECSTRIDE(F) \
>     (((F) & ARM_TBFLAG_VECSTRIDE_MASK) >> ARM_TBFLAG_VECSTRIDE_SHIFT)
> #define ARM_TBFLAG_PRIV(F) \
>     (((F) & ARM_TBFLAG_PRIV_MASK) >> ARM_TBFLAG_PRIV_SHIFT)
> #define ARM_TBFLAG_VFPEN(F) \
>     (((F) & ARM_TBFLAG_VFPEN_MASK) >> ARM_TBFLAG_VFPEN_SHIFT)
> #define ARM_TBFLAG_CONDEXEC(F) \
>     (((F) & ARM_TBFLAG_CONDEXEC_MASK) >> ARM_TBFLAG_CONDEXEC_SHIFT)
> 

Looks fine.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net