1. Patchew
  2. QEMU
  3. [PATCH 0/7] target/riscv: Fix write_misa vs aligned next_pc
  4. View patch

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

Richard Henderson posted 7 patches 6 months, 3 weeks ago
There is a newer version of this series
[PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc
Posted by Richard Henderson 6 months, 3 weeks ago 
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];
+
+    /* 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
Re: [PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc
Posted by Daniel Henrique Barboza 6 months ago 
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;
>       }
>
Re: [PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc
Posted by Richard Henderson 6 months ago 
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~
Re: [PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc
Posted by Richard Henderson 6 months, 2 weeks ago 
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~
Re: [PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc
Posted by Alistair Francis 6 months, 2 weeks ago 
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~
>
Re: [PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc
Posted by Alistair Francis 6 months, 3 weeks ago 
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
>
>
Re: [PATCH 7/7] target/riscv: Fix write_misa vs aligned next_pc
Posted by Philippe Mathieu-Daudé 6 months, 3 weeks ago 
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>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.