Re: [Qemu-devel] [PATCH 01/14] target/sh4: split ctx->flags into ctx->tbflags and ctx->envflags

[Aurelien Jarno]

On 2017-05-02 09:16, Philippe Mathieu-Daudé wrote:
> Hi Aurelien,
> 
> On 05/01/2017 07:10 PM, Aurelien Jarno wrote:
> > There is a confusion (and not only in the SH4 target) between tb->flags,
> > env->flags and ctx->flags. To avoid it, split ctx->flags into
> > ctx->tbflags and ctx->envflags. ctx->tbflags stays unchanged during the
> > whole TB translation, while ctx->envflags evolves and is kept in sync
> > with env->flags using TCG instructions. ctx->envflags now only contains
> > the part that of env->flags that is contained in the TB state, i.e. the
> > DELAY_SLOT* flags.
> 
> I agree with your split, but I'd rather put the 2nd part of your commit
> description as comments in the struct DisasContext declaration, if you mind
> :)

Thanks for the review. I'll do that in the v2, although at some point it
should probably go to some higher level documentation, as it is often a
source of confusion.

-- 
Aurelien Jarno  GPG: 4096R/1DDD8C9B
aurel...@aurel32.net http://www.aurel32.net

Re: [Qemu-devel] [PATCH 01/14] target/sh4: split ctx->flags into ctx->tbflags and ctx->envflags

[Philippe Mathieu-Daudé]

Hi Aurelien,

On 05/01/2017 07:10 PM, Aurelien Jarno wrote:

There is a confusion (and not only in the SH4 target) between tb->flags,
env->flags and ctx->flags. To avoid it, split ctx->flags into
ctx->tbflags and ctx->envflags. ctx->tbflags stays unchanged during the
whole TB translation, while ctx->envflags evolves and is kept in sync
with env->flags using TCG instructions. ctx->envflags now only contains
the part that of env->flags that is contained in the TB state, i.e. the
DELAY_SLOT* flags.


I agree with your split, but I'd rather put the 2nd part of your commit 
description as comments in the struct DisasContext declaration, if you 
mind :)




Signed-off-by: Aurelien Jarno 
---
 target/sh4/translate.c | 161 +
 1 file changed, 82 insertions(+), 79 deletions(-)

diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index c89a14733f..0b16ff33ea 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -37,7 +37,8 @@ typedef struct DisasContext {
 struct TranslationBlock *tb;
 target_ulong pc;
 uint16_t opcode;
-uint32_t flags;
+uint32_t tbflags;
+uint32_t envflags;
 int bstate;
 int memidx;
 uint32_t delayed_pc;
@@ -49,7 +50,7 @@ typedef struct DisasContext {
 #if defined(CONFIG_USER_ONLY)
 #define IS_USER(ctx) 1
 #else
-#define IS_USER(ctx) (!(ctx->flags & (1u << SR_MD)))
+#define IS_USER(ctx) (!(ctx->tbflags & (1u << SR_MD)))
 #endif

 enum {
@@ -317,51 +318,50 @@ static inline void gen_store_fpr64 (TCGv_i64 t, int reg)
 #define B11_8 ((ctx->opcode >> 8) & 0xf)
 #define B15_12 ((ctx->opcode >> 12) & 0xf)

-#define REG(x) ((x) < 8 && (ctx->flags & (1u << SR_MD))\
-&& (ctx->flags & (1u << SR_RB))\
+#define REG(x) ((x) < 8 && (ctx->tbflags & (1u << SR_MD))\
+&& (ctx->tbflags & (1u << SR_RB))\
 ? (cpu_gregs[x + 16]) : (cpu_gregs[x]))

-#define ALTREG(x) ((x) < 8 && (!(ctx->flags & (1u << SR_MD))\
-   || !(ctx->flags & (1u << SR_RB)))\
+#define ALTREG(x) ((x) < 8 && (!(ctx->tbflags & (1u << SR_MD))\
+   || !(ctx->tbflags & (1u << SR_RB)))\
? (cpu_gregs[x + 16]) : (cpu_gregs[x]))

-#define FREG(x) (ctx->flags & FPSCR_FR ? (x) ^ 0x10 : (x))
+#define FREG(x) (ctx->tbflags & FPSCR_FR ? (x) ^ 0x10 : (x))
 #define XHACK(x) x) & 1 ) << 4) | ((x) & 0xe))
-#define XREG(x) (ctx->flags & FPSCR_FR ? XHACK(x) ^ 0x10 : XHACK(x))
+#define XREG(x) (ctx->tbflags & FPSCR_FR ? XHACK(x) ^ 0x10 : XHACK(x))
 #define DREG(x) FREG(x) /* Assumes lsb of (x) is always 0 */

 #define CHECK_NOT_DELAY_SLOT \
-  if (ctx->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL)) \
-  {   \
-  tcg_gen_movi_i32(cpu_pc, ctx->pc);  \
-  gen_helper_raise_slot_illegal_instruction(cpu_env); \
-  ctx->bstate = BS_BRANCH;\
-  return; \
-  }
-
-#define CHECK_PRIVILEGED\
-  if (IS_USER(ctx)) {   \
-  tcg_gen_movi_i32(cpu_pc, ctx->pc);\
-  if (ctx->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL)) { \
-  gen_helper_raise_slot_illegal_instruction(cpu_env);   \
-  } else {  \
-  gen_helper_raise_illegal_instruction(cpu_env);\
-  } \
-  ctx->bstate = BS_BRANCH;  \
-  return;   \
-  }
-
-#define CHECK_FPU_ENABLED   \
-  if (ctx->flags & (1u << SR_FD)) { \
-  tcg_gen_movi_i32(cpu_pc, ctx->pc);\
-  if (ctx->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL)) { \
-  gen_helper_raise_slot_fpu_disable(cpu_env);   \
-  } else {  \
-  gen_helper_raise_fpu_disable(cpu_env);\
-  } \
-  ctx->bstate = BS_BRANCH;  \
-  return;   \
-  }
+if (ctx->envflags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL)) { \
+tcg_gen_movi_i32(cpu_pc, ctx->pc);   \
+gen_helper_raise_slot_illegal_instruction(cpu_env);  \
+ctx->bstate = BS_BRANCH; \
+return;  \
+}
+
+#define CHECK_PRIVILEGED \
+if (IS_USER(ctx)) {  \
+tcg_gen_movi_i32(cpu_pc, ctx->pc);   \