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

["J. Mayer" <l_indien@magic.fr>]

On Fri, 2007-10-12 at 09:01 +0200, J. Mayer wrote:
> On Thu, 2007-10-11 at 14:09 +0200, J. Mayer wrote:
> > On Wed, 2007-10-10 at 07:06 +0200, J. Mayer wrote:
> > > 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 <l_indien@magic.fr>
> > > > > Never organized
> > > > 
> > 
> > [...]
> > 
> > Here's an updated version of the patch. My comments about it stay valid,
> > with two additions:
> > 1/ when is user is needed to maintain compatibility with existing code,
> > I now define it as:
> > int is_user = mmu_idx == MMU_USER_IDX;
> > instead of just is_user = mmu_idx.
> > This definition will then remain correct even if the definition of the
> > MMU modes are later changed for a specific target
> > 2/ I now precompute the mmu_idx on PowerPC platform as it can never
> > change inside a single TB. This may save a few instructions for every
> > memory access. I guess the same optimisation can be made for the other
> > targets, but not knowing exactly when it would have to be recomputed,
> > for most targets, I prefer not to do this optimisation myself.
> 
> Here's another update, taking care of the commit I just made (which
> changes some of the target-xxx/cpu.h files).
> It also fixes an issue in softmmu_header; this was missing:
> 
>  #if (DATA_SIZE <= 4) && (TARGET_LONG_BITS == 32) && defined(__i386__)
> && \
> -    (ACCESS_TYPE <= 1) && defined(ASM_SOFTMMU)
> +    (ACCESS_TYPE < NB_MMU_MODES) && defined(ASM_SOFTMMU)
>  
>  #define CPU_TLB_ENTRY_BITS 4
>  
> As this affects only the i386 target which is defined with 2 MMU modes,
> the miss had no run-time consequence but was still a bug.
>  
> > > > 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....

What about this proposal ?
Any remarks, bugs, ... ?
I'd really like to apply it and go on working on some other
improvments...
And I'd like to commit my provision code for hypervisor mode for PowerPC
and the 4 running modes for Alpha, which depend on this one.
(the patch is still the same than the last one I did post here).

-- 
J. Mayer <l_indien@magic.fr>
Never organized