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

[J. Mayer]

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.

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