| Message ID | 20250425165055.807801-3-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/riscv: Fix write_misa vs aligned next_pc | expand |
On 4/25/25 1:50 PM, Richard Henderson wrote: > Do not examine a random host return address, but examine the > guest pc via env->pc. > > Fixes: f18637cd611 ("RISC-V: Add misa runtime write support") > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/riscv/csr.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index c52c87faae..992ec8ebff 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -2111,10 +2111,13 @@ static RISCVException write_misa(CPURISCVState *env, int csrno, > val &= env->misa_ext_mask; > > /* > - * Suppress 'C' if next instruction is not aligned > - * TODO: this should check next_pc > + * Suppress 'C' if next instruction is not aligned. > + * Outside of the context of a running cpu, env->pc contains next_pc. > + * Within the context of a running cpu, env->pc contains the pc of > + * the csrw/csrrw instruction. But since all such instructions are > + * exactly 4 bytes, next_pc has the same alignment mod 4. By "outside of the context of a running CPU" you mean a halted CPU, correct? And now, for a running CPU, env->pc has the pc of csrw/csrrw because of patch 1. Otherwise it would contain a pc that was synchronized via the last synchronize_from_tb, or any other insn that happened to sync env->pc in the same TB via gen_update_pc(). We're not keeping env->pc up to date all the time because it would be extra work that wouldn't be needed most of the time. Am I too off the mark? The reason I'm asking is because I see at least one place (get_physical_address()) where it's stated that "the env->pc at this point is incorrect". And I see env->pc being either the current PC or the next insn PC depending on the situation. Reading these 2 patches clarified it a bit (assuming I'm not completely incorrect, of course). For the patch: Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > */ > - if ((val & RVC) && (GETPC() & ~3) != 0) { > + if ((val & RVC) && (env->pc & ~3) != 0) { > val &= ~RVC; > } >
On 4/25/25 15:20, Daniel Henrique Barboza wrote: >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index c52c87faae..992ec8ebff 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -2111,10 +2111,13 @@ static RISCVException write_misa(CPURISCVState *env, int csrno, >> val &= env->misa_ext_mask; >> /* >> - * Suppress 'C' if next instruction is not aligned >> - * TODO: this should check next_pc >> + * Suppress 'C' if next instruction is not aligned. >> + * Outside of the context of a running cpu, env->pc contains next_pc. >> + * Within the context of a running cpu, env->pc contains the pc of >> + * the csrw/csrrw instruction. But since all such instructions are >> + * exactly 4 bytes, next_pc has the same alignment mod 4. > > > By "outside of the context of a running CPU" you mean a halted CPU, correct? Correct, e.g. from gdbstub. > > And now, for a running CPU, env->pc has the pc of csrw/csrrw because of patch 1. Correct. > Otherwise it would contain a pc that was synchronized via the last > synchronize_from_tb, or any other insn that happened to sync env->pc in > the same TB via gen_update_pc(). We're not keeping env->pc up to date all > the time because it would be extra work that wouldn't be needed most of the > time. Am I too off the mark? Correct. > > The reason I'm asking is because I see at least one place (get_physical_address()) > where it's stated that "the env->pc at this point is incorrect". Correct, since get_physical_address is called from riscv_cpu_tlb_fill, which can be called during the processing of any guest memory operation. > And I see env->pc > being either the current PC or the next insn PC depending on the situation. > Reading these 2 patches clarified it a bit (assuming I'm not completely incorrect, > of course). I would expect env->pc to be more or less random in get_physical_address, with a bias toward the PC of the first instruction of the TB. I'm not sure why get_physical_address has that comment. The current pc isn't relevant to resolving a virtual address. It only becomes relevant when there is a fault, and the pc is restored via unwinding along the fault path. r~
diff --git a/target/riscv/csr.c b/target/riscv/csr.c index c52c87faae..992ec8ebff 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -2111,10 +2111,13 @@ static RISCVException write_misa(CPURISCVState *env, int csrno, val &= env->misa_ext_mask; /* - * Suppress 'C' if next instruction is not aligned - * TODO: this should check next_pc + * Suppress 'C' if next instruction is not aligned. + * Outside of the context of a running cpu, env->pc contains next_pc. + * Within the context of a running cpu, env->pc contains the pc of + * the csrw/csrrw instruction. But since all such instructions are + * exactly 4 bytes, next_pc has the same alignment mod 4. */ - if ((val & RVC) && (GETPC() & ~3) != 0) { + if ((val & RVC) && (env->pc & ~3) != 0) { val &= ~RVC; }
Do not examine a random host return address, but examine the guest pc via env->pc. Fixes: f18637cd611 ("RISC-V: Add misa runtime write support") Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/riscv/csr.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)