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

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~