Hi Andrew,
On Thu, Dec 1, 2022 at 3:47 PM Andrew Jones <ajo...@ventanamicro.com> wrote:
> On Thu, Dec 01, 2022 at 10:36:23AM +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 "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>
> > ---
> > 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 | 16 ++--
> > target/riscv/cpu.c | 164 ++++++++++++++++++++++++++++++++++++++++
> > target/riscv/cpu.h | 8 ++
> > target/riscv/cpu_bits.h | 1 +
> > target/riscv/csr.c | 8 +-
> > 5 files changed, 186 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index a5bc7353b4..bb7c739a74 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -228,7 +228,7 @@ 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;
> > + 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 +236,12 @@ 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");
> > - }
> > +
> > + sv_name = g_strdup_printf("riscv,%s",
> > +
> s->soc[socket].harts[cpu].cfg.satp_mode_str);
> > + 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..51c06ed057 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -907,6 +907,66 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > }
> > #endif
> >
> > + /*
> > + * Either a cpu sets its supported satp_mode in XXX_cpu_init
> > + * or the user sets this value using satp_mode property.
>
> using the sv* and mbare properties.
>
> > + */
> > + bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +
> > + cpu->cfg.satp_mode = VM_1_10_UNDEF;
>
> Could probably just use -1 here instead of introducing VM_1_10_UNDEF.
>
> > +
> > + if (rv32) {
> > + if (cpu->cfg.sv32 == ON_OFF_AUTO_ON) {
> > + cpu->cfg.satp_mode_str = g_strdup("sv32");
>
> No need to allocate memory, satp_mode_str = "sv32".
>
> Also I'm not sure we need to keep mode_str in CPUConfig. Providing a
> function with a switch on VM_1_10_SV* cases to get it should be enough
> for its one usecase.
>
> > + cpu->cfg.satp_mode = VM_1_10_SV32;
> > + } else if (cpu->cfg.mbare == ON_OFF_AUTO_ON) {
> > + cpu->cfg.satp_mode_str = g_strdup("none");
> > + cpu->cfg.satp_mode = VM_1_10_MBARE;
> > + }
> > + } else {
> > + if (cpu->cfg.sv64 == ON_OFF_AUTO_ON) {
> > + cpu->cfg.satp_mode_str = g_strdup("sv64");
> > + cpu->cfg.satp_mode = VM_1_10_SV64;
> > + } else if (cpu->cfg.sv57 == ON_OFF_AUTO_ON) {
> > + cpu->cfg.satp_mode_str = g_strdup("sv57");
> > + cpu->cfg.satp_mode = VM_1_10_SV57;
> > + } else if (cpu->cfg.sv48 == ON_OFF_AUTO_ON) {
> > + cpu->cfg.satp_mode_str = g_strdup("sv48");
> > + cpu->cfg.satp_mode = VM_1_10_SV48;
> > + } else if (cpu->cfg.sv39 == ON_OFF_AUTO_ON) {
> > + cpu->cfg.satp_mode_str = g_strdup("sv39");
> > + cpu->cfg.satp_mode = VM_1_10_SV39;
> > + } else if (cpu->cfg.mbare == ON_OFF_AUTO_ON) {
> > + cpu->cfg.satp_mode_str = g_strdup("none");
> > + cpu->cfg.satp_mode = VM_1_10_MBARE;
> > + }
> > + }
> > +
> > + /*
> > + * 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 == VM_1_10_UNDEF) {
> > + if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > + if (rv32) {
> > + cpu->cfg.satp_mode_str = g_strdup("sv32");
> > + cpu->cfg.satp_mode = VM_1_10_SV32;
> > + } else {
> > + cpu->cfg.satp_mode_str = g_strdup("sv57");
> > + cpu->cfg.satp_mode = VM_1_10_SV57;
> > + }
> > + } else {
> > + cpu->cfg.satp_mode_str = g_strdup("none");
> > + cpu->cfg.satp_mode = VM_1_10_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 +975,102 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > mcc->parent_realize(dev, errp);
> > }
> >
> > +void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> > +{
> > + bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> > +
> > + /* First, get rid of 32-bit/64-bit incompatibilities */
> > + if (rv32) {
> > + if (cpu->cfg.sv39 == ON_OFF_AUTO_ON
> > + || cpu->cfg.sv48 == ON_OFF_AUTO_ON
> > + || cpu->cfg.sv57 == ON_OFF_AUTO_ON
> > + || cpu->cfg.sv64 == ON_OFF_AUTO_ON) {
> > + error_setg(errp, "cannot enable 64-bit satp modes "
> > + "(sv39/sv48/sv57/sv64)");
> > + error_append_hint(errp, "cpu is in 32-bit mode, 64-bit satp
> modes "
> > + "can't be enabled\n");
> > + return;
> > + }
> > + } else {
> > + if (cpu->cfg.sv32 == ON_OFF_AUTO_ON) {
> > + error_setg(errp, "cannot enable 32-bit satp mode (sv32)");
> > + error_append_hint(errp, "cpu is in 64-bit mode, 32-bit satp
> mode "
> > + "can't be enabled\n");
> > + return;
> > + }
> > + }
> > +
> > + /*
> > + * Then make sure the user did not ask for an invalid configuration
> as per
> > + * the specification.
> > + */
> > + switch (cpu->cfg.satp_mode) {
> > + case VM_1_10_SV32:
> > + if (cpu->cfg.mbare == ON_OFF_AUTO_OFF) {
> > + error_setg(errp, "cannot disable mbare satp mode");
> > + error_append_hint(errp, "mbare satp mode must be enabled if
> sv32 "
> > + "is enabled\n");
> > + return;
> > + }
> > +
> > + break;
> > + case VM_1_10_SV39:
> > + if (cpu->cfg.mbare == ON_OFF_AUTO_OFF) {
> > + error_setg(errp, "cannot disable mbare satp mode");
> > + error_append_hint(errp, "mbare satp mode must be enabled if
> sv39 "
> > + "is enabled\n");
> > + return;
> > + }
> > +
> > + break;
> > + case VM_1_10_SV48:
> > + if (cpu->cfg.mbare == ON_OFF_AUTO_OFF
> > + || cpu->cfg.sv39 == ON_OFF_AUTO_OFF) {
> > + error_setg(errp, "cannot disable mbare/sv39 satp modes");
> > + error_append_hint(errp, "mbare/sv39 satp modes must be
> enabled if "
> > + "sv48 is enabled\n");
>
> Combined errors like this make the user look to see which one it is. I
> think we can we reorganize this switch to fall through from largest to
> smallest allowing the checks for smaller widths and mbare to be shared.
> If a user has more than one problem then they'll only see an error for the
> larger first, but then they'll try again and see the next one. In each
> case they'll see exactly what needs to be fixed, though.
>
> > + return;
> > + }
> > +
> > + break;
> > + case VM_1_10_SV57:
> > + if (cpu->cfg.mbare == ON_OFF_AUTO_OFF
> > + || cpu->cfg.sv39 == ON_OFF_AUTO_OFF
> > + || cpu->cfg.sv48 == ON_OFF_AUTO_OFF) {
> > + error_setg(errp, "cannot disable mbare/sv39/sv48 satp
> modes");
> > + error_append_hint(errp, "mbare/sv39/sv48 satp modes must be
> "
> > + "enabled if sv57 is enabled\n");
> > + return;
> > + }
> > +
> > + break;
> > + case VM_1_10_SV64:
> > + if (cpu->cfg.mbare == ON_OFF_AUTO_OFF
> > + || cpu->cfg.sv39 == ON_OFF_AUTO_OFF
> > + || cpu->cfg.sv48 == ON_OFF_AUTO_OFF
> > + || cpu->cfg.sv57 == ON_OFF_AUTO_OFF) {
> > + error_setg(errp, "cannot disable mbare/sv39/sv48/sv57 satp "
> > + "modes");
> > + error_append_hint(errp, "mbare/sv39/sv48/sv57 satp modes
> must be "
> > + "enabled if sv57 is enabled\n");
> > + return;
> > + }
> > +
> > + break;
> > + }
> > +}
> > +
> > +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)
> > {
> > @@ -1094,6 +1250,14 @@ 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_ON_OFF_AUTO("mbare", RISCVCPU, cfg.mbare,
> ON_OFF_AUTO_AUTO),
> > + DEFINE_PROP_ON_OFF_AUTO("sv32", RISCVCPU, cfg.sv32,
> ON_OFF_AUTO_AUTO),
> > + DEFINE_PROP_ON_OFF_AUTO("sv39", RISCVCPU, cfg.sv39,
> ON_OFF_AUTO_AUTO),
> > + DEFINE_PROP_ON_OFF_AUTO("sv48", RISCVCPU, cfg.sv48,
> ON_OFF_AUTO_AUTO),
> > + DEFINE_PROP_ON_OFF_AUTO("sv57", RISCVCPU, cfg.sv57,
> ON_OFF_AUTO_AUTO),
> > + DEFINE_PROP_ON_OFF_AUTO("sv64", RISCVCPU, cfg.sv64,
> ON_OFF_AUTO_AUTO),
>
> I'm not sure what types of issues may arise mixing booleans and OnOffAutos
> in a future cpu-model-expansion. I also think we can simplify things by
> using arm's sve* boolean properties as a pattern. For that, each property
> is a boolean which shares the same get and set accessors. The set accessor
> not only sets the property to true/false, but also tracks that the user
> did the setting, allowing for sanity checks at finalize time.
>
I can't find the sve* properties you're talking about, can you point them
to me?
Thanks,
Alex
>
> Using a pair of bitmaps for the sv properties, where VM_1_10_SV* are used
> for the bit numbers, should work well. Validating input will likely reduce
> to some bitmap comparing operations. It would also drop all the extra cfg
> state. In fact, one of the temporary bitmaps could use the satp_mode
> member, before the final result gets written to it. So, only a single
> extra uint8_t for the other bitmap is needed.
>
> > +
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 3a9e25053f..dcdde1e0b7 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
> >
> > @@ -480,6 +481,10 @@ struct RISCVCPUConfig {
> > bool debug;
> >
> > bool short_isa_string;
> > +
> > + OnOffAuto mbare, sv32, sv39, sv48, sv57, sv64;
> > + uint8_t satp_mode;
> > + char *satp_mode_str;
> > };
> >
> > typedef struct RISCVCPUConfig RISCVCPUConfig;
> > @@ -789,4 +794,7 @@ void riscv_set_csr_ops(int csrno,
> riscv_csr_operations *ops);
> >
> > void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
> >
> > +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/cpu_bits.h b/target/riscv/cpu_bits.h
> > index d8f5f0abed..3e67a815d5 100644
> > --- a/target/riscv/cpu_bits.h
> > +++ b/target/riscv/cpu_bits.h
> > @@ -590,6 +590,7 @@ typedef enum {
> > #define VM_1_10_SV48 9
> > #define VM_1_10_SV57 10
> > #define VM_1_10_SV64 11
> > +#define VM_1_10_UNDEF 16
> >
> > /* Page table entry (PTE) fields */
> > #define PTE_V 0x001 /* Valid */
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 5c9a7ee287..d2aab1627e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1109,10 +1109,14 @@ static RISCVException read_mstatus(CPURISCVState
> *env, int csrno,
> >
> > static int validate_vm(CPURISCVState *env, target_ulong vm)
> > {
> > + vm &= 0xf;
> > +
> > if (riscv_cpu_mxl(env) == MXL_RV32) {
> > - return valid_vm_1_10_32[vm & 0xf];
> > + return valid_vm_1_10_32[vm] &&
> > + (vm <= RISCV_CPU(env_cpu(env))->cfg.satp_mode);
> > } else {
> > - return valid_vm_1_10_64[vm & 0xf];
> > + return valid_vm_1_10_64[vm] &&
> > + (vm <= RISCV_CPU(env_cpu(env))->cfg.satp_mode);
> > }
> > }
> >
> > --
> > 2.37.2
> >
>
> Thanks,
> drew
>
