* [PATCH v8 0/5] riscv: Allow user to set the satp mode
@ 2023-01-25 16:20 Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState Alexandre Ghiti
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-25 16:20 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
Cc: 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 4).
v8:
- Remove useless !map check, Andrew
- Add RB from Andrew
v7:
- Expand map to contain all valid modes, Andrew
- Fix commit log for patch 3, Andrew
- Remove is_32_bit argument from set_satp_mode_default, Andrew
- Move and fixed comment, Andrew
- Fix satp_mode_map_max in riscv_cpu_satp_mode_finalize which was set
too early, Alex
- Remove is_32_bit argument from set_satp_mode_max_supported, Andrew
- Use satp_mode directly instead of a string in
set_satp_mode_max_supported, Andrew
- Swap the patch introducing supported bitmap and the patch that sets
sv57 in the dt, Andrew
- Add various RB from Andrew and Alistair, thanks
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: Introduce satp mode hw capabilities
riscv: Correctly set the device-tree entry 'mmu-type'
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v8 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState
2023-01-25 16:20 [PATCH v8 0/5] riscv: Allow user to set the satp mode Alexandre Ghiti
@ 2023-01-25 16:20 ` Alexandre Ghiti
2023-01-29 5:54 ` Bin Meng
2023-01-25 16:20 ` [PATCH v8 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool Alexandre Ghiti
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-25 16:20 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
Cc: Alexandre Ghiti
One can extract the DeviceState pointer from the Object pointer, so pass
the Object for future commits to access other fields of Object.
No functional changes intended.
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Frank Chang <frank.chang@sifive.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v8 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool
2023-01-25 16:20 [PATCH v8 0/5] riscv: Allow user to set the satp mode Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState Alexandre Ghiti
@ 2023-01-25 16:20 ` Alexandre Ghiti
2023-01-29 5:55 ` Bin Meng
2023-01-25 16:20 ` [PATCH v8 3/5] riscv: Allow user to set the satp mode Alexandre Ghiti
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-25 16:20 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
Cc: 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].
Suggested-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v8 3/5] riscv: Allow user to set the satp mode
2023-01-25 16:20 [PATCH v8 0/5] riscv: Allow user to set the satp mode Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool Alexandre Ghiti
@ 2023-01-25 16:20 ` Alexandre Ghiti
2023-01-25 16:52 ` Andrew Jones
2023-01-30 4:22 ` Bin Meng
2023-01-25 16:20 ` [PATCH v8 4/5] riscv: Introduce satp mode hw capabilities Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 5/5] riscv: Correctly set the device-tree entry 'mmu-type' Alexandre Ghiti
4 siblings, 2 replies; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-25 16:20 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
Cc: Alexandre Ghiti, Ludovic Henry
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,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
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 | 206 +++++++++++++++++++++++++++++++++++++++++++++
target/riscv/cpu.h | 19 +++++
target/riscv/csr.c | 12 ++-
3 files changed, 230 insertions(+), 7 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 7181b34f86..54494a72be 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,81 @@ 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 rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
+
+ if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
+ cpu->cfg.satp_mode.map |=
+ (1 << satp_mode_from_str(rv32 ? "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 +695,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;
+ uint8_t satp_mode_max;
+
+ if (cpu->cfg.satp_mode.map == 0) {
+ if (cpu->cfg.satp_mode.init == 0) {
+ /* If unset by the user, we fallback to the default satp mode. */
+ set_satp_mode_default(cpu);
+ } 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.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.
+ */
+ satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
+
+ if (!rv32) {
+ 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;
+ }
+ }
+ }
+
+ /* Finally expand the map so that all valid modes are set */
+ for (int i = satp_mode_max - 1; i >= 0; --i) {
+ cpu->cfg.satp_mode.map |= (1 << i);
+ }
+}
+
+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 +1071,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 +1085,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 +1295,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v8 4/5] riscv: Introduce satp mode hw capabilities
2023-01-25 16:20 [PATCH v8 0/5] riscv: Allow user to set the satp mode Alexandre Ghiti
` (2 preceding siblings ...)
2023-01-25 16:20 ` [PATCH v8 3/5] riscv: Allow user to set the satp mode Alexandre Ghiti
@ 2023-01-25 16:20 ` Alexandre Ghiti
2023-01-30 4:29 ` Bin Meng
2023-01-25 16:20 ` [PATCH v8 5/5] riscv: Correctly set the device-tree entry 'mmu-type' Alexandre Ghiti
4 siblings, 1 reply; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-25 16:20 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
Cc: 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:
- valid_vm_1_10_[32|64] constrains which satp mode the CPU can use
- the CPU hw capabilities constrains what the user may select
- the user's selection then constrains what's available to the guest
OS.
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
target/riscv/cpu.c | 74 +++++++++++++++++++++++++++++++---------------
target/riscv/cpu.h | 8 +++--
2 files changed, 56 insertions(+), 26 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 54494a72be..e7e1fb96dc 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -292,26 +292,36 @@ 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)
+static void set_satp_mode_max_supported(RISCVCPU *cpu,
+ uint8_t satp_mode)
{
bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
+ const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
- if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
- cpu->cfg.satp_mode.map |=
- (1 << satp_mode_from_str(rv32 ? "sv32" : "sv57"));
- } else {
- cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
+ 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)
+{
+ cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported;
+}
+
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, VM_1_10_SV32);
#elif defined(TARGET_RISCV64)
set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
+ set_satp_mode_max_supported(cpu, VM_1_10_SV57);
#endif
set_priv_version(env, PRIV_VERSION_1_12_0);
register_cpu_props(obj);
@@ -321,18 +331,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, VM_1_10_SV57);
}
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, VM_1_10_SV39);
}
static void rv64_sifive_e_cpu_init(Object *obj)
@@ -343,6 +359,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, VM_1_10_MBARE);
}
static void rv128_base_cpu_init(Object *obj)
@@ -354,11 +371,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, VM_1_10_SV57);
}
#else
static void rv32_base_cpu_init(Object *obj)
@@ -369,13 +388,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, VM_1_10_SV32);
}
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, VM_1_10_SV32);
}
static void rv32_sifive_e_cpu_init(Object *obj)
@@ -386,6 +409,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, VM_1_10_MBARE);
}
static void rv32_ibex_cpu_init(Object *obj)
@@ -396,6 +420,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, VM_1_10_MBARE);
cpu->cfg.epmp = true;
}
@@ -407,6 +432,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, VM_1_10_MBARE);
}
#endif
@@ -698,8 +724,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_max;
+ uint8_t satp_mode_map_max;
+ uint8_t satp_mode_supported_max =
+ satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
if (cpu->cfg.satp_mode.map == 0) {
if (cpu->cfg.satp_mode.init == 0) {
@@ -713,9 +740,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
*/
for (int i = 1; i < 16; ++i) {
if ((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;
}
@@ -726,36 +753,35 @@ 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;
- }
+ satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
+
+ /* 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;
}
/*
* Make sure the user did not ask for an invalid configuration as per
* the specification.
*/
- satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
-
if (!rv32) {
- 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;
}
}
}
/* Finally expand the map so that all valid modes are set */
- for (int i = satp_mode_max - 1; i >= 0; --i) {
+ for (int i = satp_mode_map_max - 1; i >= 0; --i) {
cpu->cfg.satp_mode.map |= (1 << i);
}
}
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v8 5/5] riscv: Correctly set the device-tree entry 'mmu-type'
2023-01-25 16:20 [PATCH v8 0/5] riscv: Allow user to set the satp mode Alexandre Ghiti
` (3 preceding siblings ...)
2023-01-25 16:20 ` [PATCH v8 4/5] riscv: Introduce satp mode hw capabilities Alexandre Ghiti
@ 2023-01-25 16:20 ` Alexandre Ghiti
2023-01-30 4:30 ` Bin Meng
4 siblings, 1 reply; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-25 16:20 UTC (permalink / raw)
To: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
Cc: 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>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v8 3/5] riscv: Allow user to set the satp mode
2023-01-25 16:20 ` [PATCH v8 3/5] riscv: Allow user to set the satp mode Alexandre Ghiti
@ 2023-01-25 16:52 ` Andrew Jones
2023-01-31 13:04 ` Alexandre Ghiti
2023-01-30 4:22 ` Bin Meng
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Jones @ 2023-01-25 16:52 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Frank Chang,
qemu-riscv, qemu-devel, Ludovic Henry
On Wed, Jan 25, 2023 at 05:20:08PM +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,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
>
> 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 | 206 +++++++++++++++++++++++++++++++++++++++++++++
> target/riscv/cpu.h | 19 +++++
> target/riscv/csr.c | 12 ++-
> 3 files changed, 230 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7181b34f86..54494a72be 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,81 @@ 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 rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> +
> + if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> + cpu->cfg.satp_mode.map |=
> + (1 << satp_mode_from_str(rv32 ? "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 +695,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;
> + uint8_t satp_mode_max;
> +
> + if (cpu->cfg.satp_mode.map == 0) {
> + if (cpu->cfg.satp_mode.init == 0) {
> + /* If unset by the user, we fallback to the default satp mode. */
> + set_satp_mode_default(cpu);
> + } 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.init & (1 << i)) &&
> + valid_vm[i]) {
nit: Could have brought this valid_vm[i] up now that everything fits on
one line.
> + 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.
> + */
> + satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> +
> + if (!rv32) {
> + 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;
> + }
> + }
> + }
> +
> + /* Finally expand the map so that all valid modes are set */
> + for (int i = satp_mode_max - 1; i >= 0; --i) {
> + cpu->cfg.satp_mode.map |= (1 << i);
> + }
> +}
> +
> +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 +1071,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 +1085,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 +1295,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
>
Other than the minor nit
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Thanks,
drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState
2023-01-25 16:20 ` [PATCH v8 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState Alexandre Ghiti
@ 2023-01-29 5:54 ` Bin Meng
0 siblings, 0 replies; 15+ messages in thread
From: Bin Meng @ 2023-01-29 5:54 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
On Thu, Jan 26, 2023 at 12:21 AM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> One can extract the DeviceState pointer from the Object pointer, so pass
> the Object for future commits to access other fields of Object.
>
> No functional changes intended.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Frank Chang <frank.chang@sifive.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
> target/riscv/cpu.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool
2023-01-25 16:20 ` [PATCH v8 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool Alexandre Ghiti
@ 2023-01-29 5:55 ` Bin Meng
0 siblings, 0 replies; 15+ messages in thread
From: Bin Meng @ 2023-01-29 5:55 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
On Thu, Jan 26, 2023 at 12:22 AM 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].
>
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/csr.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 3/5] riscv: Allow user to set the satp mode
2023-01-25 16:20 ` [PATCH v8 3/5] riscv: Allow user to set the satp mode Alexandre Ghiti
2023-01-25 16:52 ` Andrew Jones
@ 2023-01-30 4:22 ` Bin Meng
2023-01-31 13:20 ` Alexandre Ghiti
1 sibling, 1 reply; 15+ messages in thread
From: Bin Meng @ 2023-01-30 4:22 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel, Ludovic Henry
On Thu, Jan 26, 2023 at 12:23 AM Alexandre Ghiti <alexghiti@rivosinc.com> 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,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
>
> 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 | 206 +++++++++++++++++++++++++++++++++++++++++++++
> target/riscv/cpu.h | 19 +++++
> target/riscv/csr.c | 12 ++-
> 3 files changed, 230 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7181b34f86..54494a72be 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,81 @@ 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)
This function is better named as set_satp_mode_default_map
> +{
> + bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> +
> + if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> + cpu->cfg.satp_mode.map |=
> + (1 << satp_mode_from_str(rv32 ? "sv32" : "sv57"));
> + } else {
> + cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> + }
I believe the "mbare" bit should always be set, so this can be:
cpu->cfg.satp_mode.map = 1 << satp_mode_from_str("mbare");
if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
cpu->cfg.satp_mode.map |=
(1 << satp_mode_from_str(rv32 ? "sv32" : "sv57"));
> +}
> +
> static void riscv_any_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> @@ -619,6 +695,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;
> + uint8_t satp_mode_max;
> +
> + if (cpu->cfg.satp_mode.map == 0) {
> + if (cpu->cfg.satp_mode.init == 0) {
> + /* If unset by the user, we fallback to the default satp mode. */
> + set_satp_mode_default(cpu);
> + } 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.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.
> + */
> + satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> +
> + if (!rv32) {
> + 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;
> + }
> + }
> + }
> +
> + /* Finally expand the map so that all valid modes are set */
> + for (int i = satp_mode_max - 1; i >= 0; --i) {
> + cpu->cfg.satp_mode.map |= (1 << i);
This blindly expands the map regardless whether it's a valid mode.
> + }
> +}
> +
> +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 +1071,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 +1085,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)
nits: should align to (
> +{
> + RISCVSATPMap *satp_map = opaque;
> + uint8_t satp = satp_mode_from_str(name);
> + bool value;
> +
> + value = (satp_map->map & (1 << satp));
nits: remove the outer ()
> +
> + visit_type_bool(v, name, &value, errp);
> +}
> +
> +static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
ditto
> +{
> + 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 +1295,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));
nits: remove the outer ()
> }
>
> static RISCVException write_mstatus(CPURISCVState *env, int csrno,
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 4/5] riscv: Introduce satp mode hw capabilities
2023-01-25 16:20 ` [PATCH v8 4/5] riscv: Introduce satp mode hw capabilities Alexandre Ghiti
@ 2023-01-30 4:29 ` Bin Meng
2023-01-31 13:29 ` Alexandre Ghiti
0 siblings, 1 reply; 15+ messages in thread
From: Bin Meng @ 2023-01-30 4:29 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
On Thu, Jan 26, 2023 at 12:24 AM 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].
nits: s/qemu/QEMU/g
>
> 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:
> - valid_vm_1_10_[32|64] constrains which satp mode the CPU can use
> - the CPU hw capabilities constrains what the user may select
> - the user's selection then constrains what's available to the guest
> OS.
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> ---
> target/riscv/cpu.c | 74 +++++++++++++++++++++++++++++++---------------
> target/riscv/cpu.h | 8 +++--
> 2 files changed, 56 insertions(+), 26 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 54494a72be..e7e1fb96dc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -292,26 +292,36 @@ 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)
> +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> + uint8_t satp_mode)
> {
> bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> + const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
>
> - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> - cpu->cfg.satp_mode.map |=
> - (1 << satp_mode_from_str(rv32 ? "sv32" : "sv57"));
> - } else {
> - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> + 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 */
nits: s/Sets/Set
> +static void set_satp_mode_default(RISCVCPU *cpu)
> +{
> + cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported;
> +}
> +
> 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, VM_1_10_SV32);
> #elif defined(TARGET_RISCV64)
> set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> + set_satp_mode_max_supported(cpu, VM_1_10_SV57);
> #endif
> set_priv_version(env, PRIV_VERSION_1_12_0);
> register_cpu_props(obj);
> @@ -321,18 +331,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, VM_1_10_SV57);
> }
>
> 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, VM_1_10_SV39);
> }
>
> static void rv64_sifive_e_cpu_init(Object *obj)
> @@ -343,6 +359,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, VM_1_10_MBARE);
> }
>
> static void rv128_base_cpu_init(Object *obj)
> @@ -354,11 +371,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, VM_1_10_SV57);
> }
> #else
> static void rv32_base_cpu_init(Object *obj)
> @@ -369,13 +388,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, VM_1_10_SV32);
Does this compile? 'cpu' seems undeclared ..?
> }
>
> 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, VM_1_10_SV32);
> }
>
> static void rv32_sifive_e_cpu_init(Object *obj)
> @@ -386,6 +409,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, VM_1_10_MBARE);
> }
>
> static void rv32_ibex_cpu_init(Object *obj)
> @@ -396,6 +420,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, VM_1_10_MBARE);
> cpu->cfg.epmp = true;
> }
>
> @@ -407,6 +432,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, VM_1_10_MBARE);
> }
> #endif
>
> @@ -698,8 +724,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_max;
> + uint8_t satp_mode_map_max;
> + uint8_t satp_mode_supported_max =
> + satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
>
> if (cpu->cfg.satp_mode.map == 0) {
> if (cpu->cfg.satp_mode.init == 0) {
> @@ -713,9 +740,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> */
> for (int i = 1; i < 16; ++i) {
> if ((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;
> }
> @@ -726,36 +753,35 @@ 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;
> - }
> + satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> +
> + /* 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;
> }
>
> /*
> * Make sure the user did not ask for an invalid configuration as per
> * the specification.
> */
> - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> -
> if (!rv32) {
> - 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;
> }
> }
> }
>
> /* Finally expand the map so that all valid modes are set */
> - for (int i = satp_mode_max - 1; i >= 0; --i) {
> + for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> cpu->cfg.satp_mode.map |= (1 << i);
> }
> }
> 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 {
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 5/5] riscv: Correctly set the device-tree entry 'mmu-type'
2023-01-25 16:20 ` [PATCH v8 5/5] riscv: Correctly set the device-tree entry 'mmu-type' Alexandre Ghiti
@ 2023-01-30 4:30 ` Bin Meng
0 siblings, 0 replies; 15+ messages in thread
From: Bin Meng @ 2023-01-30 4:30 UTC (permalink / raw)
To: Alexandre Ghiti
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
On Thu, Jan 26, 2023 at 12:25 AM 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: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> hw/riscv/virt.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 3/5] riscv: Allow user to set the satp mode
2023-01-25 16:52 ` Andrew Jones
@ 2023-01-31 13:04 ` Alexandre Ghiti
0 siblings, 0 replies; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-31 13:04 UTC (permalink / raw)
To: Andrew Jones
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Frank Chang,
qemu-riscv, qemu-devel, Ludovic Henry
On Wed, Jan 25, 2023 at 5:52 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Jan 25, 2023 at 05:20:08PM +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,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
> >
> > 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 | 206 +++++++++++++++++++++++++++++++++++++++++++++
> > target/riscv/cpu.h | 19 +++++
> > target/riscv/csr.c | 12 ++-
> > 3 files changed, 230 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7181b34f86..54494a72be 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,81 @@ 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 rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +
> > + if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > + cpu->cfg.satp_mode.map |=
> > + (1 << satp_mode_from_str(rv32 ? "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 +695,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;
> > + uint8_t satp_mode_max;
> > +
> > + if (cpu->cfg.satp_mode.map == 0) {
> > + if (cpu->cfg.satp_mode.init == 0) {
> > + /* If unset by the user, we fallback to the default satp mode. */
> > + set_satp_mode_default(cpu);
> > + } 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.init & (1 << i)) &&
> > + valid_vm[i]) {
>
> nit: Could have brought this valid_vm[i] up now that everything fits on
> one line.
>
Fixed in v9, thanks.
> > + 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.
> > + */
> > + satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +
> > + if (!rv32) {
> > + 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;
> > + }
> > + }
> > + }
> > +
> > + /* Finally expand the map so that all valid modes are set */
> > + for (int i = satp_mode_max - 1; i >= 0; --i) {
> > + cpu->cfg.satp_mode.map |= (1 << i);
> > + }
> > +}
> > +
> > +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 +1071,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 +1085,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 +1295,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
> >
>
> Other than the minor nit
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
\o/
Thanks for all your reviews Andrew,
Alex
>
> Thanks,
> drew
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 3/5] riscv: Allow user to set the satp mode
2023-01-30 4:22 ` Bin Meng
@ 2023-01-31 13:20 ` Alexandre Ghiti
0 siblings, 0 replies; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-31 13:20 UTC (permalink / raw)
To: Bin Meng
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel, Ludovic Henry
Hi Bin,
On Mon, Jan 30, 2023 at 5:22 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Jan 26, 2023 at 12:23 AM Alexandre Ghiti <alexghiti@rivosinc.com> 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,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
> >
> > 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 | 206 +++++++++++++++++++++++++++++++++++++++++++++
> > target/riscv/cpu.h | 19 +++++
> > target/riscv/csr.c | 12 ++-
> > 3 files changed, 230 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7181b34f86..54494a72be 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,81 @@ 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)
>
> This function is better named as set_satp_mode_default_map
>
Indeed, fixed in v9.
> > +{
> > + bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +
> > + if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > + cpu->cfg.satp_mode.map |=
> > + (1 << satp_mode_from_str(rv32 ? "sv32" : "sv57"));
> > + } else {
> > + cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > + }
>
> I believe the "mbare" bit should always be set, so this can be:
>
> cpu->cfg.satp_mode.map = 1 << satp_mode_from_str("mbare");
> if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> cpu->cfg.satp_mode.map |=
> (1 << satp_mode_from_str(rv32 ? "sv32" : "sv57"));
Actually, at the end of riscv_cpu_satp_mode_finalize, the map is
"expanded" so that all supported modes are set in the bitmap.
>
> > +}
> > +
> > static void riscv_any_cpu_init(Object *obj)
> > {
> > CPURISCVState *env = &RISCV_CPU(obj)->env;
> > @@ -619,6 +695,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;
> > + uint8_t satp_mode_max;
> > +
> > + if (cpu->cfg.satp_mode.map == 0) {
> > + if (cpu->cfg.satp_mode.init == 0) {
> > + /* If unset by the user, we fallback to the default satp mode. */
> > + set_satp_mode_default(cpu);
> > + } 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.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.
> > + */
> > + satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +
> > + if (!rv32) {
> > + 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;
> > + }
> > + }
> > + }
> > +
> > + /* Finally expand the map so that all valid modes are set */
> > + for (int i = satp_mode_max - 1; i >= 0; --i) {
> > + cpu->cfg.satp_mode.map |= (1 << i);
>
> This blindly expands the map regardless whether it's a valid mode.
Oh, indeed, good catch, fixed in v9.
>
> > + }
> > +}
> > +
> > +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 +1071,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 +1085,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)
>
> nits: should align to (
Ok
>
> > +{
> > + RISCVSATPMap *satp_map = opaque;
> > + uint8_t satp = satp_mode_from_str(name);
> > + bool value;
> > +
> > + value = (satp_map->map & (1 << satp));
>
> nits: remove the outer ()
Ok
>
> > +
> > + visit_type_bool(v, name, &value, errp);
> > +}
> > +
> > +static void cpu_riscv_set_satp(Object *obj, Visitor *v, const char *name,
> > + void *opaque, Error **errp)
>
> ditto
Ok
>
> > +{
> > + 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 +1295,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));
>
> nits: remove the outer ()
Ok
>
> > }
> >
> > static RISCVException write_mstatus(CPURISCVState *env, int csrno,
> > --
>
> Regards,
> Bin
Thanks!
Alex
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v8 4/5] riscv: Introduce satp mode hw capabilities
2023-01-30 4:29 ` Bin Meng
@ 2023-01-31 13:29 ` Alexandre Ghiti
0 siblings, 0 replies; 15+ messages in thread
From: Alexandre Ghiti @ 2023-01-31 13:29 UTC (permalink / raw)
To: Bin Meng
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Andrew Jones,
Frank Chang, qemu-riscv, qemu-devel
On Mon, Jan 30, 2023 at 5:29 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Jan 26, 2023 at 12:24 AM 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].
>
> nits: s/qemu/QEMU/g
Ok
>
> >
> > 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:
> > - valid_vm_1_10_[32|64] constrains which satp mode the CPU can use
> > - the CPU hw capabilities constrains what the user may select
> > - the user's selection then constrains what's available to the guest
> > OS.
> >
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> > target/riscv/cpu.c | 74 +++++++++++++++++++++++++++++++---------------
> > target/riscv/cpu.h | 8 +++--
> > 2 files changed, 56 insertions(+), 26 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 54494a72be..e7e1fb96dc 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -292,26 +292,36 @@ 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)
> > +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> > + uint8_t satp_mode)
> > {
> > bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > + const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> >
> > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > - cpu->cfg.satp_mode.map |=
> > - (1 << satp_mode_from_str(rv32 ? "sv32" : "sv57"));
> > - } else {
> > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> > + 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 */
>
> nits: s/Sets/Set
Ok
>
> > +static void set_satp_mode_default(RISCVCPU *cpu)
> > +{
> > + cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported;
> > +}
> > +
> > 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, VM_1_10_SV32);
> > #elif defined(TARGET_RISCV64)
> > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > + set_satp_mode_max_supported(cpu, VM_1_10_SV57);
> > #endif
> > set_priv_version(env, PRIV_VERSION_1_12_0);
> > register_cpu_props(obj);
> > @@ -321,18 +331,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, VM_1_10_SV57);
> > }
> >
> > 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, VM_1_10_SV39);
> > }
> >
> > static void rv64_sifive_e_cpu_init(Object *obj)
> > @@ -343,6 +359,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, VM_1_10_MBARE);
> > }
> >
> > static void rv128_base_cpu_init(Object *obj)
> > @@ -354,11 +371,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, VM_1_10_SV57);
> > }
> > #else
> > static void rv32_base_cpu_init(Object *obj)
> > @@ -369,13 +388,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, VM_1_10_SV32);
>
> Does this compile? 'cpu' seems undeclared ..?
Oops, thanks.
>
> > }
> >
> > 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, VM_1_10_SV32);
> > }
> >
> > static void rv32_sifive_e_cpu_init(Object *obj)
> > @@ -386,6 +409,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, VM_1_10_MBARE);
> > }
> >
> > static void rv32_ibex_cpu_init(Object *obj)
> > @@ -396,6 +420,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, VM_1_10_MBARE);
> > cpu->cfg.epmp = true;
> > }
> >
> > @@ -407,6 +432,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, VM_1_10_MBARE);
> > }
> > #endif
> >
> > @@ -698,8 +724,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_max;
> > + uint8_t satp_mode_map_max;
> > + uint8_t satp_mode_supported_max =
> > + satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> >
> > if (cpu->cfg.satp_mode.map == 0) {
> > if (cpu->cfg.satp_mode.init == 0) {
> > @@ -713,9 +740,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> > */
> > for (int i = 1; i < 16; ++i) {
> > if ((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;
> > }
> > @@ -726,36 +753,35 @@ 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;
> > - }
> > + satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > +
> > + /* 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;
> > }
> >
> > /*
> > * Make sure the user did not ask for an invalid configuration as per
> > * the specification.
> > */
> > - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> > -
> > if (!rv32) {
> > - 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;
> > }
> > }
> > }
> >
> > /* Finally expand the map so that all valid modes are set */
> > - for (int i = satp_mode_max - 1; i >= 0; --i) {
> > + for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> > cpu->cfg.satp_mode.map |= (1 << i);
> > }
> > }
> > 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 {
> > --
>
> Regards,
> Bin
Thanks again,
Alex
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-01-31 15:36 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-25 16:20 [PATCH v8 0/5] riscv: Allow user to set the satp mode Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState Alexandre Ghiti
2023-01-29 5:54 ` Bin Meng
2023-01-25 16:20 ` [PATCH v8 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool Alexandre Ghiti
2023-01-29 5:55 ` Bin Meng
2023-01-25 16:20 ` [PATCH v8 3/5] riscv: Allow user to set the satp mode Alexandre Ghiti
2023-01-25 16:52 ` Andrew Jones
2023-01-31 13:04 ` Alexandre Ghiti
2023-01-30 4:22 ` Bin Meng
2023-01-31 13:20 ` Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 4/5] riscv: Introduce satp mode hw capabilities Alexandre Ghiti
2023-01-30 4:29 ` Bin Meng
2023-01-31 13:29 ` Alexandre Ghiti
2023-01-25 16:20 ` [PATCH v8 5/5] riscv: Correctly set the device-tree entry 'mmu-type' Alexandre Ghiti
2023-01-30 4:30 ` Bin Meng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.