Re: [PATCH 06/22] target/riscv: add more RISCVCPUDef fields

[Paolo Bonzini]

On 2/18/25 01:23, Alistair Francis wrote:
On Fri, Feb 7, 2025 at 4:29 AM Paolo Bonzini <pbonz...@redhat.com> wrote:

Allow using RISCVCPUDef to replicate all the logic of custom .instance_init
functions.  To simulate inheritance, merge the child's RISCVCPUDef with
the parent and then finally move it to the CPUState at the end of
TYPE_RISCV_CPU's own instance_init function.

STRUCT_FIELD is introduced here because I am not sure it is needed;
it is a bit ugly and I wanted not to have it in the patch that
introduces cpu_cfg_fields.h.inc.  I don't really understand why satp_mode
is included in RISCVCPUConfig; therefore, the end of the series includes

I don't follow. `satp_mode` is a configurable option, hence the
inclusion in `RISCVCPUConfig`

My understanding is that satp_mode is a combination of:

- the maximum supported satp mode; this is stored in 
cpu->satp_modes.supported and actually comes from the instance_init 
function.

- the value of the properties, which is stored in satp_map->map and 
satp_map->init

For non-bare CPU models, including all vendor CPU models, 
cpu->satp_modes.supported also acts as the default value for the mode 
properties.  Furthermore, for all vendor CPU models all modes are 
supported up to the max mode, for example you never have a CPU that 
supports sv48 but not sv39.

So it seems that the CPU config is really the supported satp modes, it's 
just bare CPU models that have the further restriction that you have to 
specify at least one mode by hand.  Under this interpretation:

- all that needs to be in RISCVCPUConfig is what I called satp_mode32 
and satp_mode64

- satp_mode32 or satp_mode64 therefore can be removed from RISCVCPUDef

- init and map would be moved to RISCVCPU because they are purely a QOM 
concept

Paolo

Alistair

a patch to move satp_mode directly in RISCVCPU, thus removing the need
for STRUCT_FIELD; it can be moved before this one in a non-RFC posting.

Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
  target/riscv/cpu.h                |  6 ++++
  target/riscv/cpu_cfg.h            |  1 +
  target/riscv/cpu_cfg_fields.h.inc |  6 +++-
  target/riscv/cpu.c                | 48 ++++++++++++++++++++++++++++++-
  4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f757f0b6210..9b25c0c889b 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -519,6 +519,12 @@ struct ArchCPU {

  typedef struct RISCVCPUDef {
      RISCVMXL misa_mxl_max;  /* max mxl for this cpu */
+    uint32_t misa_ext;
+    int priv_spec;
+    int32_t vext_spec;
+    int satp_mode32;
+    int satp_mode64;
+    RISCVCPUConfig cfg;
  } RISCVCPUDef;

  /**
diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
index ad02693fa66..07789a9de88 100644
--- a/target/riscv/cpu_cfg.h
+++ b/target/riscv/cpu_cfg.h
@@ -39,6 +39,7 @@ typedef struct {
  struct RISCVCPUConfig {
  #define BOOL_FIELD(x) bool x;
  #define TYPED_FIELD(type, x) type x;
+#define STRUCT_FIELD(type, x) type x;
  #include "cpu_cfg_fields.h.inc"
  };

diff --git a/target/riscv/cpu_cfg_fields.h.inc 
b/target/riscv/cpu_cfg_fields.h.inc
index 56fffb5f177..cbedf0a703b 100644
--- a/target/riscv/cpu_cfg_fields.h.inc
+++ b/target/riscv/cpu_cfg_fields.h.inc
@@ -4,6 +4,9 @@
  #ifndef TYPED_FIELD
  #define TYPED_FIELD(type, x)
  #endif
+#ifndef STRUCT_FIELD
+#define STRUCT_FIELD(type, x)
+#endif

  BOOL_FIELD(ext_zba)
  BOOL_FIELD(ext_zbb)
@@ -160,8 +163,9 @@ TYPED_FIELD(uint16_t, cbop_blocksize)
  TYPED_FIELD(uint16_t, cboz_blocksize)

  #ifndef CONFIG_USER_ONLY
-TYPED_FIELD(RISCVSATPMap, satp_mode);
+STRUCT_FIELD(RISCVSATPMap, satp_mode)
  #endif

  #undef BOOL_FIELD
  #undef TYPED_FIELD
+#undef STRUCT_FIELD
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index baf4dd017b2..1d999488465 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -74,6 +74,15 @@ bool riscv_cpu_option_set(const char *optname)
      return g_hash_table_contains(general_user_opts, optname);
  }

+static void riscv_cpu_cfg_merge(RISCVCPUConfig *dest, RISCVCPUConfig *src)
+{
+#define BOOL_FIELD(x) dest->x |= src->x;
+#define TYPED_FIELD(type, x) if (src->x) dest->x = src->x;
+    /* only satp_mode, which is initialized by instance_init */
+#define STRUCT_FIELD(type, x)
+#include "cpu_cfg_fields.h.inc"
+}
+
  #define ISA_EXT_DATA_ENTRY(_name, _min_ver, _prop) \
      {#_name, _min_ver, CPU_CFG_OFFSET(_prop)}

@@ -432,7 +441,7 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
  }

  static void set_satp_mode_max_supported(RISCVCPU *cpu,
-                                        uint8_t satp_mode)
+                                        int satp_mode)
  {
      bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
      const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
@@ -1476,6 +1485,24 @@ static void riscv_cpu_init(Object *obj)
      cpu->cfg.cbop_blocksize = 64;
      cpu->cfg.cboz_blocksize = 64;
      cpu->env.vext_ver = VEXT_VERSION_1_00_0;
+
+    env->misa_ext_mask = env->misa_ext = mcc->def->misa_ext;
+    riscv_cpu_cfg_merge(&cpu->cfg, &mcc->def->cfg);
+
+    if (mcc->def->priv_spec != RISCV_PROFILE_ATTR_UNUSED) {
+        cpu->env.priv_ver = mcc->def->priv_spec;
+    }
+    if (mcc->def->vext_spec != RISCV_PROFILE_ATTR_UNUSED) {
+        cpu->env.vext_ver = mcc->def->vext_spec;
+    }
+#ifndef CONFIG_USER_ONLY
+    if (riscv_cpu_mxl(env) == MXL_RV32 && mcc->def->satp_mode32 != 
RISCV_PROFILE_ATTR_UNUSED) {
+        set_satp_mode_max_supported(RISCV_CPU(obj), mcc->def->satp_mode32);
+    }
+    if (riscv_cpu_mxl(env) >= MXL_RV64 && mcc->def->satp_mode64 != 
RISCV_PROFILE_ATTR_UNUSED) {
+        set_satp_mode_max_supported(RISCV_CPU(obj), mcc->def->satp_mode64);
+    }
+#endif
  }

  static void riscv_bare_cpu_init(Object *obj)
@@ -2968,6 +2995,25 @@ static void riscv_cpu_class_base_init(ObjectClass *c, 
void *data)
              assert(def->misa_mxl_max <= MXL_RV128);
              mcc->def->misa_mxl_max = def->misa_mxl_max;
          }
+        if (def->priv_spec != RISCV_PROFILE_ATTR_UNUSED) {
+            assert(def->priv_spec <= PRIV_VERSION_LATEST);
+            mcc->def->priv_spec = def->priv_spec;
+        }
+        if (def->vext_spec != RISCV_PROFILE_ATTR_UNUSED) {
+            assert(def->vext_spec != 0);
+            mcc->def->vext_spec = def->vext_spec;
+        }
+        if (def->satp_mode32 != RISCV_PROFILE_ATTR_UNUSED) {
+            assert(def->satp_mode32 <= VM_1_10_SV32);
+            mcc->def->satp_mode32 = def->satp_mode32;
+        }
+        if (def->satp_mode64 != RISCV_PROFILE_ATTR_UNUSED) {
+            assert(def->satp_mode64 <= VM_1_10_SV64);
+            mcc->def->satp_mode64 = def->satp_mode64;
+        }
+        mcc->def->misa_ext |= def->misa_ext;
+
+        riscv_cpu_cfg_merge(&mcc->def->cfg, &def->cfg);
      }

      if (!object_class_is_abstract(c)) {
--
2.48.1