HLVX address translation uses execute permission, but the final PMP check on the supervisor physical address must also require read permission. Carry the HLVX operation through the MMU index and include PMP_READ in the final check so execute-only pages fault.
Tested: RV32 system-mode RWX, HLV, and HLVX.WU PMP test cases. Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4414 Signed-off-by: wangyang <[email protected]> --- target/riscv/internals.h | 6 ++++++ target/riscv/tcg/cpu_helper.c | 24 ++++++++++++++++-------- target/riscv/tcg/op_helper.c | 11 +++++------ 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/target/riscv/internals.h b/target/riscv/internals.h index 5d84e4de96..832c6406bc 100644 --- a/target/riscv/internals.h +++ b/target/riscv/internals.h @@ -42,6 +42,7 @@ #define MMUIdx_M 3 #define MMU_2STAGE_BIT (1 << 2) #define MMU_IDX_SS_WRITE (1 << 3) +#define MMU_IDX_HLVX (1 << 4) static inline privilege_mode_t mmuidx_priv(int mmu_idx) { @@ -62,6 +63,11 @@ static inline bool mmuidx_2stage(int mmu_idx) return mmu_idx & MMU_2STAGE_BIT; } +static inline bool mmuidx_hlvx(int mmu_idx) +{ + return mmu_idx & MMU_IDX_HLVX; +} + /* * Return the endianness for the current privilege * level, based on the MSTATUS MBE/SBE/UBE bits. diff --git a/target/riscv/tcg/cpu_helper.c b/target/riscv/tcg/cpu_helper.c index 07d9222652..ad41de9d06 100644 --- a/target/riscv/tcg/cpu_helper.c +++ b/target/riscv/tcg/cpu_helper.c @@ -905,6 +905,7 @@ void riscv_cpu_set_mode(CPURISCVState *env, privilege_mode_t newpriv, */ static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr, int size, MMUAccessType access_type, + pmp_priv_t extra_privs, privilege_mode_t mode) { pmp_priv_t pmp_priv; @@ -915,7 +916,8 @@ static int get_physical_address_pmp(CPURISCVState *env, int *prot, hwaddr addr, return TRANSLATE_SUCCESS; } - pmp_has_privs = pmp_hart_has_privs(env, addr, size, 1 << access_type, + pmp_has_privs = pmp_hart_has_privs(env, addr, size, + (1 << access_type) | extra_privs, &pmp_priv, mode); if (!pmp_has_privs) { *prot = 0; @@ -1177,7 +1179,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, int pmp_prot; int pmp_ret = get_physical_address_pmp(env, &pmp_prot, pte_addr, sxlen_bytes, - MMU_DATA_LOAD, PRV_S); + MMU_DATA_LOAD, 0, PRV_S); if (pmp_ret != TRANSLATE_SUCCESS) { return TRANSLATE_PMP_FAIL; } @@ -1425,7 +1427,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical, } pmp_ret = get_physical_address_pmp(env, &pmp_prot, pte_addr, - sxlen_bytes, MMU_DATA_STORE, PRV_S); + sxlen_bytes, MMU_DATA_STORE, 0, + PRV_S); if (pmp_ret != TRANSLATE_SUCCESS) { return TRANSLATE_PMP_FAIL; } @@ -1711,7 +1714,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, if (ret == TRANSLATE_SUCCESS) { ret = get_physical_address_pmp(env, &prot_pmp, pa, - size, access_type, mode); + size, access_type, + mmuidx_hlvx(mmu_idx) ? + PMP_READ : 0, mode); tlb_size = pmp_get_tlb_size(env, pa); qemu_log_mask(CPU_LOG_MMU, @@ -1746,7 +1751,9 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, if (ret == TRANSLATE_SUCCESS) { ret = get_physical_address_pmp(env, &prot_pmp, pa, - size, access_type, mode); + size, access_type, + mmuidx_hlvx(mmu_idx) ? + PMP_READ : 0, mode); tlb_size = pmp_get_tlb_size(env, pa); qemu_log_mask(CPU_LOG_MMU, @@ -1786,9 +1793,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size, cpu_check_watchpoint(cs, address, size, MEMTXATTRS_UNSPECIFIED, wp_access, retaddr); - raise_mmu_exception(env, address, access_type, pmp_pma_violation, - first_stage_error, two_stage_lookup, - two_stage_indirect_error); + raise_mmu_exception(env, address, + mmuidx_hlvx(mmu_idx) ? MMU_DATA_LOAD : access_type, + pmp_pma_violation, first_stage_error, + two_stage_lookup, two_stage_indirect_error); cpu_loop_exit_restore(cs, retaddr); } diff --git a/target/riscv/tcg/op_helper.c b/target/riscv/tcg/op_helper.c index 3e94005d2b..060d97ee9c 100644 --- a/target/riscv/tcg/op_helper.c +++ b/target/riscv/tcg/op_helper.c @@ -650,7 +650,7 @@ static int check_access_hlsv(CPURISCVState *env, bool x, uintptr_t ra) if (!x && mode == PRV_S && get_field(env->vsstatus, MSTATUS_SUM)) { mode = MMUIdx_S_SUM; } - return mode | MMU_2STAGE_BIT; + return mode | MMU_2STAGE_BIT | (x ? MMU_IDX_HLVX : 0); } target_ulong helper_hyp_hlv_bu(CPURISCVState *env, target_ulong addr) @@ -726,11 +726,10 @@ void helper_hyp_hsv_d(CPURISCVState *env, target_ulong addr, target_ulong val) } /* - * TODO: These implementations are not quite correct. They perform the - * access using execute permission just fine, but the final PMP check - * is supposed to have read permission as well. Without replicating - * a fair fraction of cputlb.c, fixing this requires adding new mmu_idx - * which would imply that exact check in tlb_fill. + * HLVX accesses are translated with execute permission (first stage), + * but the final PMP check on the supervisor physical address must + * require read permission as well. The MMU_IDX_HLVX mmu_idx bit set + * by check_access_hlsv() makes riscv_cpu_tlb_fill() enforce this. */ target_ulong helper_hyp_hlvx_hu(CPURISCVState *env, target_ulong addr) { -- 2.55.0.windows.2
