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

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

[Alexandre Ghiti]

This introduces new properties to allow the user to set the satp mode,
see patch 1 for full syntax.

v5:
- Simplify v4 implementation by leveraging valid_vm_1_10_32/64, as
  suggested by Andrew
- Split the v4 patch into 2 patches as suggested by Andrew
- Lot of other minor corrections, from Andrew
- Set the satp mode N by disabling the satp mode N + 1
- Add a helper to set satp mode from a string, as suggested by Frank

v4:
- Use custom boolean properties instead of OnOffAuto properties, based
  on ARMVQMap, as suggested by Andrew

v3:
- Free sv_name as pointed by Bin
- Replace satp-mode with boolean properties as suggested by Andrew
- Removed RB from Atish as the patch considerably changed

v2:
- Use error_setg + return as suggested by Alistair
- Add RB from Atish
- Fixed checkpatch issues missed in v1
- Replaced Ludovic email address with the rivos one

Alexandre Ghiti (2):
  riscv: Pass Object to register_cpu_props instead of DeviceState
  riscv: Allow user to set the satp mode

 hw/riscv/virt.c    |  19 ++--
 target/riscv/cpu.c | 236 +++++++++++++++++++++++++++++++++++++++++++--
 target/riscv/cpu.h |  19 ++++
 target/riscv/csr.c |  17 +++-
 4 files changed, 270 insertions(+), 21 deletions(-)

-- 
2.37.2

[PATCH v5 1/2] riscv: Pass Object to register_cpu_props instead of DeviceState

[Alexandre Ghiti]

One can extract the DeviceState pointer from the Object pointer, so pass
the Object for future commits to access other fields of Object.

No functional changes intended.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 target/riscv/cpu.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index cc75ca7667..7181b34f86 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -200,7 +200,7 @@ static const char * const riscv_intr_names[] = {
     "reserved"
 };
 
-static void register_cpu_props(DeviceState *dev);
+static void register_cpu_props(Object *obj);
 
 const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
 {
@@ -238,7 +238,7 @@ static void riscv_any_cpu_init(Object *obj)
     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));
+    register_cpu_props(obj);
 }
 
 #if defined(TARGET_RISCV64)
@@ -247,7 +247,7 @@ static void rv64_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV64, 0);
-    register_cpu_props(DEVICE(obj));
+    register_cpu_props(obj);
     /* Set latest version of privileged specification */
     set_priv_version(env, PRIV_VERSION_1_12_0);
 }
@@ -280,7 +280,7 @@ static void rv128_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV128, 0);
-    register_cpu_props(DEVICE(obj));
+    register_cpu_props(obj);
     /* Set latest version of privileged specification */
     set_priv_version(env, PRIV_VERSION_1_12_0);
 }
@@ -290,7 +290,7 @@ static void rv32_base_cpu_init(Object *obj)
     CPURISCVState *env = &RISCV_CPU(obj)->env;
     /* We set this in the realise function */
     set_misa(env, MXL_RV32, 0);
-    register_cpu_props(DEVICE(obj));
+    register_cpu_props(obj);
     /* Set latest version of privileged specification */
     set_priv_version(env, PRIV_VERSION_1_12_0);
 }
@@ -343,7 +343,7 @@ static void riscv_host_cpu_init(Object *obj)
 #elif defined(TARGET_RISCV64)
     set_misa(env, MXL_RV64, 0);
 #endif
-    register_cpu_props(DEVICE(obj));
+    register_cpu_props(obj);
 }
 #endif
 
@@ -1083,9 +1083,10 @@ static Property riscv_cpu_extensions[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void register_cpu_props(DeviceState *dev)
+static void register_cpu_props(Object *obj)
 {
     Property *prop;
+    DeviceState *dev = DEVICE(obj);
 
     for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
         qdev_property_add_static(dev, prop);
-- 
2.37.2

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

[Alexandre Ghiti]

RISC-V specifies multiple sizes for addressable memory and Linux probes for
the machine's support at startup via the satp CSR register (done in
csr.c:validate_vm).

As per the specification, sv64 must support sv57, which in turn must
support sv48...etc. So we can restrict machine support by simply setting the
"highest" supported mode and the bare mode is always supported.

You can set the satp mode using the new properties "sv32", "sv39", "sv48",
"sv57" and "sv64" as follows:
-cpu rv64,sv57=on  # Linux will boot using sv57 scheme
-cpu rv64,sv39=on  # Linux will boot using sv39 scheme
-cpu rv64,sv57=off # Linux will boot using sv48 scheme
-cpu rv64          # Linux will boot using sv57 scheme by default

We take the highest level set by the user:
-cpu rv64,sv48=on,sv57=on # Linux will boot using sv57 scheme

We make sure that invalid configurations are rejected:
-cpu rv64,sv32=on # Can't enable 32-bit satp mode in 64-bit
-cpu rv64,sv39=off,sv48=on # sv39 must be supported if higher modes are
                           # enabled

We accept "redundant" configurations:
-cpu rv64,sv48=on,sv57=off # Linux will boot using sv48 scheme
-cpu rv64,sv32=on,sv32=off # Linux will boot using sv57 scheme (the default)

In addition, we now correctly set the device-tree entry 'mmu-type' using
those new properties.

Co-Developed-by: Ludovic Henry <ludovic@rivosinc.com>
Signed-off-by: Ludovic Henry <ludovic@rivosinc.com>
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 hw/riscv/virt.c    |  19 ++--
 target/riscv/cpu.c | 221 +++++++++++++++++++++++++++++++++++++++++++++
 target/riscv/cpu.h |  19 ++++
 target/riscv/csr.c |  17 +++-
 4 files changed, 262 insertions(+), 14 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 94ff2a1584..48d034a5f7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -228,7 +228,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
     int cpu;
     uint32_t cpu_phandle;
     MachineState *mc = MACHINE(s);
-    char *name, *cpu_name, *core_name, *intc_name;
+    uint8_t satp_mode_max;
+    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
 
     for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
         cpu_phandle = (*phandle)++;
@@ -236,14 +237,14 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
         cpu_name = g_strdup_printf("/cpus/cpu@%d",
             s->soc[socket].hartid_base + cpu);
         qemu_fdt_add_subnode(mc->fdt, cpu_name);
-        if (riscv_feature(&s->soc[socket].harts[cpu].env,
-                          RISCV_FEATURE_MMU)) {
-            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
-                                    (is_32_bit) ? "riscv,sv32" : "riscv,sv48");
-        } else {
-            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
-                                    "riscv,none");
-        }
+
+        satp_mode_max = satp_mode_max_from_map(
+                            s->soc[socket].harts[cpu].cfg.satp_mode.map);
+        sv_name = g_strdup_printf("riscv,%s",
+                                  satp_mode_str(satp_mode_max, is_32_bit));
+        qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type", sv_name);
+        g_free(sv_name);
+
         name = riscv_isa_string(&s->soc[socket].harts[cpu]);
         qemu_fdt_setprop_string(mc->fdt, cpu_name, "riscv,isa", name);
         g_free(name);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7181b34f86..1f0d040a80 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -27,6 +27,7 @@
 #include "time_helper.h"
 #include "exec/exec-all.h"
 #include "qapi/error.h"
+#include "qapi/visitor.h"
 #include "qemu/error-report.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
@@ -229,6 +230,85 @@ static void set_vext_version(CPURISCVState *env, int vext_ver)
     env->vext_ver = vext_ver;
 }
 
+static uint8_t satp_mode_from_str(const char *satp_mode_str)
+{
+    if (!strncmp(satp_mode_str, "sv32", 4)) {
+        return VM_1_10_SV32;
+    }
+
+    if (!strncmp(satp_mode_str, "sv39", 4)) {
+        return VM_1_10_SV39;
+    }
+
+    if (!strncmp(satp_mode_str, "sv48", 4)) {
+        return VM_1_10_SV48;
+    }
+
+    if (!strncmp(satp_mode_str, "sv57", 4)) {
+        return VM_1_10_SV57;
+    }
+
+    if (!strncmp(satp_mode_str, "sv64", 4)) {
+        return VM_1_10_SV64;
+    }
+
+    g_assert_not_reached();
+}
+
+uint8_t satp_mode_max_from_map(uint32_t map)
+{
+    /* map here has at least one bit set, so no problem with clz */
+    return 31 - __builtin_clz(map);
+}
+
+const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
+{
+    if (is_32_bit) {
+        switch (satp_mode) {
+        case VM_1_10_SV32:
+            return "sv32";
+        case VM_1_10_MBARE:
+            return "none";
+        }
+    } else {
+        switch (satp_mode) {
+        case VM_1_10_SV64:
+            return "sv64";
+        case VM_1_10_SV57:
+            return "sv57";
+        case VM_1_10_SV48:
+            return "sv48";
+        case VM_1_10_SV39:
+            return "sv39";
+        case VM_1_10_MBARE:
+            return "none";
+        }
+    }
+
+    g_assert_not_reached();
+}
+
+static void set_satp_mode(RISCVCPU *cpu, const char *satp_mode_str)
+{
+    cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str(satp_mode_str));
+}
+
+static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
+{
+    /*
+     * If an mmu is present, the default satp mode is:
+     * - sv32 for 32-bit
+     * - sv57 for 64-bit
+     * Otherwise, it is mbare.
+     */
+
+    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
+        set_satp_mode(cpu, is_32_bit ? "sv32" : "sv57");
+    } else {
+        set_satp_mode(cpu, "mbare");
+    }
+}
+
 static void riscv_any_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
@@ -619,6 +699,53 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
     }
 }
 
+static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
+{
+    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
+    const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
+
+    /* 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]) {
+            error_setg(errp, "satp_mode %s is not valid",
+                       satp_mode_str(i, !rv32));
+            return;
+        }
+    }
+
+    /*
+     * Make sure the user did not ask for an invalid configuration as per
+     * the specification.
+     */
+    if (!rv32) {
+        uint8_t satp_mode_max;
+
+        satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
+
+        for (int i = satp_mode_max - 1; i >= 0; --i) {
+            if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
+                (cpu->cfg.satp_mode.init & (1 << i)) &&
+                valid_vm[i]) {
+                error_setg(errp, "cannot disable %s satp mode if %s "
+                           "is enabled", satp_mode_str(i, false),
+                           satp_mode_str(satp_mode_max, false));
+                return;
+            }
+        }
+    }
+}
+
+static void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
+{
+    Error *local_err = NULL;
+
+    riscv_cpu_satp_mode_finalize(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+}
+
 static void riscv_cpu_realize(DeviceState *dev, Error **errp)
 {
     CPUState *cs = CPU(dev);
@@ -919,6 +1046,55 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
      }
 #endif
 
+    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
+
+    if (cpu->cfg.satp_mode.map == 0) {
+        /*
+         * If unset by both the user and the cpu, we fallback to the default
+         * satp mode.
+         */
+        if (cpu->cfg.satp_mode.init == 0) {
+            set_satp_mode_default(cpu, rv32);
+        } else {
+            /*
+             * Find the lowest level that was disabled and then enable the
+             * first valid level below which can be found in
+             * valid_vm_1_10_32/64.
+             */
+            const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
+
+            for (int i = 0; i < 16; ++i) {
+                if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
+                    (cpu->cfg.satp_mode.init & (1 << i)) &&
+                    valid_vm[i]) {
+                    for (int j = i - 1; j >= 0; --j) {
+                        if (valid_vm[j]) {
+                            cpu->cfg.satp_mode.map |= (1 << j);
+                            break;
+                        }
+                    }
+                    break;
+                }
+            }
+
+            /*
+             * The user actually init a satp mode but appears to be invalid
+             * (ex: "-cpu rv64,sv32=on,sv32=off"). Fallback to the default
+             * mode.
+             */
+            if (cpu->cfg.satp_mode.map == 0) {
+                set_satp_mode_default(cpu, rv32);
+            }
+        }
+    }
+
+    riscv_cpu_finalize_features(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+
     riscv_cpu_register_gdb_regs_for_features(cs);
 
     qemu_init_vcpu(cs);
@@ -927,6 +1103,49 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     mcc->parent_realize(dev, errp);
 }
 
+static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    RISCVSATPMap *satp_map = opaque;
+    uint8_t satp = satp_mode_from_str(name);
+    bool value;
+
+    value = (satp_map->map & (1 << satp));
+
+    visit_type_bool(v, name, &value, errp);
+}
+
+static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
+                           void *opaque, Error **errp)
+{
+    RISCVSATPMap *satp_map = opaque;
+    uint8_t satp = satp_mode_from_str(name);
+    bool value;
+
+    if (!visit_type_bool(v, name, &value, errp)) {
+        return;
+    }
+
+    satp_map->map = deposit32(satp_map->map, satp, 1, value);
+    satp_map->init |= 1 << satp;
+}
+
+static void riscv_add_satp_mode_properties(Object *obj)
+{
+    RISCVCPU *cpu = RISCV_CPU(obj);
+
+    object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
+                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+    object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
+                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+    object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
+                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+    object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
+                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+    object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
+                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
+}
+
 #ifndef CONFIG_USER_ONLY
 static void riscv_cpu_set_irq(void *opaque, int irq, int level)
 {
@@ -1091,6 +1310,8 @@ static void register_cpu_props(Object *obj)
     for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
         qdev_property_add_static(dev, prop);
     }
+
+    riscv_add_satp_mode_properties(obj);
 }
 
 static Property riscv_cpu_properties[] = {
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f5609b62a2..0ffa1bcfd5 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -27,6 +27,7 @@
 #include "qom/object.h"
 #include "qemu/int128.h"
 #include "cpu_bits.h"
+#include "qapi/qapi-types-common.h"
 
 #define TCG_GUEST_DEFAULT_MO 0
 
@@ -413,6 +414,17 @@ struct RISCVCPUClass {
     ResettablePhases parent_phases;
 };
 
+/*
+ * map is a 16-bit bitmap: the most significant set bit in map is the maximum
+ * satp mode that is supported.
+ *
+ * init is a 16-bit bitmap used to make sure the user selected a correct
+ * configuration as per the specification.
+ */
+typedef struct {
+    uint16_t map, init;
+} RISCVSATPMap;
+
 struct RISCVCPUConfig {
     bool ext_i;
     bool ext_e;
@@ -488,6 +500,8 @@ struct RISCVCPUConfig {
     bool debug;
 
     bool short_isa_string;
+
+    RISCVSATPMap satp_mode;
 };
 
 typedef struct RISCVCPUConfig RISCVCPUConfig;
@@ -794,9 +808,14 @@ enum riscv_pmu_event_idx {
 /* CSR function table */
 extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
 
+extern const char valid_vm_1_10_32[], valid_vm_1_10_64[];
+
 void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops);
 void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
 
 void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
 
+uint8_t satp_mode_max_from_map(uint32_t map);
+const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
+
 #endif /* RISCV_CPU_H */
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0db2c233e5..6e27299761 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1117,12 +1117,12 @@ static const target_ulong hip_writable_mask = MIP_VSSIP;
 static const target_ulong hvip_writable_mask = MIP_VSSIP | MIP_VSTIP | MIP_VSEIP;
 static const target_ulong vsip_writable_mask = MIP_VSSIP;
 
-static const char valid_vm_1_10_32[16] = {
+const char valid_vm_1_10_32[16] = {
     [VM_1_10_MBARE] = 1,
     [VM_1_10_SV32] = 1
 };
 
-static const char valid_vm_1_10_64[16] = {
+const char valid_vm_1_10_64[16] = {
     [VM_1_10_MBARE] = 1,
     [VM_1_10_SV39] = 1,
     [VM_1_10_SV48] = 1,
@@ -1211,10 +1211,17 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
 
 static int validate_vm(CPURISCVState *env, target_ulong vm)
 {
-    if (riscv_cpu_mxl(env) == MXL_RV32) {
-        return valid_vm_1_10_32[vm & 0xf];
+    uint8_t satp_mode_max;
+    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
+    bool is_32_bit = riscv_cpu_mxl(env) == MXL_RV32;
+
+    vm &= 0xf;
+    satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
+
+    if (is_32_bit) {
+        return valid_vm_1_10_32[vm] && (vm <= satp_mode_max);
     } else {
-        return valid_vm_1_10_64[vm & 0xf];
+        return valid_vm_1_10_64[vm] && (vm <= satp_mode_max);
     }
 }
 
-- 
2.37.2

Re: [PATCH v5 1/2] riscv: Pass Object to register_cpu_props instead of DeviceState

[Alistair Francis]

On Fri, Jan 13, 2023 at 8:36 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> One can extract the DeviceState pointer from the Object pointer, so pass
> the Object for future commits to access other fields of Object.
>
> No functional changes intended.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>

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

Alistair

> ---
>  target/riscv/cpu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cc75ca7667..7181b34f86 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -200,7 +200,7 @@ static const char * const riscv_intr_names[] = {
>      "reserved"
>  };
>
> -static void register_cpu_props(DeviceState *dev);
> +static void register_cpu_props(Object *obj);
>
>  const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
>  {
> @@ -238,7 +238,7 @@ static void riscv_any_cpu_init(Object *obj)
>      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));
> +    register_cpu_props(obj);
>  }
>
>  #if defined(TARGET_RISCV64)
> @@ -247,7 +247,7 @@ static void rv64_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV64, 0);
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
> @@ -280,7 +280,7 @@ static void rv128_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV128, 0);
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
> @@ -290,7 +290,7 @@ static void rv32_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV32, 0);
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
> @@ -343,7 +343,7 @@ static void riscv_host_cpu_init(Object *obj)
>  #elif defined(TARGET_RISCV64)
>      set_misa(env, MXL_RV64, 0);
>  #endif
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>  }
>  #endif
>
> @@ -1083,9 +1083,10 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> -static void register_cpu_props(DeviceState *dev)
> +static void register_cpu_props(Object *obj)
>  {
>      Property *prop;
> +    DeviceState *dev = DEVICE(obj);
>
>      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
>          qdev_property_add_static(dev, prop);
> --
> 2.37.2
>
>

Re: [PATCH v5 1/2] riscv: Pass Object to register_cpu_props instead of DeviceState

[Frank Chang]

[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]

Reviewed-by: Frank Chang <frank.chang@sifive.com>

On Fri, Jan 13, 2023 at 6:35 PM Alexandre Ghiti <alexghiti@rivosinc.com>
wrote:

> One can extract the DeviceState pointer from the Object pointer, so pass
> the Object for future commits to access other fields of Object.
>
> No functional changes intended.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  target/riscv/cpu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cc75ca7667..7181b34f86 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -200,7 +200,7 @@ static const char * const riscv_intr_names[] = {
>      "reserved"
>  };
>
> -static void register_cpu_props(DeviceState *dev);
> +static void register_cpu_props(Object *obj);
>
>  const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
>  {
> @@ -238,7 +238,7 @@ static void riscv_any_cpu_init(Object *obj)
>      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));
> +    register_cpu_props(obj);
>  }
>
>  #if defined(TARGET_RISCV64)
> @@ -247,7 +247,7 @@ static void rv64_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV64, 0);
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
> @@ -280,7 +280,7 @@ static void rv128_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV128, 0);
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
> @@ -290,7 +290,7 @@ static void rv32_base_cpu_init(Object *obj)
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV32, 0);
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>  }
> @@ -343,7 +343,7 @@ static void riscv_host_cpu_init(Object *obj)
>  #elif defined(TARGET_RISCV64)
>      set_misa(env, MXL_RV64, 0);
>  #endif
> -    register_cpu_props(DEVICE(obj));
> +    register_cpu_props(obj);
>  }
>  #endif
>
> @@ -1083,9 +1083,10 @@ static Property riscv_cpu_extensions[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> -static void register_cpu_props(DeviceState *dev)
> +static void register_cpu_props(Object *obj)
>  {
>      Property *prop;
> +    DeviceState *dev = DEVICE(obj);
>
>      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
>          qdev_property_add_static(dev, prop);
> --
> 2.37.2
>
>

[-- Attachment #2: Type: text/html, Size: 3827 bytes --]

Re: [PATCH v5 1/2] riscv: Pass Object to register_cpu_props instead of DeviceState

[Andrew Jones]

On Fri, Jan 13, 2023 at 11:34:52AM +0100, Alexandre Ghiti wrote:
> One can extract the DeviceState pointer from the Object pointer, so pass
> the Object for future commits to access other fields of Object.
> 
> No functional changes intended.
> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  target/riscv/cpu.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

[Andrew Jones]

On Fri, Jan 13, 2023 at 11:34:53AM +0100, Alexandre Ghiti wrote:
> RISC-V specifies multiple sizes for addressable memory and Linux probes for
> the machine's support at startup via the satp CSR register (done in
> csr.c:validate_vm).
> 
> As per the specification, sv64 must support sv57, which in turn must
> support sv48...etc. So we can restrict machine support by simply setting the
> "highest" supported mode and the bare mode is always supported.
> 
> You can set the satp mode using the new properties "sv32", "sv39", "sv48",
> "sv57" and "sv64" as follows:
> -cpu rv64,sv57=on  # Linux will boot using sv57 scheme
> -cpu rv64,sv39=on  # Linux will boot using sv39 scheme
> -cpu rv64,sv57=off # Linux will boot using sv48 scheme
> -cpu rv64          # Linux will boot using sv57 scheme by default
> 
> We take the highest level set by the user:
> -cpu rv64,sv48=on,sv57=on # Linux will boot using sv57 scheme
> 
> We make sure that invalid configurations are rejected:
> -cpu rv64,sv32=on # Can't enable 32-bit satp mode in 64-bit
> -cpu rv64,sv39=off,sv48=on # sv39 must be supported if higher modes are
>                            # enabled
> 
> We accept "redundant" configurations:
> -cpu rv64,sv48=on,sv57=off # Linux will boot using sv48 scheme
> -cpu rv64,sv32=on,sv32=off # Linux will boot using sv57 scheme (the default)
> 
> In addition, we now correctly set the device-tree entry 'mmu-type' using
> those new properties.
> 
> Co-Developed-by: Ludovic Henry <ludovic@rivosinc.com>
> Signed-off-by: Ludovic Henry <ludovic@rivosinc.com>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  hw/riscv/virt.c    |  19 ++--
>  target/riscv/cpu.c | 221 +++++++++++++++++++++++++++++++++++++++++++++
>  target/riscv/cpu.h |  19 ++++
>  target/riscv/csr.c |  17 +++-
>  4 files changed, 262 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 94ff2a1584..48d034a5f7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -228,7 +228,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>      int cpu;
>      uint32_t cpu_phandle;
>      MachineState *mc = MACHINE(s);
> -    char *name, *cpu_name, *core_name, *intc_name;
> +    uint8_t satp_mode_max;
> +    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
>  
>      for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
>          cpu_phandle = (*phandle)++;
> @@ -236,14 +237,14 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
>          cpu_name = g_strdup_printf("/cpus/cpu@%d",
>              s->soc[socket].hartid_base + cpu);
>          qemu_fdt_add_subnode(mc->fdt, cpu_name);
> -        if (riscv_feature(&s->soc[socket].harts[cpu].env,
> -                          RISCV_FEATURE_MMU)) {
> -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> -                                    (is_32_bit) ? "riscv,sv32" : "riscv,sv48");

I just noticed that for the virt machine type, when the user doesn't
provide a satp mode cpu property on the command line, and hence gets
the default mode, they'll be silently changed from sv48 to sv57. That
default change should be a separate patch which comes after this one.
BTW, why sv57 and not sv48 or sv64?

> -        } else {
> -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> -                                    "riscv,none");
> -        }
> +
> +        satp_mode_max = satp_mode_max_from_map(
> +                            s->soc[socket].harts[cpu].cfg.satp_mode.map);
> +        sv_name = g_strdup_printf("riscv,%s",
> +                                  satp_mode_str(satp_mode_max, is_32_bit));
> +        qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type", sv_name);
> +        g_free(sv_name);
> +
>          name = riscv_isa_string(&s->soc[socket].harts[cpu]);
>          qemu_fdt_setprop_string(mc->fdt, cpu_name, "riscv,isa", name);
>          g_free(name);
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7181b34f86..1f0d040a80 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -27,6 +27,7 @@
>  #include "time_helper.h"
>  #include "exec/exec-all.h"
>  #include "qapi/error.h"
> +#include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
> @@ -229,6 +230,85 @@ static void set_vext_version(CPURISCVState *env, int vext_ver)
>      env->vext_ver = vext_ver;
>  }
>  
> +static uint8_t satp_mode_from_str(const char *satp_mode_str)
> +{
> +    if (!strncmp(satp_mode_str, "sv32", 4)) {
> +        return VM_1_10_SV32;
> +    }
> +
> +    if (!strncmp(satp_mode_str, "sv39", 4)) {
> +        return VM_1_10_SV39;
> +    }
> +
> +    if (!strncmp(satp_mode_str, "sv48", 4)) {
> +        return VM_1_10_SV48;
> +    }
> +
> +    if (!strncmp(satp_mode_str, "sv57", 4)) {
> +        return VM_1_10_SV57;
> +    }
> +
> +    if (!strncmp(satp_mode_str, "sv64", 4)) {
> +        return VM_1_10_SV64;
> +    }
> +
> +    g_assert_not_reached();
> +}
> +
> +uint8_t satp_mode_max_from_map(uint32_t map)
> +{
> +    /* map here has at least one bit set, so no problem with clz */
> +    return 31 - __builtin_clz(map);
> +}
> +
> +const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> +{
> +    if (is_32_bit) {
> +        switch (satp_mode) {
> +        case VM_1_10_SV32:
> +            return "sv32";
> +        case VM_1_10_MBARE:
> +            return "none";
> +        }
> +    } else {
> +        switch (satp_mode) {
> +        case VM_1_10_SV64:
> +            return "sv64";
> +        case VM_1_10_SV57:
> +            return "sv57";
> +        case VM_1_10_SV48:
> +            return "sv48";
> +        case VM_1_10_SV39:
> +            return "sv39";
> +        case VM_1_10_MBARE:
> +            return "none";
> +        }
> +    }
> +
> +    g_assert_not_reached();
> +}
> +
> +static void set_satp_mode(RISCVCPU *cpu, const char *satp_mode_str)
> +{
> +    cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str(satp_mode_str));
> +}
> +
> +static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +{
> +    /*
> +     * If an mmu is present, the default satp mode is:
> +     * - sv32 for 32-bit
> +     * - sv57 for 64-bit
> +     * Otherwise, it is mbare.
> +     */

I'd drop the above comment since it only repeats what the code says.

> +
> +    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> +        set_satp_mode(cpu, is_32_bit ? "sv32" : "sv57");
> +    } else {
> +        set_satp_mode(cpu, "mbare");

nit: Could probably integrate set_satp_mode() into this function since
this function is the only place it's used.

> +    }
> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> @@ -619,6 +699,53 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>      }
>  }
>  
> +static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> +{
> +    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> +    const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;

Not a problem of this patch, but valid_vm_1_10_32/64 has a strange type.
It's used like a boolean, so should be bool. Since you're touching the
arrays and validate_vm() it'd be nice to change the array type and
the return value of validate_vm() with a separate patch first.

> +
> +    /* 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

and then drop the check here.

> +            error_setg(errp, "satp_mode %s is not valid",
> +                       satp_mode_str(i, !rv32));
> +            return;
> +        }
> +    }
> +
> +    /*
> +     * Make sure the user did not ask for an invalid configuration as per
> +     * the specification.
> +     */
> +    if (!rv32) {
> +        uint8_t satp_mode_max;
> +
> +        satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> +
> +        for (int i = satp_mode_max - 1; i >= 0; --i) {
> +            if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> +                (cpu->cfg.satp_mode.init & (1 << i)) &&
> +                valid_vm[i]) {
> +                error_setg(errp, "cannot disable %s satp mode if %s "
> +                           "is enabled", satp_mode_str(i, false),
> +                           satp_mode_str(satp_mode_max, false));
> +                return;
> +            }
> +        }
> +    }
> +}
> +
> +static void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> +{
> +    Error *local_err = NULL;
> +
> +    riscv_cpu_satp_mode_finalize(cpu, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +}
> +
>  static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>  {
>      CPUState *cs = CPU(dev);
> @@ -919,6 +1046,55 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>       }
>  #endif
>  
> +    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> +
> +    if (cpu->cfg.satp_mode.map == 0) {
> +        /*
> +         * If unset by both the user and the cpu, we fallback to the default
> +         * satp mode.
> +         */
> +        if (cpu->cfg.satp_mode.init == 0) {
> +            set_satp_mode_default(cpu, rv32);
> +        } else {
> +            /*
> +             * Find the lowest level that was disabled and then enable the
> +             * first valid level below which can be found in
> +             * valid_vm_1_10_32/64.
> +             */
> +            const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> +            for (int i = 0; i < 16; ++i) {

'init' will never have bit0 (mbare) set, so we can start at i=1, which is
good, because the condition below assumes it can index an array at i-1.

> +                if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> +                    (cpu->cfg.satp_mode.init & (1 << i)) &&
> +                    valid_vm[i]) {
> +                    for (int j = i - 1; j >= 0; --j) {
> +                        if (valid_vm[j]) {
> +                            cpu->cfg.satp_mode.map |= (1 << j);
> +                            break;
> +                        }
> +                    }
> +                    break;
> +                }
> +            }
> +
> +            /*
> +             * The user actually init a satp mode but appears to be invalid
> +             * (ex: "-cpu rv64,sv32=on,sv32=off"). Fallback to the default

This example, where sv32 is used with rv64, won't be possible if we don't
give rv64 the sv32 property.

> +             * mode.
> +             */
> +            if (cpu->cfg.satp_mode.map == 0) {
> +                set_satp_mode_default(cpu, rv32);

If the user does rv64,sv39=on,sv39=off, then I think we should be creating
an mbare machine, rather than using the default.

> +            }
> +        }
> +    }

Why isn't all this 'if (cpu->cfg.satp_mode.map == 0)' block above at the
top of riscv_cpu_satp_mode_finalize() instead of here?

> +
> +    riscv_cpu_finalize_features(cpu, &local_err);
> +    if (local_err != NULL) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +

extra blank line

>      riscv_cpu_register_gdb_regs_for_features(cs);
>  
>      qemu_init_vcpu(cs);
> @@ -927,6 +1103,49 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>      mcc->parent_realize(dev, errp);
>  }
>  
> +static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
> +{
> +    RISCVSATPMap *satp_map = opaque;
> +    uint8_t satp = satp_mode_from_str(name);
> +    bool value;
> +
> +    value = (satp_map->map & (1 << satp));
> +
> +    visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
> +                           void *opaque, Error **errp)
> +{
> +    RISCVSATPMap *satp_map = opaque;
> +    uint8_t satp = satp_mode_from_str(name);
> +    bool value;
> +
> +    if (!visit_type_bool(v, name, &value, errp)) {
> +        return;
> +    }
> +
> +    satp_map->map = deposit32(satp_map->map, satp, 1, value);
> +    satp_map->init |= 1 << satp;
> +}
> +
> +static void riscv_add_satp_mode_properties(Object *obj)
> +{
> +    RISCVCPU *cpu = RISCV_CPU(obj);

As mentioned above, I think we want to do

> +
  #if defined(TARGET_RISCV32)
> +    object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
> +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
  #elif defined(TARGET_RISCV64)
> +    object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
> +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> +    object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
> +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> +    object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
> +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> +    object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
> +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
  #endif
> +}
> +
>  #ifndef CONFIG_USER_ONLY
>  static void riscv_cpu_set_irq(void *opaque, int irq, int level)
>  {
> @@ -1091,6 +1310,8 @@ static void register_cpu_props(Object *obj)
>      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
>          qdev_property_add_static(dev, prop);
>      }
> +
> +    riscv_add_satp_mode_properties(obj);
>  }
>  
>  static Property riscv_cpu_properties[] = {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f5609b62a2..0ffa1bcfd5 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -27,6 +27,7 @@
>  #include "qom/object.h"
>  #include "qemu/int128.h"
>  #include "cpu_bits.h"
> +#include "qapi/qapi-types-common.h"
>  
>  #define TCG_GUEST_DEFAULT_MO 0
>  
> @@ -413,6 +414,17 @@ struct RISCVCPUClass {
>      ResettablePhases parent_phases;
>  };
>  
> +/*
> + * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> + * satp mode that is supported.
> + *
> + * init is a 16-bit bitmap used to make sure the user selected a correct
> + * configuration as per the specification.
> + */
> +typedef struct {
> +    uint16_t map, init;
> +} RISCVSATPMap;
> +
>  struct RISCVCPUConfig {
>      bool ext_i;
>      bool ext_e;
> @@ -488,6 +500,8 @@ struct RISCVCPUConfig {
>      bool debug;
>  
>      bool short_isa_string;
> +
> +    RISCVSATPMap satp_mode;
>  };
>  
>  typedef struct RISCVCPUConfig RISCVCPUConfig;
> @@ -794,9 +808,14 @@ enum riscv_pmu_event_idx {
>  /* CSR function table */
>  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
>  
> +extern const char valid_vm_1_10_32[], valid_vm_1_10_64[];
> +
>  void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops);
>  void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
>  
>  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
>  
> +uint8_t satp_mode_max_from_map(uint32_t map);
> +const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
> +
>  #endif /* RISCV_CPU_H */
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0db2c233e5..6e27299761 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1117,12 +1117,12 @@ static const target_ulong hip_writable_mask = MIP_VSSIP;
>  static const target_ulong hvip_writable_mask = MIP_VSSIP | MIP_VSTIP | MIP_VSEIP;
>  static const target_ulong vsip_writable_mask = MIP_VSSIP;
>  
> -static const char valid_vm_1_10_32[16] = {
> +const char valid_vm_1_10_32[16] = {
>      [VM_1_10_MBARE] = 1,
>      [VM_1_10_SV32] = 1
>  };
>  
> -static const char valid_vm_1_10_64[16] = {
> +const char valid_vm_1_10_64[16] = {
>      [VM_1_10_MBARE] = 1,
>      [VM_1_10_SV39] = 1,
>      [VM_1_10_SV48] = 1,
> @@ -1211,10 +1211,17 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
>  
>  static int validate_vm(CPURISCVState *env, target_ulong vm)
>  {
> -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        return valid_vm_1_10_32[vm & 0xf];
> +    uint8_t satp_mode_max;
> +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
> +    bool is_32_bit = riscv_cpu_mxl(env) == MXL_RV32;
> +
> +    vm &= 0xf;
> +    satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> +
> +    if (is_32_bit) {
> +        return valid_vm_1_10_32[vm] && (vm <= satp_mode_max);
>      } else {
> -        return valid_vm_1_10_64[vm & 0xf];
> +        return valid_vm_1_10_64[vm] && (vm <= satp_mode_max);
>      }
>  }
>  
> -- 
> 2.37.2
>

Thanks,
drew

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

[Alistair Francis]

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:
> > RISC-V specifies multiple sizes for addressable memory and Linux probes for
> > the machine's support at startup via the satp CSR register (done in
> > csr.c:validate_vm).
> >
> > As per the specification, sv64 must support sv57, which in turn must
> > support sv48...etc. So we can restrict machine support by simply setting the
> > "highest" supported mode and the bare mode is always supported.
> >
> > You can set the satp mode using the new properties "sv32", "sv39", "sv48",
> > "sv57" and "sv64" as follows:
> > -cpu rv64,sv57=on  # Linux will boot using sv57 scheme
> > -cpu rv64,sv39=on  # Linux will boot using sv39 scheme
> > -cpu rv64,sv57=off # Linux will boot using sv48 scheme
> > -cpu rv64          # Linux will boot using sv57 scheme by default
> >
> > We take the highest level set by the user:
> > -cpu rv64,sv48=on,sv57=on # Linux will boot using sv57 scheme
> >
> > We make sure that invalid configurations are rejected:
> > -cpu rv64,sv32=on # Can't enable 32-bit satp mode in 64-bit
> > -cpu rv64,sv39=off,sv48=on # sv39 must be supported if higher modes are
> >                            # enabled
> >
> > We accept "redundant" configurations:
> > -cpu rv64,sv48=on,sv57=off # Linux will boot using sv48 scheme
> > -cpu rv64,sv32=on,sv32=off # Linux will boot using sv57 scheme (the default)
> >
> > In addition, we now correctly set the device-tree entry 'mmu-type' using
> > those new properties.
> >
> > Co-Developed-by: Ludovic Henry <ludovic@rivosinc.com>
> > Signed-off-by: Ludovic Henry <ludovic@rivosinc.com>
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  hw/riscv/virt.c    |  19 ++--
> >  target/riscv/cpu.c | 221 +++++++++++++++++++++++++++++++++++++++++++++
> >  target/riscv/cpu.h |  19 ++++
> >  target/riscv/csr.c |  17 +++-
> >  4 files changed, 262 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 94ff2a1584..48d034a5f7 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -228,7 +228,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> >      int cpu;
> >      uint32_t cpu_phandle;
> >      MachineState *mc = MACHINE(s);
> > -    char *name, *cpu_name, *core_name, *intc_name;
> > +    uint8_t satp_mode_max;
> > +    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
> >
> >      for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
> >          cpu_phandle = (*phandle)++;
> > @@ -236,14 +237,14 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> >          cpu_name = g_strdup_printf("/cpus/cpu@%d",
> >              s->soc[socket].hartid_base + cpu);
> >          qemu_fdt_add_subnode(mc->fdt, cpu_name);
> > -        if (riscv_feature(&s->soc[socket].harts[cpu].env,
> > -                          RISCV_FEATURE_MMU)) {
> > -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> > -                                    (is_32_bit) ? "riscv,sv32" : "riscv,sv48");
>
> I just noticed that for the virt machine type, when the user doesn't
> provide a satp mode cpu property on the command line, and hence gets
> the default mode, they'll be silently changed from sv48 to sv57. That
> default change should be a separate patch which comes after this one.
> BTW, why sv57 and not sv48 or sv64?
>
> > -        } else {
> > -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> > -                                    "riscv,none");
> > -        }
> > +
> > +        satp_mode_max = satp_mode_max_from_map(
> > +                            s->soc[socket].harts[cpu].cfg.satp_mode.map);
> > +        sv_name = g_strdup_printf("riscv,%s",
> > +                                  satp_mode_str(satp_mode_max, is_32_bit));
> > +        qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type", sv_name);
> > +        g_free(sv_name);
> > +
> >          name = riscv_isa_string(&s->soc[socket].harts[cpu]);
> >          qemu_fdt_setprop_string(mc->fdt, cpu_name, "riscv,isa", name);
> >          g_free(name);
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7181b34f86..1f0d040a80 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -27,6 +27,7 @@
> >  #include "time_helper.h"
> >  #include "exec/exec-all.h"
> >  #include "qapi/error.h"
> > +#include "qapi/visitor.h"
> >  #include "qemu/error-report.h"
> >  #include "hw/qdev-properties.h"
> >  #include "migration/vmstate.h"
> > @@ -229,6 +230,85 @@ static void set_vext_version(CPURISCVState *env, int vext_ver)
> >      env->vext_ver = vext_ver;
> >  }
> >
> > +static uint8_t satp_mode_from_str(const char *satp_mode_str)
> > +{
> > +    if (!strncmp(satp_mode_str, "sv32", 4)) {
> > +        return VM_1_10_SV32;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv39", 4)) {
> > +        return VM_1_10_SV39;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv48", 4)) {
> > +        return VM_1_10_SV48;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv57", 4)) {
> > +        return VM_1_10_SV57;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv64", 4)) {
> > +        return VM_1_10_SV64;
> > +    }
> > +
> > +    g_assert_not_reached();
> > +}
> > +
> > +uint8_t satp_mode_max_from_map(uint32_t map)
> > +{
> > +    /* map here has at least one bit set, so no problem with clz */
> > +    return 31 - __builtin_clz(map);
> > +}
> > +
> > +const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> > +{
> > +    if (is_32_bit) {
> > +        switch (satp_mode) {
> > +        case VM_1_10_SV32:
> > +            return "sv32";
> > +        case VM_1_10_MBARE:
> > +            return "none";
> > +        }
> > +    } else {
> > +        switch (satp_mode) {
> > +        case VM_1_10_SV64:
> > +            return "sv64";
> > +        case VM_1_10_SV57:
> > +            return "sv57";
> > +        case VM_1_10_SV48:
> > +            return "sv48";
> > +        case VM_1_10_SV39:
> > +            return "sv39";
> > +        case VM_1_10_MBARE:
> > +            return "none";
> > +        }
> > +    }
> > +
> > +    g_assert_not_reached();
> > +}
> > +
> > +static void set_satp_mode(RISCVCPU *cpu, const char *satp_mode_str)
> > +{
> > +    cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str(satp_mode_str));
> > +}
> > +
> > +static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +{
> > +    /*
> > +     * If an mmu is present, the default satp mode is:
> > +     * - sv32 for 32-bit
> > +     * - sv57 for 64-bit
> > +     * Otherwise, it is mbare.
> > +     */
>
> I'd drop the above comment since it only repeats what the code says.
>
> > +
> > +    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > +        set_satp_mode(cpu, is_32_bit ? "sv32" : "sv57");
> > +    } else {
> > +        set_satp_mode(cpu, "mbare");
>
> nit: Could probably integrate set_satp_mode() into this function since
> this function is the only place it's used.
>
> > +    }
> > +}
> > +
> >  static void riscv_any_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > @@ -619,6 +699,53 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> >      }
> >  }
> >
> > +static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> > +{
> > +    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +    const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
>
> Not a problem of this patch, but valid_vm_1_10_32/64 has a strange type.
> It's used like a boolean, so should be bool. Since you're touching the
> arrays and validate_vm() it'd be nice to change the array type and
> the return value of validate_vm() with a separate patch first.
>
> > +
> > +    /* 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.

Alistair

>
> and then drop the check here.
>
> > +            error_setg(errp, "satp_mode %s is not valid",
> > +                       satp_mode_str(i, !rv32));
> > +            return;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * Make sure the user did not ask for an invalid configuration as per
> > +     * the specification.
> > +     */
> > +    if (!rv32) {
> > +        uint8_t satp_mode_max;
> > +
> > +        satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +
> > +        for (int i = satp_mode_max - 1; i >= 0; --i) {
> > +            if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> > +                (cpu->cfg.satp_mode.init & (1 << i)) &&
> > +                valid_vm[i]) {
> > +                error_setg(errp, "cannot disable %s satp mode if %s "
> > +                           "is enabled", satp_mode_str(i, false),
> > +                           satp_mode_str(satp_mode_max, false));
> > +                return;
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +static void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    riscv_cpu_satp_mode_finalize(cpu, &local_err);
> > +    if (local_err != NULL) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +}
> > +
> >  static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >  {
> >      CPUState *cs = CPU(dev);
> > @@ -919,6 +1046,55 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >       }
> >  #endif
> >
> > +    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +
> > +    if (cpu->cfg.satp_mode.map == 0) {
> > +        /*
> > +         * If unset by both the user and the cpu, we fallback to the default
> > +         * satp mode.
> > +         */
> > +        if (cpu->cfg.satp_mode.init == 0) {
> > +            set_satp_mode_default(cpu, rv32);
> > +        } else {
> > +            /*
> > +             * Find the lowest level that was disabled and then enable the
> > +             * first valid level below which can be found in
> > +             * valid_vm_1_10_32/64.
> > +             */
> > +            const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +
> > +            for (int i = 0; i < 16; ++i) {
>
> 'init' will never have bit0 (mbare) set, so we can start at i=1, which is
> good, because the condition below assumes it can index an array at i-1.
>
> > +                if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> > +                    (cpu->cfg.satp_mode.init & (1 << i)) &&
> > +                    valid_vm[i]) {
> > +                    for (int j = i - 1; j >= 0; --j) {
> > +                        if (valid_vm[j]) {
> > +                            cpu->cfg.satp_mode.map |= (1 << j);
> > +                            break;
> > +                        }
> > +                    }
> > +                    break;
> > +                }
> > +            }
> > +
> > +            /*
> > +             * The user actually init a satp mode but appears to be invalid
> > +             * (ex: "-cpu rv64,sv32=on,sv32=off"). Fallback to the default
>
> This example, where sv32 is used with rv64, won't be possible if we don't
> give rv64 the sv32 property.
>
> > +             * mode.
> > +             */
> > +            if (cpu->cfg.satp_mode.map == 0) {
> > +                set_satp_mode_default(cpu, rv32);
>
> If the user does rv64,sv39=on,sv39=off, then I think we should be creating
> an mbare machine, rather than using the default.
>
> > +            }
> > +        }
> > +    }
>
> Why isn't all this 'if (cpu->cfg.satp_mode.map == 0)' block above at the
> top of riscv_cpu_satp_mode_finalize() instead of here?
>
> > +
> > +    riscv_cpu_finalize_features(cpu, &local_err);
> > +    if (local_err != NULL) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +
>
> extra blank line
>
> >      riscv_cpu_register_gdb_regs_for_features(cs);
> >
> >      qemu_init_vcpu(cs);
> > @@ -927,6 +1103,49 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >      mcc->parent_realize(dev, errp);
> >  }
> >
> > +static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
> > +                           void *opaque, Error **errp)
> > +{
> > +    RISCVSATPMap *satp_map = opaque;
> > +    uint8_t satp = satp_mode_from_str(name);
> > +    bool value;
> > +
> > +    value = (satp_map->map & (1 << satp));
> > +
> > +    visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
> > +                           void *opaque, Error **errp)
> > +{
> > +    RISCVSATPMap *satp_map = opaque;
> > +    uint8_t satp = satp_mode_from_str(name);
> > +    bool value;
> > +
> > +    if (!visit_type_bool(v, name, &value, errp)) {
> > +        return;
> > +    }
> > +
> > +    satp_map->map = deposit32(satp_map->map, satp, 1, value);
> > +    satp_map->init |= 1 << satp;
> > +}
> > +
> > +static void riscv_add_satp_mode_properties(Object *obj)
> > +{
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
>
> As mentioned above, I think we want to do
>
> > +
>   #if defined(TARGET_RISCV32)
> > +    object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
>   #elif defined(TARGET_RISCV64)
> > +    object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> > +    object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> > +    object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> > +    object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
>   #endif
> > +}
> > +
> >  #ifndef CONFIG_USER_ONLY
> >  static void riscv_cpu_set_irq(void *opaque, int irq, int level)
> >  {
> > @@ -1091,6 +1310,8 @@ static void register_cpu_props(Object *obj)
> >      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
> >          qdev_property_add_static(dev, prop);
> >      }
> > +
> > +    riscv_add_satp_mode_properties(obj);
> >  }
> >
> >  static Property riscv_cpu_properties[] = {
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index f5609b62a2..0ffa1bcfd5 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -27,6 +27,7 @@
> >  #include "qom/object.h"
> >  #include "qemu/int128.h"
> >  #include "cpu_bits.h"
> > +#include "qapi/qapi-types-common.h"
> >
> >  #define TCG_GUEST_DEFAULT_MO 0
> >
> > @@ -413,6 +414,17 @@ struct RISCVCPUClass {
> >      ResettablePhases parent_phases;
> >  };
> >
> > +/*
> > + * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> > + * satp mode that is supported.
> > + *
> > + * init is a 16-bit bitmap used to make sure the user selected a correct
> > + * configuration as per the specification.
> > + */
> > +typedef struct {
> > +    uint16_t map, init;
> > +} RISCVSATPMap;
> > +
> >  struct RISCVCPUConfig {
> >      bool ext_i;
> >      bool ext_e;
> > @@ -488,6 +500,8 @@ struct RISCVCPUConfig {
> >      bool debug;
> >
> >      bool short_isa_string;
> > +
> > +    RISCVSATPMap satp_mode;
> >  };
> >
> >  typedef struct RISCVCPUConfig RISCVCPUConfig;
> > @@ -794,9 +808,14 @@ enum riscv_pmu_event_idx {
> >  /* CSR function table */
> >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> >
> > +extern const char valid_vm_1_10_32[], valid_vm_1_10_64[];
> > +
> >  void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops);
> >  void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
> >
> >  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
> >
> > +uint8_t satp_mode_max_from_map(uint32_t map);
> > +const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
> > +
> >  #endif /* RISCV_CPU_H */
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 0db2c233e5..6e27299761 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1117,12 +1117,12 @@ static const target_ulong hip_writable_mask = MIP_VSSIP;
> >  static const target_ulong hvip_writable_mask = MIP_VSSIP | MIP_VSTIP | MIP_VSEIP;
> >  static const target_ulong vsip_writable_mask = MIP_VSSIP;
> >
> > -static const char valid_vm_1_10_32[16] = {
> > +const char valid_vm_1_10_32[16] = {
> >      [VM_1_10_MBARE] = 1,
> >      [VM_1_10_SV32] = 1
> >  };
> >
> > -static const char valid_vm_1_10_64[16] = {
> > +const char valid_vm_1_10_64[16] = {
> >      [VM_1_10_MBARE] = 1,
> >      [VM_1_10_SV39] = 1,
> >      [VM_1_10_SV48] = 1,
> > @@ -1211,10 +1211,17 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
> >
> >  static int validate_vm(CPURISCVState *env, target_ulong vm)
> >  {
> > -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> > -        return valid_vm_1_10_32[vm & 0xf];
> > +    uint8_t satp_mode_max;
> > +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
> > +    bool is_32_bit = riscv_cpu_mxl(env) == MXL_RV32;
> > +
> > +    vm &= 0xf;
> > +    satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +
> > +    if (is_32_bit) {
> > +        return valid_vm_1_10_32[vm] && (vm <= satp_mode_max);
> >      } else {
> > -        return valid_vm_1_10_64[vm & 0xf];
> > +        return valid_vm_1_10_64[vm] && (vm <= satp_mode_max);
> >      }
> >  }
> >
> > --
> > 2.37.2
> >
>
> Thanks,
> drew
>

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

[Andrew Jones]

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 ...

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.

Thanks,
drew

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

[Alexandre Ghiti]

Hey Andrew,

On Tue, Jan 17, 2023 at 5:31 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jan 13, 2023 at 11:34:53AM +0100, Alexandre Ghiti wrote:
> > RISC-V specifies multiple sizes for addressable memory and Linux probes for
> > the machine's support at startup via the satp CSR register (done in
> > csr.c:validate_vm).
> >
> > As per the specification, sv64 must support sv57, which in turn must
> > support sv48...etc. So we can restrict machine support by simply setting the
> > "highest" supported mode and the bare mode is always supported.
> >
> > You can set the satp mode using the new properties "sv32", "sv39", "sv48",
> > "sv57" and "sv64" as follows:
> > -cpu rv64,sv57=on  # Linux will boot using sv57 scheme
> > -cpu rv64,sv39=on  # Linux will boot using sv39 scheme
> > -cpu rv64,sv57=off # Linux will boot using sv48 scheme
> > -cpu rv64          # Linux will boot using sv57 scheme by default
> >
> > We take the highest level set by the user:
> > -cpu rv64,sv48=on,sv57=on # Linux will boot using sv57 scheme
> >
> > We make sure that invalid configurations are rejected:
> > -cpu rv64,sv32=on # Can't enable 32-bit satp mode in 64-bit
> > -cpu rv64,sv39=off,sv48=on # sv39 must be supported if higher modes are
> >                            # enabled
> >
> > We accept "redundant" configurations:
> > -cpu rv64,sv48=on,sv57=off # Linux will boot using sv48 scheme
> > -cpu rv64,sv32=on,sv32=off # Linux will boot using sv57 scheme (the default)
> >
> > In addition, we now correctly set the device-tree entry 'mmu-type' using
> > those new properties.
> >
> > Co-Developed-by: Ludovic Henry <ludovic@rivosinc.com>
> > Signed-off-by: Ludovic Henry <ludovic@rivosinc.com>
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  hw/riscv/virt.c    |  19 ++--
> >  target/riscv/cpu.c | 221 +++++++++++++++++++++++++++++++++++++++++++++
> >  target/riscv/cpu.h |  19 ++++
> >  target/riscv/csr.c |  17 +++-
> >  4 files changed, 262 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 94ff2a1584..48d034a5f7 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -228,7 +228,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> >      int cpu;
> >      uint32_t cpu_phandle;
> >      MachineState *mc = MACHINE(s);
> > -    char *name, *cpu_name, *core_name, *intc_name;
> > +    uint8_t satp_mode_max;
> > +    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
> >
> >      for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
> >          cpu_phandle = (*phandle)++;
> > @@ -236,14 +237,14 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> >          cpu_name = g_strdup_printf("/cpus/cpu@%d",
> >              s->soc[socket].hartid_base + cpu);
> >          qemu_fdt_add_subnode(mc->fdt, cpu_name);
> > -        if (riscv_feature(&s->soc[socket].harts[cpu].env,
> > -                          RISCV_FEATURE_MMU)) {
> > -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> > -                                    (is_32_bit) ? "riscv,sv32" : "riscv,sv48");
>
> I just noticed that for the virt machine type, when the user doesn't
> provide a satp mode cpu property on the command line, and hence gets
> the default mode, they'll be silently changed from sv48 to sv57. That
> default change should be a separate patch which comes after this one.
> BTW, why sv57 and not sv48 or sv64?

The device tree entry should match the max available satp mode even
though it makes little sense to have this entry in the first place:
the max satp mode is easily discoverable at runtime (the kernel does
that and does not care about the device tree entry).

But yes, this fix was mentioned at the very end of the commit log,
which was weird anyway, so I'll move that to its own patch.

>
> > -        } else {
> > -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> > -                                    "riscv,none");
> > -        }
> > +
> > +        satp_mode_max = satp_mode_max_from_map(
> > +                            s->soc[socket].harts[cpu].cfg.satp_mode.map);
> > +        sv_name = g_strdup_printf("riscv,%s",
> > +                                  satp_mode_str(satp_mode_max, is_32_bit));
> > +        qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type", sv_name);
> > +        g_free(sv_name);
> > +
> >          name = riscv_isa_string(&s->soc[socket].harts[cpu]);
> >          qemu_fdt_setprop_string(mc->fdt, cpu_name, "riscv,isa", name);
> >          g_free(name);
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7181b34f86..1f0d040a80 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -27,6 +27,7 @@
> >  #include "time_helper.h"
> >  #include "exec/exec-all.h"
> >  #include "qapi/error.h"
> > +#include "qapi/visitor.h"
> >  #include "qemu/error-report.h"
> >  #include "hw/qdev-properties.h"
> >  #include "migration/vmstate.h"
> > @@ -229,6 +230,85 @@ static void set_vext_version(CPURISCVState *env, int vext_ver)
> >      env->vext_ver = vext_ver;
> >  }
> >
> > +static uint8_t satp_mode_from_str(const char *satp_mode_str)
> > +{
> > +    if (!strncmp(satp_mode_str, "sv32", 4)) {
> > +        return VM_1_10_SV32;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv39", 4)) {
> > +        return VM_1_10_SV39;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv48", 4)) {
> > +        return VM_1_10_SV48;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv57", 4)) {
> > +        return VM_1_10_SV57;
> > +    }
> > +
> > +    if (!strncmp(satp_mode_str, "sv64", 4)) {
> > +        return VM_1_10_SV64;
> > +    }
> > +
> > +    g_assert_not_reached();
> > +}
> > +
> > +uint8_t satp_mode_max_from_map(uint32_t map)
> > +{
> > +    /* map here has at least one bit set, so no problem with clz */
> > +    return 31 - __builtin_clz(map);
> > +}
> > +
> > +const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> > +{
> > +    if (is_32_bit) {
> > +        switch (satp_mode) {
> > +        case VM_1_10_SV32:
> > +            return "sv32";
> > +        case VM_1_10_MBARE:
> > +            return "none";
> > +        }
> > +    } else {
> > +        switch (satp_mode) {
> > +        case VM_1_10_SV64:
> > +            return "sv64";
> > +        case VM_1_10_SV57:
> > +            return "sv57";
> > +        case VM_1_10_SV48:
> > +            return "sv48";
> > +        case VM_1_10_SV39:
> > +            return "sv39";
> > +        case VM_1_10_MBARE:
> > +            return "none";
> > +        }
> > +    }
> > +
> > +    g_assert_not_reached();
> > +}
> > +
> > +static void set_satp_mode(RISCVCPU *cpu, const char *satp_mode_str)
> > +{
> > +    cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str(satp_mode_str));
> > +}
> > +
> > +static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +{
> > +    /*
> > +     * If an mmu is present, the default satp mode is:
> > +     * - sv32 for 32-bit
> > +     * - sv57 for 64-bit
> > +     * Otherwise, it is mbare.
> > +     */
>
> I'd drop the above comment since it only repeats what the code says.

Ok

>
> > +
> > +    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > +        set_satp_mode(cpu, is_32_bit ? "sv32" : "sv57");
> > +    } else {
> > +        set_satp_mode(cpu, "mbare");
>
> nit: Could probably integrate set_satp_mode() into this function since
> this function is the only place it's used.

At the moment yes, but this was a request from Frank to have a helper
set the default satp mode in the cpu init functions, which I did not
do here because I was unsure: @Frank Chang What should I use for
sifive_e and sifive_u? rv64 will use the default mode.

>
> > +    }
> > +}
> > +
> >  static void riscv_any_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > @@ -619,6 +699,53 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> >      }
> >  }
> >
> > +static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> > +{
> > +    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +    const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
>
> Not a problem of this patch, but valid_vm_1_10_32/64 has a strange type.
> It's used like a boolean, so should be bool. Since you're touching the
> arrays and validate_vm() it'd be nice to change the array type and
> the return value of validate_vm() with a separate patch first.

Sure, I can do that.

>
> > +
> > +    /* 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
>
> and then drop the check here.
>
> > +            error_setg(errp, "satp_mode %s is not valid",
> > +                       satp_mode_str(i, !rv32));
> > +            return;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * Make sure the user did not ask for an invalid configuration as per
> > +     * the specification.
> > +     */
> > +    if (!rv32) {
> > +        uint8_t satp_mode_max;
> > +
> > +        satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +
> > +        for (int i = satp_mode_max - 1; i >= 0; --i) {
> > +            if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> > +                (cpu->cfg.satp_mode.init & (1 << i)) &&
> > +                valid_vm[i]) {
> > +                error_setg(errp, "cannot disable %s satp mode if %s "
> > +                           "is enabled", satp_mode_str(i, false),
> > +                           satp_mode_str(satp_mode_max, false));
> > +                return;
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +static void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +
> > +    riscv_cpu_satp_mode_finalize(cpu, &local_err);
> > +    if (local_err != NULL) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +}
> > +
> >  static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >  {
> >      CPUState *cs = CPU(dev);
> > @@ -919,6 +1046,55 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >       }
> >  #endif
> >
> > +    bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +
> > +    if (cpu->cfg.satp_mode.map == 0) {
> > +        /*
> > +         * If unset by both the user and the cpu, we fallback to the default
> > +         * satp mode.
> > +         */
> > +        if (cpu->cfg.satp_mode.init == 0) {
> > +            set_satp_mode_default(cpu, rv32);
> > +        } else {
> > +            /*
> > +             * Find the lowest level that was disabled and then enable the
> > +             * first valid level below which can be found in
> > +             * valid_vm_1_10_32/64.
> > +             */
> > +            const char *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +
> > +            for (int i = 0; i < 16; ++i) {
>
> 'init' will never have bit0 (mbare) set, so we can start at i=1, which is
> good, because the condition below assumes it can index an array at i-1.

Yes, that will be clearer indeed :)

>
> > +                if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> > +                    (cpu->cfg.satp_mode.init & (1 << i)) &&
> > +                    valid_vm[i]) {
> > +                    for (int j = i - 1; j >= 0; --j) {
> > +                        if (valid_vm[j]) {
> > +                            cpu->cfg.satp_mode.map |= (1 << j);
> > +                            break;
> > +                        }
> > +                    }
> > +                    break;
> > +                }
> > +            }
> > +
> > +            /*
> > +             * The user actually init a satp mode but appears to be invalid
> > +             * (ex: "-cpu rv64,sv32=on,sv32=off"). Fallback to the default
>
> This example, where sv32 is used with rv64, won't be possible if we don't
> give rv64 the sv32 property.
>
> > +             * mode.
> > +             */
> > +            if (cpu->cfg.satp_mode.map == 0) {
> > +                set_satp_mode_default(cpu, rv32);
>
> If the user does rv64,sv39=on,sv39=off, then I think we should be creating
> an mbare machine, rather than using the default.
>

This will be possible when we remove this condition which was meant
for the sv32 case above.

> > +            }
> > +        }
> > +    }
>
> Why isn't all this 'if (cpu->cfg.satp_mode.map == 0)' block above at the
> top of riscv_cpu_satp_mode_finalize() instead of here?
>

Because the realize function seemed to do the properties processing
and I thought the finalize one was meant to check the consistency of
the configuration that resulted: I can change that if you don't agree.

> > +
> > +    riscv_cpu_finalize_features(cpu, &local_err);
> > +    if (local_err != NULL) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> > +
>
> extra blank line

Oops

>
> >      riscv_cpu_register_gdb_regs_for_features(cs);
> >
> >      qemu_init_vcpu(cs);
> > @@ -927,6 +1103,49 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >      mcc->parent_realize(dev, errp);
> >  }
> >
> > +static void cpu_riscv_get_satp(Object *obj, Visitor *v, const char *name,
> > +                           void *opaque, Error **errp)
> > +{
> > +    RISCVSATPMap *satp_map = opaque;
> > +    uint8_t satp = satp_mode_from_str(name);
> > +    bool value;
> > +
> > +    value = (satp_map->map & (1 << satp));
> > +
> > +    visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
> > +                           void *opaque, Error **errp)
> > +{
> > +    RISCVSATPMap *satp_map = opaque;
> > +    uint8_t satp = satp_mode_from_str(name);
> > +    bool value;
> > +
> > +    if (!visit_type_bool(v, name, &value, errp)) {
> > +        return;
> > +    }
> > +
> > +    satp_map->map = deposit32(satp_map->map, satp, 1, value);
> > +    satp_map->init |= 1 << satp;
> > +}
> > +
> > +static void riscv_add_satp_mode_properties(Object *obj)
> > +{
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
>
> As mentioned above, I think we want to do
>
> > +
>   #if defined(TARGET_RISCV32)
> > +    object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
>   #elif defined(TARGET_RISCV64)
> > +    object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> > +    object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> > +    object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
> > +    object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
> > +                        cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode);
>   #endif
> > +}
> > +
> >  #ifndef CONFIG_USER_ONLY
> >  static void riscv_cpu_set_irq(void *opaque, int irq, int level)
> >  {
> > @@ -1091,6 +1310,8 @@ static void register_cpu_props(Object *obj)
> >      for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
> >          qdev_property_add_static(dev, prop);
> >      }
> > +
> > +    riscv_add_satp_mode_properties(obj);
> >  }
> >
> >  static Property riscv_cpu_properties[] = {
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index f5609b62a2..0ffa1bcfd5 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -27,6 +27,7 @@
> >  #include "qom/object.h"
> >  #include "qemu/int128.h"
> >  #include "cpu_bits.h"
> > +#include "qapi/qapi-types-common.h"
> >
> >  #define TCG_GUEST_DEFAULT_MO 0
> >
> > @@ -413,6 +414,17 @@ struct RISCVCPUClass {
> >      ResettablePhases parent_phases;
> >  };
> >
> > +/*
> > + * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> > + * satp mode that is supported.
> > + *
> > + * init is a 16-bit bitmap used to make sure the user selected a correct
> > + * configuration as per the specification.
> > + */
> > +typedef struct {
> > +    uint16_t map, init;
> > +} RISCVSATPMap;
> > +
> >  struct RISCVCPUConfig {
> >      bool ext_i;
> >      bool ext_e;
> > @@ -488,6 +500,8 @@ struct RISCVCPUConfig {
> >      bool debug;
> >
> >      bool short_isa_string;
> > +
> > +    RISCVSATPMap satp_mode;
> >  };
> >
> >  typedef struct RISCVCPUConfig RISCVCPUConfig;
> > @@ -794,9 +808,14 @@ enum riscv_pmu_event_idx {
> >  /* CSR function table */
> >  extern riscv_csr_operations csr_ops[CSR_TABLE_SIZE];
> >
> > +extern const char valid_vm_1_10_32[], valid_vm_1_10_64[];
> > +
> >  void riscv_get_csr_ops(int csrno, riscv_csr_operations *ops);
> >  void riscv_set_csr_ops(int csrno, riscv_csr_operations *ops);
> >
> >  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
> >
> > +uint8_t satp_mode_max_from_map(uint32_t map);
> > +const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
> > +
> >  #endif /* RISCV_CPU_H */
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 0db2c233e5..6e27299761 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1117,12 +1117,12 @@ static const target_ulong hip_writable_mask = MIP_VSSIP;
> >  static const target_ulong hvip_writable_mask = MIP_VSSIP | MIP_VSTIP | MIP_VSEIP;
> >  static const target_ulong vsip_writable_mask = MIP_VSSIP;
> >
> > -static const char valid_vm_1_10_32[16] = {
> > +const char valid_vm_1_10_32[16] = {
> >      [VM_1_10_MBARE] = 1,
> >      [VM_1_10_SV32] = 1
> >  };
> >
> > -static const char valid_vm_1_10_64[16] = {
> > +const char valid_vm_1_10_64[16] = {
> >      [VM_1_10_MBARE] = 1,
> >      [VM_1_10_SV39] = 1,
> >      [VM_1_10_SV48] = 1,
> > @@ -1211,10 +1211,17 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
> >
> >  static int validate_vm(CPURISCVState *env, target_ulong vm)
> >  {
> > -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> > -        return valid_vm_1_10_32[vm & 0xf];
> > +    uint8_t satp_mode_max;
> > +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
> > +    bool is_32_bit = riscv_cpu_mxl(env) == MXL_RV32;
> > +
> > +    vm &= 0xf;
> > +    satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +
> > +    if (is_32_bit) {
> > +        return valid_vm_1_10_32[vm] && (vm <= satp_mode_max);
> >      } else {
> > -        return valid_vm_1_10_64[vm & 0xf];
> > +        return valid_vm_1_10_64[vm] && (vm <= satp_mode_max);
> >      }
> >  }
> >
> > --
> > 2.37.2
> >
>

I'll wait for Alistair feedback on how to handle the sv32 issue, and
I'll be back with a new version. Again, thank you, your review is much
appreciated.

Alex

> Thanks,
> drew

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

[Andrew Jones]

On Wed, Jan 18, 2023 at 05:29:43PM +0100, Alexandre Ghiti wrote:
> Hey Andrew,
> 
> On Tue, Jan 17, 2023 at 5:31 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Fri, Jan 13, 2023 at 11:34:53AM +0100, Alexandre Ghiti wrote:
> > > RISC-V specifies multiple sizes for addressable memory and Linux probes for
> > > the machine's support at startup via the satp CSR register (done in
> > > csr.c:validate_vm).
> > >
> > > As per the specification, sv64 must support sv57, which in turn must
> > > support sv48...etc. So we can restrict machine support by simply setting the
> > > "highest" supported mode and the bare mode is always supported.
> > >
> > > You can set the satp mode using the new properties "sv32", "sv39", "sv48",
> > > "sv57" and "sv64" as follows:
> > > -cpu rv64,sv57=on  # Linux will boot using sv57 scheme
> > > -cpu rv64,sv39=on  # Linux will boot using sv39 scheme
> > > -cpu rv64,sv57=off # Linux will boot using sv48 scheme
> > > -cpu rv64          # Linux will boot using sv57 scheme by default
> > >
> > > We take the highest level set by the user:
> > > -cpu rv64,sv48=on,sv57=on # Linux will boot using sv57 scheme
> > >
> > > We make sure that invalid configurations are rejected:
> > > -cpu rv64,sv32=on # Can't enable 32-bit satp mode in 64-bit
> > > -cpu rv64,sv39=off,sv48=on # sv39 must be supported if higher modes are
> > >                            # enabled
> > >
> > > We accept "redundant" configurations:
> > > -cpu rv64,sv48=on,sv57=off # Linux will boot using sv48 scheme
> > > -cpu rv64,sv32=on,sv32=off # Linux will boot using sv57 scheme (the default)
> > >
> > > In addition, we now correctly set the device-tree entry 'mmu-type' using
> > > those new properties.
> > >
> > > Co-Developed-by: Ludovic Henry <ludovic@rivosinc.com>
> > > Signed-off-by: Ludovic Henry <ludovic@rivosinc.com>
> > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > ---
> > >  hw/riscv/virt.c    |  19 ++--
> > >  target/riscv/cpu.c | 221 +++++++++++++++++++++++++++++++++++++++++++++
> > >  target/riscv/cpu.h |  19 ++++
> > >  target/riscv/csr.c |  17 +++-
> > >  4 files changed, 262 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > index 94ff2a1584..48d034a5f7 100644
> > > --- a/hw/riscv/virt.c
> > > +++ b/hw/riscv/virt.c
> > > @@ -228,7 +228,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> > >      int cpu;
> > >      uint32_t cpu_phandle;
> > >      MachineState *mc = MACHINE(s);
> > > -    char *name, *cpu_name, *core_name, *intc_name;
> > > +    uint8_t satp_mode_max;
> > > +    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
> > >
> > >      for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
> > >          cpu_phandle = (*phandle)++;
> > > @@ -236,14 +237,14 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
> > >          cpu_name = g_strdup_printf("/cpus/cpu@%d",
> > >              s->soc[socket].hartid_base + cpu);
> > >          qemu_fdt_add_subnode(mc->fdt, cpu_name);
> > > -        if (riscv_feature(&s->soc[socket].harts[cpu].env,
> > > -                          RISCV_FEATURE_MMU)) {
> > > -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> > > -                                    (is_32_bit) ? "riscv,sv32" : "riscv,sv48");
> >
> > I just noticed that for the virt machine type, when the user doesn't
> > provide a satp mode cpu property on the command line, and hence gets
> > the default mode, they'll be silently changed from sv48 to sv57. That
> > default change should be a separate patch which comes after this one.
> > BTW, why sv57 and not sv48 or sv64?
> 
> The device tree entry should match the max available satp mode even
> though it makes little sense to have this entry in the first place:
> the max satp mode is easily discoverable at runtime (the kernel does
> that and does not care about the device tree entry).
> 
> But yes, this fix was mentioned at the very end of the commit log,
> which was weird anyway, so I'll move that to its own patch.

Ah, I interpreted that part of the commit message as simply pointing
out that the mmu-type is getting set per the user's input. Thanks for
moving this change to another patch.

...
> > > +
> > > +    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > > +        set_satp_mode(cpu, is_32_bit ? "sv32" : "sv57");
> > > +    } else {
> > > +        set_satp_mode(cpu, "mbare");
> >
> > nit: Could probably integrate set_satp_mode() into this function since
> > this function is the only place it's used.
> 
> At the moment yes, but this was a request from Frank to have a helper
> set the default satp mode in the cpu init functions, which I did not
> do here because I was unsure: @Frank Chang What should I use for
> sifive_e and sifive_u? rv64 will use the default mode.

The sifive stuff should probably be a separate patch. If that patch will
be part of this series then the proactive refactoring makes sense as we
can immediately see the users.

...
> > Why isn't all this 'if (cpu->cfg.satp_mode.map == 0)' block above at the
> > top of riscv_cpu_satp_mode_finalize() instead of here?
> >
> 
> Because the realize function seemed to do the properties processing
> and I thought the finalize one was meant to check the consistency of
> the configuration that resulted: I can change that if you don't agree.

finalize should do all the processing and checking, basically everything
not done in the property's set function. realize should call
finalize_features, which then calls each feature's finalize. Take a look
at arm's call chain, for example

 arm_cpu_realizefn
   arm_cpu_finalize_features
     arm_cpu_sve_finalize

Thanks,
drew

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

[Alistair Francis]

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 :)

Alistair

>
> Thanks,
> drew

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

[Alexandre Ghiti]

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?

>
> Alistair
>
> >
> > Thanks,
> > drew

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),

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

[Alistair Francis]

On Thu, Jan 19, 2023 at 11:00 PM Alexandre Ghiti <alexghiti@rivosinc.com> 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()?

Wait! Sorry I didn't read this carefully enough. No, that is not what
we want to do. That then won't support the vendor CPUs.

We just want to add the properties to all CPUs. Then if an invalid
option is set we should return an error.

Note that the 64-bit only configs can be hidden behind a #if
defined(TARGET_RISCV64).

Alistair

> > > 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?
>
> >
> > Alistair
> >
> > >
> > > Thanks,
> > > drew

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

[Andrew Jones]

On Fri, Jan 20, 2023 at 09:46:05AM +1000, Alistair Francis wrote:
> On Thu, Jan 19, 2023 at 11:00 PM Alexandre Ghiti <alexghiti@rivosinc.com> 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()?
> 
> Wait! Sorry I didn't read this carefully enough. No, that is not what
> we want to do. That then won't support the vendor CPUs.
> 
> We just want to add the properties to all CPUs. Then if an invalid
> option is set we should return an error.
> 
> Note that the 64-bit only configs can be hidden behind a #if
> defined(TARGET_RISCV64).

OK, so we want the original suggestion of putting an
'if defined(TARGET_RISCV64)' in riscv_add_satp_mode_properties(),
which is called from register_cpu_props(), for the 64-bit only
configs, but to support emulation we can't put sv32 under an
'if defined(TARGET_RISCV32)'. Instead, we need to check the xlen
supported by the cpu type. That makes sense to me, and I think
it'd be easiest to do in cpu_riscv_set_satp() with something like

  if (!strncmp(name, "rv32", 4) &&
      RISCV_CPU(obj)->env.misa_mxl != MXL_RV32) {
     ... fail with error message ...
  }

Thanks,
drew

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

[Alexandre Ghiti]

On Fri, Jan 20, 2023 at 10:53 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, Jan 20, 2023 at 09:46:05AM +1000, Alistair Francis wrote:
> > On Thu, Jan 19, 2023 at 11:00 PM Alexandre Ghiti <alexghiti@rivosinc.com> 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()?
> >
> > Wait! Sorry I didn't read this carefully enough. No, that is not what
> > we want to do. That then won't support the vendor CPUs.
> >
> > We just want to add the properties to all CPUs. Then if an invalid
> > option is set we should return an error.

Maybe I just don't get this part...

> >
> > Note that the 64-bit only configs can be hidden behind a #if
> > defined(TARGET_RISCV64).
>
> OK, so we want the original suggestion of putting an
> 'if defined(TARGET_RISCV64)' in riscv_add_satp_mode_properties(),
> which is called from register_cpu_props(), for the 64-bit only
> configs, but to support emulation we can't put sv32 under an
> 'if defined(TARGET_RISCV32)'. Instead, we need to check the xlen
> supported by the cpu type. That makes sense to me, and I think
> it'd be easiest to do in cpu_riscv_set_satp() with something like
>
>   if (!strncmp(name, "rv32", 4) &&
>       RISCV_CPU(obj)->env.misa_mxl != MXL_RV32) {
>      ... fail with error message ...
>   }
>

...but what about simply using the runtime check when we add the
properties? Like this:

static void riscv_add_satp_mode_properties(Object *obj)
{
    RISCVCPU *cpu = RISCV_CPU(obj);

    if (cpu->env.misa_mxl == MXL_RV32) {
        object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
                            cpu_riscv_set_satp, NULL,
&cpu->cfg.satp_mode);
    } else {
        object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
                            cpu_riscv_set_satp, NULL,
&cpu->cfg.satp_mode);
        object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
                            cpu_riscv_set_satp, NULL,
&cpu->cfg.satp_mode);
        object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
                            cpu_riscv_set_satp, NULL,
&cpu->cfg.satp_mode);
        object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
                            cpu_riscv_set_satp, NULL,
&cpu->cfg.satp_mode);
    }
}

> Thanks,
> drew

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

[Andrew Jones]

On Fri, Jan 20, 2023 at 01:44:41PM +0100, Alexandre Ghiti wrote:
> On Fri, Jan 20, 2023 at 10:53 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Fri, Jan 20, 2023 at 09:46:05AM +1000, Alistair Francis wrote:
> > > On Thu, Jan 19, 2023 at 11:00 PM Alexandre Ghiti <alexghiti@rivosinc.com> 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()?
> > >
> > > Wait! Sorry I didn't read this carefully enough. No, that is not what
> > > we want to do. That then won't support the vendor CPUs.
> > >
> > > We just want to add the properties to all CPUs. Then if an invalid
> > > option is set we should return an error.
> 
> Maybe I just don't get this part...

Indeed, I like not adding the property at all over adding it and then
complaining when it's used. Your solution below looks good to me and
would be my preference as well.

Thanks,
drew

> 
> > >
> > > Note that the 64-bit only configs can be hidden behind a #if
> > > defined(TARGET_RISCV64).
> >
> > OK, so we want the original suggestion of putting an
> > 'if defined(TARGET_RISCV64)' in riscv_add_satp_mode_properties(),
> > which is called from register_cpu_props(), for the 64-bit only
> > configs, but to support emulation we can't put sv32 under an
> > 'if defined(TARGET_RISCV32)'. Instead, we need to check the xlen
> > supported by the cpu type. That makes sense to me, and I think
> > it'd be easiest to do in cpu_riscv_set_satp() with something like
> >
> >   if (!strncmp(name, "rv32", 4) &&
> >       RISCV_CPU(obj)->env.misa_mxl != MXL_RV32) {
> >      ... fail with error message ...
> >   }
> >
> 
> ...but what about simply using the runtime check when we add the
> properties? Like this:
> 
> static void riscv_add_satp_mode_properties(Object *obj)
> {
>     RISCVCPU *cpu = RISCV_CPU(obj);
> 
>     if (cpu->env.misa_mxl == MXL_RV32) {
>         object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>     } else {
>         object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>         object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>         object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>         object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>     }
> }
> 
> > Thanks,
> > drew