Message ID | 20210628133610.1143-3-bruno.larsen@eldorado.org.br |
---|---|
State | New |
Headers | show |
Series | Clean up MMU translation | expand |
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.
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" #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,
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(-)