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