On Sat, Mar 25, 2023 at 9:51 PM Richard Henderson
<richard.hender...@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.hender...@linaro.org>
Reviewed-by: Alistair Francis <alistair.fran...@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
>
>
