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

login
register
mail settings
Submitter David Gibson
Date Feb. 12, 2013, 2 a.m.
Message ID <1360634411-5518-9-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/219726/
State New
Headers show

Comments

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(-)
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

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