Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode

[Andrew Jones]

On Thu, Jan 19, 2023 at 02:00:27PM +0100, Alexandre Ghiti wrote:
> Hi Alistair, Andrew,
> 
> On Thu, Jan 19, 2023 at 1:25 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Jan 18, 2023 at 10:19 PM Andrew Jones <ajones@ventanamicro.com> 
> > wrote:
> > >
> > > On Wed, Jan 18, 2023 at 10:28:46AM +1000, Alistair Francis wrote:
> > > > On Wed, Jan 18, 2023 at 2:32 AM Andrew Jones <ajones@ventanamicro.com> 
> > > > wrote:
> > > > >
> > > > > On Fri, Jan 13, 2023 at 11:34:53AM +0100, Alexandre Ghiti wrote:
> > > ...
> > > > > > +
> > > > > > +    /* Get rid of 32-bit/64-bit incompatibility */
> > > > > > +    for (int i = 0; i < 16; ++i) {
> > > > > > +        if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) {
> > > > >
> > > > > If we ever define mode=1 for rv64, then 'sv32=on' will be incorrectly
> > > > > accepted as an alias. I think we should simply not define the sv32
> > > > > property for rv64 nor the rv64-only modes for rv32. So, down in
> > > > > riscv_add_satp_mode_properties() we can add some
> > > > >
> > > > >   #if defined(TARGET_RISCV32)
> > > > >   ...
> > > > >   #elif defined(TARGET_RISCV64)
> > > > >   ...
> > > > >   #endif
> > > >
> > > > Do not add any #if defined(TARGET_RISCV32) to QEMU.
> > > >
> > > > We are aiming for the riscv64-softmmu to be able to emulate 32-bit
> > > > CPUs and compile time macros are the wrong solution here. Instead you
> > > > can get the xlen of the hart and use that.
> > > >
> > >
> > > Does this mean we want to be able to do the following?
> > >
> > >   qemu-system-riscv64 -cpu rv32,sv32=on ...
> >
> > That's the plan
> >
> > >
> > > If so, then can we move the object_property_add() for sv32 to
> > > rv32_base_cpu_init() and the rest to rv64_base_cpu_init()?
> > > Currently, that would be doing the same thing as proposed above,
> > > since those functions are under TARGET_RISCV* defines, but I guess
> > > the object_property_add()'s would then be in more or less the right
> > > places for when the 32-bit emulation support work is started.
> >
> > Sounds like a good idea :)
> 
> What about riscv_any_cpu_init and riscv_host_cpu_init?

riscv_host_cpu_init depends on KVM support, so we actually don't need to
add the properties in this patch at all. That's later work. I'm not real
clear as to what riscv_any_cpu_init is. It looks like a cpu type that
tries to enable all supported standard extensions. Maybe we need a patch
like below first and then add the sv* properties in the same way we will
for the rv*_base_cpu_init functions.

Thanks,
drew


diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index cc75ca76677f..a2987205991e 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -229,19 +229,15 @@ static void set_vext_version(CPURISCVState *env, int 
vext_ver)
     env->vext_ver = vext_ver;
 }
 
-static void riscv_any_cpu_init(Object *obj)
-{
-    CPURISCVState *env = &RISCV_CPU(obj)->env;
-#if defined(TARGET_RISCV32)
-    set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
-#elif defined(TARGET_RISCV64)
-    set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
-#endif
-    set_priv_version(env, PRIV_VERSION_1_12_0);
-    register_cpu_props(DEVICE(obj));
-}
-
 #if defined(TARGET_RISCV64)
+static void rv64_any_cpu_init(Object *obj)
+{
+    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+    set_priv_version(env, PRIV_VERSION_1_12_0);
+    register_cpu_props(DEVICE(obj));
+}
+
 static void rv64_base_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
@@ -285,6 +281,14 @@ static void rv128_base_cpu_init(Object *obj)
     set_priv_version(env, PRIV_VERSION_1_12_0);
 }
 #else
+static void rv32_any_cpu_init(Object *obj)
+{
+    CPURISCVState *env = &RISCV_CPU(obj)->env;
+    set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+    set_priv_version(env, PRIV_VERSION_1_12_0);
+    register_cpu_props(DEVICE(obj));
+}
+
 static void rv32_base_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
@@ -1285,17 +1289,18 @@ static const TypeInfo riscv_cpu_type_infos[] = {
         .class_size = sizeof(RISCVCPUClass),
         .class_init = riscv_cpu_class_init,
     },
-    DEFINE_CPU(TYPE_RISCV_CPU_ANY,              riscv_any_cpu_init),
 #if defined(CONFIG_KVM)
     DEFINE_CPU(TYPE_RISCV_CPU_HOST,             riscv_host_cpu_init),
 #endif
 #if defined(TARGET_RISCV32)
+    DEFINE_CPU(TYPE_RISCV_CPU_ANY,              rv32_any_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_BASE32,           rv32_base_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_IBEX,             rv32_ibex_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E31,       rv32_sifive_e_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E34,       rv32_imafcu_nommu_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U34,       rv32_sifive_u_cpu_init),
 #elif defined(TARGET_RISCV64)
+    DEFINE_CPU(TYPE_RISCV_CPU_ANY,              rv64_any_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_BASE64,           rv64_base_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_E51,       rv64_sifive_e_cpu_init),
     DEFINE_CPU(TYPE_RISCV_CPU_SIFIVE_U54,       rv64_sifive_u_cpu_init),