Re: [PATCH 1/3] target/riscv: extract sfence_vma_allowed() from helper_tlb_flush()

[Daniel Henrique Barboza]

On 8/31/2026 3:59 PM, Trevor Gamblin wrote:

Create a new function to encapsulate the privilege/hypervisor checks
performed inside helper_tlb_flush(). The idea is to pass GETPC() as an
argument directly to it inside the helper_tlb_flush() function.

Seems to me that the idea is to use sfence_vma_allowed() in the future
helper_tlb_flush_page().  In this case I think it's a good idea to mention
it in the commit message (e.g.: "we'll use this new function in the next patch
for ...).

Otherwise this patch as is is changing code without doing anything with it.

Signed-off-by: Trevor Gamblin <tgamb...@baylibre.com>
---
  target/riscv/tcg/op_helper.c | 19 +++++++++++++------
  1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/target/riscv/tcg/op_helper.c b/target/riscv/tcg/op_helper.c
index 3e94005d2b..8039df2b48 100644
--- a/target/riscv/tcg/op_helper.c
+++ b/target/riscv/tcg/op_helper.c
@@ -588,18 +588,25 @@ void helper_wrs_nto(CPURISCVState *env)
      }
  }

-void helper_tlb_flush(CPURISCVState *env)

+static bool sfence_vma_allowed(CPURISCVState *env, uintptr_t ra)
  {
-    CPUState *cs = env_cpu(env);
      if (!env->virt_enabled &&
          (env->priv == PRV_U ||
           (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)))) {
-        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, ra);
+        return false;
      } else if (env->virt_enabled &&
                 (env->priv == PRV_U || get_field(env->hstatus, HSTATUS_VTVM))) 
{
-        riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
-    } else {
-        tlb_flush(cs);
+        riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, ra);
+        return false;
+    }
+    return true;
+}

If you read riscv_raise_exception() you'll notice that it won't return to the 
caller.
It has a G_NORETURN:

G_NORETURN void riscv_raise_exception(CPURISCVState *env, (...)


This means that everything that comes after it will not be executed.  Case in 
point:
these 'return false/true' from the function.

I suggest to turn this function into a "check_sfence_vma" style function, like 
we do
in other places like "check_ret_from_m_mode".  The idea is to call the function 
and,
if execution resumes after it, it means that no exception was thrown.

Something like this (note: untested code):

static void check_sfence_vma(CPURISCVState *env, uintptr_t ra)
    CPUState *cs = env_cpu(env);
    if (!env->virt_enabled &&
        (env->priv == PRV_U ||
         (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)))) {
        riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, ra);
    } else if (env->virt_enabled &&
               (env->priv == PRV_U || get_field(env->hstatus, HSTATUS_VTVM))) {
        riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, ra);
    }
}


And then:

void helper_tlb_flush(CPURISCVState *env)
{
    check_sfence_vma(env, GETPC());
    tlb_flush(cs);
}


Thanks,
Daniel

+
+void helper_tlb_flush(CPURISCVState *env)
+{
+    if (sfence_vma_allowed(env, GETPC())) {
+        tlb_flush(env_cpu(env));
      }
  }