| Message ID | 20250425152311.804338-8-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: Fix write_misa vs aligned next_pc | expand |
On 25/4/25 17:23, 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.henderson@linaro.org> > --- > target/riscv/csr.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Sat, Apr 26, 2025 at 1:26 AM Richard Henderson <richard.henderson@linaro.org> 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.henderson@linaro.org> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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]; > + > + /* 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; > } > > -- > 2.43.0 > >
On 4/25/25 08:23, Richard Henderson wrote: > - if ((val & RVC) && (GETPC() & ~3) != 0) { > + /* Suppress 'C' if next instruction is not aligned. */ > + if ((val & RVC) && (get_next_pc(env, ra) & ~3) != 0) { Bah. I preserved a second bug here: not "& ~3" but "& 3". r~
On Wed, Apr 30, 2025 at 12:34 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/25/25 08:23, Richard Henderson wrote: > > - if ((val & RVC) && (GETPC() & ~3) != 0) { > > + /* Suppress 'C' if next instruction is not aligned. */ > > + if ((val & RVC) && (get_next_pc(env, ra) & ~3) != 0) { > > Bah. I preserved a second bug here: not "& ~3" but "& 3". Good catch I squashed this fix into the patch Alistair > > > r~ >
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.henderson@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; > } >
On 5/14/25 22:33, Daniel Henrique Barboza wrote: > 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.henderson@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: Yes, though INSN_START_WORDS is a very recent change. Probably the week after I posted this patch set. :-) r~
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]; + + /* 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; }
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.henderson@linaro.org> --- target/riscv/csr.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)