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

[Alistair Francis]

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