Re: [Qemu-devel] [PATCH 5/6] isolate mmu code in arch-specific function

[Glauber Costa]

On Tue, May 27, 2008 at 12:30 PM, Fabrice Bellard <address@hidden> wrote:
> This is not the right solution for two reasons:
> - This part of the code is speed critical, so the function must be at least
> static inline.
> - The related code is never used.
>
> So just removing the piece of code is the right solution :-)

If it can really be safely removed, this sounds like the best solution
to me by far.

> Fabrice.
>
> Glauber Costa wrote:
>>
>> Simplify cpu_exec by removing a TARGET_I386 block, responsible
>> for conditionally resetting the mmu, and enclosing it inside
>> a function.
>> ---
>>  cpu-exec.c            |   17 +----------------
>>  exec-all.h            |    1 +
>>  target-alpha/helper.c |    1 +
>>  target-arm/helper.c   |    1 +
>>  target-cris/helper.c  |    1 +
>>  target-i386/helper.c  |   19 +++++++++++++++++++
>>  target-m68k/helper.c  |    2 ++
>>  target-mips/helper.c  |    1 +
>>  target-ppc/helper.c   |    1 +
>>  target-sh4/helper.c   |    1 +
>>  target-sparc/helper.c |    2 ++
>>  11 files changed, 31 insertions(+), 16 deletions(-)
>>
>> Index: qemu/cpu-exec.c
>> ===================================================================
>> --- qemu.orig/cpu-exec.c
>> +++ qemu/cpu-exec.c
>> @@ -432,15 +432,7 @@ int cpu_exec(CPUState *env1)
>>  #endif
>>                 next_tb = tcg_qemu_tb_exec(tc_ptr);
>>                 env->current_tb = NULL;
>> -                /* reset soft MMU for next block (it can currently
>> -                   only be set by a memory fault) */
>> -#if defined(TARGET_I386) && !defined(CONFIG_SOFTMMU)
>> -                if (env->hflags & HF_SOFTMMU_MASK) {
>> -                    env->hflags &= ~HF_SOFTMMU_MASK;
>> -                    /* do not allow linking to another block */
>> -                    next_tb = 0;
>> -                }
>> -#endif
>> +                cpu_reset_mmu(env);
>>  #if defined(USE_KQEMU)
>>  #define MIN_CYCLE_BEFORE_SWITCH (100 * 1000)
>>                 if (kqemu_is_ok(env) &&
>> @@ -448,6 +440,7 @@ int cpu_exec(CPUState *env1)
>>                     cpu_loop_exit();
>>                 }
>>  #endif
>> +
>>             } /* for(;;) */
>>         } else {
>>             env_to_regs();
>> Index: qemu/exec-all.h
>> ===================================================================
>> --- qemu.orig/exec-all.h
>> +++ qemu/exec-all.h
>> @@ -93,6 +93,7 @@ void reset_tb(void);
>>   void cpu_load_flags(CPUState *env);
>>  void cpu_put_flags(CPUState *env);
>> +void cpu_reset_mmu(CPUState *env);
>>   int page_unprotect(target_ulong address, unsigned long pc, void *puc);
>>  void tb_invalidate_phys_page_range(target_phys_addr_t start,
>> target_phys_addr_t end,
>> Index: qemu/target-alpha/helper.c
>> ===================================================================
>> --- qemu.orig/target-alpha/helper.c
>> +++ qemu/target-alpha/helper.c
>> @@ -456,6 +456,7 @@ void cpu_dump_EA (target_ulong EA)
>>  int cpu_info_ip(CPUState *env, char *buf) { return 0; }
>>  void cpu_load_flags(CPUState *env) {}
>>  void cpu_put_flags(CPUState *env) {}
>> +void cpu_reset_mmu(CPUState *env) {}
>>   void arch_handle_interrupt_request(CPUState *env)
>>  {
>> Index: qemu/target-arm/helper.c
>> ===================================================================
>> --- qemu.orig/target-arm/helper.c
>> +++ qemu/target-arm/helper.c
>> @@ -2527,6 +2527,7 @@ int cpu_info_ip(CPUState *env, char *buf
>>  void cpu_load_flags(CPUState *env) { }
>>  /* XXX: Save/restore host fpu exception state?.  */
>>  void cpu_put_flags(CPUState *env) { }
>> +void cpu_reset_mmu(CPUState *env) {}
>>   void arch_handle_interrupt_request(CPUState *env)
>>  {
>> Index: qemu/target-cris/helper.c
>> ===================================================================
>> --- qemu.orig/target-cris/helper.c
>> +++ qemu/target-cris/helper.c
>> @@ -187,6 +187,7 @@ target_phys_addr_t cpu_get_phys_page_deb
>>  int  cpu_info_ip(CPUState *env, char *buf) { return 0; }
>>  void cpu_load_flags(CPUState *env) {}
>>  void cpu_put_flags(CPUState *env) {}
>> +void cpu_reset_mmu(CPUState *env) {}
>>   void arch_handle_interrupt_request(CPUState *env)
>>  {
>> Index: qemu/target-i386/helper.c
>> ===================================================================
>> --- qemu.orig/target-i386/helper.c
>> +++ qemu/target-i386/helper.c
>> @@ -4755,6 +4755,18 @@ void cpu_put_flags(CPUState *env)
>>     env->eflags = env->eflags | cc_table[CC_OP].compute_all() | (DF &
>> DF_MASK);
>>  }
>>  +void cpu_reset_mmu(CPUState *env)
>> +{
>> +    /* reset soft MMU for next block (it can currently
>> +       only be set by a memory fault) */
>> +#if !defined(CONFIG_SOFTMMU)
>> +    if (env->hflags & HF_SOFTMMU_MASK) {
>> +        env->hflags &= ~HF_SOFTMMU_MASK;
>> +        /* do not allow linking to another block */
>> +        reset_tb();
>> +    }
>> +#endif
>> +}
>>  #if defined(CONFIG_USER_ONLY)
>>   void helper_vmrun(void) Index: qemu/target-m68k/helper.c
>> ===================================================================
>> --- qemu.orig/target-m68k/helper.c
>> +++ qemu/target-m68k/helper.c
>> @@ -355,6 +355,8 @@ void cpu_put_flags(CPUState *env)
>>               | env->cc_dest | (env->cc_x << 4);
>>  }
>>  +void cpu_reset_mmu(CPUState *env) {}
>> +
>>  void arch_handle_interrupt_request(CPUState *env)
>>  {
>>     if (env->interrupt_request & CPU_INTERRUPT_HARD
>> Index: qemu/target-mips/helper.c
>> ===================================================================
>> --- qemu.orig/target-mips/helper.c
>> +++ qemu/target-mips/helper.c
>> @@ -645,6 +645,7 @@ int cpu_info_ip(CPUState *env, char *buf
>>   void cpu_put_flags(CPUState *env) { }
>>  void cpu_load_flags(CPUState *env) { }
>> +void cpu_reset_mmu(CPUState *env) { }
>>   void arch_handle_interrupt_request(CPUState *env)
>>  {
>> Index: qemu/target-ppc/helper.c
>> ===================================================================
>> --- qemu.orig/target-ppc/helper.c
>> +++ qemu/target-ppc/helper.c
>> @@ -2997,6 +2997,7 @@ int cpu_info_ip(CPUState *env, char *buf
>>   void cpu_load_flags(CPUState *env) { }
>>  void cpu_put_flags(CPUState *env) { }
>> +void cpu_reset_mmu(CPUState *env) { }
>>   void arch_handle_interrupt_request(CPUState *env)
>>  {
>> Index: qemu/target-sh4/helper.c
>> ===================================================================
>> --- qemu.orig/target-sh4/helper.c
>> +++ qemu/target-sh4/helper.c
>> @@ -538,6 +538,7 @@ void cpu_load_tlb(CPUState * env)
>>  int  cpu_info_ip(CPUState *env, char *buf) { return 0; }
>>  void cpu_load_flags(CPUState *env) {}
>>  void cpu_put_flags(CPUState *env) {}
>> +void cpu_reset_mmu(CPUState *env) {}
>>   void arch_handle_interrupt_request(CPUState *env)
>>  {
>> Index: qemu/target-sparc/helper.c
>> ===================================================================
>> --- qemu.orig/target-sparc/helper.c
>> +++ qemu/target-sparc/helper.c
>> @@ -1340,6 +1340,8 @@ void cpu_put_flags(CPUState *env)
>>  #endif
>>  }
>>  +void cpu_reset_mmu(CPUState *env) {}
>> +
>>  #ifdef TARGET_SPARC64
>>  #if !defined(CONFIG_USER_ONLY)
>>  #include "qemu-common.h"
>>
>>
>>
>
>
>
>



-- 
Glauber Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."