[v4] target/riscv: SATP mode and CPU definition overhaul

[PATCH 08/26] target/riscv: store RISCVCPUDef struct directly in the class

Posted by Paolo Bonzini 7 months, 3 weeks ago

Prepare for adding more fields to RISCVCPUDef and reading them in
riscv_cpu_init: instead of storing the misa_mxl_max field in
RISCVCPUClass, ensure that there's always a valid RISCVCPUDef struct
and go through it.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/riscv/cpu.h         |  2 +-
 hw/riscv/boot.c            |  2 +-
 target/riscv/cpu.c         | 23 ++++++++++++++++++-----
 target/riscv/gdbstub.c     |  6 +++---
 target/riscv/kvm/kvm-cpu.c | 21 +++++++++------------
 target/riscv/machine.c     |  2 +-
 target/riscv/tcg/tcg-cpu.c | 10 +++++-----
 target/riscv/translate.c   |  2 +-
 8 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 62e303f0635..842c9d3f194 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -553,7 +553,7 @@ struct RISCVCPUClass {
 
     DeviceRealize parent_realize;
     ResettablePhases parent_phases;
-    RISCVMXL misa_mxl_max;  /* max mxl for this cpu */
+    RISCVCPUDef *def;
 };
 
 static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 765b9e2b1ab..828a867be39 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -37,7 +37,7 @@
 bool riscv_is_32bit(RISCVHartArrayState *harts)
 {
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(&harts->harts[0]);
-    return mcc->misa_mxl_max == MXL_RV32;
+    return mcc->def->misa_mxl_max == MXL_RV32;
 }
 
 /*
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f30cf1b532b..d8c189d596b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -357,7 +357,7 @@ void riscv_cpu_set_misa_ext(CPURISCVState *env, uint32_t ext)
 
 int riscv_cpu_max_xlen(RISCVCPUClass *mcc)
 {
-    return 16 << mcc->misa_mxl_max;
+    return 16 << mcc->def->misa_mxl_max;
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -1048,7 +1048,7 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
         mcc->parent_phases.hold(obj, type);
     }
 #ifndef CONFIG_USER_ONLY
-    env->misa_mxl = mcc->misa_mxl_max;
+    env->misa_mxl = mcc->def->misa_mxl_max;
     env->priv = PRV_M;
     env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
     if (env->misa_mxl > MXL_RV32) {
@@ -1450,7 +1450,7 @@ static void riscv_cpu_init(Object *obj)
     RISCVCPU *cpu = RISCV_CPU(obj);
     CPURISCVState *env = &cpu->env;
 
-    env->misa_mxl = mcc->misa_mxl_max;
+    env->misa_mxl = mcc->def->misa_mxl_max;
 
 #ifndef CONFIG_USER_ONLY
     qdev_init_gpio_in(DEVICE(obj), riscv_cpu_set_irq,
@@ -1544,7 +1544,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPUClass *mcc)
     CPUClass *cc = CPU_CLASS(mcc);
 
     /* Validate that MISA_MXL is set properly. */
-    switch (mcc->misa_mxl_max) {
+    switch (mcc->def->misa_mxl_max) {
 #ifdef TARGET_RISCV64
     case MXL_RV64:
     case MXL_RV128:
@@ -3071,12 +3071,24 @@ static void riscv_cpu_common_class_init(ObjectClass *c, const void *data)
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
+static void riscv_cpu_class_base_init(ObjectClass *c, const void *data)
+{
+    RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
+    RISCVCPUClass *pcc = RISCV_CPU_CLASS(object_class_get_parent(c));
+
+    if (pcc->def) {
+        mcc->def = g_memdup2(pcc->def, sizeof(*pcc->def));
+    } else {
+        mcc->def = g_new0(RISCVCPUDef, 1);
+    }
+}
+
 static void riscv_cpu_class_init(ObjectClass *c, const void *data)
 {
     RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
     const RISCVCPUDef *def = data;
 
-    mcc->misa_mxl_max = def->misa_mxl_max;
+    mcc->def->misa_mxl_max = def->misa_mxl_max;
     riscv_cpu_validate_misa_mxl(mcc);
 }
 
@@ -3227,6 +3239,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
         .abstract = true,
         .class_size = sizeof(RISCVCPUClass),
         .class_init = riscv_cpu_common_class_init,
+        .class_base_init = riscv_cpu_class_base_init,
     },
     {
         .name = TYPE_RISCV_DYNAMIC_CPU,
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 18e88f416af..1934f919c01 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -62,7 +62,7 @@ int riscv_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         return 0;
     }
 
-    switch (mcc->misa_mxl_max) {
+    switch (mcc->def->misa_mxl_max) {
     case MXL_RV32:
         return gdb_get_reg32(mem_buf, tmp);
     case MXL_RV64:
@@ -82,7 +82,7 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
     int length = 0;
     target_ulong tmp;
 
-    switch (mcc->misa_mxl_max) {
+    switch (mcc->def->misa_mxl_max) {
     case MXL_RV32:
         tmp = (int32_t)ldl_p(mem_buf);
         length = 4;
@@ -359,7 +359,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
                                  ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs),
                                  0);
     }
-    switch (mcc->misa_mxl_max) {
+    switch (mcc->def->misa_mxl_max) {
     case MXL_RV32:
         gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
                                  riscv_gdb_set_virtual,
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 75724b6af4f..41b6f34552a 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1997,22 +1997,19 @@ static void kvm_cpu_accel_register_types(void)
 }
 type_init(kvm_cpu_accel_register_types);
 
-static void riscv_host_cpu_class_init(ObjectClass *c, const void *data)
-{
-    RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
-
-#if defined(TARGET_RISCV32)
-    mcc->misa_mxl_max = MXL_RV32;
-#elif defined(TARGET_RISCV64)
-    mcc->misa_mxl_max = MXL_RV64;
-#endif
-}
-
 static const TypeInfo riscv_kvm_cpu_type_infos[] = {
     {
         .name = TYPE_RISCV_CPU_HOST,
         .parent = TYPE_RISCV_CPU,
-        .class_init = riscv_host_cpu_class_init,
+#if defined(TARGET_RISCV32)
+        .class_data = &((const RISCVCPUDef) {
+            .misa_mxl_max = MXL_RV32,
+        },
+#elif defined(TARGET_RISCV64)
+        .class_data = &((const RISCVCPUDef) {
+            .misa_mxl_max = MXL_RV64,
+        },
+#endif
     }
 };
 
diff --git a/target/riscv/machine.c b/target/riscv/machine.c
index a1f70cc9556..c97e9ce9df1 100644
--- a/target/riscv/machine.c
+++ b/target/riscv/machine.c
@@ -170,7 +170,7 @@ static bool rv128_needed(void *opaque)
 {
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(opaque);
 
-    return mcc->misa_mxl_max == MXL_RV128;
+    return mcc->def->misa_mxl_max == MXL_RV128;
 }
 
 static const VMStateDescription vmstate_rv128 = {
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 1192b4e1545..f3ca61103c4 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -592,7 +592,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         return;
     }
 
-    if (mcc->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
+    if (mcc->def->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
         error_setg(errp, "Zcf extension is only relevant to RV32");
         return;
     }
@@ -689,7 +689,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         return;
     }
 
-    if (mcc->misa_mxl_max == MXL_RV32 && cpu->cfg.ext_svukte) {
+    if (mcc->def->misa_mxl_max == MXL_RV32 && cpu->cfg.ext_svukte) {
         error_setg(errp, "svukte is not supported for RV32");
         return;
     }
@@ -927,7 +927,7 @@ static void cpu_enable_zc_implied_rules(RISCVCPU *cpu)
         cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmp), true);
         cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmt), true);
 
-        if (riscv_has_ext(env, RVF) && mcc->misa_mxl_max == MXL_RV32) {
+        if (riscv_has_ext(env, RVF) && mcc->def->misa_mxl_max == MXL_RV32) {
             cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
         }
     }
@@ -936,7 +936,7 @@ static void cpu_enable_zc_implied_rules(RISCVCPU *cpu)
     if (riscv_has_ext(env, RVC) && env->priv_ver >= PRIV_VERSION_1_12_0) {
         cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
 
-        if (riscv_has_ext(env, RVF) && mcc->misa_mxl_max == MXL_RV32) {
+        if (riscv_has_ext(env, RVF) && mcc->def->misa_mxl_max == MXL_RV32) {
             cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
         }
 
@@ -1062,7 +1062,7 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
 #ifndef CONFIG_USER_ONLY
     RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
 
-    if (mcc->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
+    if (mcc->def->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
         /* Missing 128-bit aligned atomics */
         error_setg(errp,
                    "128-bit RISC-V currently does not work with Multi "
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index cef61b5b290..26ac7cdcaa1 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -1282,7 +1282,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->cfg_vta_all_1s = cpu->cfg.rvv_ta_all_1s;
     ctx->vstart_eq_zero = FIELD_EX32(tb_flags, TB_FLAGS, VSTART_EQ_ZERO);
     ctx->vl_eq_vlmax = FIELD_EX32(tb_flags, TB_FLAGS, VL_EQ_VLMAX);
-    ctx->misa_mxl_max = mcc->misa_mxl_max;
+    ctx->misa_mxl_max = mcc->def->misa_mxl_max;
     ctx->xl = FIELD_EX32(tb_flags, TB_FLAGS, XL);
     ctx->address_xl = FIELD_EX32(tb_flags, TB_FLAGS, AXL);
     ctx->cs = cs;
-- 
2.49.0

Re: [PATCH 08/26] target/riscv: store RISCVCPUDef struct directly in the class

Posted by Daniel Henrique Barboza 7 months, 3 weeks ago


On 4/28/25 4:34 AM, Paolo Bonzini wrote:
> Prepare for adding more fields to RISCVCPUDef and reading them in
> riscv_cpu_init: instead of storing the misa_mxl_max field in
> RISCVCPUClass, ensure that there's always a valid RISCVCPUDef struct
> and go through it.
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/riscv/cpu.h         |  2 +-
>   hw/riscv/boot.c            |  2 +-
>   target/riscv/cpu.c         | 23 ++++++++++++++++++-----
>   target/riscv/gdbstub.c     |  6 +++---
>   target/riscv/kvm/kvm-cpu.c | 21 +++++++++------------
>   target/riscv/machine.c     |  2 +-
>   target/riscv/tcg/tcg-cpu.c | 10 +++++-----
>   target/riscv/translate.c   |  2 +-
>   8 files changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 62e303f0635..842c9d3f194 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -553,7 +553,7 @@ struct RISCVCPUClass {
>   
>       DeviceRealize parent_realize;
>       ResettablePhases parent_phases;
> -    RISCVMXL misa_mxl_max;  /* max mxl for this cpu */
> +    RISCVCPUDef *def;
>   };
>   
>   static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 765b9e2b1ab..828a867be39 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -37,7 +37,7 @@
>   bool riscv_is_32bit(RISCVHartArrayState *harts)
>   {
>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(&harts->harts[0]);
> -    return mcc->misa_mxl_max == MXL_RV32;
> +    return mcc->def->misa_mxl_max == MXL_RV32;
>   }
>   
>   /*
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f30cf1b532b..d8c189d596b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -357,7 +357,7 @@ void riscv_cpu_set_misa_ext(CPURISCVState *env, uint32_t ext)
>   
>   int riscv_cpu_max_xlen(RISCVCPUClass *mcc)
>   {
> -    return 16 << mcc->misa_mxl_max;
> +    return 16 << mcc->def->misa_mxl_max;
>   }
>   
>   #ifndef CONFIG_USER_ONLY
> @@ -1048,7 +1048,7 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
>           mcc->parent_phases.hold(obj, type);
>       }
>   #ifndef CONFIG_USER_ONLY
> -    env->misa_mxl = mcc->misa_mxl_max;
> +    env->misa_mxl = mcc->def->misa_mxl_max;
>       env->priv = PRV_M;
>       env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
>       if (env->misa_mxl > MXL_RV32) {
> @@ -1450,7 +1450,7 @@ static void riscv_cpu_init(Object *obj)
>       RISCVCPU *cpu = RISCV_CPU(obj);
>       CPURISCVState *env = &cpu->env;
>   
> -    env->misa_mxl = mcc->misa_mxl_max;
> +    env->misa_mxl = mcc->def->misa_mxl_max;
>   
>   #ifndef CONFIG_USER_ONLY
>       qdev_init_gpio_in(DEVICE(obj), riscv_cpu_set_irq,
> @@ -1544,7 +1544,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPUClass *mcc)
>       CPUClass *cc = CPU_CLASS(mcc);
>   
>       /* Validate that MISA_MXL is set properly. */
> -    switch (mcc->misa_mxl_max) {
> +    switch (mcc->def->misa_mxl_max) {
>   #ifdef TARGET_RISCV64
>       case MXL_RV64:
>       case MXL_RV128:
> @@ -3071,12 +3071,24 @@ static void riscv_cpu_common_class_init(ObjectClass *c, const void *data)
>       device_class_set_props(dc, riscv_cpu_properties);
>   }
>   
> +static void riscv_cpu_class_base_init(ObjectClass *c, const void *data)
> +{
> +    RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> +    RISCVCPUClass *pcc = RISCV_CPU_CLASS(object_class_get_parent(c));
> +
> +    if (pcc->def) {
> +        mcc->def = g_memdup2(pcc->def, sizeof(*pcc->def));
> +    } else {
> +        mcc->def = g_new0(RISCVCPUDef, 1);
> +    }
> +}
> +
>   static void riscv_cpu_class_init(ObjectClass *c, const void *data)
>   {
>       RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
>       const RISCVCPUDef *def = data;
>   
> -    mcc->misa_mxl_max = def->misa_mxl_max;
> +    mcc->def->misa_mxl_max = def->misa_mxl_max;
>       riscv_cpu_validate_misa_mxl(mcc);
>   }
>   
> @@ -3227,6 +3239,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
>           .abstract = true,
>           .class_size = sizeof(RISCVCPUClass),
>           .class_init = riscv_cpu_common_class_init,
> +        .class_base_init = riscv_cpu_class_base_init,
>       },
>       {
>           .name = TYPE_RISCV_DYNAMIC_CPU,
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 18e88f416af..1934f919c01 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -62,7 +62,7 @@ int riscv_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>           return 0;
>       }
>   
> -    switch (mcc->misa_mxl_max) {
> +    switch (mcc->def->misa_mxl_max) {
>       case MXL_RV32:
>           return gdb_get_reg32(mem_buf, tmp);
>       case MXL_RV64:
> @@ -82,7 +82,7 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>       int length = 0;
>       target_ulong tmp;
>   
> -    switch (mcc->misa_mxl_max) {
> +    switch (mcc->def->misa_mxl_max) {
>       case MXL_RV32:
>           tmp = (int32_t)ldl_p(mem_buf);
>           length = 4;
> @@ -359,7 +359,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>                                    ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs),
>                                    0);
>       }
> -    switch (mcc->misa_mxl_max) {
> +    switch (mcc->def->misa_mxl_max) {
>       case MXL_RV32:
>           gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
>                                    riscv_gdb_set_virtual,
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 75724b6af4f..41b6f34552a 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1997,22 +1997,19 @@ static void kvm_cpu_accel_register_types(void)
>   }
>   type_init(kvm_cpu_accel_register_types);
>   
> -static void riscv_host_cpu_class_init(ObjectClass *c, const void *data)
> -{
> -    RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> -
> -#if defined(TARGET_RISCV32)
> -    mcc->misa_mxl_max = MXL_RV32;
> -#elif defined(TARGET_RISCV64)
> -    mcc->misa_mxl_max = MXL_RV64;
> -#endif
> -}
> -
>   static const TypeInfo riscv_kvm_cpu_type_infos[] = {
>       {
>           .name = TYPE_RISCV_CPU_HOST,
>           .parent = TYPE_RISCV_CPU,
> -        .class_init = riscv_host_cpu_class_init,
> +#if defined(TARGET_RISCV32)
> +        .class_data = &((const RISCVCPUDef) {
> +            .misa_mxl_max = MXL_RV32,
> +        },
> +#elif defined(TARGET_RISCV64)
> +        .class_data = &((const RISCVCPUDef) {
> +            .misa_mxl_max = MXL_RV64,
> +        },
> +#endif
>       }
>   };

Are we sure this patch compiles? As I said in the v3 this except opening 2 parentheses
and closing just one after RISCVCPUDef:


> -        .class_init = riscv_host_cpu_class_init,
> +#if defined(TARGET_RISCV32)
> +        .class_data = &(( <-----
                        const RISCVCPUDef) {
> +            .misa_mxl_max = MXL_RV32,
> +        },
> +#elif defined(TARGET_RISCV64)
> +        .class_data = &(( <-----
                       const RISCVCPUDef) {
> +            .misa_mxl_max = MXL_RV64,
> +        },
> +#endif
>       }
>   };


In v3 this is the error I was having:


../target/riscv/kvm/kvm-cpu.c:2013:5: error: expected expression before '}' token
   2013 |     }
        |     ^
../target/riscv/kvm/kvm-cpu.c:2011:10: error: value computed is not used [-Werror=unused-value]
   2011 |         },
        |          ^
cc1: all warnings being treated as errors


The error is fixable by closing both parentheses right before the comma, e.g:

> +        }),


Can you mention the pull requests you based this series on top of? I can apply the
dependencies and this series on top of it to see if it builds. Thanks,


Daniel





>   
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index a1f70cc9556..c97e9ce9df1 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -170,7 +170,7 @@ static bool rv128_needed(void *opaque)
>   {
>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(opaque);
>   
> -    return mcc->misa_mxl_max == MXL_RV128;
> +    return mcc->def->misa_mxl_max == MXL_RV128;
>   }
>   
>   static const VMStateDescription vmstate_rv128 = {
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 1192b4e1545..f3ca61103c4 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -592,7 +592,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           return;
>       }
>   
> -    if (mcc->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
> +    if (mcc->def->misa_mxl_max != MXL_RV32 && cpu->cfg.ext_zcf) {
>           error_setg(errp, "Zcf extension is only relevant to RV32");
>           return;
>       }
> @@ -689,7 +689,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           return;
>       }
>   
> -    if (mcc->misa_mxl_max == MXL_RV32 && cpu->cfg.ext_svukte) {
> +    if (mcc->def->misa_mxl_max == MXL_RV32 && cpu->cfg.ext_svukte) {
>           error_setg(errp, "svukte is not supported for RV32");
>           return;
>       }
> @@ -927,7 +927,7 @@ static void cpu_enable_zc_implied_rules(RISCVCPU *cpu)
>           cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmp), true);
>           cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmt), true);
>   
> -        if (riscv_has_ext(env, RVF) && mcc->misa_mxl_max == MXL_RV32) {
> +        if (riscv_has_ext(env, RVF) && mcc->def->misa_mxl_max == MXL_RV32) {
>               cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
>           }
>       }
> @@ -936,7 +936,7 @@ static void cpu_enable_zc_implied_rules(RISCVCPU *cpu)
>       if (riscv_has_ext(env, RVC) && env->priv_ver >= PRIV_VERSION_1_12_0) {
>           cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
>   
> -        if (riscv_has_ext(env, RVF) && mcc->misa_mxl_max == MXL_RV32) {
> +        if (riscv_has_ext(env, RVF) && mcc->def->misa_mxl_max == MXL_RV32) {
>               cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
>           }
>   
> @@ -1062,7 +1062,7 @@ static bool riscv_tcg_cpu_realize(CPUState *cs, Error **errp)
>   #ifndef CONFIG_USER_ONLY
>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>   
> -    if (mcc->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
> +    if (mcc->def->misa_mxl_max >= MXL_RV128 && qemu_tcg_mttcg_enabled()) {
>           /* Missing 128-bit aligned atomics */
>           error_setg(errp,
>                      "128-bit RISC-V currently does not work with Multi "
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index cef61b5b290..26ac7cdcaa1 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -1282,7 +1282,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>       ctx->cfg_vta_all_1s = cpu->cfg.rvv_ta_all_1s;
>       ctx->vstart_eq_zero = FIELD_EX32(tb_flags, TB_FLAGS, VSTART_EQ_ZERO);
>       ctx->vl_eq_vlmax = FIELD_EX32(tb_flags, TB_FLAGS, VL_EQ_VLMAX);
> -    ctx->misa_mxl_max = mcc->misa_mxl_max;
> +    ctx->misa_mxl_max = mcc->def->misa_mxl_max;
>       ctx->xl = FIELD_EX32(tb_flags, TB_FLAGS, XL);
>       ctx->address_xl = FIELD_EX32(tb_flags, TB_FLAGS, AXL);
>       ctx->cs = cs;

Re: [PATCH 08/26] target/riscv: store RISCVCPUDef struct directly in the class

Posted by Richard Henderson 7 months, 3 weeks ago

On 4/28/25 04:50, Daniel Henrique Barboza wrote:
>>   static const TypeInfo riscv_kvm_cpu_type_infos[] = {
>>       {
>>           .name = TYPE_RISCV_CPU_HOST,
>>           .parent = TYPE_RISCV_CPU,
>> -        .class_init = riscv_host_cpu_class_init,
>> +#if defined(TARGET_RISCV32)
>> +        .class_data = &((const RISCVCPUDef) {
>> +            .misa_mxl_max = MXL_RV32,
>> +        },
>> +#elif defined(TARGET_RISCV64)
>> +        .class_data = &((const RISCVCPUDef) {
>> +            .misa_mxl_max = MXL_RV64,
>> +        },
>> +#endif
>>       }
>>   };
> 
> Are we sure this patch compiles? As I said in the v3 this except opening 2 parentheses
> and closing just one after RISCVCPUDef:
> 
> 
>> -        .class_init = riscv_host_cpu_class_init,
>> +#if defined(TARGET_RISCV32)
>> +        .class_data = &(( <-----
>                         const RISCVCPUDef) {

I'll repeat that the parenthesis are completely unnecessary, just

   = &(const RISCVCPUDef){ ... }

The extras should be dropped everywhere.


r~

Re: [PATCH 08/26] target/riscv: store RISCVCPUDef struct directly in the class

Posted by Paolo Bonzini 7 months, 3 weeks ago

On Mon, Apr 28, 2025 at 1:50 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 4/28/25 4:34 AM, Paolo Bonzini wrote:
> > Prepare for adding more fields to RISCVCPUDef and reading them in
> > riscv_cpu_init: instead of storing the misa_mxl_max field in
> > RISCVCPUClass, ensure that there's always a valid RISCVCPUDef struct
> > and go through it.
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >   target/riscv/cpu.h         |  2 +-
> >   hw/riscv/boot.c            |  2 +-
> >   target/riscv/cpu.c         | 23 ++++++++++++++++++-----
> >   target/riscv/gdbstub.c     |  6 +++---
> >   target/riscv/kvm/kvm-cpu.c | 21 +++++++++------------
> >   target/riscv/machine.c     |  2 +-
> >   target/riscv/tcg/tcg-cpu.c | 10 +++++-----
> >   target/riscv/translate.c   |  2 +-
> >   8 files changed, 39 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 62e303f0635..842c9d3f194 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -553,7 +553,7 @@ struct RISCVCPUClass {
> >
> >       DeviceRealize parent_realize;
> >       ResettablePhases parent_phases;
> > -    RISCVMXL misa_mxl_max;  /* max mxl for this cpu */
> > +    RISCVCPUDef *def;
> >   };
> >
> >   static inline int riscv_has_ext(CPURISCVState *env, target_ulong ext)
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 765b9e2b1ab..828a867be39 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -37,7 +37,7 @@
> >   bool riscv_is_32bit(RISCVHartArrayState *harts)
> >   {
> >       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(&harts->harts[0]);
> > -    return mcc->misa_mxl_max == MXL_RV32;
> > +    return mcc->def->misa_mxl_max == MXL_RV32;
> >   }
> >
> >   /*
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index f30cf1b532b..d8c189d596b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -357,7 +357,7 @@ void riscv_cpu_set_misa_ext(CPURISCVState *env, uint32_t ext)
> >
> >   int riscv_cpu_max_xlen(RISCVCPUClass *mcc)
> >   {
> > -    return 16 << mcc->misa_mxl_max;
> > +    return 16 << mcc->def->misa_mxl_max;
> >   }
> >
> >   #ifndef CONFIG_USER_ONLY
> > @@ -1048,7 +1048,7 @@ static void riscv_cpu_reset_hold(Object *obj, ResetType type)
> >           mcc->parent_phases.hold(obj, type);
> >       }
> >   #ifndef CONFIG_USER_ONLY
> > -    env->misa_mxl = mcc->misa_mxl_max;
> > +    env->misa_mxl = mcc->def->misa_mxl_max;
> >       env->priv = PRV_M;
> >       env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> >       if (env->misa_mxl > MXL_RV32) {
> > @@ -1450,7 +1450,7 @@ static void riscv_cpu_init(Object *obj)
> >       RISCVCPU *cpu = RISCV_CPU(obj);
> >       CPURISCVState *env = &cpu->env;
> >
> > -    env->misa_mxl = mcc->misa_mxl_max;
> > +    env->misa_mxl = mcc->def->misa_mxl_max;
> >
> >   #ifndef CONFIG_USER_ONLY
> >       qdev_init_gpio_in(DEVICE(obj), riscv_cpu_set_irq,
> > @@ -1544,7 +1544,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPUClass *mcc)
> >       CPUClass *cc = CPU_CLASS(mcc);
> >
> >       /* Validate that MISA_MXL is set properly. */
> > -    switch (mcc->misa_mxl_max) {
> > +    switch (mcc->def->misa_mxl_max) {
> >   #ifdef TARGET_RISCV64
> >       case MXL_RV64:
> >       case MXL_RV128:
> > @@ -3071,12 +3071,24 @@ static void riscv_cpu_common_class_init(ObjectClass *c, const void *data)
> >       device_class_set_props(dc, riscv_cpu_properties);
> >   }
> >
> > +static void riscv_cpu_class_base_init(ObjectClass *c, const void *data)
> > +{
> > +    RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> > +    RISCVCPUClass *pcc = RISCV_CPU_CLASS(object_class_get_parent(c));
> > +
> > +    if (pcc->def) {
> > +        mcc->def = g_memdup2(pcc->def, sizeof(*pcc->def));
> > +    } else {
> > +        mcc->def = g_new0(RISCVCPUDef, 1);
> > +    }
> > +}
> > +
> >   static void riscv_cpu_class_init(ObjectClass *c, const void *data)
> >   {
> >       RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> >       const RISCVCPUDef *def = data;
> >
> > -    mcc->misa_mxl_max = def->misa_mxl_max;
> > +    mcc->def->misa_mxl_max = def->misa_mxl_max;
> >       riscv_cpu_validate_misa_mxl(mcc);
> >   }
> >
> > @@ -3227,6 +3239,7 @@ static const TypeInfo riscv_cpu_type_infos[] = {
> >           .abstract = true,
> >           .class_size = sizeof(RISCVCPUClass),
> >           .class_init = riscv_cpu_common_class_init,
> > +        .class_base_init = riscv_cpu_class_base_init,
> >       },
> >       {
> >           .name = TYPE_RISCV_DYNAMIC_CPU,
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index 18e88f416af..1934f919c01 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -62,7 +62,7 @@ int riscv_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
> >           return 0;
> >       }
> >
> > -    switch (mcc->misa_mxl_max) {
> > +    switch (mcc->def->misa_mxl_max) {
> >       case MXL_RV32:
> >           return gdb_get_reg32(mem_buf, tmp);
> >       case MXL_RV64:
> > @@ -82,7 +82,7 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
> >       int length = 0;
> >       target_ulong tmp;
> >
> > -    switch (mcc->misa_mxl_max) {
> > +    switch (mcc->def->misa_mxl_max) {
> >       case MXL_RV32:
> >           tmp = (int32_t)ldl_p(mem_buf);
> >           length = 4;
> > @@ -359,7 +359,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
> >                                    ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs),
> >                                    0);
> >       }
> > -    switch (mcc->misa_mxl_max) {
> > +    switch (mcc->def->misa_mxl_max) {
> >       case MXL_RV32:
> >           gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
> >                                    riscv_gdb_set_virtual,
> > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > index 75724b6af4f..41b6f34552a 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -1997,22 +1997,19 @@ static void kvm_cpu_accel_register_types(void)
> >   }
> >   type_init(kvm_cpu_accel_register_types);
> >
> > -static void riscv_host_cpu_class_init(ObjectClass *c, const void *data)
> > -{
> > -    RISCVCPUClass *mcc = RISCV_CPU_CLASS(c);
> > -
> > -#if defined(TARGET_RISCV32)
> > -    mcc->misa_mxl_max = MXL_RV32;
> > -#elif defined(TARGET_RISCV64)
> > -    mcc->misa_mxl_max = MXL_RV64;
> > -#endif
> > -}
> > -
> >   static const TypeInfo riscv_kvm_cpu_type_infos[] = {
> >       {
> >           .name = TYPE_RISCV_CPU_HOST,
> >           .parent = TYPE_RISCV_CPU,
> > -        .class_init = riscv_host_cpu_class_init,
> > +#if defined(TARGET_RISCV32)
> > +        .class_data = &((const RISCVCPUDef) {
> > +            .misa_mxl_max = MXL_RV32,
> > +        },
> > +#elif defined(TARGET_RISCV64)
> > +        .class_data = &((const RISCVCPUDef) {
> > +            .misa_mxl_max = MXL_RV64,
> > +        },
> > +#endif
> >       }
> >   };
>
> Are we sure this patch compiles?

No, you're right; I was not aware that RISC-V KVM is not covered by
CI. I'm sorry.

https://lore.kernel.org/qemu-devel/20250425152843.69638-1-philmd@linaro.org/T/#t

> As I said in the v3 this except opening 2 parentheses
> and closing just one after RISCVCPUDef:
>
> The error is fixable by closing both parentheses right before the comma, e.g:
>
> > +        }),
>
>
> Can you mention the pull requests you based this series on top of? I can apply the
> dependencies and this series on top of it to see if it builds.

It's based on master +
https://lore.kernel.org/qemu-devel/20250425152843.69638-1-philmd@linaro.org/.

Paolo

Re: [PATCH 08/26] target/riscv: store RISCVCPUDef struct directly in the class

Posted by Philippe Mathieu-Daudé 7 months, 3 weeks ago

On 28/4/25 14:17, Paolo Bonzini wrote:
> On Mon, Apr 28, 2025 at 1:50 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 4/28/25 4:34 AM, Paolo Bonzini wrote:
>>> Prepare for adding more fields to RISCVCPUDef and reading them in
>>> riscv_cpu_init: instead of storing the misa_mxl_max field in
>>> RISCVCPUClass, ensure that there's always a valid RISCVCPUDef struct
>>> and go through it.
>>>
>>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>    target/riscv/cpu.h         |  2 +-
>>>    hw/riscv/boot.c            |  2 +-
>>>    target/riscv/cpu.c         | 23 ++++++++++++++++++-----
>>>    target/riscv/gdbstub.c     |  6 +++---
>>>    target/riscv/kvm/kvm-cpu.c | 21 +++++++++------------
>>>    target/riscv/machine.c     |  2 +-
>>>    target/riscv/tcg/tcg-cpu.c | 10 +++++-----
>>>    target/riscv/translate.c   |  2 +-
>>>    8 files changed, 39 insertions(+), 29 deletions(-)


>> Are we sure this patch compiles?
> 
> No, you're right; I was not aware that RISC-V KVM is not covered by
> CI. I'm sorry.

I remember doing it and Daniel reviewed it...:
https://lore.kernel.org/qemu-devel/20230703183145.24779-1-philmd@linaro.org/

I suppose the shame is on me for not insisting getting it merged,
wasting my own time.

Re: [PATCH 08/26] target/riscv: store RISCVCPUDef struct directly in the class

Posted by Paolo Bonzini 7 months, 3 weeks ago

Il lun 28 apr 2025, 17:21 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:

> >> Are we sure this patch compiles?
> >
> > No, you're right; I was not aware that RISC-V KVM is not covered by
> > CI. I'm sorry.
>
> I remember doing it and Daniel reviewed it...:
>
> https://lore.kernel.org/qemu-devel/20230703183145.24779-1-philmd@linaro.org/
>
> I suppose the shame is on me for not insisting getting it merged,
> wasting my own time.
>

This is not a --disable-tcg issue, it's just a RISC-V host issue. Flaky
cross RISC-V toolchains, that caused it to be removed from CI, but I didn't
make 2+2. It should be added back soon as I understand it.

Paolo

>