Hi Richard, first of all, this is a very nice series. I really enjoy reading it, thank you very much. It makes the code is more readable and likely to be more performant. A nitpick below.
On Thu, Dec 17, 2015 at 9:57 PM, Richard Henderson <r...@twiddle.net> wrote: > This gives us a trivial way to access physical addresses > (aka "real addresses", in sun4v terminology) directly from > qemu_ld/st, without having to go through another helper. > > This also fixes a bug in get_physical_address_code where > it inferred NUCLEUS from env->tl instead of from mmu_idx. > > Signed-off-by: Richard Henderson <r...@twiddle.net> > --- > target-sparc/cpu.h | 18 +++++++--- > target-sparc/mmu_helper.c | 90 > +++++++++++++++++++++++++++++------------------ > 2 files changed, 69 insertions(+), 39 deletions(-) > > diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h > index 7f4d47f..b1222a1 100644 > --- a/target-sparc/cpu.h > +++ b/target-sparc/cpu.h > @@ -220,9 +220,9 @@ enum { > #define MAX_NWINDOWS 32 > > #if !defined(TARGET_SPARC64) > -#define NB_MMU_MODES 2 > +#define NB_MMU_MODES 3 > #else > -#define NB_MMU_MODES 6 > +#define NB_MMU_MODES 7 > typedef struct trap_state { > uint64_t tpc; > uint64_t tnpc; > @@ -612,11 +612,13 @@ int cpu_sparc_signal_handler(int host_signum, void > *pinfo, void *puc); > #define MMU_MODE4_SUFFIX _nucleus > #define MMU_HYPV_IDX 5 > #define MMU_MODE5_SUFFIX _hypv > +#define MMU_REAL_IDX 6 > #else > #define MMU_USER_IDX 0 > #define MMU_MODE0_SUFFIX _user > #define MMU_KERNEL_IDX 1 > #define MMU_MODE1_SUFFIX _kernel > +#define MMU_REAL_IDX 2 > #endif > > #if defined (TARGET_SPARC64) > @@ -641,9 +643,17 @@ static inline int cpu_mmu_index(CPUSPARCState *env1, > bool ifetch) > #if defined(CONFIG_USER_ONLY) > return MMU_USER_IDX; > #elif !defined(TARGET_SPARC64) > - return env1->psrs; > + if (!(env1->mmuregs[0] & MMU_E)) { > + return MMU_REAL_IDX; /* MMU disabled */ > + } else { > + return env1->psrs; > + } > #else > - if (env1->tl > 0) { > + if (ifetch > + ? !(env1->lsu & IMMU_E) || (env1->pstate & PS_RED) > + : !(env1->lsu & DMMU_E)) { > + return MMU_REAL_IDX; /* MMU disabled */ > + } else if (env1->tl > 0) { > return MMU_NUCLEUS_IDX; > } else if (cpu_hypervisor_mode(env1)) { > return MMU_HYPV_IDX; > diff --git a/target-sparc/mmu_helper.c b/target-sparc/mmu_helper.c > index 7495406..105f00d 100644 > --- a/target-sparc/mmu_helper.c > +++ b/target-sparc/mmu_helper.c > @@ -90,7 +90,7 @@ static int get_physical_address(CPUSPARCState *env, hwaddr > *physical, > > is_user = mmu_idx == MMU_USER_IDX; > > - if ((env->mmuregs[0] & MMU_E) == 0) { /* MMU disabled */ > + if (mmu_idx == MMU_REAL_IDX) { /* MMU bypass access */ > *page_size = TARGET_PAGE_SIZE; > /* Boot mode: instruction fetches are taken from PROM */ > if (rw == 2 && (env->mmuregs[0] & env->def->mmu_bm)) { > @@ -492,33 +492,40 @@ static int get_physical_address_data(CPUSPARCState *env, > unsigned int i; > uint64_t context; > uint64_t sfsr = 0; > + bool is_user = false; > > - int is_user = (mmu_idx == MMU_USER_IDX || > - mmu_idx == MMU_USER_SECONDARY_IDX); > - > - if ((env->lsu & DMMU_E) == 0) { /* DMMU disabled */ ^^^^ in case of ASI instructions, mmu_idx can come from dc->mem_idx, right? So the check may be still necessary here, > + switch (mmu_idx) { > + case MMU_REAL_IDX: > + /* MMU bypass access */ > *physical = ultrasparc_truncate_physical(address); > - *prot = PAGE_READ | PAGE_WRITE; > + *prot = PAGE_EXEC | PAGE_READ | PAGE_WRITE; > return 0; > - } > > - switch (mmu_idx) { > + case MMU_NUCLEUS_IDX: > + sfsr |= SFSR_CT_NUCLEUS; > + /* fallthru */ > + case MMU_HYPV_IDX: > + /* No context. */ > + context = 0; > + break; > case MMU_USER_IDX: > + is_user = true; > + /* fallthru */ > case MMU_KERNEL_IDX: > + /* PRIMARY context */ > context = env->dmmu.mmu_primary_context & 0x1fff; > sfsr |= SFSR_CT_PRIMARY; > break; > case MMU_USER_SECONDARY_IDX: > + is_user = true; > + /* fallthru */ > case MMU_KERNEL_SECONDARY_IDX: > + /* PRIMARY context */ > context = env->dmmu.mmu_secondary_context & 0x1fff; > sfsr |= SFSR_CT_SECONDARY; > break; > - case MMU_NUCLEUS_IDX: > - sfsr |= SFSR_CT_NUCLEUS; > - /* FALLTHRU */ > default: > - context = 0; > - break; > + g_assert_not_reached(); > } > > if (rw == 1) { > @@ -573,8 +580,8 @@ static int get_physical_address_data(CPUSPARCState *env, > } > > if (env->dmmu.sfsr & SFSR_VALID_BIT) { /* Fault status register > */ > - sfsr |= SFSR_OW_BIT; /* overflow (not read before > - another fault) */ > + /* overflow (not read before another fault) */ > + sfsr |= SFSR_OW_BIT; > } > > if (env->pstate & PS_PRIV) { > @@ -611,23 +618,41 @@ static int get_physical_address_code(CPUSPARCState *env, > CPUState *cs = CPU(sparc_env_get_cpu(env)); > unsigned int i; > uint64_t context; > + uint64_t sfsr = 0; > + bool is_user = false; > > - int is_user = (mmu_idx == MMU_USER_IDX || > - mmu_idx == MMU_USER_SECONDARY_IDX); > - > - if ((env->lsu & IMMU_E) == 0 || (env->pstate & PS_RED) != 0) { > - /* IMMU disabled */ and here. Artyom > + switch (mmu_idx) { > + case MMU_REAL_IDX: > + /* MMU bypass access */ > *physical = ultrasparc_truncate_physical(address); > - *prot = PAGE_EXEC; > + *prot = PAGE_EXEC | PAGE_READ | PAGE_WRITE; > return 0; > - } > > - if (env->tl == 0) { > + case MMU_NUCLEUS_IDX: > + sfsr |= SFSR_CT_NUCLEUS; > + /* fallthru */ > + case MMU_HYPV_IDX: > + /* No context. */ > + context = 0; > + break; > + case MMU_USER_IDX: > + is_user = true; > + /* fallthru */ > + case MMU_KERNEL_IDX: > /* PRIMARY context */ > context = env->dmmu.mmu_primary_context & 0x1fff; > - } else { > - /* NUCLEUS context */ > - context = 0; > + sfsr |= SFSR_CT_PRIMARY; > + break; > + case MMU_USER_SECONDARY_IDX: > + is_user = true; > + /* fallthru */ > + case MMU_KERNEL_SECONDARY_IDX: > + /* PRIMARY context */ > + context = env->dmmu.mmu_secondary_context & 0x1fff; > + sfsr |= SFSR_CT_SECONDARY; > + break; > + default: > + g_assert_not_reached(); > } > > for (i = 0; i < 64; i++) { > @@ -638,20 +663,15 @@ static int get_physical_address_code(CPUSPARCState *env, > if (TTE_IS_PRIV(env->itlb[i].tte) && is_user) { > /* Fault status register */ > if (env->immu.sfsr & SFSR_VALID_BIT) { > - env->immu.sfsr = SFSR_OW_BIT; /* overflow (not read > before > - another fault) */ > - } else { > - env->immu.sfsr = 0; > + /* overflow (not read before another fault) */ > + sfsr |= SFSR_OW_BIT; > } > if (env->pstate & PS_PRIV) { > - env->immu.sfsr |= SFSR_PR_BIT; > - } > - if (env->tl > 0) { > - env->immu.sfsr |= SFSR_CT_NUCLEUS; > + sfsr |= SFSR_PR_BIT; > } > > /* FIXME: ASI field in SFSR must be set */ > - env->immu.sfsr |= SFSR_FT_PRIV_BIT | SFSR_VALID_BIT; > + env->immu.sfsr |= sfsr | SFSR_FT_PRIV_BIT | SFSR_VALID_BIT; > cs->exception_index = TT_TFAULT; > > env->immu.tag_access = (address & ~0x1fffULL) | context; > -- > 2.5.0 > > -- Regards, Artyom Tarasenko SPARC and PPC PReP under qemu blog: http://tyom.blogspot.com/search/label/qemu