Re: [Qemu-devel] [PATCH 12/25] target-sparc: Add MMU_REAL_IDX

[Artyom Tarasenko]

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 <address@hidden> 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 <address@hidden>
> ---
>  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