[v4,2/3] target/ppc: change ppc_hash32_xlate to use mmu_idx

Changed hash32 address translation to use the supplied mmu_idx, instead
of using what was stored in the msr, for parity purposes (radix64
already uses that).

Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
---
 target/ppc/mmu-hash32.c | 40 +++++++++++++++++++---------------------
 target/ppc/mmu-hash32.h |  2 +-
 target/ppc/mmu_helper.c |  2 +-
 3 files changed, 21 insertions(+), 23 deletions(-)

On Mon, Jun 28, 2021 at 10:36:09AM -0300, Bruno Larsen (billionai) wrote:
> Changed hash32 address translation to use the supplied mmu_idx, instead
> of using what was stored in the msr, for parity purposes (radix64
> already uses that).

Well.. parity and conceptual correctness.  The translation is supposed
to use mmu_idx, not look at the CPU again to get the right context.
AFAIK there isn't a situation in hash32 where they'll get out of sync,
but nothing guarantees that.

> 
> Signed-off-by: Bruno Larsen (billionai) <bruno.larsen@eldorado.org.br>
> ---
>  target/ppc/mmu-hash32.c | 40 +++++++++++++++++++---------------------
>  target/ppc/mmu-hash32.h |  2 +-
>  target/ppc/mmu_helper.c |  2 +-
>  3 files changed, 21 insertions(+), 23 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
> index 6a07c345e4..0691d553a3 100644
> --- a/target/ppc/mmu-hash32.c
> +++ b/target/ppc/mmu-hash32.c
> @@ -25,6 +25,7 @@
>  #include "kvm_ppc.h"
>  #include "internal.h"
>  #include "mmu-hash32.h"
> +#include "mmu-book3s-v3.h"

So, the mmu_idx values happen to have the same values for hash32 as
for book3sv3, but that's arguably a coincidence, and including a
book3sv3 header in hash32 code looks really dubious.

I think the right approach is to duplicate the helper macros in
mmu-hash32.h for now.  We can unify them later with a more thorough
review (which would probably involve creating a new header for things
common to all BookS family MMUs).


Apart from those nits,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

>  #include "exec/log.h"
>  
>  /* #define DEBUG_BAT */
> @@ -86,25 +87,22 @@ static int ppc_hash32_pp_prot(int key, int pp, int nx)
>      return prot;
>  }
>  
> -static int ppc_hash32_pte_prot(PowerPCCPU *cpu,
> +static int ppc_hash32_pte_prot(int mmu_idx,
>                                 target_ulong sr, ppc_hash_pte32_t pte)
>  {
> -    CPUPPCState *env = &cpu->env;
>      unsigned pp, key;
>  
> -    key = !!(msr_pr ? (sr & SR32_KP) : (sr & SR32_KS));
> +    key = !!(mmuidx_pr(mmu_idx) ? (sr & SR32_KP) : (sr & SR32_KS));
>      pp = pte.pte1 & HPTE32_R_PP;
>  
>      return ppc_hash32_pp_prot(key, pp, !!(sr & SR32_NX));
>  }
>  
> -static target_ulong hash32_bat_size(PowerPCCPU *cpu,
> +static target_ulong hash32_bat_size(int mmu_idx,
>                                      target_ulong batu, target_ulong batl)
>  {
> -    CPUPPCState *env = &cpu->env;
> -
> -    if ((msr_pr && !(batu & BATU32_VP))
> -        || (!msr_pr && !(batu & BATU32_VS))) {
> +    if ((mmuidx_pr(mmu_idx) && !(batu & BATU32_VP))
> +        || (!mmuidx_pr(mmu_idx) && !(batu & BATU32_VS))) {
>          return 0;
>      }
>  
> @@ -137,14 +135,13 @@ static target_ulong hash32_bat_601_size(PowerPCCPU *cpu,
>      return BATU32_BEPI & ~((batl & BATL32_601_BL) << 17);
>  }
>  
> -static int hash32_bat_601_prot(PowerPCCPU *cpu,
> +static int hash32_bat_601_prot(int mmu_idx,
>                                 target_ulong batu, target_ulong batl)
>  {
> -    CPUPPCState *env = &cpu->env;
>      int key, pp;
>  
>      pp = batu & BATU32_601_PP;
> -    if (msr_pr == 0) {
> +    if (mmuidx_pr(mmu_idx) == 0) {
>          key = !!(batu & BATU32_601_KS);
>      } else {
>          key = !!(batu & BATU32_601_KP);
> @@ -153,7 +150,8 @@ static int hash32_bat_601_prot(PowerPCCPU *cpu,
>  }
>  
>  static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
> -                                    MMUAccessType access_type, int *prot)
> +                                    MMUAccessType access_type, int *prot,
> +                                    int mmu_idx)
>  {
>      CPUPPCState *env = &cpu->env;
>      target_ulong *BATlt, *BATut;
> @@ -177,7 +175,7 @@ static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
>          if (unlikely(env->mmu_model == POWERPC_MMU_601)) {
>              mask = hash32_bat_601_size(cpu, batu, batl);
>          } else {
> -            mask = hash32_bat_size(cpu, batu, batl);
> +            mask = hash32_bat_size(mmu_idx, batu, batl);
>          }
>          LOG_BATS("%s: %cBAT%d v " TARGET_FMT_lx " BATu " TARGET_FMT_lx
>                   " BATl " TARGET_FMT_lx "\n", __func__,
> @@ -187,7 +185,7 @@ static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
>              hwaddr raddr = (batl & mask) | (ea & ~mask);
>  
>              if (unlikely(env->mmu_model == POWERPC_MMU_601)) {
> -                *prot = hash32_bat_601_prot(cpu, batu, batl);
> +                *prot = hash32_bat_601_prot(mmu_idx, batu, batl);
>              } else {
>                  *prot = hash32_bat_prot(cpu, batu, batl);
>              }
> @@ -221,12 +219,12 @@ static hwaddr ppc_hash32_bat_lookup(PowerPCCPU *cpu, target_ulong ea,
>  static bool ppc_hash32_direct_store(PowerPCCPU *cpu, target_ulong sr,
>                                      target_ulong eaddr,
>                                      MMUAccessType access_type,
> -                                    hwaddr *raddr, int *prot,
> +                                    hwaddr *raddr, int *prot, int mmu_idx,
>                                      bool guest_visible)
>  {
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> -    int key = !!(msr_pr ? (sr & SR32_KP) : (sr & SR32_KS));
> +    int key = !!(mmuidx_pr(mmu_idx) ? (sr & SR32_KP) : (sr & SR32_KS));
>  
>      qemu_log_mask(CPU_LOG_MMU, "direct store...\n");
>  
> @@ -425,7 +423,7 @@ static hwaddr ppc_hash32_pte_raddr(target_ulong sr, ppc_hash_pte32_t pte,
>  }
>  
>  bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> -                      hwaddr *raddrp, int *psizep, int *protp,
> +                      hwaddr *raddrp, int *psizep, int *protp, int mmu_idx,
>                        bool guest_visible)
>  {
>      CPUState *cs = CPU(cpu);
> @@ -441,7 +439,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>      *psizep = TARGET_PAGE_BITS;
>  
>      /* 1. Handle real mode accesses */
> -    if (access_type == MMU_INST_FETCH ? !msr_ir : !msr_dr) {
> +    if (mmuidx_real(mmu_idx)) {
>          /* Translation is off */
>          *raddrp = eaddr;
>          *protp = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> @@ -452,7 +450,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>  
>      /* 2. Check Block Address Translation entries (BATs) */
>      if (env->nb_BATs != 0) {
> -        raddr = ppc_hash32_bat_lookup(cpu, eaddr, access_type, protp);
> +        raddr = ppc_hash32_bat_lookup(cpu, eaddr, access_type, protp, mmu_idx);
>          if (raddr != -1) {
>              if (need_prot & ~*protp) {
>                  if (guest_visible) {
> @@ -483,7 +481,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>      /* 4. Handle direct store segments */
>      if (sr & SR32_T) {
>          return ppc_hash32_direct_store(cpu, sr, eaddr, access_type,
> -                                       raddrp, protp, guest_visible);
> +                                       raddrp, protp, mmu_idx, guest_visible);
>      }
>  
>      /* 5. Check for segment level no-execute violation */
> @@ -520,7 +518,7 @@ bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>  
>      /* 7. Check access permissions */
>  
> -    prot = ppc_hash32_pte_prot(cpu, sr, pte);
> +    prot = ppc_hash32_pte_prot(mmu_idx, sr, pte);
>  
>      if (need_prot & ~prot) {
>          /* Access right violation */
> diff --git a/target/ppc/mmu-hash32.h b/target/ppc/mmu-hash32.h
> index 8694eccabd..807d9bc6e8 100644
> --- a/target/ppc/mmu-hash32.h
> +++ b/target/ppc/mmu-hash32.h
> @@ -5,7 +5,7 @@
>  
>  hwaddr get_pteg_offset32(PowerPCCPU *cpu, hwaddr hash);
>  bool ppc_hash32_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
> -                      hwaddr *raddrp, int *psizep, int *protp,
> +                      hwaddr *raddrp, int *psizep, int *protp, int mmu_idx,
>                        bool guest_visible);
>  
>  /*
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 9dcdf88597..a3381e1aa0 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -2922,7 +2922,7 @@ static bool ppc_xlate(PowerPCCPU *cpu, vaddr eaddr, MMUAccessType access_type,
>      case POWERPC_MMU_32B:
>      case POWERPC_MMU_601:
>          return ppc_hash32_xlate(cpu, eaddr, access_type,
> -                                raddrp, psizep, protp, guest_visible);
> +                                raddrp, psizep, protp, mmu_idx, guest_visible);
>  
>      default:
>          return ppc_jumbo_xlate(cpu, eaddr, access_type, raddrp,

On 05/07/2021 01:24, David Gibson wrote:

> > Changed hash32 address translation to use the supplied mmu_idx, instead
> > of using what was stored in the msr, for parity purposes (radix64
> > already uses that).

> Well.. parity and conceptual correctness.  The translation is supposed
> to use mmu_idx, not look at the CPU again to get the right context.
> AFAIK there isn't a situation in hash32 where they'll get out of sync,
> but nothing guarantees that.


Fair point, I can change the description  if I do end up with a new version, but

> I think the right approach is to duplicate the helper macros in
> mmu-hash32.h for now.  We can unify them later with a more thorough
> review (which would probably involve creating a new header for things
> common to all BookS family MMUs).

This doesn't work directly. I'd need to put in an ifndef PPC_MMU_BOOK3S_V3_H, which also feels a bit dubious to me. I can go with whichever one you prefer

On Mon, Jul 05, 2021 at 11:31:13AM -0300, Bruno Piazera Larsen wrote:
> 
> On 05/07/2021 01:24, David Gibson wrote:
> 
> > > Changed hash32 address translation to use the supplied mmu_idx, instead
> > > of using what was stored in the msr, for parity purposes (radix64
> > > already uses that).
> 
> > Well.. parity and conceptual correctness.  The translation is supposed
> > to use mmu_idx, not look at the CPU again to get the right context.
> > AFAIK there isn't a situation in hash32 where they'll get out of sync,
> > but nothing guarantees that.
> 
> 
> Fair point, I can change the description if I do end up with a new
> version, but
> 
> > I think the right approach is to duplicate the helper macros in
> > mmu-hash32.h for now.  We can unify them later with a more thorough
> > review (which would probably involve creating a new header for things
> > common to all BookS family MMUs).
> 
> This doesn't work directly. I'd need to put in an ifndef
> PPC_MMU_BOOK3S_V3_H, which also feels a bit dubious to me. I can go
> with whichever one you prefer

Ah... good point, because both headers are included in some places.

Ok, in that case let's jump ahead instead.  Let's create a new
mmu-books.h to cover all MMUs based on the "classic" powerpc model,
and put them in there.  hash32 and book3s-v3 can include that, BookE,
4xx etc. will not.

For now these will be the only things in there, but there will
probably be more we can add later on.