[PATCH alternate 0/2] target/riscv: Fix write_misa vs aligned next_pc

[PATCH alternate 0/2] target/riscv: Fix write_misa vs aligned next_pc

[Richard Henderson]

This is an alternate, but less exact approach.  It assumes that there
will never be a 16 or 48-bit csr write instruction.  This feels dirtier,
but it's a fair assumption involves much less faff.


r~


Richard Henderson (2):
  target/riscv: Update pc before csrw, csrrw
  target/riscv: Fix write_misa vs aligned next_pc

 target/riscv/csr.c                      | 9 ++++++---
 target/riscv/insn_trans/trans_rvi.c.inc | 4 ++++
 2 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH alternate 1/2] target/riscv: Update pc before csrw, csrrw

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/riscv/insn_trans/trans_rvi.c.inc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index b9c7160468..a367fecf7e 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -992,6 +992,7 @@ static bool do_csrw(DisasContext *ctx, int rc, TCGv src)
     TCGv_i32 csr = tcg_constant_i32(rc);
 
     translator_io_start(&ctx->base);
+    gen_update_pc(ctx, 0);
     gen_helper_csrw(tcg_env, csr, src);
     return do_csr_post(ctx);
 }
@@ -1002,6 +1003,7 @@ static bool do_csrrw(DisasContext *ctx, int rd, int rc, TCGv src, TCGv mask)
     TCGv_i32 csr = tcg_constant_i32(rc);
 
     translator_io_start(&ctx->base);
+    gen_update_pc(ctx, 0);
     gen_helper_csrrw(dest, tcg_env, csr, src, mask);
     gen_set_gpr(ctx, rd, dest);
     return do_csr_post(ctx);
@@ -1025,6 +1027,7 @@ static bool do_csrw_i128(DisasContext *ctx, int rc, TCGv srcl, TCGv srch)
     TCGv_i32 csr = tcg_constant_i32(rc);
 
     translator_io_start(&ctx->base);
+    gen_update_pc(ctx, 0);
     gen_helper_csrw_i128(tcg_env, csr, srcl, srch);
     return do_csr_post(ctx);
 }
@@ -1037,6 +1040,7 @@ static bool do_csrrw_i128(DisasContext *ctx, int rd, int rc,
     TCGv_i32 csr = tcg_constant_i32(rc);
 
     translator_io_start(&ctx->base);
+    gen_update_pc(ctx, 0);
     gen_helper_csrrw_i128(destl, tcg_env, csr, srcl, srch, maskl, maskh);
     tcg_gen_ld_tl(desth, tcg_env, offsetof(CPURISCVState, retxh));
     gen_set_gpr128(ctx, rd, destl, desth);
-- 
2.43.0

[PATCH alternate 2/2] target/riscv: Fix write_misa vs aligned next_pc

[Richard Henderson]

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.
      */
-    if ((val & RVC) && (GETPC() & ~3) != 0) {
+    if ((val & RVC) && (env->pc & ~3) != 0) {
         val &= ~RVC;
     }
 
-- 
2.43.0

Re: [PATCH alternate 1/2] target/riscv: Update pc before csrw, csrrw

[Daniel Henrique Barboza]

On 4/25/25 1:50 PM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   target/riscv/insn_trans/trans_rvi.c.inc | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> index b9c7160468..a367fecf7e 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -992,6 +992,7 @@ static bool do_csrw(DisasContext *ctx, int rc, TCGv src)
>       TCGv_i32 csr = tcg_constant_i32(rc);
>   
>       translator_io_start(&ctx->base);
> +    gen_update_pc(ctx, 0);
>       gen_helper_csrw(tcg_env, csr, src);
>       return do_csr_post(ctx);
>   }
> @@ -1002,6 +1003,7 @@ static bool do_csrrw(DisasContext *ctx, int rd, int rc, TCGv src, TCGv mask)
>       TCGv_i32 csr = tcg_constant_i32(rc);
>   
>       translator_io_start(&ctx->base);
> +    gen_update_pc(ctx, 0);
>       gen_helper_csrrw(dest, tcg_env, csr, src, mask);
>       gen_set_gpr(ctx, rd, dest);
>       return do_csr_post(ctx);
> @@ -1025,6 +1027,7 @@ static bool do_csrw_i128(DisasContext *ctx, int rc, TCGv srcl, TCGv srch)
>       TCGv_i32 csr = tcg_constant_i32(rc);
>   
>       translator_io_start(&ctx->base);
> +    gen_update_pc(ctx, 0);
>       gen_helper_csrw_i128(tcg_env, csr, srcl, srch);
>       return do_csr_post(ctx);
>   }
> @@ -1037,6 +1040,7 @@ static bool do_csrrw_i128(DisasContext *ctx, int rd, int rc,
>       TCGv_i32 csr = tcg_constant_i32(rc);
>   
>       translator_io_start(&ctx->base);
> +    gen_update_pc(ctx, 0);
>       gen_helper_csrrw_i128(destl, tcg_env, csr, srcl, srch, maskl, maskh);
>       tcg_gen_ld_tl(desth, tcg_env, offsetof(CPURISCVState, retxh));
>       gen_set_gpr128(ctx, rd, destl, desth);

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

[Daniel Henrique Barboza]

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;
>       }
>   

Re: [PATCH alternate 0/2] target/riscv: Fix write_misa vs aligned next_pc

[Philippe Mathieu-Daudé]

On 25/4/25 18:50, Richard Henderson wrote:
> This is an alternate, but less exact approach.  It assumes that there
> will never be a 16 or 48-bit csr write instruction.  This feels dirtier,
> but it's a fair assumption involves much less faff.

I prefer the other safer version which properly propagate $ra, which
could be useful for callees.

Re: [PATCH alternate 0/2] target/riscv: Fix write_misa vs aligned next_pc

[Daniel Henrique Barboza]

On 4/25/25 7:35 PM, Philippe Mathieu-Daudé wrote:
> On 25/4/25 18:50, Richard Henderson wrote:
>> This is an alternate, but less exact approach.  It assumes that there
>> will never be a 16 or 48-bit csr write instruction.  This feels dirtier,
>> but it's a fair assumption involves much less faff.
> 
> I prefer the other safer version which properly propagate $ra, which
> could be useful for callees.

Both are fine to me. Alistair can pick the one he likes more.



Thanks,

Daniel

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

[Richard Henderson]

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~