diff mbox series

[v6,24/25] target/riscv: Reorg access check in get_physical_address

Message ID 20230325105429.1142530-25-richard.henderson@linaro.org
State Superseded
Headers show
Series target/riscv: MSTATUS_SUM + cleanups | expand

Commit Message

Richard Henderson March 25, 2023, 10:54 a.m. UTC 
We were effectively computing the protection bits twice,
once while performing access checks and once while returning
the valid bits to the caller.  Reorg so we do this once.

Move the computation of mxr close to its single use.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/cpu_helper.c | 69 ++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 33 deletions(-)

Comments

Alistair Francis April 11, 2023, 4:55 a.m. UTC | #1 
On Sat, Mar 25, 2023 at 9:51 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We were effectively computing the protection bits twice,
> once while performing access checks and once while returning
> the valid bits to the caller.  Reorg so we do this once.
>
> Move the computation of mxr close to its single use.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu_helper.c | 69 ++++++++++++++++++++-------------------
>  1 file changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 82a7c5f9dd..725ca45106 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -762,7 +762,7 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot,
>   * @is_debug: Is this access from a debugger or the monitor?
>   */
>  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> -                                int *prot, target_ulong addr,
> +                                int *ret_prot, target_ulong addr,
>                                  target_ulong *fault_pte_addr,
>                                  int access_type, int mmu_idx,
>                                  bool first_stage, bool two_stage,
> @@ -793,20 +793,14 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>
>      if (mode == PRV_M || !riscv_cpu_cfg(env)->mmu) {
>          *physical = addr;
> -        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +        *ret_prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>          return TRANSLATE_SUCCESS;
>      }
>
> -    *prot = 0;
> +    *ret_prot = 0;
>
>      hwaddr base;
> -    int levels, ptidxbits, ptesize, vm, sum, mxr, widened;
> -
> -    if (first_stage == true) {
> -        mxr = get_field(env->mstatus, MSTATUS_MXR);
> -    } else {
> -        mxr = get_field(env->vsstatus, MSTATUS_MXR);
> -    }
> +    int levels, ptidxbits, ptesize, vm, sum, widened;
>
>      if (first_stage == true) {
>          if (use_background) {
> @@ -849,7 +843,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>        levels = 5; ptidxbits = 9; ptesize = 8; break;
>      case VM_1_10_MBARE:
>          *physical = addr;
> -        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
> +        *ret_prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>          return TRANSLATE_SUCCESS;
>      default:
>        g_assert_not_reached();
> @@ -984,6 +978,27 @@ restart:
>          return TRANSLATE_FAIL;
>      }
>
> +    int prot = 0;
> +    if (pte & PTE_R) {
> +        prot |= PAGE_READ;
> +    }
> +    if (pte & PTE_W) {
> +        prot |= PAGE_WRITE;
> +    }
> +    if (pte & PTE_X) {
> +        bool mxr;
> +
> +        if (first_stage == true) {
> +            mxr = get_field(env->mstatus, MSTATUS_MXR);
> +        } else {
> +            mxr = get_field(env->vsstatus, MSTATUS_MXR);
> +        }
> +        if (mxr) {
> +            prot |= PAGE_READ;
> +        }
> +        prot |= PAGE_EXEC;
> +    }
> +
>      if ((pte & PTE_U) &&
>          ((mode != PRV_U) && (!sum || access_type == MMU_INST_FETCH))) {
>          /*
> @@ -996,17 +1011,9 @@ restart:
>          /* Supervisor PTE flags when not S mode */
>          return TRANSLATE_FAIL;
>      }
> -    if (access_type == MMU_DATA_LOAD &&
> -        !((pte & PTE_R) || ((pte & PTE_X) && mxr))) {
> -        /* Read access check failed */
> -        return TRANSLATE_FAIL;
> -    }
> -    if (access_type == MMU_DATA_STORE && !(pte & PTE_W)) {
> -        /* Write access check failed */
> -        return TRANSLATE_FAIL;
> -    }
> -    if (access_type == MMU_INST_FETCH && !(pte & PTE_X)) {
> -        /* Fetch access check failed */
> +
> +    if (!((prot >> access_type) & 1)) {
> +        /* Access check failed */
>          return TRANSLATE_FAIL;
>      }
>
> @@ -1071,20 +1078,16 @@ restart:
>                    (vpn & (((target_ulong)1 << ptshift) - 1))
>                   ) << PGSHIFT) | (addr & ~TARGET_PAGE_MASK);
>
> -    /* set permissions on the TLB entry */
> -    if ((pte & PTE_R) || ((pte & PTE_X) && mxr)) {
> -        *prot |= PAGE_READ;
> -    }
> -    if (pte & PTE_X) {
> -        *prot |= PAGE_EXEC;
> -    }
>      /*
> -     * Add write permission on stores or if the page is already dirty,
> -     * so that we TLB miss on later writes to update the dirty bit.
> +     * Remove write permission unless this is a store, or the page is
> +     * already dirty, so that we TLB miss on later writes to update
> +     * the dirty bit.
>       */
> -    if ((pte & PTE_W) && (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> -        *prot |= PAGE_WRITE;
> +    if (access_type != MMU_DATA_STORE && !(pte & PTE_D)) {
> +        prot &= ~PAGE_WRITE;
>      }
> +    *ret_prot = prot;
> +
>      return TRANSLATE_SUCCESS;
>  }
>
> --
> 2.34.1
>
>
diff mbox series

Patch

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 82a7c5f9dd..725ca45106 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -762,7 +762,7 @@  static int get_physical_address_pmp(CPURISCVState *env, int *prot,
  * @is_debug: Is this access from a debugger or the monitor?
  */
 static int get_physical_address(CPURISCVState *env, hwaddr *physical,
-                                int *prot, target_ulong addr,
+                                int *ret_prot, target_ulong addr,
                                 target_ulong *fault_pte_addr,
                                 int access_type, int mmu_idx,
                                 bool first_stage, bool two_stage,
@@ -793,20 +793,14 @@  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
 
     if (mode == PRV_M || !riscv_cpu_cfg(env)->mmu) {
         *physical = addr;
-        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+        *ret_prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         return TRANSLATE_SUCCESS;
     }
 
-    *prot = 0;
+    *ret_prot = 0;
 
     hwaddr base;
-    int levels, ptidxbits, ptesize, vm, sum, mxr, widened;
-
-    if (first_stage == true) {
-        mxr = get_field(env->mstatus, MSTATUS_MXR);
-    } else {
-        mxr = get_field(env->vsstatus, MSTATUS_MXR);
-    }
+    int levels, ptidxbits, ptesize, vm, sum, widened;
 
     if (first_stage == true) {
         if (use_background) {
@@ -849,7 +843,7 @@  static int get_physical_address(CPURISCVState *env, hwaddr *physical,
       levels = 5; ptidxbits = 9; ptesize = 8; break;
     case VM_1_10_MBARE:
         *physical = addr;
-        *prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
+        *ret_prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
         return TRANSLATE_SUCCESS;
     default:
       g_assert_not_reached();
@@ -984,6 +978,27 @@  restart:
         return TRANSLATE_FAIL;
     }
 
+    int prot = 0;
+    if (pte & PTE_R) {
+        prot |= PAGE_READ;
+    }
+    if (pte & PTE_W) {
+        prot |= PAGE_WRITE;
+    }
+    if (pte & PTE_X) {
+        bool mxr;
+
+        if (first_stage == true) {
+            mxr = get_field(env->mstatus, MSTATUS_MXR);
+        } else {
+            mxr = get_field(env->vsstatus, MSTATUS_MXR);
+        }
+        if (mxr) {
+            prot |= PAGE_READ;
+        }
+        prot |= PAGE_EXEC;
+    }
+
     if ((pte & PTE_U) &&
         ((mode != PRV_U) && (!sum || access_type == MMU_INST_FETCH))) {
         /*
@@ -996,17 +1011,9 @@  restart:
         /* Supervisor PTE flags when not S mode */
         return TRANSLATE_FAIL;
     }
-    if (access_type == MMU_DATA_LOAD &&
-        !((pte & PTE_R) || ((pte & PTE_X) && mxr))) {
-        /* Read access check failed */
-        return TRANSLATE_FAIL;
-    }
-    if (access_type == MMU_DATA_STORE && !(pte & PTE_W)) {
-        /* Write access check failed */
-        return TRANSLATE_FAIL;
-    }
-    if (access_type == MMU_INST_FETCH && !(pte & PTE_X)) {
-        /* Fetch access check failed */
+
+    if (!((prot >> access_type) & 1)) {
+        /* Access check failed */
         return TRANSLATE_FAIL;
     }
 
@@ -1071,20 +1078,16 @@  restart:
                   (vpn & (((target_ulong)1 << ptshift) - 1))
                  ) << PGSHIFT) | (addr & ~TARGET_PAGE_MASK);
 
-    /* set permissions on the TLB entry */
-    if ((pte & PTE_R) || ((pte & PTE_X) && mxr)) {
-        *prot |= PAGE_READ;
-    }
-    if (pte & PTE_X) {
-        *prot |= PAGE_EXEC;
-    }
     /*
-     * Add write permission on stores or if the page is already dirty,
-     * so that we TLB miss on later writes to update the dirty bit.
+     * Remove write permission unless this is a store, or the page is
+     * already dirty, so that we TLB miss on later writes to update
+     * the dirty bit.
      */
-    if ((pte & PTE_W) && (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
-        *prot |= PAGE_WRITE;
+    if (access_type != MMU_DATA_STORE && !(pte & PTE_D)) {
+        prot &= ~PAGE_WRITE;
     }
+    *ret_prot = prot;
+
     return TRANSLATE_SUCCESS;
 }