Re: [PATCH 3/5] target/riscv: require read permission for HLVX accesses

[Daniel Henrique Barboza]

On 9/5/2026 6:37 AM, wangyang wrote:

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 witness matrix.

Please remove the "witness matrix" part.

We are aware that some Gitlab issues, like 4414 that is being fixed here,
are using the word "witness" quite liberally.  In 4414 alone:

"I triaged the result and independently replayed the fixed witness on
 upstream QEMU 11.1.1."

"OS/kernel version: not applicable to the direct witness."

"Source witness and linker script are attached to this issue."

"Build the attached witness source and linker script into ..."


I'll take a guess that 'witness' is being used as a synonym for 'test case'. I
would expect reading something like "witness matrix" in a Law and Order episode,
not QEMU source.

English isn't my native language, and maybe the use of 'witness' here is valid.
Even then I'll ask to remove any references to it, replacing it with 'test case'
if needed, to keep the source code easier to follow.


ps: the code LGTM. Thanks,

Daniel

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4414

Signed-off-by: wangyang <wangyan...@otcaix.iscas.ac.cn>
---
  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)
  {