qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: li guang
Subject: Re: [Qemu-devel] [PATCH 1/2] cpu-exec: rid cs_base of TranslationBlock
Date: Wed, 24 Apr 2013 15:25:51 +0800

在 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 <address@hidden>
> > > ---
> > >  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.
> > > 
> > 
> > 
> > 
> 






reply via email to

[Prev in Thread] Current Thread [Next in Thread]