Re: [Qemu-devel] Sparc32: Mask writes to the WIM register

[Luis Pureza]

On Wed, Aug 20, 2008 at 4:30 PM, Blue Swirl <address@hidden> wrote:
> On 8/20/08, Luis Pureza <address@hidden> wrote:
>> OK, the following patch works for me:
>>
>>  ---
>>   target-sparc/translate.c |    7 +++++++
>>   1 files changed, 7 insertions(+), 0 deletions(-)
>>
>>  diff --git a/target-sparc/translate.c b/target-sparc/translate.c
>>  index 8286d36..02e497f 100644
>>
>> --- a/target-sparc/translate.c
>>  +++ b/target-sparc/translate.c
>>
>> @@ -73,6 +73,7 @@ typedef struct DisasContext {
>>      struct TranslationBlock *tb;
>>      uint32_t features;
>>      int extra_cpu_cycles;
>>  +    uint32_t wim_mask;
>>   } DisasContext;
>>
>>   // This function uses non-native bit order
>>  @@ -3485,6 +3486,7 @@ static void disas_sparc_insn(DisasContext * dc)
>>
>>                              }
>>   #else
>>                              tcg_gen_trunc_tl_i32(cpu_tmp32, cpu_tmp0);
>>
>> +                            tcg_gen_andi_tl(cpu_tmp32, cpu_tmp32,
>>  dc->wim_mask);
>>
>>                              tcg_gen_st_i32(cpu_tmp32, cpu_env,
>>                                             offsetof(CPUSPARCState, wim));
>>
>>  #endif
>>  @@ -4848,6 +4850,11 @@ static inline void
>>  gen_intermediate_code_internal(TranslationBlock * tb,
>>   #ifdef TARGET_SPARC64
>>      dc->address_mask_32bit = env->pstate & PS_AM;
>>   #endif
>>  +    if (env->nwindows == 32) {
>>  +        dc->wim_mask = 0xFFFFFFFF;
>>  +    } else {
>>  +        dc->wim_mask = (1 << env->nwindows) - 1;
>>  +    }
>>      gen_opc_end = gen_opc_buf + OPC_MAX_SIZE;
>>
>>      cpu_tmp0 = tcg_temp_new(TCG_TYPE_TL);
>
> Thanks. But now every translation would be slowed down by a tiny
> amount because the wim mask is calculated, even if it will never be
> used. More optimally, the mask should be calculated as needed and only
> nwindows should be copied to dc.
>
> A more complex solution would be to avoid most of the copying
> entirely, so that env would have a pointer to sparc_def_t structure
> and dc could copy only this "def" pointer. Then instead of
> dc->features and dc->nwindows, dc->def->features and dc->def->nwindows
> would be used. This would be more useful in the future, if I add code
> to select the ASI helper at translation time.

Complex please. One of those "integration problems" I had was
precisely related to different ASI mappings.

The following patch tries to do what you said. I guess more stuff
could be removed from CPUSPARCState now that it has a pointer to a
sparc_def_t. But I leave that to your judgment. The only thing I
removed was "features".

Note that the wrwim fix is also included.

diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 2574690..f9fa609 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -187,6 +187,23 @@ typedef struct trap_state {
 } trap_state;
 #endif

+typedef struct sparc_def_t sparc_def_t;
+
+struct sparc_def_t {
+    const char *name;
+    target_ulong iu_version;
+    uint32_t fpu_version;
+    uint32_t mmu_version;
+    uint32_t mmu_bm;
+    uint32_t mmu_ctpr_mask;
+    uint32_t mmu_cxr_mask;
+    uint32_t mmu_sfsr_mask;
+    uint32_t mmu_trcr_mask;
+    uint32_t features;
+    uint32_t nwindows;
+    uint32_t maxtl;
+};
+
 typedef struct CPUSPARCState {
     target_ulong gregs[8]; /* general registers */
     target_ulong *regwptr; /* pointer to current register window */
@@ -275,7 +292,7 @@ typedef struct CPUSPARCState {
     uint64_t hpstate, htstate[MAXTL_MAX], hintp, htba, hver, hstick_cmpr, ssr;
     void *hstick; // UA 2005
 #endif
-    uint32_t features;
+    sparc_def_t *def;
 } CPUSPARCState;

 #define CPU_FEATURE_FLOAT    (1 << 0)
diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index 811de01..09b30e3 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -34,23 +34,6 @@
 //#define DEBUG_FEATURES
 //#define DEBUG_PCALL

-typedef struct sparc_def_t sparc_def_t;
-
-struct sparc_def_t {
-    const char *name;
-    target_ulong iu_version;
-    uint32_t fpu_version;
-    uint32_t mmu_version;
-    uint32_t mmu_bm;
-    uint32_t mmu_ctpr_mask;
-    uint32_t mmu_cxr_mask;
-    uint32_t mmu_sfsr_mask;
-    uint32_t mmu_trcr_mask;
-    uint32_t features;
-    uint32_t nwindows;
-    uint32_t maxtl;
-};
-
 static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model);

 /* Sparc MMU emulation */
@@ -936,7 +919,8 @@ static int cpu_sparc_register(CPUSPARCState *env,
const char *cpu_model)
     if (cpu_sparc_find_by_name(def, cpu_model) < 0)
         return -1;

-    env->features = def->features;
+    env->def = malloc(sizeof(*def));
+    memcpy(env->def, def, sizeof(*def));
     env->cpu_model_str = cpu_model;
     env->version = def->iu_version;
     env->fsr = def->fpu_version;
@@ -960,6 +944,7 @@ static int cpu_sparc_register(CPUSPARCState *env,
const char *cpu_model)

 static void cpu_sparc_close(CPUSPARCState *env)
 {
+    free(env->def);
     free(env);
 }

diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 6315218..1796cff 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -59,7 +59,7 @@ typedef struct DisasContext {
     int fpu_enabled;
     int address_mask_32bit;
     struct TranslationBlock *tb;
-    uint32_t features;
+    sparc_def_t *def;
 } DisasContext;

 // This function uses non-native bit order
@@ -1905,10 +1905,10 @@ static inline TCGv get_src2(unsigned int insn, TCGv def)
 }

 #define CHECK_IU_FEATURE(dc, FEATURE)                      \
-    if (!((dc)->features & CPU_FEATURE_ ## FEATURE))       \
+    if (!((dc)->def->features & CPU_FEATURE_ ## FEATURE))     \
         goto illegal_insn;
 #define CHECK_FPU_FEATURE(dc, FEATURE)                     \
-    if (!((dc)->features & CPU_FEATURE_ ## FEATURE))       \
+    if (!((dc)->def->features & CPU_FEATURE_ ## FEATURE))       \
         goto nfpu_insn;

 /* before an instruction, dc->pc must be static */
@@ -3451,6 +3451,9 @@ static void disas_sparc_insn(DisasContext * dc)
                             }
 #else
                             tcg_gen_trunc_tl_i32(cpu_tmp32, cpu_tmp0);
+                            if (dc->def->nwindows != 32) {
+                                tcg_gen_andi_tl(cpu_tmp32, cpu_tmp32,
(1 << dc->def->nwindows) - 1);
+                            }
                             tcg_gen_st_i32(cpu_tmp32, cpu_env,
                                            offsetof(CPUSPARCState, wim));
 #endif
@@ -4141,7 +4144,7 @@ static void disas_sparc_insn(DisasContext * dc)
                     goto jmp_insn;
 #endif
                 case 0x3b: /* flush */
-                    if (!((dc)->features & CPU_FEATURE_FLUSH))
+                    if (!((dc)->def->features & CPU_FEATURE_FLUSH))
                         goto unimp_flush;
                     tcg_gen_helper_0_1(helper_flush, cpu_dst);
                     break;
@@ -4742,11 +4745,11 @@ static inline void
gen_intermediate_code_internal(TranslationBlock * tb,
     last_pc = dc->pc;
     dc->npc = (target_ulong) tb->cs_base;
     dc->mem_idx = cpu_mmu_index(env);
-    dc->features = env->features;
-    if ((dc->features & CPU_FEATURE_FLOAT)) {
+    dc->def = env->def;
+    if ((dc->def->features & CPU_FEATURE_FLOAT)) {
         dc->fpu_enabled = cpu_fpu_enabled(env);
 #if defined(CONFIG_USER_ONLY)
-        dc->features |= CPU_FEATURE_FLOAT128;
+        dc->def->features |= CPU_FEATURE_FLOAT128;
 #endif
     } else
         dc->fpu_enabled = 0;