On Fri, Dec 16, 2022 at 2:03 PM Alexandre Ghiti <alexgh...@rivosinc.com> wrote: > > Hi Frank, > > On Fri, Dec 16, 2022 at 10:32 AM Frank Chang <frank.ch...@sifive.com> wrote: > > > > Hi Alexandre, > > > > Thanks for the contribution. This is really helpful. > > > > It seems like if we want to specify the SATP mode for the "named" CPUs, > > we have to do, e.g.: > > cpu->cfg.satp_mode.map |= (1 << idx_satp_mode_from_str("sv39")); > > in each CPU's init function. > > > > Can we add another helper function to wrap this for the "named" CPUs? > > Yes sure, I'll add some helpers for the bit operations in general, > that will be cleaner. And I'll set the default satp mode for the > current cpus in each cpu init function too. > > Thanks for your remarks, > > Alex > > > > > Regards, > > Frank Chang > > > > > > On Mon, Dec 12, 2022 at 6:23 PM Alexandre Ghiti <alexgh...@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 "mbare", "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 > >> > >> We take the highest level set by the user: > >> -cpu rv64,sv48=on,sv57=on # Linux will boot using sv57 scheme > >> > >> We make sure that invalid configurations are rejected: > >> -cpu rv64,sv32=on # Can't enable 32-bit satp mode in 64-bit > >> -cpu rv64,sv39=off,sv48=on # sv39 must be supported if higher modes are > >> # enabled > >> > >> We accept "redundant" configurations: > >> -cpu rv64,sv48=on,sv57=off # sv39 must be supported if higher modes are > >> > >> In addition, we now correctly set the device-tree entry 'mmu-type' using > >> those new properties. > >> > >> Co-Developed-by: Ludovic Henry <ludo...@rivosinc.com> > >> Signed-off-by: Ludovic Henry <ludo...@rivosinc.com> > >> Signed-off-by: Alexandre Ghiti <alexgh...@rivosinc.com> > >> --- > >> 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 > >> > >> hw/riscv/virt.c | 20 +++-- > >> target/riscv/cpu.c | 217 +++++++++++++++++++++++++++++++++++++++++++-- > >> target/riscv/cpu.h | 25 ++++++ > >> target/riscv/csr.c | 13 ++- > >> 4 files changed, 256 insertions(+), 19 deletions(-) > >> > >> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > >> index a5bc7353b4..9bb5ba7366 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,15 @@ 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, > >> + is_32_bit); > >> + sv_name = g_strdup_printf("riscv,%s", > >> + satp_mode_str(satp_mode_max, > >> is_32_bit)); > >> + qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type", sv_name); > >> + g_free(sv_name); > >> + > >> name = riscv_isa_string(&s->soc[socket].harts[cpu]); > >> qemu_fdt_setprop_string(mc->fdt, cpu_name, "riscv,isa", name); > >> g_free(name); > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > >> index d14e95c9dc..639231ce2e 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" > >> @@ -199,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) > >> { > >> @@ -237,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) > >> @@ -246,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); > >> } > >> @@ -279,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); > >> } > >> @@ -289,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); > >> } > >> @@ -342,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 > >> > >> @@ -612,6 +613,71 @@ static void riscv_cpu_disas_set_info(CPUState *s, > >> disassemble_info *info) > >> } > >> } > >> > >> +#define OFFSET_SATP_MODE_64 16 > >> + > >> +static uint8_t idx_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 + OFFSET_SATP_MODE_64; > >> + } > >> + > >> + if (!strncmp(satp_mode_str, "sv48", 4)) { > >> + return VM_1_10_SV48 + OFFSET_SATP_MODE_64; > >> + } > >> + > >> + if (!strncmp(satp_mode_str, "sv57", 4)) { > >> + return VM_1_10_SV57 + OFFSET_SATP_MODE_64; > >> + } > >> + > >> + if (!strncmp(satp_mode_str, "sv64", 4)) { > >> + return VM_1_10_SV64 + OFFSET_SATP_MODE_64; > >> + } > >> + > >> + /* Will never get there */ > >> + return -1; > >> +} > >> + > >> +uint8_t satp_mode_max_from_map(uint32_t map, bool is_32_bit) > >> +{ > >> + return is_32_bit ? > >> + (31 - __builtin_clz(map & 0xFFFF)) : (31 - __builtin_clz(map >> > >> 16)); > >> +} > >> + > >> +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"; > >> + } > >> + } > >> + > >> + return NULL; > >> +} > >> + > >> static void riscv_cpu_realize(DeviceState *dev, Error **errp) > >> { > >> CPUState *cs = CPU(dev); > >> @@ -907,6 +973,30 @@ static void riscv_cpu_realize(DeviceState *dev, Error > >> **errp) > >> } > >> #endif > >> > >> + bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; > >> + > >> + /* > >> + * If unset by both the user and the cpu, we fallback to sv32 for > >> 32-bit > >> + * or sv57 for 64-bit when a MMU is present, and bare otherwise. > >> + */ > >> + if (cpu->cfg.satp_mode.map == 0) { > >> + if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > >> + if (rv32) { > >> + cpu->cfg.satp_mode.map |= (1 << > >> idx_satp_mode_from_str("sv32")); > >> + } else { > >> + cpu->cfg.satp_mode.map |= (1 << > >> idx_satp_mode_from_str("sv57")); > >> + } > >> + } else { > >> + cpu->cfg.satp_mode.map |= (1 << > >> idx_satp_mode_from_str("mbare")); > >> + } > >> + } > >> + > >> + 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); > >> @@ -915,6 +1005,115 @@ 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 idx_satp = idx_satp_mode_from_str(name); > >> + bool value; > >> + > >> + value = (satp_map->map & (1 << idx_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 idx_satp = idx_satp_mode_from_str(name); > >> + bool value; > >> + > >> + if (!visit_type_bool(v, name, &value, errp)) { > >> + return; > >> + } > >> + > >> + if (value) { > >> + satp_map->map |= 1 << idx_satp; > >> + } > >> + > >> + satp_map->init |= 1 << idx_satp; > >> +} > >> + > >> +static void riscv_add_satp_mode_properties(Object *obj) > >> +{ > >> + RISCVCPU *cpu = RISCV_CPU(obj); > >> + > >> + object_property_add(obj, "mbare", "bool", cpu_riscv_get_satp, > >> + cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode); > >> + object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp, > >> + cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode); > >> + object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp, > >> + cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode); > >> + object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp, > >> + cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode); > >> + object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp, > >> + cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode); > >> + object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp, > >> + cpu_riscv_set_satp, NULL, &cpu->cfg.satp_mode); > >> +} > >> + > >> +#define error_append_or_setg(errp, str, ...) ({ \ > >> + if (*errp) \ > >> + error_append_hint(errp, str"\n", ##__VA_ARGS__);\ > >> + else \ > >> + error_setg(errp, str, ##__VA_ARGS__); \ > >> + }) > >> + > >> +void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > >> +{ > >> + bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; > >> + > >> + /* Get rid of 32-bit/64-bit incompatibility */ > >> + if (rv32) { > >> + if (cpu->cfg.satp_mode.map >= (1 << OFFSET_SATP_MODE_64)) > >> + error_append_or_setg(errp, "cannot enable 64-bit satp modes " > >> + "(sv39/sv48/sv57/sv64) if cpu is in > >> 32-bit " > >> + "mode"); > >> + } else { > >> + if (cpu->cfg.satp_mode.map & (1 << VM_1_10_SV32)) > >> + error_append_or_setg(errp, "cannot enable 32-bit satp mode > >> (sv32) " > >> + "if cpu is in 64-bit mode"); > >> + } > >> + > >> + /* > >> + * Then make sure the user did not ask for an invalid configuration > >> as per > >> + * the specification. > >> + */ > >> + if (rv32) { > >> + if (cpu->cfg.satp_mode.map & (1 << VM_1_10_SV32)) { > >> + if (!(cpu->cfg.satp_mode.map & (1 << VM_1_10_MBARE)) && > >> + (cpu->cfg.satp_mode.init & (1 << VM_1_10_MBARE))) > >> + error_append_or_setg(errp, "cannot disable mbare satp > >> mode if " > >> + "sv32 is enabled"); > >> + } > >> + } else { > >> + uint8_t satp_mode_max; > >> + > >> + satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map, > >> false); > >> + > >> + for (int i = satp_mode_max - 1; i >= 0; --i) { > >> + if (!(cpu->cfg.satp_mode.map & (1 << (i + > >> OFFSET_SATP_MODE_64))) && > >> + (cpu->cfg.satp_mode.init & (1 << (i + > >> OFFSET_SATP_MODE_64)))) > >> + error_append_or_setg(errp, "cannot disable %s satp mode > >> if %s " > >> + "is enabled", > >> + satp_mode_str(i, false), > >> + satp_mode_str(satp_mode_max, false)); > >> + } > >> + } > >> +} > >> + > >> +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; > >> + } > >> +} > >> + > >> #ifndef CONFIG_USER_ONLY > >> static void riscv_cpu_set_irq(void *opaque, int irq, int level) > >> { > >> @@ -1070,13 +1269,16 @@ 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); > >> } > >> + > >> + riscv_add_satp_mode_properties(obj); > >> } > >> > >> static Property riscv_cpu_properties[] = { > >> @@ -1094,6 +1296,7 @@ static Property riscv_cpu_properties[] = { > >> > >> DEFINE_PROP_BOOL("rvv_ta_all_1s", RISCVCPU, cfg.rvv_ta_all_1s, false), > >> DEFINE_PROP_BOOL("rvv_ma_all_1s", RISCVCPU, cfg.rvv_ma_all_1s, false), > >> + > >> DEFINE_PROP_END_OF_LIST(), > >> }; > >> > >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > >> index 3a9e25053f..1717b33321 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 > >> > >> @@ -407,6 +408,22 @@ struct RISCVCPUClass { > >> DeviceReset parent_reset; > >> }; > >> > >> +/* > >> + * map and init are divided into two 16bit bitmaps: the upper one is for > >> rv64 > >> + * and the lower one is for rv32, this is because the value for sv32 (ie. > >> 1) > >> + * may be reused later for another purpose for rv64 (see the > >> specification which > >> + * states that it is "reserved for standard use"). > >> + * > >> + * In a 16bit bitmap in map, the most significant set bit is the maximum > >> + * satp mode that is supported. > >> + * > >> + * Both 16bit bitmaps in init are used to make sure the user selected a > >> correct > >> + * combination as per the specification. > >> + */ > >> +typedef struct { > >> + uint32_t map, init; > >> +} RISCVSATPMap; > >> + > >> struct RISCVCPUConfig { > >> bool ext_i; > >> bool ext_e; > >> @@ -480,6 +497,8 @@ struct RISCVCPUConfig { > >> bool debug; > >> > >> bool short_isa_string; > >> + > >> + RISCVSATPMap satp_mode; > >> }; > >> > >> typedef struct RISCVCPUConfig RISCVCPUConfig; > >> @@ -789,4 +808,10 @@ 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, bool is_32_bit); > >> +const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit); > >> + > >> +void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp); > >> +void riscv_cpu_finalize_features(RISCVCPU *cpu, Error **errp); > >> + > >> #endif /* RISCV_CPU_H */ > >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c > >> index 5c9a7ee287..5c732653b2 100644 > >> --- a/target/riscv/csr.c > >> +++ b/target/riscv/csr.c > >> @@ -1109,10 +1109,17 @@ static RISCVException read_mstatus(CPURISCVState > >> *env, int csrno, > >> > >> static int validate_vm(CPURISCVState *env, target_ulong vm) > >> { > >> - if (riscv_cpu_mxl(env) == MXL_RV32) { > >> - return valid_vm_1_10_32[vm & 0xf]; > >> + uint8_t satp_mode_max; > >> + RISCVCPU *cpu = RISCV_CPU(env_cpu(env)); > >> + bool is_32_bit = riscv_cpu_mxl(env) == MXL_RV32; > >> + > >> + vm &= 0xf; > >> + satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map, > >> is_32_bit); > >> + > >> + if (is_32_bit) { > >> + return valid_vm_1_10_32[vm] && (vm <= satp_mode_max); > >> } else { > >> - return valid_vm_1_10_64[vm & 0xf]; > >> + return valid_vm_1_10_64[vm] && (vm <= satp_mode_max); > >> } > >> } > >> > >> -- > >> 2.37.2 > >> > >>
@Andrew: Please let me know when you have some cycles to review this, Thanks, Alex