Re: [Qemu-devel] [RFC PATCH 05/10] exec: small adjustments for TCG separation

[Blue Swirl]

On Mon, Sep 17, 2012 at 4:00 PM, Paolo Bonzini <address@hidden> wrote:
> Add stubs for cpu_restore_state and tlb_fill, which should respectively
> have no effect and never be used outside TCG mode.
>
> Add assertions that TCG is enabled around code that calls to TCG from
> exec.c, so that the compiler can remove the entire functions as dead
> (the proper way to do this should be to move them to cpu-exec.c
> instead).
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  cpu-exec.c |  2 ++
>  exec-all.h | 20 ++++++++++++++++++--
>  exec.c     | 55 ++++++++++++++++++++++++++++++++++---------------------
>  3 file modificati, 54 inserzioni(+), 23 rimozioni(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 134b3c4..b047b6c 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -189,6 +189,8 @@ int cpu_exec(CPUArchState *env)
>      uint8_t *tc_ptr;
>      tcg_target_ulong next_tb;
>
> +    assert(tcg_enabled());
> +
>      if (env->halted) {
>          if (!cpu_has_work(env)) {
>              return EXCP_HALTED;
> diff --git a/exec-all.h b/exec-all.h
> index cecafa1..4c61ed1 100644
> --- a/exec-all.h
> +++ b/exec-all.h
> @@ -84,8 +84,6 @@ void restore_state_to_opc(CPUArchState *env, struct 
> TranslationBlock *tb,
>  void cpu_gen_init(void);
>  int cpu_gen_code(CPUArchState *env, struct TranslationBlock *tb,
>                   int *gen_code_size_ptr);
> -void cpu_restore_state(struct TranslationBlock *tb,
> -                       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,
> @@ -282,6 +280,16 @@ static inline void tb_add_jump(TranslationBlock *tb, int 
> n,
>
>  TranslationBlock *tb_find_pc(uintptr_t pc_ptr);
>
> +#ifdef CONFIG_TCG
> +void cpu_restore_state(struct TranslationBlock *tb,
> +                       CPUArchState *env, uintptr_t searched_pc);
> +#else
> +static inline void cpu_restore_state(struct TranslationBlock *tb,
> +                                     CPUArchState *env, uintptr_t 
> searched_pc)
> +{
> +}
> +#endif
> +
>  #include "qemu-lock.h"
>
>  extern spinlock_t tb_lock;
> @@ -316,8 +324,16 @@ uint64_t io_mem_read(struct MemoryRegion *mr, 
> target_phys_addr_t addr,
>  void io_mem_write(struct MemoryRegion *mr, target_phys_addr_t addr,
>                    uint64_t value, unsigned size);
>
> +#ifdef CONFIG_TCG
>  void tlb_fill(CPUArchState *env1, target_ulong addr, int is_write, int 
> mmu_idx,
>                uintptr_t retaddr);
> +#else
> +static inline void tlb_fill(CPUArchState *env1, target_ulong addr,
> +                            int is_write, int mmu_idx, uintptr_t retaddr)
> +{
> +    abort();
> +}
> +#endif
>
>  #include "softmmu_defs.h"
>
> diff --git a/exec.c b/exec.c
> index 1270d78..3e77de2 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -108,6 +108,8 @@ static unsigned long code_gen_buffer_size;
>  static unsigned long code_gen_buffer_max_size;
>  static uint8_t *code_gen_ptr;
>
> +CPUInterruptHandler cpu_interrupt_handler;
> +
>  #if !defined(CONFIG_USER_ONLY)
>  int phys_ram_fd;
>  static int in_migration;
> @@ -610,24 +612,6 @@ static void code_gen_alloc(unsigned long tb_size)
>      tbs = g_malloc(code_gen_max_blocks * sizeof(TranslationBlock));
>  }
>
> -/* Must be called before using the QEMU cpus. 'tb_size' is the size
> -   (in bytes) allocated to the translation buffer. Zero means default
> -   size. */
> -void tcg_exec_init(unsigned long tb_size)
> -{
> -    tcg_allowed = 1;
> -    cpu_gen_init();
> -    code_gen_alloc(tb_size);
> -    code_gen_ptr = code_gen_buffer;
> -    tcg_register_jit(code_gen_buffer, code_gen_buffer_size);
> -    page_init();
> -#if !defined(CONFIG_USER_ONLY) || !defined(CONFIG_USE_GUEST_BASE)
> -    /* There's no guest base to take into account, so go ahead and
> -       initialize the prologue now.  */
> -    tcg_prologue_init(&tcg_ctx);
> -#endif
> -}
> -
>  void cpu_exec_init_all(void)
>  {
>  #if !defined(CONFIG_USER_ONLY)
> @@ -1032,6 +1016,8 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
>      target_ulong virt_page2;
>      int code_gen_size;
>
> +    assert(tcg_enabled());
> +
>      phys_pc = get_page_addr_code(env, pc);
>      tb = tb_alloc(pc);
>      if (!tb) {
> @@ -1707,9 +1693,6 @@ static void tcg_handle_interrupt(CPUArchState *env, int 
> mask)
>          cpu_unlink_tb(env);
>      }
>  }
> -
> -CPUInterruptHandler cpu_interrupt_handler = tcg_handle_interrupt;
> -
>  #else /* CONFIG_USER_ONLY */
>
>  void cpu_interrupt(CPUArchState *env, int mask)
> @@ -2934,6 +2917,8 @@ static void check_watchpoint(int offset, int len_mask, 
> int flags)
>      CPUWatchpoint *wp;
>      int cpu_flags;
>
> +    assert(tcg_enabled());
> +
>      if (env->watchpoint_hit) {
>          /* We re-entered the check after replacing the TB. Now raise
>           * the debug interrupt so that is will trigger after the
> @@ -4112,6 +4097,8 @@ void cpu_io_recompile(CPUArchState *env, uintptr_t 
> retaddr)
>      target_ulong pc, cs_base;
>      uint64_t flags;
>
> +    assert(tcg_enabled());
> +
>      tb = tb_find_pc(retaddr);
>      if (!tb) {
>          cpu_abort(env, "cpu_io_recompile: could not find TB for pc=%p",
> @@ -4243,3 +4230,29 @@ bool cpu_physical_memory_is_io(target_phys_addr_t 
> phys_addr)
>               memory_region_is_romd(section->mr));
>  }
>  #endif
> +
> +/* Must be called before using the QEMU cpus. 'tb_size' is the size
> +   (in bytes) allocated to the translation buffer. Zero means default
> +   size. */
> +void tcg_exec_init(unsigned long tb_size)
> +{
> +#ifndef CONFIG_TCG
> +    /* We cannot yet use tcg_enabled() here, it is set below.  */
> +    return;

This leaves a lot of unreachable code after return, possibly
introducing warnings and with low enough optimization level, maybe
break linking. Please add #else and move #endif to end of the
function.

> +#endif
> +
> +    tcg_allowed = 1;
> +#ifndef CONFIG_USER_ONLY
> +    cpu_interrupt_handler = tcg_handle_interrupt;
> +#endif
> +    cpu_gen_init();
> +    code_gen_alloc(tb_size);
> +    code_gen_ptr = code_gen_buffer;
> +    tcg_register_jit(code_gen_buffer, code_gen_buffer_size);
> +    page_init();
> +#if !defined(CONFIG_USER_ONLY) || !defined(CONFIG_USE_GUEST_BASE)
> +    /* There's no guest base to take into account, so go ahead and
> +       initialize the prologue now.  */
> +    tcg_prologue_init(&tcg_ctx);
> +#endif
> +}
> --
> 1.7.12
>
>
>