[Qemu-devel] [PATCH 1/2] cpu-exec: rid cs_base of TranslationBlock

[Qemu-devel] [PATCH 1/2] cpu-exec: rid cs_base of TranslationBlock

[liguang]

cs_base is only meaningful for target-i386/sparc,
so, get rid of cs_base for other target

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 cpu-exec.c              |   26 ++++++++++++++++++--------
 exec.c                  |    6 +++---
 hw/i386/kvmvapic.c      |    6 ++----
 include/exec/exec-all.h |    5 +++--
 target-i386/cpu.h       |    6 ++----
 translate-all.c         |   24 ++++++++++++------------
 6 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 31c089d..f3c1d1c 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -84,7 +84,7 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
     if (max_cycles > CF_COUNT_MASK)
         max_cycles = CF_COUNT_MASK;
 
-    tb = tb_gen_code(env, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
+    tb = tb_gen_code(env, orig_tb->pc, orig_tb->flags,
                      max_cycles);
     cpu->current_tb = tb;
     /* execute the generated code */
@@ -96,7 +96,6 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
 
 static TranslationBlock *tb_find_slow(CPUArchState *env,
                                       target_ulong pc,
-                                      target_ulong cs_base,
                                       uint64_t flags)
 {
     TranslationBlock *tb, **ptb1;
@@ -117,7 +116,12 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
             goto not_found;
         if (tb->pc == pc &&
             tb->page_addr[0] == phys_page1 &&
-            tb->cs_base == cs_base &&
+#if defined(TARGET_I386)
+            tb->cs_base == env->segs[R_CS].base &&
+#endif
+#if defined(TARGET_SPARC)
+            tb->cs_base == env->npc &&
+#endif
             tb->flags == flags) {
             /* check next page if needed */
             if (tb->page_addr[1] != -1) {
@@ -136,7 +140,7 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
     }
  not_found:
    /* if no translated code available, then translate it now */
-    tb = tb_gen_code(env, pc, cs_base, flags, 0);
+    tb = tb_gen_code(env, pc, flags, 0);
 
  found:
     /* Move the last found TB to the head of the list */
@@ -153,17 +157,23 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
 static inline TranslationBlock *tb_find_fast(CPUArchState *env)
 {
     TranslationBlock *tb;
-    target_ulong cs_base, pc;
+    target_ulong pc;
     int flags;
 
     /* we record a subset of the CPU state. It will
        always be the same before a given translated block
        is executed. */
-    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
+    cpu_get_tb_cpu_state(env, &pc, &flags);
     tb = env->tb_jmp_cache[tb_jmp_cache_hash_func(pc)];
-    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
+    if (unlikely(!tb || tb->pc != pc ||
+#if defined(TARGET_I386)
+                 tb->cs_base != env->segs[R_CS].base ||
+#endif
+#if defined(TARGET_SPARC)
+                 tb->cs_base != env->npc ||
+#endif
                  tb->flags != flags)) {
-        tb = tb_find_slow(env, pc, cs_base, flags);
+        tb = tb_find_slow(env, pc, flags);
     }
     return tb;
 }
diff --git a/exec.c b/exec.c
index fa1e0c3..a14db2c 100644
--- a/exec.c
+++ b/exec.c
@@ -1471,7 +1471,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
 static void check_watchpoint(int offset, int len_mask, int flags)
 {
     CPUArchState *env = cpu_single_env;
-    target_ulong pc, cs_base;
+    target_ulong pc;
     target_ulong vaddr;
     CPUWatchpoint *wp;
     int cpu_flags;
@@ -1495,8 +1495,8 @@ static void check_watchpoint(int offset, int len_mask, int flags)
                     env->exception_index = EXCP_DEBUG;
                     cpu_loop_exit(env);
                 } else {
-                    cpu_get_tb_cpu_state(env, &pc, &cs_base, &cpu_flags);
-                    tb_gen_code(env, pc, cs_base, cpu_flags, 1);
+                    cpu_get_tb_cpu_state(env, &pc, &cpu_flags);
+                    tb_gen_code(env, pc, cpu_flags, 1);
                     cpu_resume_from_signal(env, NULL);
                 }
             }
diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index ed9b448..8b4260e 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -388,7 +388,6 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
     uint8_t opcode[2];
     uint32_t imm32;
     target_ulong current_pc = 0;
-    target_ulong current_cs_base = 0;
     int current_flags = 0;
 
     if (smp_cpus == 1) {
@@ -399,8 +398,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
 
     if (!kvm_enabled()) {
         cpu_restore_state(env, env->mem_io_pc);
-        cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
-                             &current_flags);
+        cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
     }
 
     pause_all_vcpus();
@@ -440,7 +438,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
 
     if (!kvm_enabled()) {
         cs->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+        tb_gen_code(env, current_pc, current_flags, 1);
         cpu_resume_from_signal(env, NULL);
     }
 }
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index e856191..560a7d1 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -85,8 +85,7 @@ bool cpu_restore_state(CPUArchState *env, uintptr_t searched_pc);
 void QEMU_NORETURN cpu_resume_from_signal(CPUArchState *env1, void *puc);
 void QEMU_NORETURN cpu_io_recompile(CPUArchState *env, uintptr_t retaddr);
 TranslationBlock *tb_gen_code(CPUArchState *env, 
-                              target_ulong pc, target_ulong cs_base, int flags,
-                              int cflags);
+                              target_ulong pc, int flags, int cflags);
 void cpu_exec_init(CPUArchState *env);
 void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
 int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
@@ -135,7 +134,9 @@ static inline void tlb_flush(CPUArchState *env, int flush_global)
 
 struct TranslationBlock {
     target_ulong pc;   /* simulated PC corresponding to this block (EIP + CS base) */
+#if defined(TARGET_I386) || defined(TARGET_SPARC)
     target_ulong cs_base; /* CS base for this block */
+#endif
     uint64_t flags; /* flags defining in which context the code was generated */
     uint16_t size;      /* size of target code for this block (1 <=
                            size <= TARGET_PAGE_SIZE) */
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 2b4e319..3ce1b2e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1188,11 +1188,9 @@ static inline void cpu_pc_from_tb(CPUX86State *env, TranslationBlock *tb)
     env->eip = tb->pc - tb->cs_base;
 }
 
-static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc, int *flags)
 {
-    *cs_base = env->segs[R_CS].base;
-    *pc = *cs_base + env->eip;
+    *pc = env->segs[R_CS].base + env->eip;
     *flags = env->hflags |
         (env->eflags & (IOPL_MASK | TF_MASK | RF_MASK | VM_MASK | AC_MASK));
 }
diff --git a/translate-all.c b/translate-all.c
index a98c646..f9efbbd 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -931,8 +931,7 @@ static void build_page_bitmap(PageDesc *p)
 }
 
 TranslationBlock *tb_gen_code(CPUArchState *env,
-                              target_ulong pc, target_ulong cs_base,
-                              int flags, int cflags)
+                              target_ulong pc, int flags, int cflags)
 {
     TranslationBlock *tb;
     uint8_t *tc_ptr;
@@ -952,7 +951,12 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
     }
     tc_ptr = tcg_ctx.code_gen_ptr;
     tb->tc_ptr = tc_ptr;
-    tb->cs_base = cs_base;
+#if defined(TARGET_I386)
+    tb->cs_base = env->segs[R_CS].base;
+#endif
+#if defined(TARGET_SPARC)
+    tb->cs_base = env->npc;
+#endif
     tb->flags = flags;
     tb->cflags = cflags;
     cpu_gen_code(env, tb, &code_gen_size);
@@ -1007,7 +1011,6 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
     TranslationBlock *current_tb = NULL;
     int current_tb_modified = 0;
     target_ulong current_pc = 0;
-    target_ulong current_cs_base = 0;
     int current_flags = 0;
 #endif /* TARGET_HAS_PRECISE_SMC */
 
@@ -1063,8 +1066,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
 
                 current_tb_modified = 1;
                 cpu_restore_state_from_tb(current_tb, env, env->mem_io_pc);
-                cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
-                                     &current_flags);
+                cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
             }
 #endif /* TARGET_HAS_PRECISE_SMC */
             /* we need to do that to handle the case where a signal
@@ -1099,7 +1101,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
            modifying the memory. It will ensure that it cannot modify
            itself */
         cpu->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+        tb_gen_code(env, current_pc, current_flags, 1);
         cpu_resume_from_signal(env, NULL);
     }
 #endif
@@ -1149,7 +1151,6 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
     CPUState *cpu = NULL;
     int current_tb_modified = 0;
     target_ulong current_pc = 0;
-    target_ulong current_cs_base = 0;
     int current_flags = 0;
 #endif
 
@@ -1181,8 +1182,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
 
             current_tb_modified = 1;
             cpu_restore_state_from_tb(current_tb, env, pc);
-            cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
-                                 &current_flags);
+            cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
         }
 #endif /* TARGET_HAS_PRECISE_SMC */
         tb_phys_invalidate(tb, addr);
@@ -1195,7 +1195,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
            modifying the memory. It will ensure that it cannot modify
            itself */
         cpu->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
+        tb_gen_code(env, current_pc, current_flags, 1);
         cpu_resume_from_signal(env, puc);
     }
 #endif
@@ -1463,7 +1463,7 @@ void cpu_io_recompile(CPUArchState *env, uintptr_t retaddr)
     tb_phys_invalidate(tb, -1);
     /* FIXME: In theory this could raise an exception.  In practice
        we have already translated the block once so it's probably ok.  */
-    tb_gen_code(env, pc, cs_base, flags, cflags);
+    tb_gen_code(env, pc, flags, cflags);
     /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
        the first in the TB) then we end up generating a whole new TB and
        repeating the fault, which is horribly inefficient.
-- 
1.7.2.5

[Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[liguang]

Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 target-alpha/cpu.h      |    3 +--
 target-arm/cpu.h        |    3 +--
 target-cris/cpu.h       |    3 +--
 target-lm32/cpu.h       |    3 +--
 target-m68k/cpu.h       |    3 +--
 target-microblaze/cpu.h |    3 +--
 target-mips/cpu.h       |    3 +--
 target-moxie/cpu.h      |    3 +--
 target-openrisc/cpu.h   |    3 +--
 target-ppc/cpu.h        |    3 +--
 target-s390x/cpu.h      |    3 +--
 target-sh4/cpu.h        |    3 +--
 target-sparc/cpu.h      |    2 +-
 target-unicore32/cpu.h  |    3 +--
 target-xtensa/cpu.h     |    3 +--
 15 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
index 2156a1e..2d5af9b 100644
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -478,12 +478,11 @@ enum {
 };
 
 static inline void cpu_get_tb_cpu_state(CPUAlphaState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *pflags)
+                                        int *pflags)
 {
     int flags = 0;
 
     *pc = env->pc;
-    *cs_base = 0;
 
     if (env->pal_mode) {
         flags = TB_FLAGS_PAL_MODE;
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 2b97221..e8a9aae 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -696,11 +696,10 @@ static inline void cpu_clone_regs(CPUARMState *env, target_ulong newsp)
     (((F) & ARM_TBFLAG_BSWAP_CODE_MASK) >> ARM_TBFLAG_BSWAP_CODE_SHIFT)
 
 static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     int privmode;
     *pc = env->regs[15];
-    *cs_base = 0;
     *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
         | (env->vfp.vec_len << ARM_TBFLAG_VECLEN_SHIFT)
         | (env->vfp.vec_stride << ARM_TBFLAG_VECSTRIDE_SHIFT)
diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index dbd7d36..41aeebf 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -274,10 +274,9 @@ static inline void cpu_set_tls(CPUCRISState *env, target_ulong newtls)
 #include "exec/cpu-all.h"
 
 static inline void cpu_get_tb_cpu_state(CPUCRISState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->pc;
-    *cs_base = 0;
     *flags = env->dslot |
             (env->pregs[PR_CCS] & (S_FLAG | P_FLAG | U_FLAG
 				     | X_FLAG | PFIX_FLAG));
diff --git a/target-lm32/cpu.h b/target-lm32/cpu.h
index bfb9150..8721cb9 100644
--- a/target-lm32/cpu.h
+++ b/target-lm32/cpu.h
@@ -232,10 +232,9 @@ static inline void cpu_set_tls(CPULM32State *env, target_ulong newtls)
 #include "exec/cpu-all.h"
 
 static inline void cpu_get_tb_cpu_state(CPULM32State *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->pc;
-    *cs_base = 0;
     *flags = 0;
 }
 
diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index c90c40c..5b48d87 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -253,10 +253,9 @@ static inline void cpu_clone_regs(CPUM68KState *env, target_ulong newsp)
 #include "exec/cpu-all.h"
 
 static inline void cpu_get_tb_cpu_state(CPUM68KState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->pc;
-    *cs_base = 0;
     *flags = (env->fpcr & M68K_FPCR_PREC)       /* Bit  6 */
             | (env->sr & SR_S)                  /* Bit  13 */
             | ((env->macsr >> 4) & 0xf);        /* Bits 0-3 */
diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
index 1813939..a87d9ef 100644
--- a/target-microblaze/cpu.h
+++ b/target-microblaze/cpu.h
@@ -358,10 +358,9 @@ static inline target_ulong cpu_get_pc(CPUMBState *env)
 }
 
 static inline void cpu_get_tb_cpu_state(CPUMBState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->sregs[SR_PC];
-    *cs_base = 0;
     *flags = (env->iflags & IFLAGS_TB_MASK) |
                  (env->sregs[SR_MSR] & (MSR_UM | MSR_VM | MSR_EE));
 }
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index cedf03d..b8d00fb 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -670,10 +670,9 @@ hwaddr cpu_mips_translate_address (CPUMIPSState *env, target_ulong address,
 #endif
 
 static inline void cpu_get_tb_cpu_state(CPUMIPSState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->active_tc.PC;
-    *cs_base = 0;
     *flags = env->hflags & (MIPS_HFLAG_TMASK | MIPS_HFLAG_BMASK);
 }
 
diff --git a/target-moxie/cpu.h b/target-moxie/cpu.h
index a9d9ace..7ae375f 100644
--- a/target-moxie/cpu.h
+++ b/target-moxie/cpu.h
@@ -147,10 +147,9 @@ static inline void cpu_pc_from_tb(CPUMoxieState *env, TranslationBlock *tb)
 }
 
 static inline void cpu_get_tb_cpu_state(CPUMoxieState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->pc;
-    *cs_base = 0;
     *flags = 0;
 }
 
diff --git a/target-openrisc/cpu.h b/target-openrisc/cpu.h
index b9c55ba..f57c76e 100644
--- a/target-openrisc/cpu.h
+++ b/target-openrisc/cpu.h
@@ -404,10 +404,9 @@ static inline void cpu_clone_regs(CPUOpenRISCState *env, target_ulong newsp)
 
 static inline void cpu_get_tb_cpu_state(CPUOpenRISCState *env,
                                         target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->pc;
-    *cs_base = 0;
     /* D_FLAG -- branch instruction exception */
     *flags = (env->flags & D_FLAG);
 }
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 42c36e2..8a3dbeb 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -2022,10 +2022,9 @@ static inline void cpu_write_xer(CPUPPCState *env, target_ulong xer)
 }
 
 static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->nip;
-    *cs_base = 0;
     *flags = env->hflags;
 }
 
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index e351005..6e33400 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -278,10 +278,9 @@ static inline int cpu_mmu_index (CPUS390XState *env)
 }
 
 static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->psw.addr;
-    *cs_base = 0;
     *flags = ((env->psw.mask >> 32) & ~FLAG_MASK_CC) |
              ((env->psw.mask & PSW_MASK_32) ? FLAG_MASK_32 : 0);
 }
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index fd7da92..a2dbd98 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -354,10 +354,9 @@ static inline int cpu_ptel_pr (uint32_t ptel)
 #define TB_FLAG_PENDING_MOVCA  (1 << 4)
 
 static inline void cpu_get_tb_cpu_state(CPUSH4State *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->pc;
-    *cs_base = 0;
     *flags = (env->flags & (DELAY_SLOT | DELAY_SLOT_CONDITIONAL
                     | DELAY_SLOT_TRUE | DELAY_SLOT_CLEARME))   /* Bits  0- 3 */
             | (env->fpscr & (FPSCR_FR | FPSCR_SZ | FPSCR_PR))  /* Bits 19-21 */
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 6fa7778..5331ad5 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
 #define TB_FLAG_AM_ENABLED (1 << 5)
 
 static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->pc;
     *cs_base = env->npc;
diff --git a/target-unicore32/cpu.h b/target-unicore32/cpu.h
index 5b2b9d1..7b46696 100644
--- a/target-unicore32/cpu.h
+++ b/target-unicore32/cpu.h
@@ -165,10 +165,9 @@ static inline void cpu_pc_from_tb(CPUUniCore32State *env, TranslationBlock *tb)
 }
 
 static inline void cpu_get_tb_cpu_state(CPUUniCore32State *env, target_ulong *pc,
-                                        target_ulong *cs_base, int *flags)
+                                        int *flags)
 {
     *pc = env->regs[31];
-    *cs_base = 0;
     *flags = 0;
     if ((env->uncached_asr & ASR_M) != ASR_MODE_USER) {
         *flags |= (1 << 6);
diff --git a/target-xtensa/cpu.h b/target-xtensa/cpu.h
index 6c9fc35..d6e00f6 100644
--- a/target-xtensa/cpu.h
+++ b/target-xtensa/cpu.h
@@ -486,10 +486,9 @@ static inline int cpu_mmu_index(CPUXtensaState *env)
 #define XTENSA_TBFLAG_CPENABLE_SHIFT 6
 
 static inline void cpu_get_tb_cpu_state(CPUXtensaState *env, target_ulong *pc,
-        target_ulong *cs_base, int *flags)
+        int *flags)
 {
     *pc = env->pc;
-    *cs_base = 0;
     *flags = 0;
     *flags |= xtensa_get_ring(env);
     if (env->sregs[PS] & PS_EXCM) {
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[Peter Maydell]

On 24 April 2013 02:48, liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>

I'm afraid this is definitely wrong. It has a less than
helpful name, but cs_base is actually just "another 32/64 bits
of state that the target can use to distinguish translation
blocks", and some non-x86 targets do use it. For instance:


> --- a/target-sparc/cpu.h
> +++ b/target-sparc/cpu.h
> @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
>  #define TB_FLAG_AM_ENABLED (1 << 5)
>
>  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
> -                                        target_ulong *cs_base, int *flags)
> +                                        int *flags)
>  {
>      *pc = env->pc;
>      *cs_base = env->npc;

...surely this doesn't even compile after your changes?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[li guang]

在 2013-04-24三的 08:05 +0100，Peter Maydell写道：
> On 24 April 2013 02:48, liguang <lig.fnst@cn.fujitsu.com> wrote:
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> 
> I'm afraid this is definitely wrong. It has a less than
> helpful name, but cs_base is actually just "another 32/64 bits
> of state that the target can use to distinguish translation
> blocks", and some non-x86 targets do use it. For instance:

only sparc use it as a tmp buffer for pc.

> 
> 
> > --- a/target-sparc/cpu.h
> > +++ b/target-sparc/cpu.h
> > @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
> >  #define TB_FLAG_AM_ENABLED (1 << 5)
> >
> >  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
> > -                                        target_ulong *cs_base, int *flags)
> > +                                        int *flags)
> >  {
> >      *pc = env->pc;
> >      *cs_base = env->npc;
> 
> ...surely this doesn't even compile after your changes?
> 

seems no problem for me.

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[Peter Maydell]

On 24 April 2013 08:15, li guang <lig.fnst@cn.fujitsu.com> wrote:
> 在 2013-04-24三的 08:05 +0100，Peter Maydell写道：
>> On 24 April 2013 02:48, liguang <lig.fnst@cn.fujitsu.com> wrote:
>> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>
>> I'm afraid this is definitely wrong. It has a less than
>> helpful name, but cs_base is actually just "another 32/64 bits
>> of state that the target can use to distinguish translation
>> blocks", and some non-x86 targets do use it. For instance:
>
> only sparc use it as a tmp buffer for pc.

And x86 uses it. And tomorrow anybody could submit a patch
to another target which makes use of it, if they find they
need to do something and there's not enough room left in
'flags'. It's a generic mechanism which happens to be used
by two targets today.

>> > --- a/target-sparc/cpu.h
>> > +++ b/target-sparc/cpu.h
>> > @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
>> >  #define TB_FLAG_AM_ENABLED (1 << 5)
>> >
>> >  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
>> > -                                        target_ulong *cs_base, int *flags)
>> > +                                        int *flags)
>> >  {
>> >      *pc = env->pc;
>> >      *cs_base = env->npc;
>>
>> ...surely this doesn't even compile after your changes?
>>
>
> seems no problem for me.

You clearly have a problem with your compile and test
process then, because it is clear from the patch that
you've removed the cs_base argument from this function
but the function still has a use of 'cs_base' in it.

-- PMM

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[li guang]

在 2013-04-24三的 08:28 +0100，Peter Maydell写道：
> On 24 April 2013 08:15, li guang <lig.fnst@cn.fujitsu.com> wrote:
> > 在 2013-04-24三的 08:05 +0100，Peter Maydell写道：
> >> On 24 April 2013 02:48, liguang <lig.fnst@cn.fujitsu.com> wrote:
> >> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >>
> >> I'm afraid this is definitely wrong. It has a less than
> >> helpful name, but cs_base is actually just "another 32/64 bits
> >> of state that the target can use to distinguish translation
> >> blocks", and some non-x86 targets do use it. For instance:
> >
> > only sparc use it as a tmp buffer for pc.
> 
> And x86 uses it. And tomorrow anybody could submit a patch
> to another target which makes use of it, if they find they
> need to do something and there's not enough room left in
> 'flags'. It's a generic mechanism which happens to be used
> by two targets today.

I think even others want to use something like you said,
it should not 'cs_base', or, it's a bad name.

> 
> >> > --- a/target-sparc/cpu.h
> >> > +++ b/target-sparc/cpu.h
> >> > @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
> >> >  #define TB_FLAG_AM_ENABLED (1 << 5)
> >> >
> >> >  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
> >> > -                                        target_ulong *cs_base, int *flags)
> >> > +                                        int *flags)
> >> >  {
> >> >      *pc = env->pc;
> >> >      *cs_base = env->npc;
> >>
> >> ...surely this doesn't even compile after your changes?
> >>
> >
> > seems no problem for me.
> 
> You clearly have a problem with your compile and test
> process then, because it is clear from the patch that
> you've removed the cs_base argument from this function
> but the function still has a use of 'cs_base' in it.

???, sorry, where do I miss 'cs_base' removing?

> 
> -- PMM

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[Peter Maydell]

On 24 April 2013 08:32, li guang <lig.fnst@cn.fujitsu.com> wrote:
> I think even others want to use something like you said,
> it should not 'cs_base', or, it's a bad name.

Yes, this is why I said "has a less than helpful name".

>>
>> >> > --- a/target-sparc/cpu.h
>> >> > +++ b/target-sparc/cpu.h
>> >> > @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
>> >> >  #define TB_FLAG_AM_ENABLED (1 << 5)
>> >> >
>> >> >  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
>> >> > -                                        target_ulong *cs_base, int *flags)
>> >> > +                                        int *flags)
>> >> >  {
>> >> >      *pc = env->pc;
>> >> >      *cs_base = env->npc;

>> You clearly have a problem with your compile and test
>> process then, because it is clear from the patch that
>> you've removed the cs_base argument from this function
>> but the function still has a use of 'cs_base' in it.
>
> ???, sorry, where do I miss 'cs_base' removing?

Last quoted line of source: "*cs_base = env->npc".

-- PMM

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[li guang]

在 2013-04-24三的 08:36 +0100，Peter Maydell写道：
> On 24 April 2013 08:32, li guang <lig.fnst@cn.fujitsu.com> wrote:
> > I think even others want to use something like you said,
> > it should not 'cs_base', or, it's a bad name.
> 
> Yes, this is why I said "has a less than helpful name".
> 
> >>
> >> >> > --- a/target-sparc/cpu.h
> >> >> > +++ b/target-sparc/cpu.h
> >> >> > @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
> >> >> >  #define TB_FLAG_AM_ENABLED (1 << 5)
> >> >> >
> >> >> >  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
> >> >> > -                                        target_ulong *cs_base, int *flags)
> >> >> > +                                        int *flags)
> >> >> >  {
> >> >> >      *pc = env->pc;
> >> >> >      *cs_base = env->npc;
> 
> >> You clearly have a problem with your compile and test
> >> process then, because it is clear from the patch that
> >> you've removed the cs_base argument from this function
> >> but the function still has a use of 'cs_base' in it.
> >
> > ???, sorry, where do I miss 'cs_base' removing?
> 
> Last quoted line of source: "*cs_base = env->npc".

OK, thanks!
that remove by overshoot script!

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[Andreas Färber]

Am 24.04.2013 09:40, schrieb li guang:
> 在 2013-04-24三的 08:36 +0100，Peter Maydell写道：
>> On 24 April 2013 08:32, li guang <lig.fnst@cn.fujitsu.com> wrote:
>>> I think even others want to use something like you said,
>>> it should not 'cs_base', or, it's a bad name.
>>
>> Yes, this is why I said "has a less than helpful name".
>>
>>>>
>>>>>>> --- a/target-sparc/cpu.h
>>>>>>> +++ b/target-sparc/cpu.h
>>>>>>> @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
>>>>>>>  #define TB_FLAG_AM_ENABLED (1 << 5)
>>>>>>>
>>>>>>>  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
>>>>>>> -                                        target_ulong *cs_base, int *flags)
>>>>>>> +                                        int *flags)
>>>>>>>  {
>>>>>>>      *pc = env->pc;
>>>>>>>      *cs_base = env->npc;
>>
>>>> You clearly have a problem with your compile and test
>>>> process then, because it is clear from the patch that
>>>> you've removed the cs_base argument from this function
>>>> but the function still has a use of 'cs_base' in it.
>>>
>>> ???, sorry, where do I miss 'cs_base' removing?
>>
>> Last quoted line of source: "*cs_base = env->npc".
> 
> OK, thanks!
> that remove by overshoot script!

Some general reminders:

We're in Soft Freeze, so in general no new big patch series will go into
1.5 unless there's a maintainer willing to take care of it - for i386
there is none, and random code cleanups do not look like something we
must absolutely have in the release last minute. At least no one brought
up on yesterday's call that this is a must-have, so maybe after the
release would be a better time to let people review this?

Whenever you send more than one patch, please include a cover letter.

When you resend a series modified, please include a version such as v2,
v3, etc. and a change log in the cover letter rather than resending with
[updated] or in a way that can't be distinguished at all.

You're expected to assure that your patches compile and don't break
`make check` at least. Repeatedly sending patches that cannot possibly
build okay means you're either overlooking error output from your build
or maybe building the wrong source directory?

Generally, touching the innards of TCG requires a good code
understanding and testing of multiple targets; personally I try to avoid
unnecessary changes for fear of breaking rare corner cases. ;)

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[Aurelien Jarno]

On Wed, Apr 24, 2013 at 04:22:01PM +0200, Andreas Färber wrote:
> Am 24.04.2013 09:40, schrieb li guang:
> > 在 2013-04-24三的 08:36 +0100，Peter Maydell写道：
> >> On 24 April 2013 08:32, li guang <lig.fnst@cn.fujitsu.com> wrote:
> >>> I think even others want to use something like you said,
> >>> it should not 'cs_base', or, it's a bad name.
> >>
> >> Yes, this is why I said "has a less than helpful name".
> >>
> >>>>
> >>>>>>> --- a/target-sparc/cpu.h
> >>>>>>> +++ b/target-sparc/cpu.h
> >>>>>>> @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
> >>>>>>>  #define TB_FLAG_AM_ENABLED (1 << 5)
> >>>>>>>
> >>>>>>>  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
> >>>>>>> -                                        target_ulong *cs_base, int *flags)
> >>>>>>> +                                        int *flags)
> >>>>>>>  {
> >>>>>>>      *pc = env->pc;
> >>>>>>>      *cs_base = env->npc;
> >>
> >>>> You clearly have a problem with your compile and test
> >>>> process then, because it is clear from the patch that
> >>>> you've removed the cs_base argument from this function
> >>>> but the function still has a use of 'cs_base' in it.
> >>>
> >>> ???, sorry, where do I miss 'cs_base' removing?
> >>
> >> Last quoted line of source: "*cs_base = env->npc".
> > 
> > OK, thanks!
> > that remove by overshoot script!
> 
> Some general reminders:
> 
> We're in Soft Freeze, so in general no new big patch series will go into
> 1.5 unless there's a maintainer willing to take care of it - for i386
> there is none, and random code cleanups do not look like something we
> must absolutely have in the release last minute. At least no one brought
> up on yesterday's call that this is a must-have, so maybe after the
> release would be a better time to let people review this?
> 

Being in soft or hard freeze should not prevent people to send patches,
and nothing here says that they should be included in 1.5.

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

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[Andreas Färber]

Am 24.04.2013 17:40, schrieb Aurelien Jarno:
> On Wed, Apr 24, 2013 at 04:22:01PM +0200, Andreas Färber wrote:
>> We're in Soft Freeze, so in general no new big patch series will go into
>> 1.5 unless there's a maintainer willing to take care of it - for i386
>> there is none, and random code cleanups do not look like something we
>> must absolutely have in the release last minute. At least no one brought
>> up on yesterday's call that this is a must-have, so maybe after the
>> release would be a better time to let people review this?
>>
> 
> Being in soft or hard freeze should not prevent people to send patches,
> and nothing here says that they should be included in 1.5.

True, but then again reviewers are more likely to look at such a
touch-all patch when there's not more pressing patches to be reviewed
before Hard Freeze.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] target-*/cpu.h: remove cs_base for other targets

[li guang]

在 2013-04-24三的 16:22 +0200，Andreas Färber写道：
> Am 24.04.2013 09:40, schrieb li guang:
> > 在 2013-04-24三的 08:36 +0100，Peter Maydell写道：
> >> On 24 April 2013 08:32, li guang <lig.fnst@cn.fujitsu.com> wrote:
> >>> I think even others want to use something like you said,
> >>> it should not 'cs_base', or, it's a bad name.
> >>
> >> Yes, this is why I said "has a less than helpful name".
> >>
> >>>>
> >>>>>>> --- a/target-sparc/cpu.h
> >>>>>>> +++ b/target-sparc/cpu.h
> >>>>>>> @@ -715,7 +715,7 @@ trap_state* cpu_tsptr(CPUSPARCState* env);
> >>>>>>>  #define TB_FLAG_AM_ENABLED (1 << 5)
> >>>>>>>
> >>>>>>>  static inline void cpu_get_tb_cpu_state(CPUSPARCState *env, target_ulong *pc,
> >>>>>>> -                                        target_ulong *cs_base, int *flags)
> >>>>>>> +                                        int *flags)
> >>>>>>>  {
> >>>>>>>      *pc = env->pc;
> >>>>>>>      *cs_base = env->npc;
> >>
> >>>> You clearly have a problem with your compile and test
> >>>> process then, because it is clear from the patch that
> >>>> you've removed the cs_base argument from this function
> >>>> but the function still has a use of 'cs_base' in it.
> >>>
> >>> ???, sorry, where do I miss 'cs_base' removing?
> >>
> >> Last quoted line of source: "*cs_base = env->npc".
> > 
> > OK, thanks!
> > that remove by overshoot script!
> 
> Some general reminders:
> 
> We're in Soft Freeze, so in general no new big patch series will go into
> 1.5 unless there's a maintainer willing to take care of it - for i386
> there is none, and random code cleanups do not look like something we
> must absolutely have in the release last minute. At least no one brought
> up on yesterday's call that this is a must-have, so maybe after the
> release would be a better time to let people review this?
> 
> Whenever you send more than one patch, please include a cover letter.
> 
> When you resend a series modified, please include a version such as v2,
> v3, etc. and a change log in the cover letter rather than resending with
> [updated] or in a way that can't be distinguished at all.
> 
> You're expected to assure that your patches compile and don't break
> `make check` at least. Repeatedly sending patches that cannot possibly
> build okay means you're either overlooking error output from your build
> or maybe building the wrong source directory?
> 
> Generally, touching the innards of TCG requires a good code
> understanding and testing of multiple targets; personally I try to avoid
> unnecessary changes for fear of breaking rare corner cases. ;)
> 

OK, I will, Thanks!

Re: [Qemu-devel] [PATCH 1/2] cpu-exec: rid cs_base of TranslationBlock

[Paolo Bonzini]

Il 24/04/2013 03:48, liguang ha scritto:
> cs_base is only meaningful for target-i386/sparc,
> so, get rid of cs_base for other target

This is really ugly, we're trying to get less target-dependent code
outside target-*, not more.

Also, please limit the number of people that you CC.

Paolo

> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  cpu-exec.c              |   26 ++++++++++++++++++--------
>  exec.c                  |    6 +++---
>  hw/i386/kvmvapic.c      |    6 ++----
>  include/exec/exec-all.h |    5 +++--
>  target-i386/cpu.h       |    6 ++----
>  translate-all.c         |   24 ++++++++++++------------
>  6 files changed, 40 insertions(+), 33 deletions(-)
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 31c089d..f3c1d1c 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -84,7 +84,7 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
>      if (max_cycles > CF_COUNT_MASK)
>          max_cycles = CF_COUNT_MASK;
>  
> -    tb = tb_gen_code(env, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
> +    tb = tb_gen_code(env, orig_tb->pc, orig_tb->flags,
>                       max_cycles);
>      cpu->current_tb = tb;
>      /* execute the generated code */
> @@ -96,7 +96,6 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
>  
>  static TranslationBlock *tb_find_slow(CPUArchState *env,
>                                        target_ulong pc,
> -                                      target_ulong cs_base,
>                                        uint64_t flags)
>  {
>      TranslationBlock *tb, **ptb1;
> @@ -117,7 +116,12 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
>              goto not_found;
>          if (tb->pc == pc &&
>              tb->page_addr[0] == phys_page1 &&
> -            tb->cs_base == cs_base &&
> +#if defined(TARGET_I386)
> +            tb->cs_base == env->segs[R_CS].base &&
> +#endif
> +#if defined(TARGET_SPARC)
> +            tb->cs_base == env->npc &&
> +#endif
>              tb->flags == flags) {
>              /* check next page if needed */
>              if (tb->page_addr[1] != -1) {
> @@ -136,7 +140,7 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
>      }
>   not_found:
>     /* if no translated code available, then translate it now */
> -    tb = tb_gen_code(env, pc, cs_base, flags, 0);
> +    tb = tb_gen_code(env, pc, flags, 0);
>  
>   found:
>      /* Move the last found TB to the head of the list */
> @@ -153,17 +157,23 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
>  static inline TranslationBlock *tb_find_fast(CPUArchState *env)
>  {
>      TranslationBlock *tb;
> -    target_ulong cs_base, pc;
> +    target_ulong pc;
>      int flags;
>  
>      /* we record a subset of the CPU state. It will
>         always be the same before a given translated block
>         is executed. */
> -    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
> +    cpu_get_tb_cpu_state(env, &pc, &flags);
>      tb = env->tb_jmp_cache[tb_jmp_cache_hash_func(pc)];
> -    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
> +    if (unlikely(!tb || tb->pc != pc ||
> +#if defined(TARGET_I386)
> +                 tb->cs_base != env->segs[R_CS].base ||
> +#endif
> +#if defined(TARGET_SPARC)
> +                 tb->cs_base != env->npc ||
> +#endif
>                   tb->flags != flags)) {
> -        tb = tb_find_slow(env, pc, cs_base, flags);
> +        tb = tb_find_slow(env, pc, flags);
>      }
>      return tb;
>  }
> diff --git a/exec.c b/exec.c
> index fa1e0c3..a14db2c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1471,7 +1471,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
>  static void check_watchpoint(int offset, int len_mask, int flags)
>  {
>      CPUArchState *env = cpu_single_env;
> -    target_ulong pc, cs_base;
> +    target_ulong pc;
>      target_ulong vaddr;
>      CPUWatchpoint *wp;
>      int cpu_flags;
> @@ -1495,8 +1495,8 @@ static void check_watchpoint(int offset, int len_mask, int flags)
>                      env->exception_index = EXCP_DEBUG;
>                      cpu_loop_exit(env);
>                  } else {
> -                    cpu_get_tb_cpu_state(env, &pc, &cs_base, &cpu_flags);
> -                    tb_gen_code(env, pc, cs_base, cpu_flags, 1);
> +                    cpu_get_tb_cpu_state(env, &pc, &cpu_flags);
> +                    tb_gen_code(env, pc, cpu_flags, 1);
>                      cpu_resume_from_signal(env, NULL);
>                  }
>              }
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index ed9b448..8b4260e 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -388,7 +388,6 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
>      uint8_t opcode[2];
>      uint32_t imm32;
>      target_ulong current_pc = 0;
> -    target_ulong current_cs_base = 0;
>      int current_flags = 0;
>  
>      if (smp_cpus == 1) {
> @@ -399,8 +398,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
>  
>      if (!kvm_enabled()) {
>          cpu_restore_state(env, env->mem_io_pc);
> -        cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> -                             &current_flags);
> +        cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
>      }
>  
>      pause_all_vcpus();
> @@ -440,7 +438,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
>  
>      if (!kvm_enabled()) {
>          cs->current_tb = NULL;
> -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> +        tb_gen_code(env, current_pc, current_flags, 1);
>          cpu_resume_from_signal(env, NULL);
>      }
>  }
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index e856191..560a7d1 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -85,8 +85,7 @@ bool cpu_restore_state(CPUArchState *env, uintptr_t searched_pc);
>  void QEMU_NORETURN cpu_resume_from_signal(CPUArchState *env1, void *puc);
>  void QEMU_NORETURN cpu_io_recompile(CPUArchState *env, uintptr_t retaddr);
>  TranslationBlock *tb_gen_code(CPUArchState *env, 
> -                              target_ulong pc, target_ulong cs_base, int flags,
> -                              int cflags);
> +                              target_ulong pc, int flags, int cflags);
>  void cpu_exec_init(CPUArchState *env);
>  void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
>  int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
> @@ -135,7 +134,9 @@ static inline void tlb_flush(CPUArchState *env, int flush_global)
>  
>  struct TranslationBlock {
>      target_ulong pc;   /* simulated PC corresponding to this block (EIP + CS base) */
> +#if defined(TARGET_I386) || defined(TARGET_SPARC)
>      target_ulong cs_base; /* CS base for this block */
> +#endif
>      uint64_t flags; /* flags defining in which context the code was generated */
>      uint16_t size;      /* size of target code for this block (1 <=
>                             size <= TARGET_PAGE_SIZE) */
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 2b4e319..3ce1b2e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1188,11 +1188,9 @@ static inline void cpu_pc_from_tb(CPUX86State *env, TranslationBlock *tb)
>      env->eip = tb->pc - tb->cs_base;
>  }
>  
> -static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc,
> -                                        target_ulong *cs_base, int *flags)
> +static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc, int *flags)
>  {
> -    *cs_base = env->segs[R_CS].base;
> -    *pc = *cs_base + env->eip;
> +    *pc = env->segs[R_CS].base + env->eip;
>      *flags = env->hflags |
>          (env->eflags & (IOPL_MASK | TF_MASK | RF_MASK | VM_MASK | AC_MASK));
>  }
> diff --git a/translate-all.c b/translate-all.c
> index a98c646..f9efbbd 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -931,8 +931,7 @@ static void build_page_bitmap(PageDesc *p)
>  }
>  
>  TranslationBlock *tb_gen_code(CPUArchState *env,
> -                              target_ulong pc, target_ulong cs_base,
> -                              int flags, int cflags)
> +                              target_ulong pc, int flags, int cflags)
>  {
>      TranslationBlock *tb;
>      uint8_t *tc_ptr;
> @@ -952,7 +951,12 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
>      }
>      tc_ptr = tcg_ctx.code_gen_ptr;
>      tb->tc_ptr = tc_ptr;
> -    tb->cs_base = cs_base;
> +#if defined(TARGET_I386)
> +    tb->cs_base = env->segs[R_CS].base;
> +#endif
> +#if defined(TARGET_SPARC)
> +    tb->cs_base = env->npc;
> +#endif
>      tb->flags = flags;
>      tb->cflags = cflags;
>      cpu_gen_code(env, tb, &code_gen_size);
> @@ -1007,7 +1011,6 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>      TranslationBlock *current_tb = NULL;
>      int current_tb_modified = 0;
>      target_ulong current_pc = 0;
> -    target_ulong current_cs_base = 0;
>      int current_flags = 0;
>  #endif /* TARGET_HAS_PRECISE_SMC */
>  
> @@ -1063,8 +1066,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>  
>                  current_tb_modified = 1;
>                  cpu_restore_state_from_tb(current_tb, env, env->mem_io_pc);
> -                cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> -                                     &current_flags);
> +                cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
>              }
>  #endif /* TARGET_HAS_PRECISE_SMC */
>              /* we need to do that to handle the case where a signal
> @@ -1099,7 +1101,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>             modifying the memory. It will ensure that it cannot modify
>             itself */
>          cpu->current_tb = NULL;
> -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> +        tb_gen_code(env, current_pc, current_flags, 1);
>          cpu_resume_from_signal(env, NULL);
>      }
>  #endif
> @@ -1149,7 +1151,6 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>      CPUState *cpu = NULL;
>      int current_tb_modified = 0;
>      target_ulong current_pc = 0;
> -    target_ulong current_cs_base = 0;
>      int current_flags = 0;
>  #endif
>  
> @@ -1181,8 +1182,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>  
>              current_tb_modified = 1;
>              cpu_restore_state_from_tb(current_tb, env, pc);
> -            cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> -                                 &current_flags);
> +            cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
>          }
>  #endif /* TARGET_HAS_PRECISE_SMC */
>          tb_phys_invalidate(tb, addr);
> @@ -1195,7 +1195,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
>             modifying the memory. It will ensure that it cannot modify
>             itself */
>          cpu->current_tb = NULL;
> -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> +        tb_gen_code(env, current_pc, current_flags, 1);
>          cpu_resume_from_signal(env, puc);
>      }
>  #endif
> @@ -1463,7 +1463,7 @@ void cpu_io_recompile(CPUArchState *env, uintptr_t retaddr)
>      tb_phys_invalidate(tb, -1);
>      /* FIXME: In theory this could raise an exception.  In practice
>         we have already translated the block once so it's probably ok.  */
> -    tb_gen_code(env, pc, cs_base, flags, cflags);
> +    tb_gen_code(env, pc, flags, cflags);
>      /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
>         the first in the TB) then we end up generating a whole new TB and
>         repeating the fault, which is horribly inefficient.
> 

Re: [Qemu-devel] [PATCH 1/2] cpu-exec: rid cs_base of TranslationBlock

[Aurelien Jarno]

On Wed, Apr 24, 2013 at 08:36:54AM +0200, Paolo Bonzini wrote:
> Il 24/04/2013 03:48, liguang ha scritto:
> > cs_base is only meaningful for target-i386/sparc,
> > so, get rid of cs_base for other target
> 
> This is really ugly, we're trying to get less target-dependent code
> outside target-*, not more.

Fully agreed. It also breaks the interface between the target and
cpu-exec.c by assuming tb->cs_base will always be env->segs[R_CS].base.

The only cleanup that can be done here is to rename cs_base into flags2
to make it less target dependent, and the code in cpu-exec.c should just
guarantee to choose tbs which match both flags and flags2 without
actually caring about the meaning of the values.

Even that way, this is the kind of cleanup touching a lot of code
without real benefit, except maybe for sparc which currently "abuse"
cs_base.

> Also, please limit the number of people that you CC.
> 
> Paolo
> 
> > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > ---
> >  cpu-exec.c              |   26 ++++++++++++++++++--------
> >  exec.c                  |    6 +++---
> >  hw/i386/kvmvapic.c      |    6 ++----
> >  include/exec/exec-all.h |    5 +++--
> >  target-i386/cpu.h       |    6 ++----
> >  translate-all.c         |   24 ++++++++++++------------
> >  6 files changed, 40 insertions(+), 33 deletions(-)
> > 
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 31c089d..f3c1d1c 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -84,7 +84,7 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
> >      if (max_cycles > CF_COUNT_MASK)
> >          max_cycles = CF_COUNT_MASK;
> >  
> > -    tb = tb_gen_code(env, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
> > +    tb = tb_gen_code(env, orig_tb->pc, orig_tb->flags,
> >                       max_cycles);
> >      cpu->current_tb = tb;
> >      /* execute the generated code */
> > @@ -96,7 +96,6 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
> >  
> >  static TranslationBlock *tb_find_slow(CPUArchState *env,
> >                                        target_ulong pc,
> > -                                      target_ulong cs_base,
> >                                        uint64_t flags)
> >  {
> >      TranslationBlock *tb, **ptb1;
> > @@ -117,7 +116,12 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
> >              goto not_found;
> >          if (tb->pc == pc &&
> >              tb->page_addr[0] == phys_page1 &&
> > -            tb->cs_base == cs_base &&
> > +#if defined(TARGET_I386)
> > +            tb->cs_base == env->segs[R_CS].base &&
> > +#endif
> > +#if defined(TARGET_SPARC)
> > +            tb->cs_base == env->npc &&
> > +#endif
> >              tb->flags == flags) {
> >              /* check next page if needed */
> >              if (tb->page_addr[1] != -1) {
> > @@ -136,7 +140,7 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
> >      }
> >   not_found:
> >     /* if no translated code available, then translate it now */
> > -    tb = tb_gen_code(env, pc, cs_base, flags, 0);
> > +    tb = tb_gen_code(env, pc, flags, 0);
> >  
> >   found:
> >      /* Move the last found TB to the head of the list */
> > @@ -153,17 +157,23 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
> >  static inline TranslationBlock *tb_find_fast(CPUArchState *env)
> >  {
> >      TranslationBlock *tb;
> > -    target_ulong cs_base, pc;
> > +    target_ulong pc;
> >      int flags;
> >  
> >      /* we record a subset of the CPU state. It will
> >         always be the same before a given translated block
> >         is executed. */
> > -    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
> > +    cpu_get_tb_cpu_state(env, &pc, &flags);
> >      tb = env->tb_jmp_cache[tb_jmp_cache_hash_func(pc)];
> > -    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
> > +    if (unlikely(!tb || tb->pc != pc ||
> > +#if defined(TARGET_I386)
> > +                 tb->cs_base != env->segs[R_CS].base ||
> > +#endif
> > +#if defined(TARGET_SPARC)
> > +                 tb->cs_base != env->npc ||
> > +#endif
> >                   tb->flags != flags)) {
> > -        tb = tb_find_slow(env, pc, cs_base, flags);
> > +        tb = tb_find_slow(env, pc, flags);
> >      }
> >      return tb;
> >  }
> > diff --git a/exec.c b/exec.c
> > index fa1e0c3..a14db2c 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1471,7 +1471,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
> >  static void check_watchpoint(int offset, int len_mask, int flags)
> >  {
> >      CPUArchState *env = cpu_single_env;
> > -    target_ulong pc, cs_base;
> > +    target_ulong pc;
> >      target_ulong vaddr;
> >      CPUWatchpoint *wp;
> >      int cpu_flags;
> > @@ -1495,8 +1495,8 @@ static void check_watchpoint(int offset, int len_mask, int flags)
> >                      env->exception_index = EXCP_DEBUG;
> >                      cpu_loop_exit(env);
> >                  } else {
> > -                    cpu_get_tb_cpu_state(env, &pc, &cs_base, &cpu_flags);
> > -                    tb_gen_code(env, pc, cs_base, cpu_flags, 1);
> > +                    cpu_get_tb_cpu_state(env, &pc, &cpu_flags);
> > +                    tb_gen_code(env, pc, cpu_flags, 1);
> >                      cpu_resume_from_signal(env, NULL);
> >                  }
> >              }
> > diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> > index ed9b448..8b4260e 100644
> > --- a/hw/i386/kvmvapic.c
> > +++ b/hw/i386/kvmvapic.c
> > @@ -388,7 +388,6 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> >      uint8_t opcode[2];
> >      uint32_t imm32;
> >      target_ulong current_pc = 0;
> > -    target_ulong current_cs_base = 0;
> >      int current_flags = 0;
> >  
> >      if (smp_cpus == 1) {
> > @@ -399,8 +398,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> >  
> >      if (!kvm_enabled()) {
> >          cpu_restore_state(env, env->mem_io_pc);
> > -        cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> > -                             &current_flags);
> > +        cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
> >      }
> >  
> >      pause_all_vcpus();
> > @@ -440,7 +438,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> >  
> >      if (!kvm_enabled()) {
> >          cs->current_tb = NULL;
> > -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> > +        tb_gen_code(env, current_pc, current_flags, 1);
> >          cpu_resume_from_signal(env, NULL);
> >      }
> >  }
> > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > index e856191..560a7d1 100644
> > --- a/include/exec/exec-all.h
> > +++ b/include/exec/exec-all.h
> > @@ -85,8 +85,7 @@ bool cpu_restore_state(CPUArchState *env, uintptr_t searched_pc);
> >  void QEMU_NORETURN cpu_resume_from_signal(CPUArchState *env1, void *puc);
> >  void QEMU_NORETURN cpu_io_recompile(CPUArchState *env, uintptr_t retaddr);
> >  TranslationBlock *tb_gen_code(CPUArchState *env, 
> > -                              target_ulong pc, target_ulong cs_base, int flags,
> > -                              int cflags);
> > +                              target_ulong pc, int flags, int cflags);
> >  void cpu_exec_init(CPUArchState *env);
> >  void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
> >  int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
> > @@ -135,7 +134,9 @@ static inline void tlb_flush(CPUArchState *env, int flush_global)
> >  
> >  struct TranslationBlock {
> >      target_ulong pc;   /* simulated PC corresponding to this block (EIP + CS base) */
> > +#if defined(TARGET_I386) || defined(TARGET_SPARC)
> >      target_ulong cs_base; /* CS base for this block */
> > +#endif
> >      uint64_t flags; /* flags defining in which context the code was generated */
> >      uint16_t size;      /* size of target code for this block (1 <=
> >                             size <= TARGET_PAGE_SIZE) */
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 2b4e319..3ce1b2e 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1188,11 +1188,9 @@ static inline void cpu_pc_from_tb(CPUX86State *env, TranslationBlock *tb)
> >      env->eip = tb->pc - tb->cs_base;
> >  }
> >  
> > -static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc,
> > -                                        target_ulong *cs_base, int *flags)
> > +static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc, int *flags)
> >  {
> > -    *cs_base = env->segs[R_CS].base;
> > -    *pc = *cs_base + env->eip;
> > +    *pc = env->segs[R_CS].base + env->eip;
> >      *flags = env->hflags |
> >          (env->eflags & (IOPL_MASK | TF_MASK | RF_MASK | VM_MASK | AC_MASK));
> >  }
> > diff --git a/translate-all.c b/translate-all.c
> > index a98c646..f9efbbd 100644
> > --- a/translate-all.c
> > +++ b/translate-all.c
> > @@ -931,8 +931,7 @@ static void build_page_bitmap(PageDesc *p)
> >  }
> >  
> >  TranslationBlock *tb_gen_code(CPUArchState *env,
> > -                              target_ulong pc, target_ulong cs_base,
> > -                              int flags, int cflags)
> > +                              target_ulong pc, int flags, int cflags)
> >  {
> >      TranslationBlock *tb;
> >      uint8_t *tc_ptr;
> > @@ -952,7 +951,12 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
> >      }
> >      tc_ptr = tcg_ctx.code_gen_ptr;
> >      tb->tc_ptr = tc_ptr;
> > -    tb->cs_base = cs_base;
> > +#if defined(TARGET_I386)
> > +    tb->cs_base = env->segs[R_CS].base;
> > +#endif
> > +#if defined(TARGET_SPARC)
> > +    tb->cs_base = env->npc;
> > +#endif
> >      tb->flags = flags;
> >      tb->cflags = cflags;
> >      cpu_gen_code(env, tb, &code_gen_size);
> > @@ -1007,7 +1011,6 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
> >      TranslationBlock *current_tb = NULL;
> >      int current_tb_modified = 0;
> >      target_ulong current_pc = 0;
> > -    target_ulong current_cs_base = 0;
> >      int current_flags = 0;
> >  #endif /* TARGET_HAS_PRECISE_SMC */
> >  
> > @@ -1063,8 +1066,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
> >  
> >                  current_tb_modified = 1;
> >                  cpu_restore_state_from_tb(current_tb, env, env->mem_io_pc);
> > -                cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> > -                                     &current_flags);
> > +                cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
> >              }
> >  #endif /* TARGET_HAS_PRECISE_SMC */
> >              /* we need to do that to handle the case where a signal
> > @@ -1099,7 +1101,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
> >             modifying the memory. It will ensure that it cannot modify
> >             itself */
> >          cpu->current_tb = NULL;
> > -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> > +        tb_gen_code(env, current_pc, current_flags, 1);
> >          cpu_resume_from_signal(env, NULL);
> >      }
> >  #endif
> > @@ -1149,7 +1151,6 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
> >      CPUState *cpu = NULL;
> >      int current_tb_modified = 0;
> >      target_ulong current_pc = 0;
> > -    target_ulong current_cs_base = 0;
> >      int current_flags = 0;
> >  #endif
> >  
> > @@ -1181,8 +1182,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
> >  
> >              current_tb_modified = 1;
> >              cpu_restore_state_from_tb(current_tb, env, pc);
> > -            cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> > -                                 &current_flags);
> > +            cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
> >          }
> >  #endif /* TARGET_HAS_PRECISE_SMC */
> >          tb_phys_invalidate(tb, addr);
> > @@ -1195,7 +1195,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
> >             modifying the memory. It will ensure that it cannot modify
> >             itself */
> >          cpu->current_tb = NULL;
> > -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> > +        tb_gen_code(env, current_pc, current_flags, 1);
> >          cpu_resume_from_signal(env, puc);
> >      }
> >  #endif
> > @@ -1463,7 +1463,7 @@ void cpu_io_recompile(CPUArchState *env, uintptr_t retaddr)
> >      tb_phys_invalidate(tb, -1);
> >      /* FIXME: In theory this could raise an exception.  In practice
> >         we have already translated the block once so it's probably ok.  */
> > -    tb_gen_code(env, pc, cs_base, flags, cflags);
> > +    tb_gen_code(env, pc, flags, cflags);
> >      /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
> >         the first in the TB) then we end up generating a whole new TB and
> >         repeating the fault, which is horribly inefficient.
> > 
> 
> 
> 

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

Re: [Qemu-devel] [PATCH 1/2] cpu-exec: rid cs_base of TranslationBlock

[li guang]

在 2013-04-24三的 09:11 +0200，Aurelien Jarno写道：
> On Wed, Apr 24, 2013 at 08:36:54AM +0200, Paolo Bonzini wrote:
> > Il 24/04/2013 03:48, liguang ha scritto:
> > > cs_base is only meaningful for target-i386/sparc,
> > > so, get rid of cs_base for other target
> > 
> > This is really ugly, we're trying to get less target-dependent code
> > outside target-*, not more.

I think it's easy to be arch independent by just
call a generic function instead of "#if defined(*)",
and archs can implement their own specific in this function.


> 
> Fully agreed. It also breaks the interface between the target and
> cpu-exec.c by assuming tb->cs_base will always be env->segs[R_CS].base.
> 

I'm not going to assume that (maybe it's the fact),
I did some random tests, seems break nothing.

> The only cleanup that can be done here is to rename cs_base into flags2
> to make it less target dependent, and the code in cpu-exec.c should just
> guarantee to choose tbs which match both flags and flags2 without
> actually caring about the meaning of the values.
> 
> Even that way, this is the kind of cleanup touching a lot of code
> without real benefit, except maybe for sparc which currently "abuse"
> cs_base.
> 
> > Also, please limit the number of people that you CC.
> > 
> > Paolo
> > 
> > > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> > > ---
> > >  cpu-exec.c              |   26 ++++++++++++++++++--------
> > >  exec.c                  |    6 +++---
> > >  hw/i386/kvmvapic.c      |    6 ++----
> > >  include/exec/exec-all.h |    5 +++--
> > >  target-i386/cpu.h       |    6 ++----
> > >  translate-all.c         |   24 ++++++++++++------------
> > >  6 files changed, 40 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/cpu-exec.c b/cpu-exec.c
> > > index 31c089d..f3c1d1c 100644
> > > --- a/cpu-exec.c
> > > +++ b/cpu-exec.c
> > > @@ -84,7 +84,7 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
> > >      if (max_cycles > CF_COUNT_MASK)
> > >          max_cycles = CF_COUNT_MASK;
> > >  
> > > -    tb = tb_gen_code(env, orig_tb->pc, orig_tb->cs_base, orig_tb->flags,
> > > +    tb = tb_gen_code(env, orig_tb->pc, orig_tb->flags,
> > >                       max_cycles);
> > >      cpu->current_tb = tb;
> > >      /* execute the generated code */
> > > @@ -96,7 +96,6 @@ static void cpu_exec_nocache(CPUArchState *env, int max_cycles,
> > >  
> > >  static TranslationBlock *tb_find_slow(CPUArchState *env,
> > >                                        target_ulong pc,
> > > -                                      target_ulong cs_base,
> > >                                        uint64_t flags)
> > >  {
> > >      TranslationBlock *tb, **ptb1;
> > > @@ -117,7 +116,12 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
> > >              goto not_found;
> > >          if (tb->pc == pc &&
> > >              tb->page_addr[0] == phys_page1 &&
> > > -            tb->cs_base == cs_base &&
> > > +#if defined(TARGET_I386)
> > > +            tb->cs_base == env->segs[R_CS].base &&
> > > +#endif
> > > +#if defined(TARGET_SPARC)
> > > +            tb->cs_base == env->npc &&
> > > +#endif
> > >              tb->flags == flags) {
> > >              /* check next page if needed */
> > >              if (tb->page_addr[1] != -1) {
> > > @@ -136,7 +140,7 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
> > >      }
> > >   not_found:
> > >     /* if no translated code available, then translate it now */
> > > -    tb = tb_gen_code(env, pc, cs_base, flags, 0);
> > > +    tb = tb_gen_code(env, pc, flags, 0);
> > >  
> > >   found:
> > >      /* Move the last found TB to the head of the list */
> > > @@ -153,17 +157,23 @@ static TranslationBlock *tb_find_slow(CPUArchState *env,
> > >  static inline TranslationBlock *tb_find_fast(CPUArchState *env)
> > >  {
> > >      TranslationBlock *tb;
> > > -    target_ulong cs_base, pc;
> > > +    target_ulong pc;
> > >      int flags;
> > >  
> > >      /* we record a subset of the CPU state. It will
> > >         always be the same before a given translated block
> > >         is executed. */
> > > -    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
> > > +    cpu_get_tb_cpu_state(env, &pc, &flags);
> > >      tb = env->tb_jmp_cache[tb_jmp_cache_hash_func(pc)];
> > > -    if (unlikely(!tb || tb->pc != pc || tb->cs_base != cs_base ||
> > > +    if (unlikely(!tb || tb->pc != pc ||
> > > +#if defined(TARGET_I386)
> > > +                 tb->cs_base != env->segs[R_CS].base ||
> > > +#endif
> > > +#if defined(TARGET_SPARC)
> > > +                 tb->cs_base != env->npc ||
> > > +#endif
> > >                   tb->flags != flags)) {
> > > -        tb = tb_find_slow(env, pc, cs_base, flags);
> > > +        tb = tb_find_slow(env, pc, flags);
> > >      }
> > >      return tb;
> > >  }
> > > diff --git a/exec.c b/exec.c
> > > index fa1e0c3..a14db2c 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1471,7 +1471,7 @@ static const MemoryRegionOps notdirty_mem_ops = {
> > >  static void check_watchpoint(int offset, int len_mask, int flags)
> > >  {
> > >      CPUArchState *env = cpu_single_env;
> > > -    target_ulong pc, cs_base;
> > > +    target_ulong pc;
> > >      target_ulong vaddr;
> > >      CPUWatchpoint *wp;
> > >      int cpu_flags;
> > > @@ -1495,8 +1495,8 @@ static void check_watchpoint(int offset, int len_mask, int flags)
> > >                      env->exception_index = EXCP_DEBUG;
> > >                      cpu_loop_exit(env);
> > >                  } else {
> > > -                    cpu_get_tb_cpu_state(env, &pc, &cs_base, &cpu_flags);
> > > -                    tb_gen_code(env, pc, cs_base, cpu_flags, 1);
> > > +                    cpu_get_tb_cpu_state(env, &pc, &cpu_flags);
> > > +                    tb_gen_code(env, pc, cpu_flags, 1);
> > >                      cpu_resume_from_signal(env, NULL);
> > >                  }
> > >              }
> > > diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> > > index ed9b448..8b4260e 100644
> > > --- a/hw/i386/kvmvapic.c
> > > +++ b/hw/i386/kvmvapic.c
> > > @@ -388,7 +388,6 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> > >      uint8_t opcode[2];
> > >      uint32_t imm32;
> > >      target_ulong current_pc = 0;
> > > -    target_ulong current_cs_base = 0;
> > >      int current_flags = 0;
> > >  
> > >      if (smp_cpus == 1) {
> > > @@ -399,8 +398,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> > >  
> > >      if (!kvm_enabled()) {
> > >          cpu_restore_state(env, env->mem_io_pc);
> > > -        cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> > > -                             &current_flags);
> > > +        cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
> > >      }
> > >  
> > >      pause_all_vcpus();
> > > @@ -440,7 +438,7 @@ static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
> > >  
> > >      if (!kvm_enabled()) {
> > >          cs->current_tb = NULL;
> > > -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> > > +        tb_gen_code(env, current_pc, current_flags, 1);
> > >          cpu_resume_from_signal(env, NULL);
> > >      }
> > >  }
> > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> > > index e856191..560a7d1 100644
> > > --- a/include/exec/exec-all.h
> > > +++ b/include/exec/exec-all.h
> > > @@ -85,8 +85,7 @@ bool cpu_restore_state(CPUArchState *env, uintptr_t searched_pc);
> > >  void QEMU_NORETURN cpu_resume_from_signal(CPUArchState *env1, void *puc);
> > >  void QEMU_NORETURN cpu_io_recompile(CPUArchState *env, uintptr_t retaddr);
> > >  TranslationBlock *tb_gen_code(CPUArchState *env, 
> > > -                              target_ulong pc, target_ulong cs_base, int flags,
> > > -                              int cflags);
> > > +                              target_ulong pc, int flags, int cflags);
> > >  void cpu_exec_init(CPUArchState *env);
> > >  void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
> > >  int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
> > > @@ -135,7 +134,9 @@ static inline void tlb_flush(CPUArchState *env, int flush_global)
> > >  
> > >  struct TranslationBlock {
> > >      target_ulong pc;   /* simulated PC corresponding to this block (EIP + CS base) */
> > > +#if defined(TARGET_I386) || defined(TARGET_SPARC)
> > >      target_ulong cs_base; /* CS base for this block */
> > > +#endif
> > >      uint64_t flags; /* flags defining in which context the code was generated */
> > >      uint16_t size;      /* size of target code for this block (1 <=
> > >                             size <= TARGET_PAGE_SIZE) */
> > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > > index 2b4e319..3ce1b2e 100644
> > > --- a/target-i386/cpu.h
> > > +++ b/target-i386/cpu.h
> > > @@ -1188,11 +1188,9 @@ static inline void cpu_pc_from_tb(CPUX86State *env, TranslationBlock *tb)
> > >      env->eip = tb->pc - tb->cs_base;
> > >  }
> > >  
> > > -static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc,
> > > -                                        target_ulong *cs_base, int *flags)
> > > +static inline void cpu_get_tb_cpu_state(CPUX86State *env, target_ulong *pc, int *flags)
> > >  {
> > > -    *cs_base = env->segs[R_CS].base;
> > > -    *pc = *cs_base + env->eip;
> > > +    *pc = env->segs[R_CS].base + env->eip;
> > >      *flags = env->hflags |
> > >          (env->eflags & (IOPL_MASK | TF_MASK | RF_MASK | VM_MASK | AC_MASK));
> > >  }
> > > diff --git a/translate-all.c b/translate-all.c
> > > index a98c646..f9efbbd 100644
> > > --- a/translate-all.c
> > > +++ b/translate-all.c
> > > @@ -931,8 +931,7 @@ static void build_page_bitmap(PageDesc *p)
> > >  }
> > >  
> > >  TranslationBlock *tb_gen_code(CPUArchState *env,
> > > -                              target_ulong pc, target_ulong cs_base,
> > > -                              int flags, int cflags)
> > > +                              target_ulong pc, int flags, int cflags)
> > >  {
> > >      TranslationBlock *tb;
> > >      uint8_t *tc_ptr;
> > > @@ -952,7 +951,12 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
> > >      }
> > >      tc_ptr = tcg_ctx.code_gen_ptr;
> > >      tb->tc_ptr = tc_ptr;
> > > -    tb->cs_base = cs_base;
> > > +#if defined(TARGET_I386)
> > > +    tb->cs_base = env->segs[R_CS].base;
> > > +#endif
> > > +#if defined(TARGET_SPARC)
> > > +    tb->cs_base = env->npc;
> > > +#endif
> > >      tb->flags = flags;
> > >      tb->cflags = cflags;
> > >      cpu_gen_code(env, tb, &code_gen_size);
> > > @@ -1007,7 +1011,6 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
> > >      TranslationBlock *current_tb = NULL;
> > >      int current_tb_modified = 0;
> > >      target_ulong current_pc = 0;
> > > -    target_ulong current_cs_base = 0;
> > >      int current_flags = 0;
> > >  #endif /* TARGET_HAS_PRECISE_SMC */
> > >  
> > > @@ -1063,8 +1066,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
> > >  
> > >                  current_tb_modified = 1;
> > >                  cpu_restore_state_from_tb(current_tb, env, env->mem_io_pc);
> > > -                cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> > > -                                     &current_flags);
> > > +                cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
> > >              }
> > >  #endif /* TARGET_HAS_PRECISE_SMC */
> > >              /* we need to do that to handle the case where a signal
> > > @@ -1099,7 +1101,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
> > >             modifying the memory. It will ensure that it cannot modify
> > >             itself */
> > >          cpu->current_tb = NULL;
> > > -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> > > +        tb_gen_code(env, current_pc, current_flags, 1);
> > >          cpu_resume_from_signal(env, NULL);
> > >      }
> > >  #endif
> > > @@ -1149,7 +1151,6 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
> > >      CPUState *cpu = NULL;
> > >      int current_tb_modified = 0;
> > >      target_ulong current_pc = 0;
> > > -    target_ulong current_cs_base = 0;
> > >      int current_flags = 0;
> > >  #endif
> > >  
> > > @@ -1181,8 +1182,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
> > >  
> > >              current_tb_modified = 1;
> > >              cpu_restore_state_from_tb(current_tb, env, pc);
> > > -            cpu_get_tb_cpu_state(env, &current_pc, &current_cs_base,
> > > -                                 &current_flags);
> > > +            cpu_get_tb_cpu_state(env, &current_pc, &current_flags);
> > >          }
> > >  #endif /* TARGET_HAS_PRECISE_SMC */
> > >          tb_phys_invalidate(tb, addr);
> > > @@ -1195,7 +1195,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr,
> > >             modifying the memory. It will ensure that it cannot modify
> > >             itself */
> > >          cpu->current_tb = NULL;
> > > -        tb_gen_code(env, current_pc, current_cs_base, current_flags, 1);
> > > +        tb_gen_code(env, current_pc, current_flags, 1);
> > >          cpu_resume_from_signal(env, puc);
> > >      }
> > >  #endif
> > > @@ -1463,7 +1463,7 @@ void cpu_io_recompile(CPUArchState *env, uintptr_t retaddr)
> > >      tb_phys_invalidate(tb, -1);
> > >      /* FIXME: In theory this could raise an exception.  In practice
> > >         we have already translated the block once so it's probably ok.  */
> > > -    tb_gen_code(env, pc, cs_base, flags, cflags);
> > > +    tb_gen_code(env, pc, flags, cflags);
> > >      /* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
> > >         the first in the TB) then we end up generating a whole new TB and
> > >         repeating the fault, which is horribly inefficient.
> > > 
> > 
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 1/2] cpu-exec: rid cs_base of TranslationBlock

[Peter Maydell]

On 24 April 2013 08:25, li guang <lig.fnst@cn.fujitsu.com> wrote:
> 在 2013-04-24三的 09:11 +0200，Aurelien Jarno写道：
>> On Wed, Apr 24, 2013 at 08:36:54AM +0200, Paolo Bonzini wrote:
>> > Il 24/04/2013 03:48, liguang ha scritto:
>> > > cs_base is only meaningful for target-i386/sparc,
>> > > so, get rid of cs_base for other target
>> >
>> > This is really ugly, we're trying to get less target-dependent code
>> > outside target-*, not more.
>
> I think it's easy to be arch independent by just
> call a generic function

We already have that, this is exactly what the target
cpu_get_tb_cpu_state() function is for! It abstracts
away the target's specific use of these fields, so the
common code can treat it as an opaque blob of state.

> I'm not going to assume that (maybe it's the fact),
> I did some random tests, seems break nothing.

You have absolutely broken things here -- if your random
tests didn't identify what then your testing process was
just not solid enough to find the corner cases.

-- PMM