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

[J. Mayer]

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 <address@hidden>
> > > > 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....
> > 
-- 
J. Mayer <address@hidden>
Never organized

cpu_mmu_index.diff
Description: Text Data