Re: [Qemu-devel] RFC: cleanups: CPU_MEM_INDEX

[Thiemo Seufer <ths@networkno.de>]

J. Mayer wrote:
> Here's a proposal to add a int cpu_mem_index (CPUState *env) function in
> targets cpu.h header.
> The idea of this patch is:
> - avoid many #ifdef TARGET_xxx in exec-all.h and  softmmu_header.h then
> make the code more readable
> - avoid multiple implementation of the same code (3, in that particular
> case) this to avoid potential conflicts if the definition has to be
> updated for any reason (ie support for new memory access modes,
> emulation optimisation...)
> 
> Please comment.
> 
> -- 
> J. Mayer <l_indien@magic.fr>
> Never organized

> Index: cpu-exec.c
> ===================================================================
> RCS file: /sources/qemu/qemu/cpu-exec.c,v
> retrieving revision 1.119
> diff -u -d -d -p -r1.119 cpu-exec.c
> --- cpu-exec.c	8 Oct 2007 13:16:13 -0000	1.119
> +++ cpu-exec.c	9 Oct 2007 10:36:07 -0000
> @@ -885,7 +885,7 @@ static inline int handle_cpu_signal(unsi
>  
>      /* see if it is an MMU fault */
>      ret = cpu_x86_handle_mmu_fault(env, address, is_write,
> -                                   ((env->hflags & HF_CPL_MASK) == 3), 0);
> +                                   cpu_mem_index(env), 0);
>      if (ret < 0)
>          return 0; /* not an MMU fault */
>      if (ret == 0)
> @@ -1007,7 +1009,8 @@ static inline int handle_cpu_signal(unsi
>      }
>  
>      /* see if it is an MMU fault */
> -    ret = cpu_ppc_handle_mmu_fault(env, address, is_write, msr_pr, 0);
> +    ret = cpu_ppc_handle_mmu_fault(env, address, is_write,
> +                                   cpu_mem_index(env), 0);
>      if (ret < 0)
>          return 0; /* not an MMU fault */
>      if (ret == 0)
> @@ -1191,7 +1197,8 @@ static inline int handle_cpu_signal(unsi
>      }
>  
>      /* see if it is an MMU fault */
> -    ret = cpu_alpha_handle_mmu_fault(env, address, is_write, 1, 0);
> +    ret = cpu_alpha_handle_mmu_fault(env, address, is_write,
> +                                     cpu_mem_index(env), 0);

I have the feeling some architectures are missing here. :-)

>      if (ret < 0)
>          return 0; /* not an MMU fault */
>      if (ret == 0)
> Index: exec-all.h
> ===================================================================
> RCS file: /sources/qemu/qemu/exec-all.h,v
> retrieving revision 1.67
> diff -u -d -d -p -r1.67 exec-all.h
> --- exec-all.h	8 Oct 2007 13:16:14 -0000	1.67
> +++ exec-all.h	9 Oct 2007 10:36:07 -0000
> @@ -601,27 +606,7 @@ static inline target_ulong get_phys_addr
>      int is_user, index, pd;
>  
>      index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -#if defined(TARGET_I386)
> -    is_user = ((env->hflags & HF_CPL_MASK) == 3);
> -#elif defined (TARGET_PPC)
> -    is_user = msr_pr;
> -#elif defined (TARGET_MIPS)
> -    is_user = ((env->hflags & MIPS_HFLAG_MODE) == MIPS_HFLAG_UM);
> -#elif defined (TARGET_SPARC)
> -    is_user = (env->psrs == 0);
> -#elif defined (TARGET_ARM)
> -    is_user = ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_USR);
> -#elif defined (TARGET_SH4)
> -    is_user = ((env->sr & SR_MD) == 0);
> -#elif defined (TARGET_ALPHA)
> -    is_user = ((env->ps >> 3) & 3);
> -#elif defined (TARGET_M68K)
> -    is_user = ((env->sr & SR_S) == 0);
> -#elif defined (TARGET_CRIS)
> -    is_user = (0);
> -#else
> -#error unimplemented CPU
> -#endif
> +    is_user = cpu_mem_index(env);
>      if (__builtin_expect(env->tlb_table[is_user][index].addr_code !=
>                           (addr & TARGET_PAGE_MASK), 0)) {

I presume cpu_mem_index is supposed to do more than checking for
usermode. In that case, is_user should get renamed, and the
cpu_mem_index implementation of some (most?) CPUs should have a
FIXME comment as reminder to implement the missing MMU modes.

Other than that it looks good to me (and reminds me to check what the
supervisor mode on MIPS actually does now :-).


Thiemo