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 <[EMAIL PROTECTED]> > > > > > 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 <[EMAIL PROTECTED]> Never organized