| Message ID | 1360634411-5518-9-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 12.02.2013, at 03:00, David Gibson wrote: > cpu_ppc_handle_mmu_fault() calls get_physical_address() (whose behaviour > depends on MMU type) then, if that fails, issues an appropriate exception > - which again has a number of dependencies on MMU type. > > This patch starts converting cpu_ppc_handle_mmu_fault() to have a single > switch on MMU type, calling MMU specific fault handler functions which > deal with both translation and exception delivery appropriately for the > MMU type. We convert 64-bit hash MMUs to this new model, but the > existing code is left in place for other MMU types for now. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > target-ppc/cpu.h | 2 ++ > target-ppc/mmu-hash64.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ > target-ppc/mmu_helper.c | 37 +++++++------------- > 3 files changed, 101 insertions(+), 25 deletions(-) > > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > index 6143142..da01b06 100644 > --- a/target-ppc/cpu.h > +++ b/target-ppc/cpu.h > @@ -1155,6 +1155,8 @@ int ppc_store_slb (CPUPPCState *env, target_ulong rb, target_ulong rs); > void dump_slb(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env); > int ppc_hash64_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > target_ulong eaddr, int rw, int access_type); > +int ppc_hash64_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > + int mmu_idx); > #endif /* defined(TARGET_PPC64) */ > #endif /* !defined(CONFIG_USER_ONLY) */ > void ppc_store_msr (CPUPPCState *env, target_ulong value); > diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c > index 0f40e0a..ea5fb1f 100644 > --- a/target-ppc/mmu-hash64.c > +++ b/target-ppc/mmu-hash64.c > @@ -471,3 +471,90 @@ int ppc_hash64_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, > return get_segment64(env, ctx, eaddr, rw, access_type); > } > } > + > +int ppc_hash64_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > + int mmu_idx) > +{ > + mmu_ctx_t ctx; > + int access_type; > + int ret = 0; > + > + if (rw == 2) { > + /* code access */ > + rw = 0; > + access_type = ACCESS_CODE; > + } else { > + /* data access */ > + access_type = env->access_type; > + } > + ret = ppc_hash64_get_physical_address(env, &ctx, address, rw, access_type); > + if (ret == 0) { > + tlb_set_page(env, address & TARGET_PAGE_MASK, > + ctx.raddr & TARGET_PAGE_MASK, ctx.prot, > + mmu_idx, TARGET_PAGE_SIZE); > + ret = 0; > + } else if (ret < 0) { > + LOG_MMU_STATE(env); > + if (access_type == ACCESS_CODE) { > + switch (ret) { > + case -1: > + env->exception_index = POWERPC_EXCP_ISI; > + env->error_code = 0x40000000; > + break; > + case -2: > + /* Access rights violation */ These really should become an enum or defines. Something where the thing we check on tells us what we're checking for without a comment :). > + env->exception_index = POWERPC_EXCP_ISI; > + env->error_code = 0x08000000; > + break; > + case -3: > + /* No execute protection violation */ > + env->exception_index = POWERPC_EXCP_ISI; > + env->error_code = 0x10000000; > + break; > + case -5: > + /* No match in segment table */ > + env->exception_index = POWERPC_EXCP_ISEG; > + env->error_code = 0; > + break; > + } > + } else { > + switch (ret) { > + case -1: > + /* No matches in page tables or TLB */ > + env->exception_index = POWERPC_EXCP_DSI; > + env->error_code = 0; > + env->spr[SPR_DAR] = address; > + if (rw == 1) { > + env->spr[SPR_DSISR] = 0x42000000; > + } else { > + env->spr[SPR_DSISR] = 0x40000000; > + } > + break; > + case -2: > + /* Access rights violation */ > + env->exception_index = POWERPC_EXCP_DSI; > + env->error_code = 0; > + env->spr[SPR_DAR] = address; > + if (rw == 1) { > + env->spr[SPR_DSISR] = 0x0A000000; > + } else { > + env->spr[SPR_DSISR] = 0x08000000; > + } > + break; > + case -5: > + /* No match in segment table */ > + env->exception_index = POWERPC_EXCP_DSEG; > + env->error_code = 0; > + env->spr[SPR_DAR] = address; > + break; > + } > + } > +#if 0 #ifdef DEBUG_MMU? > + printf("%s: set exception to %d %02x\n", __func__, > + env->exception, env->error_code); > +#endif > + ret = 1; > + } > + > + return ret; > +} > diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c > index 98143dd..f8f213b 100644 > --- a/target-ppc/mmu_helper.c > +++ b/target-ppc/mmu_helper.c > @@ -1533,6 +1533,18 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > int access_type; > int ret = 0; > > + switch (env->mmu_model) { > +#if defined(TARGET_PPC64) > + case POWERPC_MMU_64B: > + case POWERPC_MMU_2_06: > + case POWERPC_MMU_2_06d: > + return ppc_hash64_handle_mmu_fault(env, address, rw, mmu_idx); > +#endif > + > + default: > + ; /* Otherwise fall through to the general code below */ > + } Sigh. This too should become a class function. Maybe it is within the scope of this patch set after all :). You could keep the "legacy" handling code alive and just point the non-converted targets to the existing function. Then override the handler for book3s_64 families to ppc_hash64_handle_mmu_fault. Alex > + > if (rw == 2) { > /* code access */ > rw = 0; > @@ -1572,11 +1584,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > break; > case POWERPC_MMU_32B: > case POWERPC_MMU_601: > -#if defined(TARGET_PPC64) > - case POWERPC_MMU_64B: > - case POWERPC_MMU_2_06: > - case POWERPC_MMU_2_06d: > -#endif > env->exception_index = POWERPC_EXCP_ISI; > env->error_code = 0x40000000; > break; > @@ -1621,13 +1628,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > env->exception_index = POWERPC_EXCP_ISI; > env->error_code = 0x10000000; > break; > -#if defined(TARGET_PPC64) > - case -5: > - /* No match in segment table */ > - env->exception_index = POWERPC_EXCP_ISEG; > - env->error_code = 0; > - break; > -#endif > } > } else { > switch (ret) { > @@ -1677,11 +1677,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > break; > case POWERPC_MMU_32B: > case POWERPC_MMU_601: > -#if defined(TARGET_PPC64) > - case POWERPC_MMU_64B: > - case POWERPC_MMU_2_06: > - case POWERPC_MMU_2_06d: > -#endif > env->exception_index = POWERPC_EXCP_DSI; > env->error_code = 0; > env->spr[SPR_DAR] = address; > @@ -1776,14 +1771,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > break; > } > break; > -#if defined(TARGET_PPC64) > - case -5: > - /* No match in segment table */ > - env->exception_index = POWERPC_EXCP_DSEG; > - env->error_code = 0; > - env->spr[SPR_DAR] = address; > - break; > -#endif > } > } > #if 0 > -- > 1.7.10.4 >
On Fri, Feb 22, 2013 at 05:13:56PM +0100, Alexander Graf wrote: > On 12.02.2013, at 03:00, David Gibson wrote: [snip] > > + } else if (ret < 0) { > > + LOG_MMU_STATE(env); > > + if (access_type == ACCESS_CODE) { > > + switch (ret) { > > + case -1: > > + env->exception_index = POWERPC_EXCP_ISI; > > + env->error_code = 0x40000000; > > + break; > > + case -2: > > + /* Access rights violation */ > > These really should become an enum or defines. Something where the > thing we check on tells us what we're checking for without a comment > :). So, I didn't want to introduce an emum or whatever at the same time as I was just moving the code around - makes for more chance of mistakes. And by the end of the series, I've gotten rid of these magic error values anyway, by reporting the exceptions in-line in mmu fault handle, so I think it's kind of moot. [snip] > > + } > > + } > > +#if 0 > > #ifdef DEBUG_MMU? Again, this is just moved code. > > + printf("%s: set exception to %d %02x\n", __func__, > > + env->exception, env->error_code); > > +#endif > > + ret = 1; > > + } > > + > > + return ret; > > +} > > diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c > > index 98143dd..f8f213b 100644 > > --- a/target-ppc/mmu_helper.c > > +++ b/target-ppc/mmu_helper.c > > @@ -1533,6 +1533,18 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, > > int access_type; > > int ret = 0; > > > > + switch (env->mmu_model) { > > +#if defined(TARGET_PPC64) > > + case POWERPC_MMU_64B: > > + case POWERPC_MMU_2_06: > > + case POWERPC_MMU_2_06d: > > + return ppc_hash64_handle_mmu_fault(env, address, rw, mmu_idx); > > +#endif > > + > > + default: > > + ; /* Otherwise fall through to the general code below */ > > + } > > Sigh. This too should become a class function. Maybe it is within > the scope of this patch set after all :). You could keep the > "legacy" handling code alive and just point the non-converted > targets to the existing function. Then override the handler for > book3s_64 families to ppc_hash64_handle_mmu_fault. Well, as I said, I think it makes sense to move the switch to one place first, then change it to a method dispatch. I can look at adding patches to do that conversion to the end of the series, but I'll need to pull down the cpu qoming stuff furst.
On 23.02.2013, at 09:13, David Gibson wrote: > On Fri, Feb 22, 2013 at 05:13:56PM +0100, Alexander Graf wrote: >> On 12.02.2013, at 03:00, David Gibson wrote: > [snip] >>> + } else if (ret < 0) { >>> + LOG_MMU_STATE(env); >>> + if (access_type == ACCESS_CODE) { >>> + switch (ret) { >>> + case -1: >>> + env->exception_index = POWERPC_EXCP_ISI; >>> + env->error_code = 0x40000000; >>> + break; >>> + case -2: >>> + /* Access rights violation */ >> >> These really should become an enum or defines. Something where the >> thing we check on tells us what we're checking for without a comment >> :). > > So, I didn't want to introduce an emum or whatever at the same time as > I was just moving the code around - makes for more chance of > mistakes. And by the end of the series, I've gotten rid of these > magic error values anyway, by reporting the exceptions in-line in mmu > fault handle, so I think it's kind of moot. I was reviewing the earlier 8 patch set which didn't convert the return values to inline exception paths yet. The new set looks very nice :) Alex
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h index 6143142..da01b06 100644 --- a/target-ppc/cpu.h +++ b/target-ppc/cpu.h @@ -1155,6 +1155,8 @@ int ppc_store_slb (CPUPPCState *env, target_ulong rb, target_ulong rs); void dump_slb(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env); int ppc_hash64_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, target_ulong eaddr, int rw, int access_type); +int ppc_hash64_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, + int mmu_idx); #endif /* defined(TARGET_PPC64) */ #endif /* !defined(CONFIG_USER_ONLY) */ void ppc_store_msr (CPUPPCState *env, target_ulong value); diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c index 0f40e0a..ea5fb1f 100644 --- a/target-ppc/mmu-hash64.c +++ b/target-ppc/mmu-hash64.c @@ -471,3 +471,90 @@ int ppc_hash64_get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, return get_segment64(env, ctx, eaddr, rw, access_type); } } + +int ppc_hash64_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, + int mmu_idx) +{ + mmu_ctx_t ctx; + int access_type; + int ret = 0; + + if (rw == 2) { + /* code access */ + rw = 0; + access_type = ACCESS_CODE; + } else { + /* data access */ + access_type = env->access_type; + } + ret = ppc_hash64_get_physical_address(env, &ctx, address, rw, access_type); + if (ret == 0) { + tlb_set_page(env, address & TARGET_PAGE_MASK, + ctx.raddr & TARGET_PAGE_MASK, ctx.prot, + mmu_idx, TARGET_PAGE_SIZE); + ret = 0; + } else if (ret < 0) { + LOG_MMU_STATE(env); + if (access_type == ACCESS_CODE) { + switch (ret) { + case -1: + env->exception_index = POWERPC_EXCP_ISI; + env->error_code = 0x40000000; + break; + case -2: + /* Access rights violation */ + env->exception_index = POWERPC_EXCP_ISI; + env->error_code = 0x08000000; + break; + case -3: + /* No execute protection violation */ + env->exception_index = POWERPC_EXCP_ISI; + env->error_code = 0x10000000; + break; + case -5: + /* No match in segment table */ + env->exception_index = POWERPC_EXCP_ISEG; + env->error_code = 0; + break; + } + } else { + switch (ret) { + case -1: + /* No matches in page tables or TLB */ + env->exception_index = POWERPC_EXCP_DSI; + env->error_code = 0; + env->spr[SPR_DAR] = address; + if (rw == 1) { + env->spr[SPR_DSISR] = 0x42000000; + } else { + env->spr[SPR_DSISR] = 0x40000000; + } + break; + case -2: + /* Access rights violation */ + env->exception_index = POWERPC_EXCP_DSI; + env->error_code = 0; + env->spr[SPR_DAR] = address; + if (rw == 1) { + env->spr[SPR_DSISR] = 0x0A000000; + } else { + env->spr[SPR_DSISR] = 0x08000000; + } + break; + case -5: + /* No match in segment table */ + env->exception_index = POWERPC_EXCP_DSEG; + env->error_code = 0; + env->spr[SPR_DAR] = address; + break; + } + } +#if 0 + printf("%s: set exception to %d %02x\n", __func__, + env->exception, env->error_code); +#endif + ret = 1; + } + + return ret; +} diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c index 98143dd..f8f213b 100644 --- a/target-ppc/mmu_helper.c +++ b/target-ppc/mmu_helper.c @@ -1533,6 +1533,18 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, int access_type; int ret = 0; + switch (env->mmu_model) { +#if defined(TARGET_PPC64) + case POWERPC_MMU_64B: + case POWERPC_MMU_2_06: + case POWERPC_MMU_2_06d: + return ppc_hash64_handle_mmu_fault(env, address, rw, mmu_idx); +#endif + + default: + ; /* Otherwise fall through to the general code below */ + } + if (rw == 2) { /* code access */ rw = 0; @@ -1572,11 +1584,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, break; case POWERPC_MMU_32B: case POWERPC_MMU_601: -#if defined(TARGET_PPC64) - case POWERPC_MMU_64B: - case POWERPC_MMU_2_06: - case POWERPC_MMU_2_06d: -#endif env->exception_index = POWERPC_EXCP_ISI; env->error_code = 0x40000000; break; @@ -1621,13 +1628,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, env->exception_index = POWERPC_EXCP_ISI; env->error_code = 0x10000000; break; -#if defined(TARGET_PPC64) - case -5: - /* No match in segment table */ - env->exception_index = POWERPC_EXCP_ISEG; - env->error_code = 0; - break; -#endif } } else { switch (ret) { @@ -1677,11 +1677,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, break; case POWERPC_MMU_32B: case POWERPC_MMU_601: -#if defined(TARGET_PPC64) - case POWERPC_MMU_64B: - case POWERPC_MMU_2_06: - case POWERPC_MMU_2_06d: -#endif env->exception_index = POWERPC_EXCP_DSI; env->error_code = 0; env->spr[SPR_DAR] = address; @@ -1776,14 +1771,6 @@ int cpu_ppc_handle_mmu_fault(CPUPPCState *env, target_ulong address, int rw, break; } break; -#if defined(TARGET_PPC64) - case -5: - /* No match in segment table */ - env->exception_index = POWERPC_EXCP_DSEG; - env->error_code = 0; - env->spr[SPR_DAR] = address; - break; -#endif } } #if 0
cpu_ppc_handle_mmu_fault() calls get_physical_address() (whose behaviour depends on MMU type) then, if that fails, issues an appropriate exception - which again has a number of dependencies on MMU type. This patch starts converting cpu_ppc_handle_mmu_fault() to have a single switch on MMU type, calling MMU specific fault handler functions which deal with both translation and exception delivery appropriately for the MMU type. We convert 64-bit hash MMUs to this new model, but the existing code is left in place for other MMU types for now. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- target-ppc/cpu.h | 2 ++ target-ppc/mmu-hash64.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++ target-ppc/mmu_helper.c | 37 +++++++------------- 3 files changed, 101 insertions(+), 25 deletions(-)