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
