[PATCH v3 10/34] target/riscv: Fix size of gpr and gprh

Anton Johansson via posted 34 patches 3 months, 4 weeks ago
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Laurent Vivier <laurent@vivier.eu>, Christoph Muellner <christoph.muellner@vrull.eu>
There is a newer version of this series
[PATCH v3 10/34] target/riscv: Fix size of gpr and gprh
Posted by Anton Johansson via 3 months, 4 weeks ago
gprh is only needed for TARGET_RISCV64 when modeling 128-bit registers,
fixing their size to 64 bits makes sense.

gpr is also fixed to 64 bits since all direct uses of env->gpr
correctly zero extend/truncate to/from target_ulong, meaning
!TARGET_RISCV64 will behave as expected.

We do however need to be a bit careful when mapping 64-bit fields to
32-bit TCGv globals on big endian hosts.

Note, the cpu/rv128 VMSTATE version is bumped, breaking migration from
older versions.

Signed-off-by: Anton Johansson <anjo@rev.ng>
---
 target/riscv/cpu.h            |  4 ++--
 target/riscv/cpu.c            |  2 +-
 target/riscv/machine.c        |  8 ++++----
 target/riscv/riscv-qmp-cmds.c |  2 +-
 target/riscv/translate.c      | 17 +++++++++++++++--
 5 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 09d9e4c33c..7573d5aa7e 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -212,8 +212,8 @@ typedef struct PMUFixedCtrState {
 } PMUFixedCtrState;
 
 struct CPUArchState {
-    target_ulong gpr[32];
-    target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
+    uint64_t gpr[32];
+    uint64_t gprh[32]; /* 64 top bits of the 128-bit registers */
 
     /* vector coprocessor state. */
     uint64_t vreg[32 * RV_VLEN_MAX / 64] QEMU_ALIGNED(16);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index a877018ab0..b7690ac00f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -583,7 +583,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
 #endif
 
     for (i = 0; i < 32; i++) {
-        qemu_fprintf(f, " %-8s " TARGET_FMT_lx,
+        qemu_fprintf(f, " %-8s %" PRIx64,
                      riscv_int_regnames[i], env->gpr[i]);
         if ((i & 3) == 3) {
             qemu_fprintf(f, "\n");
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index 09c032a879..7349383eab 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -177,11 +177,11 @@ static bool rv128_needed(void *opaque)
 
 static const VMStateDescription vmstate_rv128 = {
     .name = "cpu/rv128",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .needed = rv128_needed,
     .fields = (const VMStateField[]) {
-        VMSTATE_UINTTL_ARRAY(env.gprh, RISCVCPU, 32),
+        VMSTATE_UINT64_ARRAY(env.gprh, RISCVCPU, 32),
         VMSTATE_UINT64(env.mscratchh, RISCVCPU),
         VMSTATE_UINT64(env.sscratchh, RISCVCPU),
         VMSTATE_END_OF_LIST()
@@ -429,7 +429,7 @@ const VMStateDescription vmstate_riscv_cpu = {
     .minimum_version_id = 11,
     .post_load = riscv_cpu_post_load,
     .fields = (const VMStateField[]) {
-        VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
+        VMSTATE_UINT64_ARRAY(env.gpr, RISCVCPU, 32),
         VMSTATE_UINT64_ARRAY(env.fpr, RISCVCPU, 32),
         VMSTATE_UINT8_ARRAY(env.miprio, RISCVCPU, 64),
         VMSTATE_UINT8_ARRAY(env.siprio, RISCVCPU, 64),
diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
index c499f9b9a7..95fa713c69 100644
--- a/target/riscv/riscv-qmp-cmds.c
+++ b/target/riscv/riscv-qmp-cmds.c
@@ -262,7 +262,7 @@ static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
                                  target_ulong *val, bool is_gprh)
 {
     const char * const *reg_names;
-    target_ulong *vals;
+    uint64_t *vals;
 
     if (is_gprh) {
         reg_names = riscv_int_regnamesh;
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 6fc06c71f5..4308b7712e 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -27,6 +27,7 @@
 #include "accel/tcg/cpu-ldst.h"
 #include "exec/translation-block.h"
 #include "exec/log.h"
+#include "exec/tswap.h"
 #include "semihosting/semihost.h"
 
 #include "internals.h"
@@ -1428,12 +1429,24 @@ void riscv_translate_init(void)
      */
     cpu_gpr[0] = NULL;
     cpu_gprh[0] = NULL;
+    /*
+     * Be careful with big endian hosts when mapping 64-bit CPUArchState fields
+     * to 32-bit TCGv globals.  An offset of 4 bytes is applied so the least
+     * significant bytes are correctly written to.
+     */
+#if HOST_BIG_ENDIAN && !defined(TARGET_RISCV64)
+    size_t field_offset = 4;
+#else
+    size_t field_offset = 0;
+#endif
 
     for (i = 1; i < 32; i++) {
         cpu_gpr[i] = tcg_global_mem_new(tcg_env,
-            offsetof(CPURISCVState, gpr[i]), riscv_int_regnames[i]);
+            offsetof(CPURISCVState, gpr[i]) + field_offset,
+            riscv_int_regnames[i]);
         cpu_gprh[i] = tcg_global_mem_new(tcg_env,
-            offsetof(CPURISCVState, gprh[i]), riscv_int_regnamesh[i]);
+            offsetof(CPURISCVState, gprh[i]) + field_offset,
+            riscv_int_regnamesh[i]);
     }
 
     for (i = 0; i < 32; i++) {
-- 
2.51.0
Re: [PATCH v3 10/34] target/riscv: Fix size of gpr and gprh
Posted by Alistair Francis 3 months, 3 weeks ago
On Wed, Oct 15, 2025 at 6:36 AM Anton Johansson via
<qemu-devel@nongnu.org> wrote:
>
> gprh is only needed for TARGET_RISCV64 when modeling 128-bit registers,
> fixing their size to 64 bits makes sense.
>
> gpr is also fixed to 64 bits since all direct uses of env->gpr
> correctly zero extend/truncate to/from target_ulong, meaning
> !TARGET_RISCV64 will behave as expected.
>
> We do however need to be a bit careful when mapping 64-bit fields to
> 32-bit TCGv globals on big endian hosts.
>
> Note, the cpu/rv128 VMSTATE version is bumped, breaking migration from
> older versions.
>
> Signed-off-by: Anton Johansson <anjo@rev.ng>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.h            |  4 ++--
>  target/riscv/cpu.c            |  2 +-
>  target/riscv/machine.c        |  8 ++++----
>  target/riscv/riscv-qmp-cmds.c |  2 +-
>  target/riscv/translate.c      | 17 +++++++++++++++--
>  5 files changed, 23 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 09d9e4c33c..7573d5aa7e 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -212,8 +212,8 @@ typedef struct PMUFixedCtrState {
>  } PMUFixedCtrState;
>
>  struct CPUArchState {
> -    target_ulong gpr[32];
> -    target_ulong gprh[32]; /* 64 top bits of the 128-bit registers */
> +    uint64_t gpr[32];
> +    uint64_t gprh[32]; /* 64 top bits of the 128-bit registers */
>
>      /* vector coprocessor state. */
>      uint64_t vreg[32 * RV_VLEN_MAX / 64] QEMU_ALIGNED(16);
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index a877018ab0..b7690ac00f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -583,7 +583,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #endif
>
>      for (i = 0; i < 32; i++) {
> -        qemu_fprintf(f, " %-8s " TARGET_FMT_lx,
> +        qemu_fprintf(f, " %-8s %" PRIx64,
>                       riscv_int_regnames[i], env->gpr[i]);
>          if ((i & 3) == 3) {
>              qemu_fprintf(f, "\n");
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index 09c032a879..7349383eab 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -177,11 +177,11 @@ static bool rv128_needed(void *opaque)
>
>  static const VMStateDescription vmstate_rv128 = {
>      .name = "cpu/rv128",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .needed = rv128_needed,
>      .fields = (const VMStateField[]) {
> -        VMSTATE_UINTTL_ARRAY(env.gprh, RISCVCPU, 32),
> +        VMSTATE_UINT64_ARRAY(env.gprh, RISCVCPU, 32),
>          VMSTATE_UINT64(env.mscratchh, RISCVCPU),
>          VMSTATE_UINT64(env.sscratchh, RISCVCPU),
>          VMSTATE_END_OF_LIST()
> @@ -429,7 +429,7 @@ const VMStateDescription vmstate_riscv_cpu = {
>      .minimum_version_id = 11,
>      .post_load = riscv_cpu_post_load,
>      .fields = (const VMStateField[]) {
> -        VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
> +        VMSTATE_UINT64_ARRAY(env.gpr, RISCVCPU, 32),
>          VMSTATE_UINT64_ARRAY(env.fpr, RISCVCPU, 32),
>          VMSTATE_UINT8_ARRAY(env.miprio, RISCVCPU, 64),
>          VMSTATE_UINT8_ARRAY(env.siprio, RISCVCPU, 64),
> diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
> index c499f9b9a7..95fa713c69 100644
> --- a/target/riscv/riscv-qmp-cmds.c
> +++ b/target/riscv/riscv-qmp-cmds.c
> @@ -262,7 +262,7 @@ static bool reg_is_ulong_integer(CPURISCVState *env, const char *name,
>                                   target_ulong *val, bool is_gprh)
>  {
>      const char * const *reg_names;
> -    target_ulong *vals;
> +    uint64_t *vals;
>
>      if (is_gprh) {
>          reg_names = riscv_int_regnamesh;
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 6fc06c71f5..4308b7712e 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -27,6 +27,7 @@
>  #include "accel/tcg/cpu-ldst.h"
>  #include "exec/translation-block.h"
>  #include "exec/log.h"
> +#include "exec/tswap.h"
>  #include "semihosting/semihost.h"
>
>  #include "internals.h"
> @@ -1428,12 +1429,24 @@ void riscv_translate_init(void)
>       */
>      cpu_gpr[0] = NULL;
>      cpu_gprh[0] = NULL;
> +    /*
> +     * Be careful with big endian hosts when mapping 64-bit CPUArchState fields
> +     * to 32-bit TCGv globals.  An offset of 4 bytes is applied so the least
> +     * significant bytes are correctly written to.
> +     */
> +#if HOST_BIG_ENDIAN && !defined(TARGET_RISCV64)
> +    size_t field_offset = 4;
> +#else
> +    size_t field_offset = 0;
> +#endif
>
>      for (i = 1; i < 32; i++) {
>          cpu_gpr[i] = tcg_global_mem_new(tcg_env,
> -            offsetof(CPURISCVState, gpr[i]), riscv_int_regnames[i]);
> +            offsetof(CPURISCVState, gpr[i]) + field_offset,
> +            riscv_int_regnames[i]);
>          cpu_gprh[i] = tcg_global_mem_new(tcg_env,
> -            offsetof(CPURISCVState, gprh[i]), riscv_int_regnamesh[i]);
> +            offsetof(CPURISCVState, gprh[i]) + field_offset,
> +            riscv_int_regnamesh[i]);
>      }
>
>      for (i = 0; i < 32; i++) {
> --
> 2.51.0
>
>
Re: [PATCH v3 10/34] target/riscv: Fix size of gpr and gprh
Posted by Pierrick Bouvier 3 months, 3 weeks ago
On 10/14/25 1:34 PM, Anton Johansson wrote:
> gprh is only needed for TARGET_RISCV64 when modeling 128-bit registers,
> fixing their size to 64 bits makes sense.
> 
> gpr is also fixed to 64 bits since all direct uses of env->gpr
> correctly zero extend/truncate to/from target_ulong, meaning
> !TARGET_RISCV64 will behave as expected.
> 
> We do however need to be a bit careful when mapping 64-bit fields to
> 32-bit TCGv globals on big endian hosts.
> 
> Note, the cpu/rv128 VMSTATE version is bumped, breaking migration from
> older versions.
> 
> Signed-off-by: Anton Johansson <anjo@rev.ng>
> ---
>   target/riscv/cpu.h            |  4 ++--
>   target/riscv/cpu.c            |  2 +-
>   target/riscv/machine.c        |  8 ++++----
>   target/riscv/riscv-qmp-cmds.c |  2 +-
>   target/riscv/translate.c      | 17 +++++++++++++++--
>   5 files changed, 23 insertions(+), 10 deletions(-)
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>