[PATCH v6 0/5] riscv: Allow user to set the satp mode

[PATCH v6 0/5] 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 3 for full syntax. In addition, it prevents cpus to boot in a
satp mode they do not support (see patch 5).

v6:
- Remove the valid_vm check in validate_vm and add it to the finalize function
  so that map already contains the constraint, Alex
- Add forgotten mbare to satp_mode_from_str, Alex
- Move satp mode properties handling to riscv_cpu_satp_mode_finalize, Andrew
- Only add satp mode properties corresponding to the cpu, and then remove the
  check against valid_vm_1_10_32/64 in riscv_cpu_satp_mode_finalize,
  Andrew/Alistair/Alex
- Move mmu-type setting to its own patch, Andrew
- patch 5 is new and is a fix, Alex

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 (5):
  riscv: Pass Object to register_cpu_props instead of DeviceState
  riscv: Change type of valid_vm_1_10_[32|64] to bool
  riscv: Allow user to set the satp mode
  riscv: Correctly set the device-tree entry 'mmu-type'
  riscv: Introduce satp mode hw capabilities

 hw/riscv/virt.c    |  19 ++--
 target/riscv/cpu.c | 247 +++++++++++++++++++++++++++++++++++++++++++--
 target/riscv/cpu.h |  23 +++++
 target/riscv/csr.c |  29 +++---
 4 files changed, 287 insertions(+), 31 deletions(-)

-- 
2.37.2

[PATCH v6 1/5] 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.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
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 v6 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool

[Alexandre Ghiti]

This array is actually used as a boolean so swap its current char type
to a boolean and at the same time, change the type of validate_vm to
bool since it returns valid_vm_1_10_[32|64].

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 target/riscv/csr.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 0db2c233e5..6b157806a5 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1117,16 +1117,16 @@ 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] = {
-    [VM_1_10_MBARE] = 1,
-    [VM_1_10_SV32] = 1
+static const bool valid_vm_1_10_32[16] = {
+    [VM_1_10_MBARE] = true,
+    [VM_1_10_SV32] = true
 };
 
-static const char valid_vm_1_10_64[16] = {
-    [VM_1_10_MBARE] = 1,
-    [VM_1_10_SV39] = 1,
-    [VM_1_10_SV48] = 1,
-    [VM_1_10_SV57] = 1
+static const bool valid_vm_1_10_64[16] = {
+    [VM_1_10_MBARE] = true,
+    [VM_1_10_SV39] = true,
+    [VM_1_10_SV48] = true,
+    [VM_1_10_SV57] = true
 };
 
 /* Machine Information Registers */
@@ -1209,7 +1209,7 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
     return RISCV_EXCP_NONE;
 }
 
-static int validate_vm(CPURISCVState *env, target_ulong vm)
+static bool validate_vm(CPURISCVState *env, target_ulong vm)
 {
     if (riscv_cpu_mxl(env) == MXL_RV32) {
         return valid_vm_1_10_32[vm & 0xf];
@@ -2648,7 +2648,8 @@ static RISCVException read_satp(CPURISCVState *env, int csrno,
 static RISCVException write_satp(CPURISCVState *env, int csrno,
                                  target_ulong val)
 {
-    target_ulong vm, mask;
+    target_ulong mask;
+    bool vm;
 
     if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
         return RISCV_EXCP_NONE;
-- 
2.37.2

Re: [PATCH v6 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool

[Andrew Jones]

On Mon, Jan 23, 2023 at 10:03:21AM +0100, Alexandre Ghiti wrote:
> This array is actually used as a boolean so swap its current char type
> to a boolean and at the same time, change the type of validate_vm to
> bool since it returns valid_vm_1_10_[32|64].
> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>

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

> ---
>  target/riscv/csr.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0db2c233e5..6b157806a5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1117,16 +1117,16 @@ 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] = {
> -    [VM_1_10_MBARE] = 1,
> -    [VM_1_10_SV32] = 1
> +static const bool valid_vm_1_10_32[16] = {
> +    [VM_1_10_MBARE] = true,
> +    [VM_1_10_SV32] = true
>  };
>  
> -static const char valid_vm_1_10_64[16] = {
> -    [VM_1_10_MBARE] = 1,
> -    [VM_1_10_SV39] = 1,
> -    [VM_1_10_SV48] = 1,
> -    [VM_1_10_SV57] = 1
> +static const bool valid_vm_1_10_64[16] = {
> +    [VM_1_10_MBARE] = true,
> +    [VM_1_10_SV39] = true,
> +    [VM_1_10_SV48] = true,
> +    [VM_1_10_SV57] = true
>  };
>  
>  /* Machine Information Registers */
> @@ -1209,7 +1209,7 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>  
> -static int validate_vm(CPURISCVState *env, target_ulong vm)
> +static bool validate_vm(CPURISCVState *env, target_ulong vm)
>  {
>      if (riscv_cpu_mxl(env) == MXL_RV32) {
>          return valid_vm_1_10_32[vm & 0xf];
> @@ -2648,7 +2648,8 @@ static RISCVException read_satp(CPURISCVState *env, int csrno,
>  static RISCVException write_satp(CPURISCVState *env, int csrno,
>                                   target_ulong val)
>  {
> -    target_ulong vm, mask;
> +    target_ulong mask;
> +    bool vm;
>  
>      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
>          return RISCV_EXCP_NONE;
> -- 
> 2.37.2
> 

Re: [PATCH v6 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool

[Alistair Francis]

On Mon, Jan 23, 2023 at 7:06 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> This array is actually used as a boolean so swap its current char type
> to a boolean and at the same time, change the type of validate_vm to
> bool since it returns valid_vm_1_10_[32|64].
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>

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

Alistair

> ---
>  target/riscv/csr.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0db2c233e5..6b157806a5 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1117,16 +1117,16 @@ 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] = {
> -    [VM_1_10_MBARE] = 1,
> -    [VM_1_10_SV32] = 1
> +static const bool valid_vm_1_10_32[16] = {
> +    [VM_1_10_MBARE] = true,
> +    [VM_1_10_SV32] = true
>  };
>
> -static const char valid_vm_1_10_64[16] = {
> -    [VM_1_10_MBARE] = 1,
> -    [VM_1_10_SV39] = 1,
> -    [VM_1_10_SV48] = 1,
> -    [VM_1_10_SV57] = 1
> +static const bool valid_vm_1_10_64[16] = {
> +    [VM_1_10_MBARE] = true,
> +    [VM_1_10_SV39] = true,
> +    [VM_1_10_SV48] = true,
> +    [VM_1_10_SV57] = true
>  };
>
>  /* Machine Information Registers */
> @@ -1209,7 +1209,7 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
>      return RISCV_EXCP_NONE;
>  }
>
> -static int validate_vm(CPURISCVState *env, target_ulong vm)
> +static bool validate_vm(CPURISCVState *env, target_ulong vm)
>  {
>      if (riscv_cpu_mxl(env) == MXL_RV32) {
>          return valid_vm_1_10_32[vm & 0xf];
> @@ -2648,7 +2648,8 @@ static RISCVException read_satp(CPURISCVState *env, int csrno,
>  static RISCVException write_satp(CPURISCVState *env, int csrno,
>                                   target_ulong val)
>  {
> -    target_ulong vm, mask;
> +    target_ulong mask;
> +    bool vm;
>
>      if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
>          return RISCV_EXCP_NONE;
> --
> 2.37.2
>
>

[PATCH v6 3/5] 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

And contradictory configurations:
-cpu rv64,sv48=on,sv48=off # Linux will boot using sv39 scheme

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>
---
 target/riscv/cpu.c | 204 +++++++++++++++++++++++++++++++++++++++++++++
 target/riscv/cpu.h |  19 +++++
 target/riscv/csr.c |  12 ++-
 3 files changed, 228 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7181b34f86..e409e6ab64 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,79 @@ 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, "mbare", 5)) {
+        return VM_1_10_MBARE;
+    }
+
+    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();
+}
+
+/* Sets the satp mode to the max supported */
+static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
+{
+    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
+        cpu->cfg.satp_mode.map |=
+                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
+    } else {
+        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
+    }
+}
+
 static void riscv_any_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
@@ -619,6 +693,82 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
+
+    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.
+             */
+            for (int i = 1; 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;
+                }
+            }
+        }
+    }
+
+    /* Make sure the configuration asked is supported by qemu */
+    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 +1069,12 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
      }
 #endif
 
+    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 +1083,52 @@ 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);
+
+    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);
+    }
+}
+
 #ifndef CONFIG_USER_ONLY
 static void riscv_cpu_set_irq(void *opaque, int irq, int level)
 {
@@ -1091,6 +1293,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..e37177db5c 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 bool 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 6b157806a5..3c02055825 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 bool valid_vm_1_10_32[16] = {
+const bool valid_vm_1_10_32[16] = {
     [VM_1_10_MBARE] = true,
     [VM_1_10_SV32] = true
 };
 
-static const bool valid_vm_1_10_64[16] = {
+const bool valid_vm_1_10_64[16] = {
     [VM_1_10_MBARE] = true,
     [VM_1_10_SV39] = true,
     [VM_1_10_SV48] = true,
@@ -1211,11 +1211,9 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
 
 static bool validate_vm(CPURISCVState *env, target_ulong vm)
 {
-    if (riscv_cpu_mxl(env) == MXL_RV32) {
-        return valid_vm_1_10_32[vm & 0xf];
-    } else {
-        return valid_vm_1_10_64[vm & 0xf];
-    }
+    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
+
+    return ((vm & 0xf) <= satp_mode_max_from_map(cpu->cfg.satp_mode.map));
 }
 
 static RISCVException write_mstatus(CPURISCVState *env, int csrno,
-- 
2.37.2

Re: [PATCH v6 3/5] riscv: Allow user to set the satp mode

[Andrew Jones]

On Mon, Jan 23, 2023 at 10:03:22AM +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

The property doesn't exist for rv64 anymore, so I'm not sure we need
this info in the commit message.

> -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
> 
> And contradictory configurations:
> -cpu rv64,sv48=on,sv48=off # Linux will boot using sv39 scheme
> 
> 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>
> ---
>  target/riscv/cpu.c | 204 +++++++++++++++++++++++++++++++++++++++++++++
>  target/riscv/cpu.h |  19 +++++
>  target/riscv/csr.c |  12 ++-
>  3 files changed, 228 insertions(+), 7 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7181b34f86..e409e6ab64 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,79 @@ 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, "mbare", 5)) {
> +        return VM_1_10_MBARE;
> +    }
> +
> +    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();
> +}
> +
> +/* Sets the satp mode to the max supported */
> +static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +{
> +    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> +        cpu->cfg.satp_mode.map |=
> +                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> +    } else {
> +        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> +    }
> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> @@ -619,6 +693,82 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> +    if (cpu->cfg.satp_mode.map == 0) {
> +        /*
> +         * If unset by both the user and the cpu, we fallback to the default
> +         * satp mode.
> +         */

nit: I'd put the above comment under the if init == 0 below.

> +        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.
> +             */
> +            for (int i = 1; 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;

We don't want to break here, we want fully populate the map. Otherwise the
future coming qmp_query_cpu_model_expansion() is going to produce results
like rv39=off,rv48=off,rv57=on,rv64=off for the default because it gets
its info from cpu_riscv_get_satp(), which only checks the map.

> +                        }
> +                    }
> +                    break;
> +                }
> +            }
> +        }
> +    }
> +
> +    /* Make sure the configuration asked is supported by qemu */
> +    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 +1069,12 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>       }
>  #endif
>  
> +    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 +1083,52 @@ 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);
> +
> +    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);
> +    }
> +}
> +
>  #ifndef CONFIG_USER_ONLY
>  static void riscv_cpu_set_irq(void *opaque, int irq, int level)
>  {
> @@ -1091,6 +1293,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..e37177db5c 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 bool 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 6b157806a5..3c02055825 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 bool valid_vm_1_10_32[16] = {
> +const bool valid_vm_1_10_32[16] = {
>      [VM_1_10_MBARE] = true,
>      [VM_1_10_SV32] = true
>  };
>  
> -static const bool valid_vm_1_10_64[16] = {
> +const bool valid_vm_1_10_64[16] = {
>      [VM_1_10_MBARE] = true,
>      [VM_1_10_SV39] = true,
>      [VM_1_10_SV48] = true,
> @@ -1211,11 +1211,9 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
>  
>  static bool validate_vm(CPURISCVState *env, target_ulong vm)
>  {
> -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> -        return valid_vm_1_10_32[vm & 0xf];
> -    } else {
> -        return valid_vm_1_10_64[vm & 0xf];
> -    }
> +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
> +
> +    return ((vm & 0xf) <= satp_mode_max_from_map(cpu->cfg.satp_mode.map));
>  }
>  
>  static RISCVException write_mstatus(CPURISCVState *env, int csrno,
> -- 
> 2.37.2
>

Thanks,
drew

Re: [PATCH v6 3/5] riscv: Allow user to set the satp mode

[Alexandre Ghiti]

Hi Andrew,

On Mon, Jan 23, 2023 at 11:11 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 23, 2023 at 10:03:22AM +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
>
> The property doesn't exist for rv64 anymore, so I'm not sure we need
> this info in the commit message.

Sorry about that...

>
> > -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
> >
> > And contradictory configurations:
> > -cpu rv64,sv48=on,sv48=off # Linux will boot using sv39 scheme
> >
> > 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>
> > ---
> >  target/riscv/cpu.c | 204 +++++++++++++++++++++++++++++++++++++++++++++
> >  target/riscv/cpu.h |  19 +++++
> >  target/riscv/csr.c |  12 ++-
> >  3 files changed, 228 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7181b34f86..e409e6ab64 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,79 @@ 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, "mbare", 5)) {
> > +        return VM_1_10_MBARE;
> > +    }
> > +
> > +    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();
> > +}
> > +
> > +/* Sets the satp mode to the max supported */
> > +static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +{
> > +    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > +        cpu->cfg.satp_mode.map |=
> > +                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> > +    } else {
> > +        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > +    }
> > +}
> > +
> >  static void riscv_any_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > @@ -619,6 +693,82 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +
> > +    if (cpu->cfg.satp_mode.map == 0) {
> > +        /*
> > +         * If unset by both the user and the cpu, we fallback to the default
> > +         * satp mode.
> > +         */
>
> nit: I'd put the above comment under the if init == 0 below.

Ok, and I fixed the comment as the cpu does not set map at all, we set
map in set_satp_mode_default using qemu/hw capabilities, thanks.

>
> > +        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.
> > +             */
> > +            for (int i = 1; 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;
>
> We don't want to break here, we want fully populate the map. Otherwise the
> future coming qmp_query_cpu_model_expansion() is going to produce results
> like rv39=off,rv48=off,rv57=on,rv64=off for the default because it gets
> its info from cpu_riscv_get_satp(), which only checks the map.

Ok then I'll "expand" the map too in case map != 0.

>
> > +                        }
> > +                    }
> > +                    break;
> > +                }
> > +            }
> > +        }
> > +    }
> > +
> > +    /* Make sure the configuration asked is supported by qemu */
> > +    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 +1069,12 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
> >       }
> >  #endif
> >
> > +    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 +1083,52 @@ 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);
> > +
> > +    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);
> > +    }
> > +}
> > +
> >  #ifndef CONFIG_USER_ONLY
> >  static void riscv_cpu_set_irq(void *opaque, int irq, int level)
> >  {
> > @@ -1091,6 +1293,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..e37177db5c 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 bool 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 6b157806a5..3c02055825 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 bool valid_vm_1_10_32[16] = {
> > +const bool valid_vm_1_10_32[16] = {
> >      [VM_1_10_MBARE] = true,
> >      [VM_1_10_SV32] = true
> >  };
> >
> > -static const bool valid_vm_1_10_64[16] = {
> > +const bool valid_vm_1_10_64[16] = {
> >      [VM_1_10_MBARE] = true,
> >      [VM_1_10_SV39] = true,
> >      [VM_1_10_SV48] = true,
> > @@ -1211,11 +1211,9 @@ static RISCVException read_mstatus(CPURISCVState *env, int csrno,
> >
> >  static bool validate_vm(CPURISCVState *env, target_ulong vm)
> >  {
> > -    if (riscv_cpu_mxl(env) == MXL_RV32) {
> > -        return valid_vm_1_10_32[vm & 0xf];
> > -    } else {
> > -        return valid_vm_1_10_64[vm & 0xf];
> > -    }
> > +    RISCVCPU *cpu = RISCV_CPU(env_cpu(env));
> > +
> > +    return ((vm & 0xf) <= satp_mode_max_from_map(cpu->cfg.satp_mode.map));
> >  }
> >
> >  static RISCVException write_mstatus(CPURISCVState *env, int csrno,
> > --
> > 2.37.2
> >
>
> Thanks,
> drew

Re: [PATCH v6 3/5] riscv: Allow user to set the satp mode

[Andrew Jones]

On Mon, Jan 23, 2023 at 10:03:22AM +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
> 
> And contradictory configurations:
> -cpu rv64,sv48=on,sv48=off # Linux will boot using sv39 scheme
> 
> In addition, we now correctly set the device-tree entry 'mmu-type' using
> those new properties.

This sentence no longer applies to this patch.

Thanks,
drew

Re: [PATCH v6 3/5] riscv: Allow user to set the satp mode

[Alexandre Ghiti]

On Mon, Jan 23, 2023 at 11:14 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 23, 2023 at 10:03:22AM +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
> >
> > And contradictory configurations:
> > -cpu rv64,sv48=on,sv48=off # Linux will boot using sv39 scheme
> >
> > In addition, we now correctly set the device-tree entry 'mmu-type' using
> > those new properties.
>
> This sentence no longer applies to this patch.


Sorry about that and thanks for noticing.

Alex

>
> Thanks,
> drew

Re: [PATCH v6 3/5] riscv: Allow user to set the satp mode

[Andrew Jones]

On Mon, Jan 23, 2023 at 10:03:22AM +0100, Alexandre Ghiti wrote:
...
> +/* Sets the satp mode to the max supported */
> +static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +{

nit: When passing in the cpu object pointer there's no need to also pass
is_32_bit, we can just use it from the pointer, cpu->env.misa_mxl == MXL_RV32

Thanks,
drew

Re: [PATCH v6 3/5] riscv: Allow user to set the satp mode

[Alexandre Ghiti]

On Mon, Jan 23, 2023 at 11:29 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 23, 2023 at 10:03:22AM +0100, Alexandre Ghiti wrote:
> ...
> > +/* Sets the satp mode to the max supported */
> > +static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +{
>
> nit: When passing in the cpu object pointer there's no need to also pass
> is_32_bit, we can just use it from the pointer, cpu->env.misa_mxl == MXL_RV32

Ok

Thanks,

Alex

>
> Thanks,
> drew

[PATCH v6 4/5] riscv: Correctly set the device-tree entry 'mmu-type'

[Alexandre Ghiti]

The 'mmu-type' should reflect what the hardware is capable of so use the
new satp_mode field in RISCVCPUConfig to do that.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 hw/riscv/virt.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 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);
-- 
2.37.2

Re: [PATCH v6 4/5] riscv: Correctly set the device-tree entry 'mmu-type'

[Andrew Jones]

On Mon, Jan 23, 2023 at 10:03:23AM +0100, Alexandre Ghiti wrote:
> The 'mmu-type' should reflect what the hardware is capable of so use the
> new satp_mode field in RISCVCPUConfig to do that.
> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  hw/riscv/virt.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)


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

Re: [PATCH v6 4/5] riscv: Correctly set the device-tree entry 'mmu-type'

[Alistair Francis]

On Mon, Jan 23, 2023 at 7:08 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> The 'mmu-type' should reflect what the hardware is capable of so use the
> new satp_mode field in RISCVCPUConfig to do that.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>

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

Alistair

> ---
>  hw/riscv/virt.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 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);
> --
> 2.37.2
>
>

[PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Alexandre Ghiti]

Currently, the max satp mode is set with the only constraint that it must be
implemented in qemu, i.e. set in valid_vm_1_10_[32|64].

But we actually need to add another level of constraint: what the hw is
actually capable of, because currently, a linux booting on a sifive-u54
boots in sv57 mode which is incompatible with the cpu's sv39 max
capability.

So add a new bitmap to RISCVSATPMap which contains this capability and
initialize it in every XXX_cpu_init.

Finally, we have the following chain of constraints:

Qemu capability > HW capability > User choice > Software capability

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
 target/riscv/cpu.h |  8 +++--
 2 files changed, 59 insertions(+), 27 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index e409e6ab64..19a37fee2b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
     g_assert_not_reached();
 }
 
-/* Sets the satp mode to the max supported */
-static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
+static void set_satp_mode_max_supported(RISCVCPU *cpu,
+                                        const char *satp_mode_str,
+                                        bool is_32_bit)
 {
-    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
-        cpu->cfg.satp_mode.map |=
-                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
-    } else {
-        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
+    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
+    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
+
+    for (int i = 0; i <= satp_mode; ++i) {
+        if (valid_vm[i]) {
+            cpu->cfg.satp_mode.supported |= (1 << i);
+        }
     }
 }
 
+/* Sets the satp mode to the max supported */
+static void set_satp_mode_default(RISCVCPU *cpu)
+{
+    uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
+
+    cpu->cfg.satp_mode.map |= (1 << satp_mode);
+}
+
 static void riscv_any_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+
 #if defined(TARGET_RISCV32)
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+    set_satp_mode_max_supported(cpu, "sv32", true);
 #elif defined(TARGET_RISCV64)
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+    set_satp_mode_max_supported(cpu, "sv57", false);
 #endif
     set_priv_version(env, PRIV_VERSION_1_12_0);
     register_cpu_props(obj);
@@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj)
 static void rv64_base_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+
     /* We set this in the realise function */
     set_misa(env, MXL_RV64, 0);
     register_cpu_props(obj);
     /* Set latest version of privileged specification */
     set_priv_version(env, PRIV_VERSION_1_12_0);
+    set_satp_mode_max_supported(cpu, "sv57", false);
 }
 
 static void rv64_sifive_u_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
+    set_satp_mode_max_supported(cpu, "sv39", false);
 }
 
 static void rv64_sifive_e_cpu_init(Object *obj)
@@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
     set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
     cpu->cfg.mmu = false;
+    set_satp_mode_max_supported(cpu, "mbare", false);
 }
 
 static void rv128_base_cpu_init(Object *obj)
@@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj)
         exit(EXIT_FAILURE);
     }
     CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
     /* We set this in the realise function */
     set_misa(env, MXL_RV128, 0);
     register_cpu_props(obj);
     /* Set latest version of privileged specification */
     set_priv_version(env, PRIV_VERSION_1_12_0);
+    set_satp_mode_max_supported(cpu, "sv57", false);
 }
 #else
 static void rv32_base_cpu_init(Object *obj)
@@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj)
     register_cpu_props(obj);
     /* Set latest version of privileged specification */
     set_priv_version(env, PRIV_VERSION_1_12_0);
+    set_satp_mode_max_supported(cpu, "sv32", true);
 }
 
 static void rv32_sifive_u_cpu_init(Object *obj)
 {
     CPURISCVState *env = &RISCV_CPU(obj)->env;
+    RISCVCPU *cpu = RISCV_CPU(obj);
+
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
+    set_satp_mode_max_supported(cpu, "sv32", true);
 }
 
 static void rv32_sifive_e_cpu_init(Object *obj)
@@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
     cpu->cfg.mmu = false;
+    set_satp_mode_max_supported(cpu, "mbare", true);
 }
 
 static void rv32_ibex_cpu_init(Object *obj)
@@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_11_0);
     cpu->cfg.mmu = false;
+    set_satp_mode_max_supported(cpu, "mbare", true);
     cpu->cfg.epmp = true;
 }
 
@@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
     set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
     set_priv_version(env, PRIV_VERSION_1_10_0);
     cpu->cfg.mmu = false;
+    set_satp_mode_max_supported(cpu, "mbare", true);
 }
 #endif
 
@@ -696,7 +727,9 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
+    uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
+    uint8_t satp_mode_supported_max =
+                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
 
     if (cpu->cfg.satp_mode.map == 0) {
         /*
@@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
          * satp mode.
          */
         if (cpu->cfg.satp_mode.init == 0) {
-            set_satp_mode_default(cpu, rv32);
+            set_satp_mode_default(cpu);
         } else {
             /*
              * Find the lowest level that was disabled and then enable the
@@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
             for (int i = 1; i < 16; ++i) {
                 if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
                     (cpu->cfg.satp_mode.init & (1 << i)) &&
-                    valid_vm[i]) {
+                    (cpu->cfg.satp_mode.supported & (1 << i))) {
                     for (int j = i - 1; j >= 0; --j) {
-                        if (valid_vm[j]) {
+                        if (cpu->cfg.satp_mode.supported & (1 << j)) {
                             cpu->cfg.satp_mode.map |= (1 << j);
                             break;
                         }
@@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
         }
     }
 
-    /* Make sure the configuration asked is supported by qemu */
-    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 asked for a supported configuration (HW and qemu) */
+    if (satp_mode_map_max > satp_mode_supported_max) {
+        error_setg(errp, "satp_mode %s is higher than hw max capability %s",
+                   satp_mode_str(satp_mode_map_max, rv32),
+                   satp_mode_str(satp_mode_supported_max, rv32));
+        return;
     }
 
     /*
@@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
      * 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) {
+        for (int i = satp_mode_map_max - 1; i >= 0; --i) {
             if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
                 (cpu->cfg.satp_mode.init & (1 << i)) &&
-                valid_vm[i]) {
+                (cpu->cfg.satp_mode.supported & (1 << 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));
+                           satp_mode_str(satp_mode_map_max, false));
                 return;
             }
         }
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index e37177db5c..b591122099 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -416,13 +416,17 @@ struct RISCVCPUClass {
 
 /*
  * map is a 16-bit bitmap: the most significant set bit in map is the maximum
- * satp mode that is supported.
+ * satp mode that is supported. It may be chosen by the user and must respect
+ * what qemu implements (valid_1_10_32/64) and what the hw is capable of
+ * (supported bitmap below).
  *
  * init is a 16-bit bitmap used to make sure the user selected a correct
  * configuration as per the specification.
+ *
+ * supported is a 16-bit bitmap used to reflect the hw capabilities.
  */
 typedef struct {
-    uint16_t map, init;
+    uint16_t map, init, supported;
 } RISCVSATPMap;
 
 struct RISCVCPUConfig {
-- 
2.37.2

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Andrew Jones]

On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> Currently, the max satp mode is set with the only constraint that it must be
> implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> 
> But we actually need to add another level of constraint: what the hw is
> actually capable of, because currently, a linux booting on a sifive-u54
> boots in sv57 mode which is incompatible with the cpu's sv39 max
> capability.
> 
> So add a new bitmap to RISCVSATPMap which contains this capability and
> initialize it in every XXX_cpu_init.
> 
> Finally, we have the following chain of constraints:
> 
> Qemu capability > HW capability > User choice > Software capability

                                                  ^ What software is this?
                         I'd think the user's choice would always be last.

> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
>  target/riscv/cpu.h |  8 +++--
>  2 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e409e6ab64..19a37fee2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
>      g_assert_not_reached();
>  }
>  
> -/* Sets the satp mode to the max supported */
> -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> +                                        const char *satp_mode_str,
> +                                        bool is_32_bit)
>  {
> -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> -        cpu->cfg.satp_mode.map |=
> -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> -    } else {
> -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> +    for (int i = 0; i <= satp_mode; ++i) {
> +        if (valid_vm[i]) {
> +            cpu->cfg.satp_mode.supported |= (1 << i);

I don't think we need a new 'supported' bitmap, I think each board that
needs to further constrain va-bits from what QEMU supports should just set
valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init
function something like

 #define QEMU_SATP_MODE_MAX VM_1_10_SV64

 void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max)
 {
     bool is_32_bit = cpu->env.misa_mxl == MXL_RV32;
     bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;

     g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX);
     g_assert(!is_32_bit || satp_mode_max < 2);

     memset(valid_vm, 0, sizeof(*valid_vm));

     for (int i = 0; i <= satp_mode_max; i++) {
         valid_vm[i] = true;
     }
 }

The valid_vm[] checks already in finalize should then manage the
validation needed to constrain boards. Only boards that care about
this need to call this function, otherwise they'll get the default.

Also, this patch should come before the patch that changes the default
for all boards to sv57 in order to avoid breaking bisection.

Thanks,
drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Alexandre Ghiti]

On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> > Currently, the max satp mode is set with the only constraint that it must be
> > implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> >
> > But we actually need to add another level of constraint: what the hw is
> > actually capable of, because currently, a linux booting on a sifive-u54
> > boots in sv57 mode which is incompatible with the cpu's sv39 max
> > capability.
> >
> > So add a new bitmap to RISCVSATPMap which contains this capability and
> > initialize it in every XXX_cpu_init.
> >
> > Finally, we have the following chain of constraints:
> >
> > Qemu capability > HW capability > User choice > Software capability
>
>                                                   ^ What software is this?
>                          I'd think the user's choice would always be last.
>
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
> >  target/riscv/cpu.h |  8 +++--
> >  2 files changed, 59 insertions(+), 27 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index e409e6ab64..19a37fee2b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> >      g_assert_not_reached();
> >  }
> >
> > -/* Sets the satp mode to the max supported */
> > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> > +                                        const char *satp_mode_str,
> > +                                        bool is_32_bit)
> >  {
> > -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > -        cpu->cfg.satp_mode.map |=
> > -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> > -    } else {
> > -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> > +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +
> > +    for (int i = 0; i <= satp_mode; ++i) {
> > +        if (valid_vm[i]) {
> > +            cpu->cfg.satp_mode.supported |= (1 << i);
>
> I don't think we need a new 'supported' bitmap, I think each board that
> needs to further constrain va-bits from what QEMU supports should just set
> valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init
> function something like

This was my first idea too, but those arrays are global and I have to
admit that I thought it was possible to emulate a cpu with different
cores. Anyway, isn't it a bit weird to store this into some global
array whereas it is intimately linked to the CPU? To me, it makes
sense to keep those variables as a way to know what qemu is able to
emulate and have a CPU specific map like in this patch for the hw
capabilities. Does it make sense to you?

>
>  #define QEMU_SATP_MODE_MAX VM_1_10_SV64
>
>  void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max)
>  {
>      bool is_32_bit = cpu->env.misa_mxl == MXL_RV32;
>      bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
>
>      g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX);
>      g_assert(!is_32_bit || satp_mode_max < 2);
>
>      memset(valid_vm, 0, sizeof(*valid_vm));
>
>      for (int i = 0; i <= satp_mode_max; i++) {
>          valid_vm[i] = true;
>      }
>  }
>
> The valid_vm[] checks already in finalize should then manage the
> validation needed to constrain boards. Only boards that care about
> this need to call this function, otherwise they'll get the default.
>
> Also, this patch should come before the patch that changes the default
> for all boards to sv57 in order to avoid breaking bisection.

As I explained earlier, I didn't change the default to sv57! Just
fixed what was passed via the device tree, which should not be used
anyway :)

Alex

>
> Thanks,
> drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Andrew Jones]

On Mon, Jan 23, 2023 at 12:15:08PM +0100, Alexandre Ghiti wrote:
> On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> > > Currently, the max satp mode is set with the only constraint that it must be
> > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> > >
> > > But we actually need to add another level of constraint: what the hw is
> > > actually capable of, because currently, a linux booting on a sifive-u54
> > > boots in sv57 mode which is incompatible with the cpu's sv39 max
> > > capability.
> > >
> > > So add a new bitmap to RISCVSATPMap which contains this capability and
> > > initialize it in every XXX_cpu_init.
> > >
> > > Finally, we have the following chain of constraints:
> > >
> > > Qemu capability > HW capability > User choice > Software capability
> >
> >                                                   ^ What software is this?
> >                          I'd think the user's choice would always be last.
> >
> > >
> > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > ---
> > >  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
> > >  target/riscv/cpu.h |  8 +++--
> > >  2 files changed, 59 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index e409e6ab64..19a37fee2b 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> > >      g_assert_not_reached();
> > >  }
> > >
> > > -/* Sets the satp mode to the max supported */
> > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > > +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> > > +                                        const char *satp_mode_str,
> > > +                                        bool is_32_bit)
> > >  {
> > > -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > > -        cpu->cfg.satp_mode.map |=
> > > -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> > > -    } else {
> > > -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > > +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> > > +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > > +
> > > +    for (int i = 0; i <= satp_mode; ++i) {
> > > +        if (valid_vm[i]) {
> > > +            cpu->cfg.satp_mode.supported |= (1 << i);
> >
> > I don't think we need a new 'supported' bitmap, I think each board that
> > needs to further constrain va-bits from what QEMU supports should just set
> > valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init
> > function something like
> 
> This was my first idea too, but those arrays are global and I have to
> admit that I thought it was possible to emulate a cpu with different
> cores. Anyway, isn't it a bit weird to store this into some global
> array whereas it is intimately linked to the CPU? To me, it makes
> sense to keep those variables as a way to know what qemu is able to
> emulate and have a CPU specific map like in this patch for the hw
> capabilities. Does it make sense to you?

Ah, yes, to support heterogeneous configs it's best to keep this
information per-cpu. I'll take another look at the patch.

> 
> >
> >  #define QEMU_SATP_MODE_MAX VM_1_10_SV64
> >
> >  void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max)
> >  {
> >      bool is_32_bit = cpu->env.misa_mxl == MXL_RV32;
> >      bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> >
> >      g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX);
> >      g_assert(!is_32_bit || satp_mode_max < 2);
> >
> >      memset(valid_vm, 0, sizeof(*valid_vm));
> >
> >      for (int i = 0; i <= satp_mode_max; i++) {
> >          valid_vm[i] = true;
> >      }
> >  }
> >
> > The valid_vm[] checks already in finalize should then manage the
> > validation needed to constrain boards. Only boards that care about
> > this need to call this function, otherwise they'll get the default.
> >
> > Also, this patch should come before the patch that changes the default
> > for all boards to sv57 in order to avoid breaking bisection.
> 
> As I explained earlier, I didn't change the default to sv57! Just
> fixed what was passed via the device tree, which should not be used
> anyway :)

OK, I keep misunderstanding how we're "fixing" something which is
is wrong, but apparently doesn't exhibit any symptoms. So, assuming
it doesn't matter, then I guess it can come anywhere in the series.

Thanks,
drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Alexandre Ghiti]

On Mon, Jan 23, 2023 at 2:31 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 23, 2023 at 12:15:08PM +0100, Alexandre Ghiti wrote:
> > On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> > > > Currently, the max satp mode is set with the only constraint that it must be
> > > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> > > >
> > > > But we actually need to add another level of constraint: what the hw is
> > > > actually capable of, because currently, a linux booting on a sifive-u54
> > > > boots in sv57 mode which is incompatible with the cpu's sv39 max
> > > > capability.
> > > >
> > > > So add a new bitmap to RISCVSATPMap which contains this capability and
> > > > initialize it in every XXX_cpu_init.
> > > >
> > > > Finally, we have the following chain of constraints:
> > > >
> > > > Qemu capability > HW capability > User choice > Software capability
> > >
> > >                                                   ^ What software is this?
> > >                          I'd think the user's choice would always be last.
> > >
> > > >
> > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > ---
> > > >  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
> > > >  target/riscv/cpu.h |  8 +++--
> > > >  2 files changed, 59 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index e409e6ab64..19a37fee2b 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> > > >      g_assert_not_reached();
> > > >  }
> > > >
> > > > -/* Sets the satp mode to the max supported */
> > > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > > > +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> > > > +                                        const char *satp_mode_str,
> > > > +                                        bool is_32_bit)
> > > >  {
> > > > -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > > > -        cpu->cfg.satp_mode.map |=
> > > > -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> > > > -    } else {
> > > > -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > > > +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> > > > +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > > > +
> > > > +    for (int i = 0; i <= satp_mode; ++i) {
> > > > +        if (valid_vm[i]) {
> > > > +            cpu->cfg.satp_mode.supported |= (1 << i);
> > >
> > > I don't think we need a new 'supported' bitmap, I think each board that
> > > needs to further constrain va-bits from what QEMU supports should just set
> > > valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init
> > > function something like
> >
> > This was my first idea too, but those arrays are global and I have to
> > admit that I thought it was possible to emulate a cpu with different
> > cores. Anyway, isn't it a bit weird to store this into some global
> > array whereas it is intimately linked to the CPU? To me, it makes
> > sense to keep those variables as a way to know what qemu is able to
> > emulate and have a CPU specific map like in this patch for the hw
> > capabilities. Does it make sense to you?
>
> Ah, yes, to support heterogeneous configs it's best to keep this
> information per-cpu. I'll take another look at the patch.
>
> >
> > >
> > >  #define QEMU_SATP_MODE_MAX VM_1_10_SV64
> > >
> > >  void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max)
> > >  {
> > >      bool is_32_bit = cpu->env.misa_mxl == MXL_RV32;
> > >      bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > >
> > >      g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX);
> > >      g_assert(!is_32_bit || satp_mode_max < 2);
> > >
> > >      memset(valid_vm, 0, sizeof(*valid_vm));
> > >
> > >      for (int i = 0; i <= satp_mode_max; i++) {
> > >          valid_vm[i] = true;
> > >      }
> > >  }
> > >
> > > The valid_vm[] checks already in finalize should then manage the
> > > validation needed to constrain boards. Only boards that care about
> > > this need to call this function, otherwise they'll get the default.
> > >
> > > Also, this patch should come before the patch that changes the default
> > > for all boards to sv57 in order to avoid breaking bisection.
> >
> > As I explained earlier, I didn't change the default to sv57! Just
> > fixed what was passed via the device tree, which should not be used
> > anyway :)
>
> OK, I keep misunderstanding how we're "fixing" something which is
> is wrong, but apparently doesn't exhibit any symptoms. So, assuming
> it doesn't matter, then I guess it can come anywhere in the series.

Actually *I* think it should not matter, but I can't be sure so I'll
do what you ask.

>
> Thanks,
> drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Alexandre Ghiti]

On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> > Currently, the max satp mode is set with the only constraint that it must be
> > implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> >
> > But we actually need to add another level of constraint: what the hw is
> > actually capable of, because currently, a linux booting on a sifive-u54
> > boots in sv57 mode which is incompatible with the cpu's sv39 max
> > capability.
> >
> > So add a new bitmap to RISCVSATPMap which contains this capability and
> > initialize it in every XXX_cpu_init.
> >
> > Finally, we have the following chain of constraints:
> >
> > Qemu capability > HW capability > User choice > Software capability
>
>                                                   ^ What software is this?
>                          I'd think the user's choice would always be last.

Hmm maybe that's not clear, but I meant that the last constraint was
what the emulated software is capable of handling.

>
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
> >  target/riscv/cpu.h |  8 +++--
> >  2 files changed, 59 insertions(+), 27 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index e409e6ab64..19a37fee2b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> >      g_assert_not_reached();
> >  }
> >
> > -/* Sets the satp mode to the max supported */
> > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> > +                                        const char *satp_mode_str,
> > +                                        bool is_32_bit)
> >  {
> > -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > -        cpu->cfg.satp_mode.map |=
> > -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> > -    } else {
> > -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> > +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +
> > +    for (int i = 0; i <= satp_mode; ++i) {
> > +        if (valid_vm[i]) {
> > +            cpu->cfg.satp_mode.supported |= (1 << i);
>
> I don't think we need a new 'supported' bitmap, I think each board that
> needs to further constrain va-bits from what QEMU supports should just set
> valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init
> function something like
>
>  #define QEMU_SATP_MODE_MAX VM_1_10_SV64
>
>  void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max)
>  {
>      bool is_32_bit = cpu->env.misa_mxl == MXL_RV32;
>      bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
>
>      g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX);
>      g_assert(!is_32_bit || satp_mode_max < 2);
>
>      memset(valid_vm, 0, sizeof(*valid_vm));
>
>      for (int i = 0; i <= satp_mode_max; i++) {
>          valid_vm[i] = true;
>      }
>  }
>
> The valid_vm[] checks already in finalize should then manage the
> validation needed to constrain boards. Only boards that care about
> this need to call this function, otherwise they'll get the default.
>
> Also, this patch should come before the patch that changes the default
> for all boards to sv57 in order to avoid breaking bisection.
>
> Thanks,
> drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Andrew Jones]

On Tue, Jan 24, 2023 at 11:07:53AM +0100, Alexandre Ghiti wrote:
> On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> > > Currently, the max satp mode is set with the only constraint that it must be
> > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> > >
> > > But we actually need to add another level of constraint: what the hw is
> > > actually capable of, because currently, a linux booting on a sifive-u54
> > > boots in sv57 mode which is incompatible with the cpu's sv39 max
> > > capability.
> > >
> > > So add a new bitmap to RISCVSATPMap which contains this capability and
> > > initialize it in every XXX_cpu_init.
> > >
> > > Finally, we have the following chain of constraints:
> > >
> > > Qemu capability > HW capability > User choice > Software capability
> >
> >                                                   ^ What software is this?
> >                          I'd think the user's choice would always be last.
> 
> Hmm maybe that's not clear, but I meant that the last constraint was
> what the emulated software is capable of handling.

Assuming "emulated software" is the guest OS, then OK. How about rewording
as

target/riscv's general satp mode support constrains what the boards
support and the boards constrain what the user may select. The user's
selection will then constrain what's available to the guest OS.

Thanks,
drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Andrew Jones]

On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> Currently, the max satp mode is set with the only constraint that it must be
> implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> 
> But we actually need to add another level of constraint: what the hw is
> actually capable of, because currently, a linux booting on a sifive-u54
> boots in sv57 mode which is incompatible with the cpu's sv39 max
> capability.
> 
> So add a new bitmap to RISCVSATPMap which contains this capability and
> initialize it in every XXX_cpu_init.
> 
> Finally, we have the following chain of constraints:
> 
> Qemu capability > HW capability > User choice > Software capability
> 
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
>  target/riscv/cpu.h |  8 +++--
>  2 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e409e6ab64..19a37fee2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
>      g_assert_not_reached();
>  }
>  
> -/* Sets the satp mode to the max supported */
> -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> +                                        const char *satp_mode_str,
> +                                        bool is_32_bit)

I'd drop 'is_32_bit' and get it from 'cpu', which would "clean up" all the
callsites by getting rid of all the true/false stuff. Also, why take the
string instead of the VM_1_10_SV* define?

>  {
> -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> -        cpu->cfg.satp_mode.map |=
> -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> -    } else {
> -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> +    for (int i = 0; i <= satp_mode; ++i) {
> +        if (valid_vm[i]) {
> +            cpu->cfg.satp_mode.supported |= (1 << i);
> +        }
>      }
>  }
>  
> +/* Sets the satp mode to the max supported */
> +static void set_satp_mode_default(RISCVCPU *cpu)
> +{
> +    uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> +
> +    cpu->cfg.satp_mode.map |= (1 << satp_mode);

Let's do 'cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported' to make
sure 'map' has all supported bits set for property probing.

> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>  #if defined(TARGET_RISCV32)
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> +    set_satp_mode_max_supported(cpu, "sv32", true);
>  #elif defined(TARGET_RISCV64)
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> +    set_satp_mode_max_supported(cpu, "sv57", false);
>  #endif
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>      register_cpu_props(obj);
> @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj)
>  static void rv64_base_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV64, 0);
>      register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
> +    set_satp_mode_max_supported(cpu, "sv57", false);
>  }
>  
>  static void rv64_sifive_u_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
> +    set_satp_mode_max_supported(cpu, "sv39", false);
>  }
>  
>  static void rv64_sifive_e_cpu_init(Object *obj)
> @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", false);
>  }
>  
>  static void rv128_base_cpu_init(Object *obj)
> @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj)
>          exit(EXIT_FAILURE);
>      }
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV128, 0);
>      register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
> +    set_satp_mode_max_supported(cpu, "sv57", false);
>  }
>  #else
>  static void rv32_base_cpu_init(Object *obj)
> @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj)
>      register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
> +    set_satp_mode_max_supported(cpu, "sv32", true);
>  }
>  
>  static void rv32_sifive_u_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
> +    set_satp_mode_max_supported(cpu, "sv32", true);
>  }
>  
>  static void rv32_sifive_e_cpu_init(Object *obj)
> @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", true);
>  }
>  
>  static void rv32_ibex_cpu_init(Object *obj)
> @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj)
>      set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_11_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", true);
>      cpu->cfg.epmp = true;
>  }
>  
> @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", true);
>  }
>  #endif
>  
> @@ -696,7 +727,9 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +    uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> +    uint8_t satp_mode_supported_max =
> +                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
>  
>      if (cpu->cfg.satp_mode.map == 0) {
>          /*
> @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>           * satp mode.
>           */
>          if (cpu->cfg.satp_mode.init == 0) {
> -            set_satp_mode_default(cpu, rv32);
> +            set_satp_mode_default(cpu);
>          } else {
>              /*
>               * Find the lowest level that was disabled and then enable the
> @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>              for (int i = 1; i < 16; ++i) {
>                  if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
>                      (cpu->cfg.satp_mode.init & (1 << i)) &&
> -                    valid_vm[i]) {
> +                    (cpu->cfg.satp_mode.supported & (1 << i))) {
>                      for (int j = i - 1; j >= 0; --j) {
> -                        if (valid_vm[j]) {
> +                        if (cpu->cfg.satp_mode.supported & (1 << j)) {
>                              cpu->cfg.satp_mode.map |= (1 << j);
>                              break;
>                          }
> @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>          }
>      }
>  
> -    /* Make sure the configuration asked is supported by qemu */
> -    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 asked for a supported configuration (HW and qemu) */
> +    if (satp_mode_map_max > satp_mode_supported_max) {
> +        error_setg(errp, "satp_mode %s is higher than hw max capability %s",
> +                   satp_mode_str(satp_mode_map_max, rv32),
> +                   satp_mode_str(satp_mode_supported_max, rv32));
> +        return;
>      }
>  
>      /*
> @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>       * 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) {
> +        for (int i = satp_mode_map_max - 1; i >= 0; --i) {
>              if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
>                  (cpu->cfg.satp_mode.init & (1 << i)) &&
> -                valid_vm[i]) {
> +                (cpu->cfg.satp_mode.supported & (1 << 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));
> +                           satp_mode_str(satp_mode_map_max, false));
>                  return;
>              }
>          }
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index e37177db5c..b591122099 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -416,13 +416,17 @@ struct RISCVCPUClass {
>  
>  /*
>   * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> - * satp mode that is supported.
> + * satp mode that is supported. It may be chosen by the user and must respect
> + * what qemu implements (valid_1_10_32/64) and what the hw is capable of
> + * (supported bitmap below).
>   *
>   * init is a 16-bit bitmap used to make sure the user selected a correct
>   * configuration as per the specification.
> + *
> + * supported is a 16-bit bitmap used to reflect the hw capabilities.
>   */
>  typedef struct {
> -    uint16_t map, init;
> +    uint16_t map, init, supported;
>  } RISCVSATPMap;
>  
>  struct RISCVCPUConfig {
> -- 
> 2.37.2
>

Thanks,
drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Alexandre Ghiti]

On Mon, Jan 23, 2023 at 2:51 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote:
> > Currently, the max satp mode is set with the only constraint that it must be
> > implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> >
> > But we actually need to add another level of constraint: what the hw is
> > actually capable of, because currently, a linux booting on a sifive-u54
> > boots in sv57 mode which is incompatible with the cpu's sv39 max
> > capability.
> >
> > So add a new bitmap to RISCVSATPMap which contains this capability and
> > initialize it in every XXX_cpu_init.
> >
> > Finally, we have the following chain of constraints:
> >
> > Qemu capability > HW capability > User choice > Software capability
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
> >  target/riscv/cpu.h |  8 +++--
> >  2 files changed, 59 insertions(+), 27 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index e409e6ab64..19a37fee2b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> >      g_assert_not_reached();
> >  }
> >
> > -/* Sets the satp mode to the max supported */
> > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> > +                                        const char *satp_mode_str,
> > +                                        bool is_32_bit)
>
> I'd drop 'is_32_bit' and get it from 'cpu', which would "clean up" all the
> callsites by getting rid of all the true/false stuff.

Indeed, better this way

> Also, why take the string instead of the VM_1_10_SV* define?

No particular reason, but I changed it to VM_1_10_SV*, thanks

>
> >  {
> > -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > -        cpu->cfg.satp_mode.map |=
> > -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> > -    } else {
> > -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> > +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +
> > +    for (int i = 0; i <= satp_mode; ++i) {
> > +        if (valid_vm[i]) {
> > +            cpu->cfg.satp_mode.supported |= (1 << i);
> > +        }
> >      }
> >  }
> >
> > +/* Sets the satp mode to the max supported */
> > +static void set_satp_mode_default(RISCVCPU *cpu)
> > +{
> > +    uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> > +
> > +    cpu->cfg.satp_mode.map |= (1 << satp_mode);
>
> Let's do 'cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported' to make
> sure 'map' has all supported bits set for property probing.

Indeed now the map is fully set.

>
> > +}
> > +
> >  static void riscv_any_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >  #if defined(TARGET_RISCV32)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > +    set_satp_mode_max_supported(cpu, "sv32", true);
> >  #elif defined(TARGET_RISCV64)
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > +    set_satp_mode_max_supported(cpu, "sv57", false);
> >  #endif
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> >      register_cpu_props(obj);
> > @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj)
> >  static void rv64_base_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      /* We set this in the realise function */
> >      set_misa(env, MXL_RV64, 0);
> >      register_cpu_props(obj);
> >      /* Set latest version of privileged specification */
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> > +    set_satp_mode_max_supported(cpu, "sv57", false);
> >  }
> >
> >  static void rv64_sifive_u_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> > +    set_satp_mode_max_supported(cpu, "sv39", false);
> >  }
> >
> >  static void rv64_sifive_e_cpu_init(Object *obj)
> > @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", false);
> >  }
> >
> >  static void rv128_base_cpu_init(Object *obj)
> > @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj)
> >          exit(EXIT_FAILURE);
> >      }
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> >      /* We set this in the realise function */
> >      set_misa(env, MXL_RV128, 0);
> >      register_cpu_props(obj);
> >      /* Set latest version of privileged specification */
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> > +    set_satp_mode_max_supported(cpu, "sv57", false);
> >  }
> >  #else
> >  static void rv32_base_cpu_init(Object *obj)
> > @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj)
> >      register_cpu_props(obj);
> >      /* Set latest version of privileged specification */
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> > +    set_satp_mode_max_supported(cpu, "sv32", true);
> >  }
> >
> >  static void rv32_sifive_u_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> > +    set_satp_mode_max_supported(cpu, "sv32", true);
> >  }
> >
> >  static void rv32_sifive_e_cpu_init(Object *obj)
> > @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", true);
> >  }
> >
> >  static void rv32_ibex_cpu_init(Object *obj)
> > @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_11_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", true);
> >      cpu->cfg.epmp = true;
> >  }
> >
> > @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", true);
> >  }
> >  #endif
> >
> > @@ -696,7 +727,9 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +    uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +    uint8_t satp_mode_supported_max =
> > +                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> >
> >      if (cpu->cfg.satp_mode.map == 0) {
> >          /*
> > @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >           * satp mode.
> >           */
> >          if (cpu->cfg.satp_mode.init == 0) {
> > -            set_satp_mode_default(cpu, rv32);
> > +            set_satp_mode_default(cpu);
> >          } else {
> >              /*
> >               * Find the lowest level that was disabled and then enable the
> > @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >              for (int i = 1; i < 16; ++i) {
> >                  if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> >                      (cpu->cfg.satp_mode.init & (1 << i)) &&
> > -                    valid_vm[i]) {
> > +                    (cpu->cfg.satp_mode.supported & (1 << i))) {
> >                      for (int j = i - 1; j >= 0; --j) {
> > -                        if (valid_vm[j]) {
> > +                        if (cpu->cfg.satp_mode.supported & (1 << j)) {
> >                              cpu->cfg.satp_mode.map |= (1 << j);
> >                              break;
> >                          }
> > @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >          }
> >      }
> >
> > -    /* Make sure the configuration asked is supported by qemu */
> > -    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 asked for a supported configuration (HW and qemu) */
> > +    if (satp_mode_map_max > satp_mode_supported_max) {
> > +        error_setg(errp, "satp_mode %s is higher than hw max capability %s",
> > +                   satp_mode_str(satp_mode_map_max, rv32),
> > +                   satp_mode_str(satp_mode_supported_max, rv32));
> > +        return;
> >      }
> >
> >      /*
> > @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >       * 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) {
> > +        for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> >              if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> >                  (cpu->cfg.satp_mode.init & (1 << i)) &&
> > -                valid_vm[i]) {
> > +                (cpu->cfg.satp_mode.supported & (1 << 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));
> > +                           satp_mode_str(satp_mode_map_max, false));
> >                  return;
> >              }
> >          }
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index e37177db5c..b591122099 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -416,13 +416,17 @@ struct RISCVCPUClass {
> >
> >  /*
> >   * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> > - * satp mode that is supported.
> > + * satp mode that is supported. It may be chosen by the user and must respect
> > + * what qemu implements (valid_1_10_32/64) and what the hw is capable of
> > + * (supported bitmap below).
> >   *
> >   * init is a 16-bit bitmap used to make sure the user selected a correct
> >   * configuration as per the specification.
> > + *
> > + * supported is a 16-bit bitmap used to reflect the hw capabilities.
> >   */
> >  typedef struct {
> > -    uint16_t map, init;
> > +    uint16_t map, init, supported;
> >  } RISCVSATPMap;
> >
> >  struct RISCVCPUConfig {
> > --
> > 2.37.2
> >
>
> Thanks,
> drew

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Alistair Francis]

On Mon, Jan 23, 2023 at 7:09 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Currently, the max satp mode is set with the only constraint that it must be
> implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
>
> But we actually need to add another level of constraint: what the hw is
> actually capable of, because currently, a linux booting on a sifive-u54
> boots in sv57 mode which is incompatible with the cpu's sv39 max
> capability.
>
> So add a new bitmap to RISCVSATPMap which contains this capability and
> initialize it in every XXX_cpu_init.
>
> Finally, we have the following chain of constraints:
>
> Qemu capability > HW capability > User choice > Software capability
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
>  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
>  target/riscv/cpu.h |  8 +++--
>  2 files changed, 59 insertions(+), 27 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e409e6ab64..19a37fee2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
>      g_assert_not_reached();
>  }
>
> -/* Sets the satp mode to the max supported */
> -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> +                                        const char *satp_mode_str,
> +                                        bool is_32_bit)
>  {
> -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> -        cpu->cfg.satp_mode.map |=
> -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> -    } else {
> -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> +    for (int i = 0; i <= satp_mode; ++i) {
> +        if (valid_vm[i]) {
> +            cpu->cfg.satp_mode.supported |= (1 << i);
> +        }
>      }
>  }
>
> +/* Sets the satp mode to the max supported */
> +static void set_satp_mode_default(RISCVCPU *cpu)
> +{
> +    uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> +
> +    cpu->cfg.satp_mode.map |= (1 << satp_mode);
> +}
> +
>  static void riscv_any_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>  #if defined(TARGET_RISCV32)
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> +    set_satp_mode_max_supported(cpu, "sv32", true);
>  #elif defined(TARGET_RISCV64)
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> +    set_satp_mode_max_supported(cpu, "sv57", false);
>  #endif
>      set_priv_version(env, PRIV_VERSION_1_12_0);
>      register_cpu_props(obj);
> @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj)
>  static void rv64_base_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV64, 0);
>      register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
> +    set_satp_mode_max_supported(cpu, "sv57", false);
>  }
>
>  static void rv64_sifive_u_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
> +    set_satp_mode_max_supported(cpu, "sv39", false);

Can we just not expose the properties on these vendor CPUs and then
not worry about setting maximums?

Alistair

>  }
>
>  static void rv64_sifive_e_cpu_init(Object *obj)
> @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
>      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", false);
>  }
>
>  static void rv128_base_cpu_init(Object *obj)
> @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj)
>          exit(EXIT_FAILURE);
>      }
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
>      /* We set this in the realise function */
>      set_misa(env, MXL_RV128, 0);
>      register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
> +    set_satp_mode_max_supported(cpu, "sv57", false);
>  }
>  #else
>  static void rv32_base_cpu_init(Object *obj)
> @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj)
>      register_cpu_props(obj);
>      /* Set latest version of privileged specification */
>      set_priv_version(env, PRIV_VERSION_1_12_0);
> +    set_satp_mode_max_supported(cpu, "sv32", true);
>  }
>
>  static void rv32_sifive_u_cpu_init(Object *obj)
>  {
>      CPURISCVState *env = &RISCV_CPU(obj)->env;
> +    RISCVCPU *cpu = RISCV_CPU(obj);
> +
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
> +    set_satp_mode_max_supported(cpu, "sv32", true);
>  }
>
>  static void rv32_sifive_e_cpu_init(Object *obj)
> @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", true);
>  }
>
>  static void rv32_ibex_cpu_init(Object *obj)
> @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj)
>      set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_11_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", true);
>      cpu->cfg.epmp = true;
>  }
>
> @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
>      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
>      set_priv_version(env, PRIV_VERSION_1_10_0);
>      cpu->cfg.mmu = false;
> +    set_satp_mode_max_supported(cpu, "mbare", true);
>  }
>  #endif
>
> @@ -696,7 +727,9 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +    uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> +    uint8_t satp_mode_supported_max =
> +                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
>
>      if (cpu->cfg.satp_mode.map == 0) {
>          /*
> @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>           * satp mode.
>           */
>          if (cpu->cfg.satp_mode.init == 0) {
> -            set_satp_mode_default(cpu, rv32);
> +            set_satp_mode_default(cpu);
>          } else {
>              /*
>               * Find the lowest level that was disabled and then enable the
> @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>              for (int i = 1; i < 16; ++i) {
>                  if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
>                      (cpu->cfg.satp_mode.init & (1 << i)) &&
> -                    valid_vm[i]) {
> +                    (cpu->cfg.satp_mode.supported & (1 << i))) {
>                      for (int j = i - 1; j >= 0; --j) {
> -                        if (valid_vm[j]) {
> +                        if (cpu->cfg.satp_mode.supported & (1 << j)) {
>                              cpu->cfg.satp_mode.map |= (1 << j);
>                              break;
>                          }
> @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>          }
>      }
>
> -    /* Make sure the configuration asked is supported by qemu */
> -    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 asked for a supported configuration (HW and qemu) */
> +    if (satp_mode_map_max > satp_mode_supported_max) {
> +        error_setg(errp, "satp_mode %s is higher than hw max capability %s",
> +                   satp_mode_str(satp_mode_map_max, rv32),
> +                   satp_mode_str(satp_mode_supported_max, rv32));
> +        return;
>      }
>
>      /*
> @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>       * 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) {
> +        for (int i = satp_mode_map_max - 1; i >= 0; --i) {
>              if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
>                  (cpu->cfg.satp_mode.init & (1 << i)) &&
> -                valid_vm[i]) {
> +                (cpu->cfg.satp_mode.supported & (1 << 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));
> +                           satp_mode_str(satp_mode_map_max, false));
>                  return;
>              }
>          }
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index e37177db5c..b591122099 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -416,13 +416,17 @@ struct RISCVCPUClass {
>
>  /*
>   * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> - * satp mode that is supported.
> + * satp mode that is supported. It may be chosen by the user and must respect
> + * what qemu implements (valid_1_10_32/64) and what the hw is capable of
> + * (supported bitmap below).
>   *
>   * init is a 16-bit bitmap used to make sure the user selected a correct
>   * configuration as per the specification.
> + *
> + * supported is a 16-bit bitmap used to reflect the hw capabilities.
>   */
>  typedef struct {
> -    uint16_t map, init;
> +    uint16_t map, init, supported;
>  } RISCVSATPMap;
>
>  struct RISCVCPUConfig {
> --
> 2.37.2
>
>

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities

[Alexandre Ghiti]

Hi Alistair,

On Tue, Jan 24, 2023 at 1:41 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Mon, Jan 23, 2023 at 7:09 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > Currently, the max satp mode is set with the only constraint that it must be
> > implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
> >
> > But we actually need to add another level of constraint: what the hw is
> > actually capable of, because currently, a linux booting on a sifive-u54
> > boots in sv57 mode which is incompatible with the cpu's sv39 max
> > capability.
> >
> > So add a new bitmap to RISCVSATPMap which contains this capability and
> > initialize it in every XXX_cpu_init.
> >
> > Finally, we have the following chain of constraints:
> >
> > Qemu capability > HW capability > User choice > Software capability
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > ---
> >  target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
> >  target/riscv/cpu.h |  8 +++--
> >  2 files changed, 59 insertions(+), 27 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index e409e6ab64..19a37fee2b 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> >      g_assert_not_reached();
> >  }
> >
> > -/* Sets the satp mode to the max supported */
> > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> > +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> > +                                        const char *satp_mode_str,
> > +                                        bool is_32_bit)
> >  {
> > -    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > -        cpu->cfg.satp_mode.map |=
> > -                        (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> > -    } else {
> > -        cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > +    uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> > +    const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +
> > +    for (int i = 0; i <= satp_mode; ++i) {
> > +        if (valid_vm[i]) {
> > +            cpu->cfg.satp_mode.supported |= (1 << i);
> > +        }
> >      }
> >  }
> >
> > +/* Sets the satp mode to the max supported */
> > +static void set_satp_mode_default(RISCVCPU *cpu)
> > +{
> > +    uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> > +
> > +    cpu->cfg.satp_mode.map |= (1 << satp_mode);
> > +}
> > +
> >  static void riscv_any_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >  #if defined(TARGET_RISCV32)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > +    set_satp_mode_max_supported(cpu, "sv32", true);
> >  #elif defined(TARGET_RISCV64)
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > +    set_satp_mode_max_supported(cpu, "sv57", false);
> >  #endif
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> >      register_cpu_props(obj);
> > @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj)
> >  static void rv64_base_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      /* We set this in the realise function */
> >      set_misa(env, MXL_RV64, 0);
> >      register_cpu_props(obj);
> >      /* Set latest version of privileged specification */
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> > +    set_satp_mode_max_supported(cpu, "sv57", false);
> >  }
> >
> >  static void rv64_sifive_u_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> > +    set_satp_mode_max_supported(cpu, "sv39", false);
>
> Can we just not expose the properties on these vendor CPUs and then
> not worry about setting maximums?
>

I'm not sure I understand: the properties are actually not exposed to
the vendor cpus from what I see (no calls to register_cpu_props).

The problem this patch fixes is that the only constraint on satp is
valid_vm_1_10_32/64, which reflects the qemu capabilities: as said in
the commit log, a sifive-u54 will allow a kernel to boot in sv57, and
not sv39 as it should.

This patch only takes advantage of the newly introduced RISCVSATPMap
structure to fix this issue, I should maybe emphasize in the commit
description that this is a fix.

Alex

> Alistair
>
> >  }
> >
> >  static void rv64_sifive_e_cpu_init(Object *obj)
> > @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", false);
> >  }
> >
> >  static void rv128_base_cpu_init(Object *obj)
> > @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj)
> >          exit(EXIT_FAILURE);
> >      }
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> >      /* We set this in the realise function */
> >      set_misa(env, MXL_RV128, 0);
> >      register_cpu_props(obj);
> >      /* Set latest version of privileged specification */
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> > +    set_satp_mode_max_supported(cpu, "sv57", false);
> >  }
> >  #else
> >  static void rv32_base_cpu_init(Object *obj)
> > @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj)
> >      register_cpu_props(obj);
> >      /* Set latest version of privileged specification */
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> > +    set_satp_mode_max_supported(cpu, "sv32", true);
> >  }
> >
> >  static void rv32_sifive_u_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> > +    set_satp_mode_max_supported(cpu, "sv32", true);
> >  }
> >
> >  static void rv32_sifive_e_cpu_init(Object *obj)
> > @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", true);
> >  }
> >
> >  static void rv32_ibex_cpu_init(Object *obj)
> > @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_11_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", true);
> >      cpu->cfg.epmp = true;
> >  }
> >
> > @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> >      cpu->cfg.mmu = false;
> > +    set_satp_mode_max_supported(cpu, "mbare", true);
> >  }
> >  #endif
> >
> > @@ -696,7 +727,9 @@ 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 bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> > +    uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +    uint8_t satp_mode_supported_max =
> > +                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> >
> >      if (cpu->cfg.satp_mode.map == 0) {
> >          /*
> > @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >           * satp mode.
> >           */
> >          if (cpu->cfg.satp_mode.init == 0) {
> > -            set_satp_mode_default(cpu, rv32);
> > +            set_satp_mode_default(cpu);
> >          } else {
> >              /*
> >               * Find the lowest level that was disabled and then enable the
> > @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >              for (int i = 1; i < 16; ++i) {
> >                  if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> >                      (cpu->cfg.satp_mode.init & (1 << i)) &&
> > -                    valid_vm[i]) {
> > +                    (cpu->cfg.satp_mode.supported & (1 << i))) {
> >                      for (int j = i - 1; j >= 0; --j) {
> > -                        if (valid_vm[j]) {
> > +                        if (cpu->cfg.satp_mode.supported & (1 << j)) {
> >                              cpu->cfg.satp_mode.map |= (1 << j);
> >                              break;
> >                          }
> > @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >          }
> >      }
> >
> > -    /* Make sure the configuration asked is supported by qemu */
> > -    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 asked for a supported configuration (HW and qemu) */
> > +    if (satp_mode_map_max > satp_mode_supported_max) {
> > +        error_setg(errp, "satp_mode %s is higher than hw max capability %s",
> > +                   satp_mode_str(satp_mode_map_max, rv32),
> > +                   satp_mode_str(satp_mode_supported_max, rv32));
> > +        return;
> >      }
> >
> >      /*
> > @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> >       * 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) {
> > +        for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> >              if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> >                  (cpu->cfg.satp_mode.init & (1 << i)) &&
> > -                valid_vm[i]) {
> > +                (cpu->cfg.satp_mode.supported & (1 << 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));
> > +                           satp_mode_str(satp_mode_map_max, false));
> >                  return;
> >              }
> >          }
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index e37177db5c..b591122099 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -416,13 +416,17 @@ struct RISCVCPUClass {
> >
> >  /*
> >   * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> > - * satp mode that is supported.
> > + * satp mode that is supported. It may be chosen by the user and must respect
> > + * what qemu implements (valid_1_10_32/64) and what the hw is capable of
> > + * (supported bitmap below).
> >   *
> >   * init is a 16-bit bitmap used to make sure the user selected a correct
> >   * configuration as per the specification.
> > + *
> > + * supported is a 16-bit bitmap used to reflect the hw capabilities.
> >   */
> >  typedef struct {
> > -    uint16_t map, init;
> > +    uint16_t map, init, supported;
> >  } RISCVSATPMap;
> >
> >  struct RISCVCPUConfig {
> > --
> > 2.37.2
> >
> >