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

[Artyom Tarasenko]

Hi Richard,

please ignore my 2 previous mails: I've misread the commit message.
The actual problem and a possible solution 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)

In sun4v terminology "real address" is not "physical addresses".
There is just one more level of translation:
VA->RA->PA
which it's only visible from the hypervisor mode.

But I see where the confusion is coming from:
to make the hypervisor presence transparent for the existing sun4u drivers,
Sun mapped all sun4u ASIs that used to access physical space, to
access real space in sun4v instead.
This way every virtual machine (a "partition" in sun4v terminology)
may have it's own pseudo-physical ("real") space without overlapping
with other virtual machines.
I.e. "ASI_PHYS_USE_EC_L" became "ASI_REAL_L" which triggers one more
translation and so on.

The support of sun4v in current QEMU is very rudimentary, so if we
just ignore it for the moment and focus on sun4u, the approach of this
patch is correct, except for the naming.
With MMU_REAL_IDX renamed to MMU_PHYS_IDX, I think we are fine.

I suggest we keep sun4v names also for the sun4u ASIs (as in this
series) because having different names for sun4u and sun4v would make
the code ugly: it's not possible to #ifdef them because the
sun4u/sun4v cpu/mmu type is a runtime information [1].

Luckily I'm not a maintainer, so it's up to you guys to decide.

As for the sun4v support, I think, we can add one more index, this
time the real MMU_REAL_IDX, which can only be used by sun4v MMU(s).

(As I was reading this patch the last time, I thought the plan was to
use the MMU_REAL_IDX for both real and physical accesses, hence the
confusion. But using one index for two modes would have been a bad
idea because we'd have to flush the translations every time we switch
to/from hypervisor mode which is too often).

Kind regards,
Artyom

1. https://lists.gnu.org/archive/html/qemu-devel/2012-04/msg04281.html

> 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 */
> +    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 */
> +    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