[8/8] target-ppc Disentangle ppc64 hash mmu path for cpu_ppc_handle_mmu_fault

Submitted by David Gibson on Feb. 12, 2013, 2 a.m.

Details

Message ID 1360634411-5518-9-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Feb. 12, 2013, 2 a.m.
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(-)

Comments

Alexander Graf Feb. 22, 2013, 4:13 p.m.
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
>
David Gibson Feb. 23, 2013, 8:13 a.m.
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.
Alexander Graf Feb. 23, 2013, 1:47 p.m.
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

Patch hide | download patch | download mbox

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