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

[J. Mayer]

On Wed, 2007-10-10 at 01:12 +0100, Thiemo Seufer wrote:
> 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 <address@hidden>
> > 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. :-)

Right, but as this code is used only with !CONFIG_SOFTMMU, I wonder if
we really want to give the cpu_mem_index to the called function, as the
CPU emulation code may not maintain it properly in that case. Then, I
choosed not to change the current behavior: when the index was given to
cpu_handle_mmu_fault (or when I think it's safe), I still does it. When
it was hardcoded to 1, I did let it hardcoded.

> >      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.

You're right, calling this variable is_user is only valid because this
code supposes it knows what cpu_mem_index means. For targets with more
than 2 modes of execution, this is not correct.
My first idea was to try not to change the code too much. After thinking
more about the problem, it appears to me that:
1/ in the softmmu routines, we should do no assumption about the
signification of the memory index
2/ then, softmmu routines should use an index and all exported
interfaces (ie tlb_fill and handle_mmu_fault) should take an index
instead of is_user as an argument.
3/ to maintain compatibility with the existing code, I choosed to add a
is_user variable inside most handle_mmu_fault implementation,
initialized with the value of the given index, which is then given to
the target mmu translation routines.
4/ to ease implementation of targets with more than 2 execution modes, I
choosed to define a per-target NB_MMU_MODES in each target_xxx/cpu.h
(instead of the hack for PowerPC 64 and Alpha that did pre-exist) and
add a local definition of the meaning of each mmu_index index. Then, for
PowerPC, I choosed to use the same convention than I do in translate.c,
which seems more logical to me, then: 0 => user, 1 => supervisor, 2 =>
hypervisor.
5/ to avoid confusion between the memory index used in the translation
context, which may contain more than the access mode information, and
the one used by the softmmu routines, I choosed to name the one used in
softmmu 'mmu_idx' (the one in target_xxx/translate.c is called mem_idx).
6/ I choosed to add a constant MMU_USER_IDX which is used in the
user-mode handle_cpu_signal routine, then addressing your first remark.

This patch solves a problem I had no solution to until today: how to add
new mmu modes (ie hypervisor for PowerPC 64, supervisor and executive
for Alpha) for some specific targets.

The result is a much more invasive patch but the is supposed (!) to;
1/ do not change the behavior of the current targets implementations
2/ be less hardcoded, more flexible and extensible for any specific
targets requirements.
As this new version of the patch could deadly break the softmmu mode and
I got no way to properly check all targets, I would greatly appreciate
that some do some tests for arm, cris, m68k, mips, sh4 and sparc
targets.

For now, I did a few tests, running Linux (debian hdd installation) for
PowerPC on PPC 603 & 750 in 32 bits and 64 bits mode emulation on x86
and amd64 and a few OSes using the i386-softmmu emulation (Linux Gentoo,
solaris 10 installation CDROM, ...), with success; then I guess I did
not deadly broke everything !
The good point is that PowerPC runs, considering it is the only target
for which I did change the mmu_idx semantics...

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

This updated patch gives the opportunity to define a per-target semantic
of the mmu_idx... Time to check what it means in actual CPU
implementation !!!! ;-)

Thanks in advance for any comments or improvment suggestions....

-- 
J. Mayer <address@hidden>
Never organized

cpu_mmu_index.diff
Description: Text Data