| Message ID | 1360634411-5518-7-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 12.02.2013, at 03:00, David Gibson wrote: > Currently get_physical_address() first checks to see if translation is > enabled in the MSR, then in the translation on case switches on the mmu > type. Except that for BookE MMUs, translation is always on, and so it > has to switch in the "translation off" case as well and do the same thing > as the translation on path for those MMUs. Plus, even translation off > doesn't behave exactly the same on the various MMU types so there are > further mmu type checks in the "translation off" path. > > As a first step to cleaning this up, this patch moves the switch on mmu > type to the top level, then makes the translation on/off check just for > those mmu types where it is meaningful. Eventually the mmu translation should just be a class dispatched indirect function call. I don't think it's within the scope of this first round of cleanup though. Alex
On Fri, Feb 22, 2013 at 05:06:52PM +0100, Alexander Graf wrote: > > On 12.02.2013, at 03:00, David Gibson wrote: > > > Currently get_physical_address() first checks to see if translation is > > enabled in the MSR, then in the translation on case switches on the mmu > > type. Except that for BookE MMUs, translation is always on, and so it > > has to switch in the "translation off" case as well and do the same thing > > as the translation on path for those MMUs. Plus, even translation off > > doesn't behave exactly the same on the various MMU types so there are > > further mmu type checks in the "translation off" path. > > > > As a first step to cleaning this up, this patch moves the switch on mmu > > type to the top level, then makes the translation on/off check just for > > those mmu types where it is meaningful. > > Eventually the mmu translation should just be a class dispatched > indirect function call. I don't think it's within the scope of this > first round of cleanup though. I agree. My plan was to first push the switching all up to the top level as I've done, then replace that with a method dispatch afterwards.
Am 23.02.2013 um 07:28 schrieb David Gibson <david@gibson.dropbear.id.au>: > On Fri, Feb 22, 2013 at 05:06:52PM +0100, Alexander Graf wrote: >> >> On 12.02.2013, at 03:00, David Gibson wrote: >> >>> Currently get_physical_address() first checks to see if translation is >>> enabled in the MSR, then in the translation on case switches on the mmu >>> type. Except that for BookE MMUs, translation is always on, and so it >>> has to switch in the "translation off" case as well and do the same thing >>> as the translation on path for those MMUs. Plus, even translation off >>> doesn't behave exactly the same on the various MMU types so there are >>> further mmu type checks in the "translation off" path. >>> >>> As a first step to cleaning this up, this patch moves the switch on mmu >>> type to the top level, then makes the translation on/off check just for >>> those mmu types where it is meaningful. >> >> Eventually the mmu translation should just be a class dispatched >> indirect function call. I don't think it's within the scope of this >> first round of cleanup though. > > I agree. My plan was to first push the switching all up to the top > level as I've done, then replace that with a method dispatch > afterwards. If you're convinced the path you're taking is already correct, don't post patches as RFC :). It would also be nice to mention that plan in the cover letter, so that I can potentially comment on it. Alex > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Sat, Feb 23, 2013 at 11:38:23AM +0100, Alexander Graf wrote: > > > Am 23.02.2013 um 07:28 schrieb David Gibson <david@gibson.dropbear.id.au>: > > > On Fri, Feb 22, 2013 at 05:06:52PM +0100, Alexander Graf wrote: > >> > >> On 12.02.2013, at 03:00, David Gibson wrote: > >> > >>> Currently get_physical_address() first checks to see if translation is > >>> enabled in the MSR, then in the translation on case switches on the mmu > >>> type. Except that for BookE MMUs, translation is always on, and so it > >>> has to switch in the "translation off" case as well and do the same thing > >>> as the translation on path for those MMUs. Plus, even translation off > >>> doesn't behave exactly the same on the various MMU types so there are > >>> further mmu type checks in the "translation off" path. > >>> > >>> As a first step to cleaning this up, this patch moves the switch on mmu > >>> type to the top level, then makes the translation on/off check just for > >>> those mmu types where it is meaningful. > >> > >> Eventually the mmu translation should just be a class dispatched > >> indirect function call. I don't think it's within the scope of this > >> first round of cleanup though. > > > > I agree. My plan was to first push the switching all up to the top > > level as I've done, then replace that with a method dispatch > > afterwards. > > If you're convinced the path you're taking is already correct, don't > post patches as RFC :). Well, I'm not sure about all of it. More to the point, I'm really not sure what you're suggesting I do differently. We can't switch to a method invocation when there are mmu type dependencies scattered all through the path. So that change more or less has to go after what I have already.
On 23.02.2013, at 12:56, David Gibson wrote: > On Sat, Feb 23, 2013 at 11:38:23AM +0100, Alexander Graf wrote: >> >> >> Am 23.02.2013 um 07:28 schrieb David Gibson <david@gibson.dropbear.id.au>: >> >>> On Fri, Feb 22, 2013 at 05:06:52PM +0100, Alexander Graf wrote: >>>> >>>> On 12.02.2013, at 03:00, David Gibson wrote: >>>> >>>>> Currently get_physical_address() first checks to see if translation is >>>>> enabled in the MSR, then in the translation on case switches on the mmu >>>>> type. Except that for BookE MMUs, translation is always on, and so it >>>>> has to switch in the "translation off" case as well and do the same thing >>>>> as the translation on path for those MMUs. Plus, even translation off >>>>> doesn't behave exactly the same on the various MMU types so there are >>>>> further mmu type checks in the "translation off" path. >>>>> >>>>> As a first step to cleaning this up, this patch moves the switch on mmu >>>>> type to the top level, then makes the translation on/off check just for >>>>> those mmu types where it is meaningful. >>>> >>>> Eventually the mmu translation should just be a class dispatched >>>> indirect function call. I don't think it's within the scope of this >>>> first round of cleanup though. >>> >>> I agree. My plan was to first push the switching all up to the top >>> level as I've done, then replace that with a method dispatch >>> afterwards. >> >> If you're convinced the path you're taking is already correct, don't >> post patches as RFC :). > > Well, I'm not sure about all of it. > > More to the point, I'm really not sure what you're suggesting I do > differently. We can't switch to a method invocation when there are > mmu type dependencies scattered all through the path. So that change > more or less has to go after what I have already. I never disagreed. I think your patches are fine, nothing really needs to be done differently. I just don't want the conversion to be done half-way. I can understand if you don't feel comfortable not converting non-book3s mmus, but I want to have it done at least completely for all book3s ones then. I'm not a big fan of aborting a race right before the goal :) Alex
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c index e9f8949..e787843 100644 --- a/target-ppc/mmu_helper.c +++ b/target-ppc/mmu_helper.c @@ -1412,32 +1412,22 @@ static inline int check_physical(CPUPPCState *env, mmu_ctx_t *ctx, static int get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, target_ulong eaddr, int rw, int access_type) { - int ret; + int ret = -1; + bool real_mode = (access_type == ACCESS_CODE && msr_ir == 0) + || (access_type != ACCESS_CODE && msr_dr == 0); #if 0 qemu_log("%s\n", __func__); #endif - if ((access_type == ACCESS_CODE && msr_ir == 0) || - (access_type != ACCESS_CODE && msr_dr == 0)) { - if (env->mmu_model == POWERPC_MMU_BOOKE) { - /* The BookE MMU always performs address translation. The - IS and DS bits only affect the address space. */ - ret = mmubooke_get_physical_address(env, ctx, eaddr, - rw, access_type); - } else if (env->mmu_model == POWERPC_MMU_BOOKE206) { - ret = mmubooke206_get_physical_address(env, ctx, eaddr, rw, - access_type); - } else { - /* No address translation. */ + + switch (env->mmu_model) { + case POWERPC_MMU_32B: + case POWERPC_MMU_601: + case POWERPC_MMU_SOFT_6xx: + case POWERPC_MMU_SOFT_74xx: + if (real_mode) { ret = check_physical(env, ctx, eaddr, rw); - } - } else { - ret = -1; - switch (env->mmu_model) { - case POWERPC_MMU_32B: - case POWERPC_MMU_601: - case POWERPC_MMU_SOFT_6xx: - case POWERPC_MMU_SOFT_74xx: + } else { /* Try to find a BAT */ if (env->nb_BATs != 0) { ret = get_bat(env, ctx, eaddr, rw, access_type); @@ -1446,40 +1436,52 @@ static int get_physical_address(CPUPPCState *env, mmu_ctx_t *ctx, /* We didn't match any BAT entry or don't have BATs */ ret = get_segment32(env, ctx, eaddr, rw, access_type); } - break; + } + break; #if defined(TARGET_PPC64) - case POWERPC_MMU_64B: - case POWERPC_MMU_2_06: - case POWERPC_MMU_2_06d: + case POWERPC_MMU_64B: + case POWERPC_MMU_2_06: + case POWERPC_MMU_2_06d: + if (real_mode) { + ret = check_physical(env, ctx, eaddr, rw); + } else { ret = get_segment64(env, ctx, eaddr, rw, access_type); - break; + } + break; #endif - case POWERPC_MMU_SOFT_4xx: - case POWERPC_MMU_SOFT_4xx_Z: + case POWERPC_MMU_SOFT_4xx: + case POWERPC_MMU_SOFT_4xx_Z: + if (real_mode) { + ret = check_physical(env, ctx, eaddr, rw); + } else { ret = mmu40x_get_physical_address(env, ctx, eaddr, rw, access_type); - break; - case POWERPC_MMU_BOOKE: - ret = mmubooke_get_physical_address(env, ctx, eaddr, - rw, access_type); - break; - case POWERPC_MMU_BOOKE206: - ret = mmubooke206_get_physical_address(env, ctx, eaddr, rw, + } + break; + case POWERPC_MMU_BOOKE: + ret = mmubooke_get_physical_address(env, ctx, eaddr, + rw, access_type); + break; + case POWERPC_MMU_BOOKE206: + ret = mmubooke206_get_physical_address(env, ctx, eaddr, rw, access_type); - break; - case POWERPC_MMU_MPC8xx: - /* XXX: TODO */ - cpu_abort(env, "MPC8xx MMU model is not implemented\n"); - break; - case POWERPC_MMU_REAL: + break; + case POWERPC_MMU_MPC8xx: + /* XXX: TODO */ + cpu_abort(env, "MPC8xx MMU model is not implemented\n"); + break; + case POWERPC_MMU_REAL: + if (real_mode) { + ret = check_physical(env, ctx, eaddr, rw); + } else { cpu_abort(env, "PowerPC in real mode do not do any translation\n"); - return -1; - default: - cpu_abort(env, "Unknown or invalid MMU model\n"); - return -1; } + return -1; + default: + cpu_abort(env, "Unknown or invalid MMU model\n"); + return -1; } #if 0 qemu_log("%s address " TARGET_FMT_lx " => %d " TARGET_FMT_plx "\n",
Currently get_physical_address() first checks to see if translation is enabled in the MSR, then in the translation on case switches on the mmu type. Except that for BookE MMUs, translation is always on, and so it has to switch in the "translation off" case as well and do the same thing as the translation on path for those MMUs. Plus, even translation off doesn't behave exactly the same on the various MMU types so there are further mmu type checks in the "translation off" path. As a first step to cleaning this up, this patch moves the switch on mmu type to the top level, then makes the translation on/off check just for those mmu types where it is meaningful. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- target-ppc/mmu_helper.c | 92 ++++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 45 deletions(-)