Re: [PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc

[Daniel Henrique Barboza]

Richard,

On 4/25/25 12:23 PM, Richard Henderson wrote:

Do not examine a random host return address, but
properly compute the next pc for the guest cpu.

Fixes: f18637cd611 ("RISC-V: Add misa runtime write support")
Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
---
  target/riscv/csr.c | 22 +++++++++++++++++-----
  1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index a663f527a4..85f9b4c3d2 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -30,6 +30,8 @@
  #include "exec/icount.h"
  #include "qemu/guest-random.h"
  #include "qapi/error.h"
+#include "tcg/insn-start-words.h"
+#include "internals.h"
  #include <stdbool.h>

/* CSR function table public API */

@@ -2099,6 +2101,19 @@ static RISCVException read_misa(CPURISCVState *env, int 
csrno,
      return RISCV_EXCP_NONE;
  }

+static target_ulong get_next_pc(CPURISCVState *env, uintptr_t ra)

+{
+    uint64_t data[TARGET_INSN_START_WORDS];

Isn't this 'INSN_START_WORDS'?  I'm seeing code in i386 that is similar
to what we're doing here and it's using INSN_START_WORDS:

====
static inline target_ulong get_memio_eip(CPUX86State *env)
{
#ifdef CONFIG_TCG
    uint64_t data[INSN_START_WORDS];
    CPUState *cs = env_cpu(env);

    if (!cpu_unwind_state_data(cs, cs->mem_io_pc, data)) {
        return env->eip;
    }

    /* Per x86_restore_state_to_opc. */
    if (tcg_cflags_has(cs, CF_PCREL)) {
        return (env->eip & TARGET_PAGE_MASK) | data[0];
    } else {
        return data[0] - env->segs[R_CS].base;
    }
#else
    qemu_build_not_reached();
#endif
}
====

I'm asking because the build in Alistair's branch is failing with this error:


../target/riscv/csr.c: In function ‘get_next_pc’:
../target/riscv/csr.c:2106:19: error: ‘TARGET_INSN_START_WORDS’ undeclared 
(first use in this function); did you mean ‘TCG_INSN_START_WORDS’?
 2106 |     uint64_t data[TARGET_INSN_START_WORDS];
      |                   ^~~~~~~~~~~~~~~~~~~~~~~
      |                   TCG_INSN_START_WORDS
../target/riscv/csr.c:2106:19: note: each undeclared identifier is reported 
only once for each function it appears in
../target/riscv/csr.c:2106:14: error: unused variable ‘data’ 
[-Werror=unused-variable]
 2106 |     uint64_t data[TARGET_INSN_START_WORDS];
      |              ^~~~
../target/riscv/csr.c:2115:1: error: control reaches end of non-void function 
[-Werror=return-type]
 2115 | }
      | ^
cc1: all warnings being treated as errors
[2206/3000] Compiling C object libqemu-riscv


Changing TARGET_INSN_START_WORDS to INSN_START_WORDS fixes the build issue.
Thanks,


Daniel

+
+    /* Outside of a running cpu, env contains the next pc. */
+    if (ra == 0 || !cpu_unwind_state_data(env_cpu(env), ra, data)) {
+        return env->pc;
+    }
+
+    /* Within unwind data, [0] is pc and [1] is the opcode. */
+    return data[0] + insn_len(data[1]);
+}
+
  static RISCVException write_misa(CPURISCVState *env, int csrno,
                                   target_ulong val, uintptr_t ra)
  {
@@ -2114,11 +2129,8 @@ static RISCVException write_misa(CPURISCVState *env, int 
csrno,
      /* Mask extensions that are not supported by this hart */
      val &= env->misa_ext_mask;

- /*

-     * Suppress 'C' if next instruction is not aligned
-     * TODO: this should check next_pc
-     */
-    if ((val & RVC) && (GETPC() & ~3) != 0) {
+    /* Suppress 'C' if next instruction is not aligned. */
+    if ((val & RVC) && (get_next_pc(env, ra) & ~3) != 0) {
          val &= ~RVC;
      }