| Message ID | 20230123090324.732681-6-alexghiti@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: Allow user to set the satp mode | expand |
On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > Currently, the max satp mode is set with the only constraint that it must be > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > But we actually need to add another level of constraint: what the hw is > actually capable of, because currently, a linux booting on a sifive-u54 > boots in sv57 mode which is incompatible with the cpu's sv39 max > capability. > > So add a new bitmap to RISCVSATPMap which contains this capability and > initialize it in every XXX_cpu_init. > > Finally, we have the following chain of constraints: > > Qemu capability > HW capability > User choice > Software capability ^ What software is this? I'd think the user's choice would always be last. > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > target/riscv/cpu.h | 8 +++-- > 2 files changed, 59 insertions(+), 27 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index e409e6ab64..19a37fee2b 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > g_assert_not_reached(); > } > > -/* Sets the satp mode to the max supported */ > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > + const char *satp_mode_str, > + bool is_32_bit) > { > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > - cpu->cfg.satp_mode.map |= > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > - } else { > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > + > + for (int i = 0; i <= satp_mode; ++i) { > + if (valid_vm[i]) { > + cpu->cfg.satp_mode.supported |= (1 << i); I don't think we need a new 'supported' bitmap, I think each board that needs to further constrain va-bits from what QEMU supports should just set valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init function something like #define QEMU_SATP_MODE_MAX VM_1_10_SV64 void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max) { bool is_32_bit = cpu->env.misa_mxl == MXL_RV32; bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX); g_assert(!is_32_bit || satp_mode_max < 2); memset(valid_vm, 0, sizeof(*valid_vm)); for (int i = 0; i <= satp_mode_max; i++) { valid_vm[i] = true; } } The valid_vm[] checks already in finalize should then manage the validation needed to constrain boards. Only boards that care about this need to call this function, otherwise they'll get the default. Also, this patch should come before the patch that changes the default for all boards to sv57 in order to avoid breaking bisection. Thanks, drew
On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > > Currently, the max satp mode is set with the only constraint that it must be > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > > > But we actually need to add another level of constraint: what the hw is > > actually capable of, because currently, a linux booting on a sifive-u54 > > boots in sv57 mode which is incompatible with the cpu's sv39 max > > capability. > > > > So add a new bitmap to RISCVSATPMap which contains this capability and > > initialize it in every XXX_cpu_init. > > > > Finally, we have the following chain of constraints: > > > > Qemu capability > HW capability > User choice > Software capability > > ^ What software is this? > I'd think the user's choice would always be last. > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > --- > > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > > target/riscv/cpu.h | 8 +++-- > > 2 files changed, 59 insertions(+), 27 deletions(-) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index e409e6ab64..19a37fee2b 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > > g_assert_not_reached(); > > } > > > > -/* Sets the satp mode to the max supported */ > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > > + const char *satp_mode_str, > > + bool is_32_bit) > > { > > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > > - cpu->cfg.satp_mode.map |= > > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > > - } else { > > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > + > > + for (int i = 0; i <= satp_mode; ++i) { > > + if (valid_vm[i]) { > > + cpu->cfg.satp_mode.supported |= (1 << i); > > I don't think we need a new 'supported' bitmap, I think each board that > needs to further constrain va-bits from what QEMU supports should just set > valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init > function something like This was my first idea too, but those arrays are global and I have to admit that I thought it was possible to emulate a cpu with different cores. Anyway, isn't it a bit weird to store this into some global array whereas it is intimately linked to the CPU? To me, it makes sense to keep those variables as a way to know what qemu is able to emulate and have a CPU specific map like in this patch for the hw capabilities. Does it make sense to you? > > #define QEMU_SATP_MODE_MAX VM_1_10_SV64 > > void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max) > { > bool is_32_bit = cpu->env.misa_mxl == MXL_RV32; > bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX); > g_assert(!is_32_bit || satp_mode_max < 2); > > memset(valid_vm, 0, sizeof(*valid_vm)); > > for (int i = 0; i <= satp_mode_max; i++) { > valid_vm[i] = true; > } > } > > The valid_vm[] checks already in finalize should then manage the > validation needed to constrain boards. Only boards that care about > this need to call this function, otherwise they'll get the default. > > Also, this patch should come before the patch that changes the default > for all boards to sv57 in order to avoid breaking bisection. As I explained earlier, I didn't change the default to sv57! Just fixed what was passed via the device tree, which should not be used anyway :) Alex > > Thanks, > drew
On Mon, Jan 23, 2023 at 12:15:08PM +0100, Alexandre Ghiti wrote: > On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > > > Currently, the max satp mode is set with the only constraint that it must be > > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > > > > > But we actually need to add another level of constraint: what the hw is > > > actually capable of, because currently, a linux booting on a sifive-u54 > > > boots in sv57 mode which is incompatible with the cpu's sv39 max > > > capability. > > > > > > So add a new bitmap to RISCVSATPMap which contains this capability and > > > initialize it in every XXX_cpu_init. > > > > > > Finally, we have the following chain of constraints: > > > > > > Qemu capability > HW capability > User choice > Software capability > > > > ^ What software is this? > > I'd think the user's choice would always be last. > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > > --- > > > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > > > target/riscv/cpu.h | 8 +++-- > > > 2 files changed, 59 insertions(+), 27 deletions(-) > > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > > index e409e6ab64..19a37fee2b 100644 > > > --- a/target/riscv/cpu.c > > > +++ b/target/riscv/cpu.c > > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > > > g_assert_not_reached(); > > > } > > > > > > -/* Sets the satp mode to the max supported */ > > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > > > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > > > + const char *satp_mode_str, > > > + bool is_32_bit) > > > { > > > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > > > - cpu->cfg.satp_mode.map |= > > > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > > > - } else { > > > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > > > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > > > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > > + > > > + for (int i = 0; i <= satp_mode; ++i) { > > > + if (valid_vm[i]) { > > > + cpu->cfg.satp_mode.supported |= (1 << i); > > > > I don't think we need a new 'supported' bitmap, I think each board that > > needs to further constrain va-bits from what QEMU supports should just set > > valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init > > function something like > > This was my first idea too, but those arrays are global and I have to > admit that I thought it was possible to emulate a cpu with different > cores. Anyway, isn't it a bit weird to store this into some global > array whereas it is intimately linked to the CPU? To me, it makes > sense to keep those variables as a way to know what qemu is able to > emulate and have a CPU specific map like in this patch for the hw > capabilities. Does it make sense to you? Ah, yes, to support heterogeneous configs it's best to keep this information per-cpu. I'll take another look at the patch. > > > > > #define QEMU_SATP_MODE_MAX VM_1_10_SV64 > > > > void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max) > > { > > bool is_32_bit = cpu->env.misa_mxl == MXL_RV32; > > bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > > > g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX); > > g_assert(!is_32_bit || satp_mode_max < 2); > > > > memset(valid_vm, 0, sizeof(*valid_vm)); > > > > for (int i = 0; i <= satp_mode_max; i++) { > > valid_vm[i] = true; > > } > > } > > > > The valid_vm[] checks already in finalize should then manage the > > validation needed to constrain boards. Only boards that care about > > this need to call this function, otherwise they'll get the default. > > > > Also, this patch should come before the patch that changes the default > > for all boards to sv57 in order to avoid breaking bisection. > > As I explained earlier, I didn't change the default to sv57! Just > fixed what was passed via the device tree, which should not be used > anyway :) OK, I keep misunderstanding how we're "fixing" something which is is wrong, but apparently doesn't exhibit any symptoms. So, assuming it doesn't matter, then I guess it can come anywhere in the series. Thanks, drew
On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > Currently, the max satp mode is set with the only constraint that it must be > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > But we actually need to add another level of constraint: what the hw is > actually capable of, because currently, a linux booting on a sifive-u54 > boots in sv57 mode which is incompatible with the cpu's sv39 max > capability. > > So add a new bitmap to RISCVSATPMap which contains this capability and > initialize it in every XXX_cpu_init. > > Finally, we have the following chain of constraints: > > Qemu capability > HW capability > User choice > Software capability > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > target/riscv/cpu.h | 8 +++-- > 2 files changed, 59 insertions(+), 27 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index e409e6ab64..19a37fee2b 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > g_assert_not_reached(); > } > > -/* Sets the satp mode to the max supported */ > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > + const char *satp_mode_str, > + bool is_32_bit) I'd drop 'is_32_bit' and get it from 'cpu', which would "clean up" all the callsites by getting rid of all the true/false stuff. Also, why take the string instead of the VM_1_10_SV* define? > { > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > - cpu->cfg.satp_mode.map |= > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > - } else { > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > + > + for (int i = 0; i <= satp_mode; ++i) { > + if (valid_vm[i]) { > + cpu->cfg.satp_mode.supported |= (1 << i); > + } > } > } > > +/* Sets the satp mode to the max supported */ > +static void set_satp_mode_default(RISCVCPU *cpu) > +{ > + uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > + > + cpu->cfg.satp_mode.map |= (1 << satp_mode); Let's do 'cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported' to make sure 'map' has all supported bits set for property probing. > +} > + > static void riscv_any_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > #if defined(TARGET_RISCV32) > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > + set_satp_mode_max_supported(cpu, "sv32", true); > #elif defined(TARGET_RISCV64) > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > + set_satp_mode_max_supported(cpu, "sv57", false); > #endif > set_priv_version(env, PRIV_VERSION_1_12_0); > register_cpu_props(obj); > @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj) > static void rv64_base_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > /* We set this in the realise function */ > set_misa(env, MXL_RV64, 0); > register_cpu_props(obj); > /* Set latest version of privileged specification */ > set_priv_version(env, PRIV_VERSION_1_12_0); > + set_satp_mode_max_supported(cpu, "sv57", false); > } > > static void rv64_sifive_u_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > + set_satp_mode_max_supported(cpu, "sv39", false); > } > > static void rv64_sifive_e_cpu_init(Object *obj) > @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj) > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", false); > } > > static void rv128_base_cpu_init(Object *obj) > @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj) > exit(EXIT_FAILURE); > } > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > /* We set this in the realise function */ > set_misa(env, MXL_RV128, 0); > register_cpu_props(obj); > /* Set latest version of privileged specification */ > set_priv_version(env, PRIV_VERSION_1_12_0); > + set_satp_mode_max_supported(cpu, "sv57", false); > } > #else > static void rv32_base_cpu_init(Object *obj) > @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj) > register_cpu_props(obj); > /* Set latest version of privileged specification */ > set_priv_version(env, PRIV_VERSION_1_12_0); > + set_satp_mode_max_supported(cpu, "sv32", true); > } > > static void rv32_sifive_u_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > + set_satp_mode_max_supported(cpu, "sv32", true); > } > > static void rv32_sifive_e_cpu_init(Object *obj) > @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj) > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", true); > } > > static void rv32_ibex_cpu_init(Object *obj) > @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj) > set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_11_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", true); > cpu->cfg.epmp = true; > } > > @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj) > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", true); > } > #endif > > @@ -696,7 +727,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info) > static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > { > bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; > - const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64; > + uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > + uint8_t satp_mode_supported_max = > + satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > > if (cpu->cfg.satp_mode.map == 0) { > /* > @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > * satp mode. > */ > if (cpu->cfg.satp_mode.init == 0) { > - set_satp_mode_default(cpu, rv32); > + set_satp_mode_default(cpu); > } else { > /* > * Find the lowest level that was disabled and then enable the > @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > for (int i = 1; i < 16; ++i) { > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > (cpu->cfg.satp_mode.init & (1 << i)) && > - valid_vm[i]) { > + (cpu->cfg.satp_mode.supported & (1 << i))) { > for (int j = i - 1; j >= 0; --j) { > - if (valid_vm[j]) { > + if (cpu->cfg.satp_mode.supported & (1 << j)) { > cpu->cfg.satp_mode.map |= (1 << j); > break; > } > @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > } > } > > - /* Make sure the configuration asked is supported by qemu */ > - for (int i = 0; i < 16; ++i) { > - if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) { > - error_setg(errp, "satp_mode %s is not valid", > - satp_mode_str(i, rv32)); > - return; > - } > + /* Make sure the user asked for a supported configuration (HW and qemu) */ > + if (satp_mode_map_max > satp_mode_supported_max) { > + error_setg(errp, "satp_mode %s is higher than hw max capability %s", > + satp_mode_str(satp_mode_map_max, rv32), > + satp_mode_str(satp_mode_supported_max, rv32)); > + return; > } > > /* > @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > * the specification. > */ > if (!rv32) { > - uint8_t satp_mode_max; > - > - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > - > - for (int i = satp_mode_max - 1; i >= 0; --i) { > + for (int i = satp_mode_map_max - 1; i >= 0; --i) { > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > (cpu->cfg.satp_mode.init & (1 << i)) && > - valid_vm[i]) { > + (cpu->cfg.satp_mode.supported & (1 << i))) { > error_setg(errp, "cannot disable %s satp mode if %s " > "is enabled", satp_mode_str(i, false), > - satp_mode_str(satp_mode_max, false)); > + satp_mode_str(satp_mode_map_max, false)); > return; > } > } > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index e37177db5c..b591122099 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -416,13 +416,17 @@ struct RISCVCPUClass { > > /* > * map is a 16-bit bitmap: the most significant set bit in map is the maximum > - * satp mode that is supported. > + * satp mode that is supported. It may be chosen by the user and must respect > + * what qemu implements (valid_1_10_32/64) and what the hw is capable of > + * (supported bitmap below). > * > * init is a 16-bit bitmap used to make sure the user selected a correct > * configuration as per the specification. > + * > + * supported is a 16-bit bitmap used to reflect the hw capabilities. > */ > typedef struct { > - uint16_t map, init; > + uint16_t map, init, supported; > } RISCVSATPMap; > > struct RISCVCPUConfig { > -- > 2.37.2 > Thanks, drew
On Mon, Jan 23, 2023 at 7:09 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > Currently, the max satp mode is set with the only constraint that it must be > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > But we actually need to add another level of constraint: what the hw is > actually capable of, because currently, a linux booting on a sifive-u54 > boots in sv57 mode which is incompatible with the cpu's sv39 max > capability. > > So add a new bitmap to RISCVSATPMap which contains this capability and > initialize it in every XXX_cpu_init. > > Finally, we have the following chain of constraints: > > Qemu capability > HW capability > User choice > Software capability > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > target/riscv/cpu.h | 8 +++-- > 2 files changed, 59 insertions(+), 27 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index e409e6ab64..19a37fee2b 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > g_assert_not_reached(); > } > > -/* Sets the satp mode to the max supported */ > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > + const char *satp_mode_str, > + bool is_32_bit) > { > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > - cpu->cfg.satp_mode.map |= > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > - } else { > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > + > + for (int i = 0; i <= satp_mode; ++i) { > + if (valid_vm[i]) { > + cpu->cfg.satp_mode.supported |= (1 << i); > + } > } > } > > +/* Sets the satp mode to the max supported */ > +static void set_satp_mode_default(RISCVCPU *cpu) > +{ > + uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > + > + cpu->cfg.satp_mode.map |= (1 << satp_mode); > +} > + > static void riscv_any_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > #if defined(TARGET_RISCV32) > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > + set_satp_mode_max_supported(cpu, "sv32", true); > #elif defined(TARGET_RISCV64) > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > + set_satp_mode_max_supported(cpu, "sv57", false); > #endif > set_priv_version(env, PRIV_VERSION_1_12_0); > register_cpu_props(obj); > @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj) > static void rv64_base_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > /* We set this in the realise function */ > set_misa(env, MXL_RV64, 0); > register_cpu_props(obj); > /* Set latest version of privileged specification */ > set_priv_version(env, PRIV_VERSION_1_12_0); > + set_satp_mode_max_supported(cpu, "sv57", false); > } > > static void rv64_sifive_u_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > + set_satp_mode_max_supported(cpu, "sv39", false); Can we just not expose the properties on these vendor CPUs and then not worry about setting maximums? Alistair > } > > static void rv64_sifive_e_cpu_init(Object *obj) > @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj) > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", false); > } > > static void rv128_base_cpu_init(Object *obj) > @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj) > exit(EXIT_FAILURE); > } > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > /* We set this in the realise function */ > set_misa(env, MXL_RV128, 0); > register_cpu_props(obj); > /* Set latest version of privileged specification */ > set_priv_version(env, PRIV_VERSION_1_12_0); > + set_satp_mode_max_supported(cpu, "sv57", false); > } > #else > static void rv32_base_cpu_init(Object *obj) > @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj) > register_cpu_props(obj); > /* Set latest version of privileged specification */ > set_priv_version(env, PRIV_VERSION_1_12_0); > + set_satp_mode_max_supported(cpu, "sv32", true); > } > > static void rv32_sifive_u_cpu_init(Object *obj) > { > CPURISCVState *env = &RISCV_CPU(obj)->env; > + RISCVCPU *cpu = RISCV_CPU(obj); > + > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > + set_satp_mode_max_supported(cpu, "sv32", true); > } > > static void rv32_sifive_e_cpu_init(Object *obj) > @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj) > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", true); > } > > static void rv32_ibex_cpu_init(Object *obj) > @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj) > set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_11_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", true); > cpu->cfg.epmp = true; > } > > @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj) > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU); > set_priv_version(env, PRIV_VERSION_1_10_0); > cpu->cfg.mmu = false; > + set_satp_mode_max_supported(cpu, "mbare", true); > } > #endif > > @@ -696,7 +727,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info) > static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > { > bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; > - const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64; > + uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > + uint8_t satp_mode_supported_max = > + satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > > if (cpu->cfg.satp_mode.map == 0) { > /* > @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > * satp mode. > */ > if (cpu->cfg.satp_mode.init == 0) { > - set_satp_mode_default(cpu, rv32); > + set_satp_mode_default(cpu); > } else { > /* > * Find the lowest level that was disabled and then enable the > @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > for (int i = 1; i < 16; ++i) { > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > (cpu->cfg.satp_mode.init & (1 << i)) && > - valid_vm[i]) { > + (cpu->cfg.satp_mode.supported & (1 << i))) { > for (int j = i - 1; j >= 0; --j) { > - if (valid_vm[j]) { > + if (cpu->cfg.satp_mode.supported & (1 << j)) { > cpu->cfg.satp_mode.map |= (1 << j); > break; > } > @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > } > } > > - /* Make sure the configuration asked is supported by qemu */ > - for (int i = 0; i < 16; ++i) { > - if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) { > - error_setg(errp, "satp_mode %s is not valid", > - satp_mode_str(i, rv32)); > - return; > - } > + /* Make sure the user asked for a supported configuration (HW and qemu) */ > + if (satp_mode_map_max > satp_mode_supported_max) { > + error_setg(errp, "satp_mode %s is higher than hw max capability %s", > + satp_mode_str(satp_mode_map_max, rv32), > + satp_mode_str(satp_mode_supported_max, rv32)); > + return; > } > > /* > @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > * the specification. > */ > if (!rv32) { > - uint8_t satp_mode_max; > - > - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > - > - for (int i = satp_mode_max - 1; i >= 0; --i) { > + for (int i = satp_mode_map_max - 1; i >= 0; --i) { > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > (cpu->cfg.satp_mode.init & (1 << i)) && > - valid_vm[i]) { > + (cpu->cfg.satp_mode.supported & (1 << i))) { > error_setg(errp, "cannot disable %s satp mode if %s " > "is enabled", satp_mode_str(i, false), > - satp_mode_str(satp_mode_max, false)); > + satp_mode_str(satp_mode_map_max, false)); > return; > } > } > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index e37177db5c..b591122099 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -416,13 +416,17 @@ struct RISCVCPUClass { > > /* > * map is a 16-bit bitmap: the most significant set bit in map is the maximum > - * satp mode that is supported. > + * satp mode that is supported. It may be chosen by the user and must respect > + * what qemu implements (valid_1_10_32/64) and what the hw is capable of > + * (supported bitmap below). > * > * init is a 16-bit bitmap used to make sure the user selected a correct > * configuration as per the specification. > + * > + * supported is a 16-bit bitmap used to reflect the hw capabilities. > */ > typedef struct { > - uint16_t map, init; > + uint16_t map, init, supported; > } RISCVSATPMap; > > struct RISCVCPUConfig { > -- > 2.37.2 > >
Hi Alistair, On Tue, Jan 24, 2023 at 1:41 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Jan 23, 2023 at 7:09 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > > > Currently, the max satp mode is set with the only constraint that it must be > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > > > But we actually need to add another level of constraint: what the hw is > > actually capable of, because currently, a linux booting on a sifive-u54 > > boots in sv57 mode which is incompatible with the cpu's sv39 max > > capability. > > > > So add a new bitmap to RISCVSATPMap which contains this capability and > > initialize it in every XXX_cpu_init. > > > > Finally, we have the following chain of constraints: > > > > Qemu capability > HW capability > User choice > Software capability > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > --- > > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > > target/riscv/cpu.h | 8 +++-- > > 2 files changed, 59 insertions(+), 27 deletions(-) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index e409e6ab64..19a37fee2b 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > > g_assert_not_reached(); > > } > > > > -/* Sets the satp mode to the max supported */ > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > > + const char *satp_mode_str, > > + bool is_32_bit) > > { > > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > > - cpu->cfg.satp_mode.map |= > > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > > - } else { > > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > + > > + for (int i = 0; i <= satp_mode; ++i) { > > + if (valid_vm[i]) { > > + cpu->cfg.satp_mode.supported |= (1 << i); > > + } > > } > > } > > > > +/* Sets the satp mode to the max supported */ > > +static void set_satp_mode_default(RISCVCPU *cpu) > > +{ > > + uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > > + > > + cpu->cfg.satp_mode.map |= (1 << satp_mode); > > +} > > + > > static void riscv_any_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > #if defined(TARGET_RISCV32) > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > > + set_satp_mode_max_supported(cpu, "sv32", true); > > #elif defined(TARGET_RISCV64) > > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > > + set_satp_mode_max_supported(cpu, "sv57", false); > > #endif > > set_priv_version(env, PRIV_VERSION_1_12_0); > > register_cpu_props(obj); > > @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj) > > static void rv64_base_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > /* We set this in the realise function */ > > set_misa(env, MXL_RV64, 0); > > register_cpu_props(obj); > > /* Set latest version of privileged specification */ > > set_priv_version(env, PRIV_VERSION_1_12_0); > > + set_satp_mode_max_supported(cpu, "sv57", false); > > } > > > > static void rv64_sifive_u_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > + set_satp_mode_max_supported(cpu, "sv39", false); > > Can we just not expose the properties on these vendor CPUs and then > not worry about setting maximums? > I'm not sure I understand: the properties are actually not exposed to the vendor cpus from what I see (no calls to register_cpu_props). The problem this patch fixes is that the only constraint on satp is valid_vm_1_10_32/64, which reflects the qemu capabilities: as said in the commit log, a sifive-u54 will allow a kernel to boot in sv57, and not sv39 as it should. This patch only takes advantage of the newly introduced RISCVSATPMap structure to fix this issue, I should maybe emphasize in the commit description that this is a fix. Alex > Alistair > > > } > > > > static void rv64_sifive_e_cpu_init(Object *obj) > > @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj) > > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", false); > > } > > > > static void rv128_base_cpu_init(Object *obj) > > @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj) > > exit(EXIT_FAILURE); > > } > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > /* We set this in the realise function */ > > set_misa(env, MXL_RV128, 0); > > register_cpu_props(obj); > > /* Set latest version of privileged specification */ > > set_priv_version(env, PRIV_VERSION_1_12_0); > > + set_satp_mode_max_supported(cpu, "sv57", false); > > } > > #else > > static void rv32_base_cpu_init(Object *obj) > > @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj) > > register_cpu_props(obj); > > /* Set latest version of privileged specification */ > > set_priv_version(env, PRIV_VERSION_1_12_0); > > + set_satp_mode_max_supported(cpu, "sv32", true); > > } > > > > static void rv32_sifive_u_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > + set_satp_mode_max_supported(cpu, "sv32", true); > > } > > > > static void rv32_sifive_e_cpu_init(Object *obj) > > @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj) > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", true); > > } > > > > static void rv32_ibex_cpu_init(Object *obj) > > @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj) > > set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_11_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", true); > > cpu->cfg.epmp = true; > > } > > > > @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj) > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", true); > > } > > #endif > > > > @@ -696,7 +727,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info) > > static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > { > > bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; > > - const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64; > > + uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > > + uint8_t satp_mode_supported_max = > > + satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > > > > if (cpu->cfg.satp_mode.map == 0) { > > /* > > @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > * satp mode. > > */ > > if (cpu->cfg.satp_mode.init == 0) { > > - set_satp_mode_default(cpu, rv32); > > + set_satp_mode_default(cpu); > > } else { > > /* > > * Find the lowest level that was disabled and then enable the > > @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > for (int i = 1; i < 16; ++i) { > > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > > (cpu->cfg.satp_mode.init & (1 << i)) && > > - valid_vm[i]) { > > + (cpu->cfg.satp_mode.supported & (1 << i))) { > > for (int j = i - 1; j >= 0; --j) { > > - if (valid_vm[j]) { > > + if (cpu->cfg.satp_mode.supported & (1 << j)) { > > cpu->cfg.satp_mode.map |= (1 << j); > > break; > > } > > @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > } > > } > > > > - /* Make sure the configuration asked is supported by qemu */ > > - for (int i = 0; i < 16; ++i) { > > - if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) { > > - error_setg(errp, "satp_mode %s is not valid", > > - satp_mode_str(i, rv32)); > > - return; > > - } > > + /* Make sure the user asked for a supported configuration (HW and qemu) */ > > + if (satp_mode_map_max > satp_mode_supported_max) { > > + error_setg(errp, "satp_mode %s is higher than hw max capability %s", > > + satp_mode_str(satp_mode_map_max, rv32), > > + satp_mode_str(satp_mode_supported_max, rv32)); > > + return; > > } > > > > /* > > @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > * the specification. > > */ > > if (!rv32) { > > - uint8_t satp_mode_max; > > - > > - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > > - > > - for (int i = satp_mode_max - 1; i >= 0; --i) { > > + for (int i = satp_mode_map_max - 1; i >= 0; --i) { > > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > > (cpu->cfg.satp_mode.init & (1 << i)) && > > - valid_vm[i]) { > > + (cpu->cfg.satp_mode.supported & (1 << i))) { > > error_setg(errp, "cannot disable %s satp mode if %s " > > "is enabled", satp_mode_str(i, false), > > - satp_mode_str(satp_mode_max, false)); > > + satp_mode_str(satp_mode_map_max, false)); > > return; > > } > > } > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > index e37177db5c..b591122099 100644 > > --- a/target/riscv/cpu.h > > +++ b/target/riscv/cpu.h > > @@ -416,13 +416,17 @@ struct RISCVCPUClass { > > > > /* > > * map is a 16-bit bitmap: the most significant set bit in map is the maximum > > - * satp mode that is supported. > > + * satp mode that is supported. It may be chosen by the user and must respect > > + * what qemu implements (valid_1_10_32/64) and what the hw is capable of > > + * (supported bitmap below). > > * > > * init is a 16-bit bitmap used to make sure the user selected a correct > > * configuration as per the specification. > > + * > > + * supported is a 16-bit bitmap used to reflect the hw capabilities. > > */ > > typedef struct { > > - uint16_t map, init; > > + uint16_t map, init, supported; > > } RISCVSATPMap; > > > > struct RISCVCPUConfig { > > -- > > 2.37.2 > > > >
On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > > Currently, the max satp mode is set with the only constraint that it must be > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > > > But we actually need to add another level of constraint: what the hw is > > actually capable of, because currently, a linux booting on a sifive-u54 > > boots in sv57 mode which is incompatible with the cpu's sv39 max > > capability. > > > > So add a new bitmap to RISCVSATPMap which contains this capability and > > initialize it in every XXX_cpu_init. > > > > Finally, we have the following chain of constraints: > > > > Qemu capability > HW capability > User choice > Software capability > > ^ What software is this? > I'd think the user's choice would always be last. Hmm maybe that's not clear, but I meant that the last constraint was what the emulated software is capable of handling. > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > --- > > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > > target/riscv/cpu.h | 8 +++-- > > 2 files changed, 59 insertions(+), 27 deletions(-) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index e409e6ab64..19a37fee2b 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > > g_assert_not_reached(); > > } > > > > -/* Sets the satp mode to the max supported */ > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > > + const char *satp_mode_str, > > + bool is_32_bit) > > { > > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > > - cpu->cfg.satp_mode.map |= > > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > > - } else { > > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > + > > + for (int i = 0; i <= satp_mode; ++i) { > > + if (valid_vm[i]) { > > + cpu->cfg.satp_mode.supported |= (1 << i); > > I don't think we need a new 'supported' bitmap, I think each board that > needs to further constrain va-bits from what QEMU supports should just set > valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init > function something like > > #define QEMU_SATP_MODE_MAX VM_1_10_SV64 > > void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max) > { > bool is_32_bit = cpu->env.misa_mxl == MXL_RV32; > bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX); > g_assert(!is_32_bit || satp_mode_max < 2); > > memset(valid_vm, 0, sizeof(*valid_vm)); > > for (int i = 0; i <= satp_mode_max; i++) { > valid_vm[i] = true; > } > } > > The valid_vm[] checks already in finalize should then manage the > validation needed to constrain boards. Only boards that care about > this need to call this function, otherwise they'll get the default. > > Also, this patch should come before the patch that changes the default > for all boards to sv57 in order to avoid breaking bisection. > > Thanks, > drew
On Mon, Jan 23, 2023 at 2:31 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Mon, Jan 23, 2023 at 12:15:08PM +0100, Alexandre Ghiti wrote: > > On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > > > > Currently, the max satp mode is set with the only constraint that it must be > > > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > > > > > > > But we actually need to add another level of constraint: what the hw is > > > > actually capable of, because currently, a linux booting on a sifive-u54 > > > > boots in sv57 mode which is incompatible with the cpu's sv39 max > > > > capability. > > > > > > > > So add a new bitmap to RISCVSATPMap which contains this capability and > > > > initialize it in every XXX_cpu_init. > > > > > > > > Finally, we have the following chain of constraints: > > > > > > > > Qemu capability > HW capability > User choice > Software capability > > > > > > ^ What software is this? > > > I'd think the user's choice would always be last. > > > > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > > > --- > > > > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > > > > target/riscv/cpu.h | 8 +++-- > > > > 2 files changed, 59 insertions(+), 27 deletions(-) > > > > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > > > index e409e6ab64..19a37fee2b 100644 > > > > --- a/target/riscv/cpu.c > > > > +++ b/target/riscv/cpu.c > > > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > > > > g_assert_not_reached(); > > > > } > > > > > > > > -/* Sets the satp mode to the max supported */ > > > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > > > > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > > > > + const char *satp_mode_str, > > > > + bool is_32_bit) > > > > { > > > > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > > > > - cpu->cfg.satp_mode.map |= > > > > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > > > > - } else { > > > > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > > > > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > > > > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > > > + > > > > + for (int i = 0; i <= satp_mode; ++i) { > > > > + if (valid_vm[i]) { > > > > + cpu->cfg.satp_mode.supported |= (1 << i); > > > > > > I don't think we need a new 'supported' bitmap, I think each board that > > > needs to further constrain va-bits from what QEMU supports should just set > > > valid_vm_1_10_32/64. I.e. drop const from the arrays and add an init > > > function something like > > > > This was my first idea too, but those arrays are global and I have to > > admit that I thought it was possible to emulate a cpu with different > > cores. Anyway, isn't it a bit weird to store this into some global > > array whereas it is intimately linked to the CPU? To me, it makes > > sense to keep those variables as a way to know what qemu is able to > > emulate and have a CPU specific map like in this patch for the hw > > capabilities. Does it make sense to you? > > Ah, yes, to support heterogeneous configs it's best to keep this > information per-cpu. I'll take another look at the patch. > > > > > > > > > #define QEMU_SATP_MODE_MAX VM_1_10_SV64 > > > > > > void riscv_cpu_set_satp_mode_max(RISCVCPU *cpu, uint8_t satp_mode_max) > > > { > > > bool is_32_bit = cpu->env.misa_mxl == MXL_RV32; > > > bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > > > > > g_assert(satp_mode_max <= QEMU_SATP_MODE_MAX); > > > g_assert(!is_32_bit || satp_mode_max < 2); > > > > > > memset(valid_vm, 0, sizeof(*valid_vm)); > > > > > > for (int i = 0; i <= satp_mode_max; i++) { > > > valid_vm[i] = true; > > > } > > > } > > > > > > The valid_vm[] checks already in finalize should then manage the > > > validation needed to constrain boards. Only boards that care about > > > this need to call this function, otherwise they'll get the default. > > > > > > Also, this patch should come before the patch that changes the default > > > for all boards to sv57 in order to avoid breaking bisection. > > > > As I explained earlier, I didn't change the default to sv57! Just > > fixed what was passed via the device tree, which should not be used > > anyway :) > > OK, I keep misunderstanding how we're "fixing" something which is > is wrong, but apparently doesn't exhibit any symptoms. So, assuming > it doesn't matter, then I guess it can come anywhere in the series. Actually *I* think it should not matter, but I can't be sure so I'll do what you ask. > > Thanks, > drew
On Mon, Jan 23, 2023 at 2:51 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > > Currently, the max satp mode is set with the only constraint that it must be > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > > > But we actually need to add another level of constraint: what the hw is > > actually capable of, because currently, a linux booting on a sifive-u54 > > boots in sv57 mode which is incompatible with the cpu's sv39 max > > capability. > > > > So add a new bitmap to RISCVSATPMap which contains this capability and > > initialize it in every XXX_cpu_init. > > > > Finally, we have the following chain of constraints: > > > > Qemu capability > HW capability > User choice > Software capability > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > --- > > target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- > > target/riscv/cpu.h | 8 +++-- > > 2 files changed, 59 insertions(+), 27 deletions(-) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index e409e6ab64..19a37fee2b 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) > > g_assert_not_reached(); > > } > > > > -/* Sets the satp mode to the max supported */ > > -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) > > +static void set_satp_mode_max_supported(RISCVCPU *cpu, > > + const char *satp_mode_str, > > + bool is_32_bit) > > I'd drop 'is_32_bit' and get it from 'cpu', which would "clean up" all the > callsites by getting rid of all the true/false stuff. Indeed, better this way > Also, why take the string instead of the VM_1_10_SV* define? No particular reason, but I changed it to VM_1_10_SV*, thanks > > > { > > - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { > > - cpu->cfg.satp_mode.map |= > > - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); > > - } else { > > - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); > > + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); > > + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; > > + > > + for (int i = 0; i <= satp_mode; ++i) { > > + if (valid_vm[i]) { > > + cpu->cfg.satp_mode.supported |= (1 << i); > > + } > > } > > } > > > > +/* Sets the satp mode to the max supported */ > > +static void set_satp_mode_default(RISCVCPU *cpu) > > +{ > > + uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > > + > > + cpu->cfg.satp_mode.map |= (1 << satp_mode); > > Let's do 'cpu->cfg.satp_mode.map = cpu->cfg.satp_mode.supported' to make > sure 'map' has all supported bits set for property probing. Indeed now the map is fully set. > > > +} > > + > > static void riscv_any_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > #if defined(TARGET_RISCV32) > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > > + set_satp_mode_max_supported(cpu, "sv32", true); > > #elif defined(TARGET_RISCV64) > > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU); > > + set_satp_mode_max_supported(cpu, "sv57", false); > > #endif > > set_priv_version(env, PRIV_VERSION_1_12_0); > > register_cpu_props(obj); > > @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj) > > static void rv64_base_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > /* We set this in the realise function */ > > set_misa(env, MXL_RV64, 0); > > register_cpu_props(obj); > > /* Set latest version of privileged specification */ > > set_priv_version(env, PRIV_VERSION_1_12_0); > > + set_satp_mode_max_supported(cpu, "sv57", false); > > } > > > > static void rv64_sifive_u_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > + set_satp_mode_max_supported(cpu, "sv39", false); > > } > > > > static void rv64_sifive_e_cpu_init(Object *obj) > > @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj) > > set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", false); > > } > > > > static void rv128_base_cpu_init(Object *obj) > > @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj) > > exit(EXIT_FAILURE); > > } > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > /* We set this in the realise function */ > > set_misa(env, MXL_RV128, 0); > > register_cpu_props(obj); > > /* Set latest version of privileged specification */ > > set_priv_version(env, PRIV_VERSION_1_12_0); > > + set_satp_mode_max_supported(cpu, "sv57", false); > > } > > #else > > static void rv32_base_cpu_init(Object *obj) > > @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj) > > register_cpu_props(obj); > > /* Set latest version of privileged specification */ > > set_priv_version(env, PRIV_VERSION_1_12_0); > > + set_satp_mode_max_supported(cpu, "sv32", true); > > } > > > > static void rv32_sifive_u_cpu_init(Object *obj) > > { > > CPURISCVState *env = &RISCV_CPU(obj)->env; > > + RISCVCPU *cpu = RISCV_CPU(obj); > > + > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > + set_satp_mode_max_supported(cpu, "sv32", true); > > } > > > > static void rv32_sifive_e_cpu_init(Object *obj) > > @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj) > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", true); > > } > > > > static void rv32_ibex_cpu_init(Object *obj) > > @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj) > > set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_11_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", true); > > cpu->cfg.epmp = true; > > } > > > > @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj) > > set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU); > > set_priv_version(env, PRIV_VERSION_1_10_0); > > cpu->cfg.mmu = false; > > + set_satp_mode_max_supported(cpu, "mbare", true); > > } > > #endif > > > > @@ -696,7 +727,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info) > > static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > { > > bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; > > - const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64; > > + uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > > + uint8_t satp_mode_supported_max = > > + satp_mode_max_from_map(cpu->cfg.satp_mode.supported); > > > > if (cpu->cfg.satp_mode.map == 0) { > > /* > > @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > * satp mode. > > */ > > if (cpu->cfg.satp_mode.init == 0) { > > - set_satp_mode_default(cpu, rv32); > > + set_satp_mode_default(cpu); > > } else { > > /* > > * Find the lowest level that was disabled and then enable the > > @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > for (int i = 1; i < 16; ++i) { > > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > > (cpu->cfg.satp_mode.init & (1 << i)) && > > - valid_vm[i]) { > > + (cpu->cfg.satp_mode.supported & (1 << i))) { > > for (int j = i - 1; j >= 0; --j) { > > - if (valid_vm[j]) { > > + if (cpu->cfg.satp_mode.supported & (1 << j)) { > > cpu->cfg.satp_mode.map |= (1 << j); > > break; > > } > > @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > } > > } > > > > - /* Make sure the configuration asked is supported by qemu */ > > - for (int i = 0; i < 16; ++i) { > > - if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) { > > - error_setg(errp, "satp_mode %s is not valid", > > - satp_mode_str(i, rv32)); > > - return; > > - } > > + /* Make sure the user asked for a supported configuration (HW and qemu) */ > > + if (satp_mode_map_max > satp_mode_supported_max) { > > + error_setg(errp, "satp_mode %s is higher than hw max capability %s", > > + satp_mode_str(satp_mode_map_max, rv32), > > + satp_mode_str(satp_mode_supported_max, rv32)); > > + return; > > } > > > > /* > > @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) > > * the specification. > > */ > > if (!rv32) { > > - uint8_t satp_mode_max; > > - > > - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); > > - > > - for (int i = satp_mode_max - 1; i >= 0; --i) { > > + for (int i = satp_mode_map_max - 1; i >= 0; --i) { > > if (!(cpu->cfg.satp_mode.map & (1 << i)) && > > (cpu->cfg.satp_mode.init & (1 << i)) && > > - valid_vm[i]) { > > + (cpu->cfg.satp_mode.supported & (1 << i))) { > > error_setg(errp, "cannot disable %s satp mode if %s " > > "is enabled", satp_mode_str(i, false), > > - satp_mode_str(satp_mode_max, false)); > > + satp_mode_str(satp_mode_map_max, false)); > > return; > > } > > } > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > index e37177db5c..b591122099 100644 > > --- a/target/riscv/cpu.h > > +++ b/target/riscv/cpu.h > > @@ -416,13 +416,17 @@ struct RISCVCPUClass { > > > > /* > > * map is a 16-bit bitmap: the most significant set bit in map is the maximum > > - * satp mode that is supported. > > + * satp mode that is supported. It may be chosen by the user and must respect > > + * what qemu implements (valid_1_10_32/64) and what the hw is capable of > > + * (supported bitmap below). > > * > > * init is a 16-bit bitmap used to make sure the user selected a correct > > * configuration as per the specification. > > + * > > + * supported is a 16-bit bitmap used to reflect the hw capabilities. > > */ > > typedef struct { > > - uint16_t map, init; > > + uint16_t map, init, supported; > > } RISCVSATPMap; > > > > struct RISCVCPUConfig { > > -- > > 2.37.2 > > > > Thanks, > drew
On Tue, Jan 24, 2023 at 11:07:53AM +0100, Alexandre Ghiti wrote: > On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Mon, Jan 23, 2023 at 10:03:24AM +0100, Alexandre Ghiti wrote: > > > Currently, the max satp mode is set with the only constraint that it must be > > > implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. > > > > > > But we actually need to add another level of constraint: what the hw is > > > actually capable of, because currently, a linux booting on a sifive-u54 > > > boots in sv57 mode which is incompatible with the cpu's sv39 max > > > capability. > > > > > > So add a new bitmap to RISCVSATPMap which contains this capability and > > > initialize it in every XXX_cpu_init. > > > > > > Finally, we have the following chain of constraints: > > > > > > Qemu capability > HW capability > User choice > Software capability > > > > ^ What software is this? > > I'd think the user's choice would always be last. > > Hmm maybe that's not clear, but I meant that the last constraint was > what the emulated software is capable of handling. Assuming "emulated software" is the guest OS, then OK. How about rewording as target/riscv's general satp mode support constrains what the boards support and the boards constrain what the user may select. The user's selection will then constrain what's available to the guest OS. Thanks, drew
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index e409e6ab64..19a37fee2b 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit) g_assert_not_reached(); } -/* Sets the satp mode to the max supported */ -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit) +static void set_satp_mode_max_supported(RISCVCPU *cpu, + const char *satp_mode_str, + bool is_32_bit) { - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) { - cpu->cfg.satp_mode.map |= - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57")); - } else { - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare")); + uint8_t satp_mode = satp_mode_from_str(satp_mode_str); + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64; + + for (int i = 0; i <= satp_mode; ++i) { + if (valid_vm[i]) { + cpu->cfg.satp_mode.supported |= (1 << i); + } } } +/* Sets the satp mode to the max supported */ +static void set_satp_mode_default(RISCVCPU *cpu) +{ + uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported); + + cpu->cfg.satp_mode.map |= (1 << satp_mode); +} + static void riscv_any_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; + RISCVCPU *cpu = RISCV_CPU(obj); + #if defined(TARGET_RISCV32) set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU); + set_satp_mode_max_supported(cpu, "sv32", true); #elif defined(TARGET_RISCV64) set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU); + set_satp_mode_max_supported(cpu, "sv57", false); #endif set_priv_version(env, PRIV_VERSION_1_12_0); register_cpu_props(obj); @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj) static void rv64_base_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; + RISCVCPU *cpu = RISCV_CPU(obj); + /* We set this in the realise function */ set_misa(env, MXL_RV64, 0); register_cpu_props(obj); /* Set latest version of privileged specification */ set_priv_version(env, PRIV_VERSION_1_12_0); + set_satp_mode_max_supported(cpu, "sv57", false); } static void rv64_sifive_u_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; + RISCVCPU *cpu = RISCV_CPU(obj); + set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); set_priv_version(env, PRIV_VERSION_1_10_0); + set_satp_mode_max_supported(cpu, "sv39", false); } static void rv64_sifive_e_cpu_init(Object *obj) @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj) set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU); set_priv_version(env, PRIV_VERSION_1_10_0); cpu->cfg.mmu = false; + set_satp_mode_max_supported(cpu, "mbare", false); } static void rv128_base_cpu_init(Object *obj) @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj) exit(EXIT_FAILURE); } CPURISCVState *env = &RISCV_CPU(obj)->env; + RISCVCPU *cpu = RISCV_CPU(obj); /* We set this in the realise function */ set_misa(env, MXL_RV128, 0); register_cpu_props(obj); /* Set latest version of privileged specification */ set_priv_version(env, PRIV_VERSION_1_12_0); + set_satp_mode_max_supported(cpu, "sv57", false); } #else static void rv32_base_cpu_init(Object *obj) @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj) register_cpu_props(obj); /* Set latest version of privileged specification */ set_priv_version(env, PRIV_VERSION_1_12_0); + set_satp_mode_max_supported(cpu, "sv32", true); } static void rv32_sifive_u_cpu_init(Object *obj) { CPURISCVState *env = &RISCV_CPU(obj)->env; + RISCVCPU *cpu = RISCV_CPU(obj); + set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU); set_priv_version(env, PRIV_VERSION_1_10_0); + set_satp_mode_max_supported(cpu, "sv32", true); } static void rv32_sifive_e_cpu_init(Object *obj) @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj) set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU); set_priv_version(env, PRIV_VERSION_1_10_0); cpu->cfg.mmu = false; + set_satp_mode_max_supported(cpu, "mbare", true); } static void rv32_ibex_cpu_init(Object *obj) @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj) set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU); set_priv_version(env, PRIV_VERSION_1_11_0); cpu->cfg.mmu = false; + set_satp_mode_max_supported(cpu, "mbare", true); cpu->cfg.epmp = true; } @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj) set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU); set_priv_version(env, PRIV_VERSION_1_10_0); cpu->cfg.mmu = false; + set_satp_mode_max_supported(cpu, "mbare", true); } #endif @@ -696,7 +727,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info) static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) { bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32; - const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64; + uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); + uint8_t satp_mode_supported_max = + satp_mode_max_from_map(cpu->cfg.satp_mode.supported); if (cpu->cfg.satp_mode.map == 0) { /* @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) * satp mode. */ if (cpu->cfg.satp_mode.init == 0) { - set_satp_mode_default(cpu, rv32); + set_satp_mode_default(cpu); } else { /* * Find the lowest level that was disabled and then enable the @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) for (int i = 1; i < 16; ++i) { if (!(cpu->cfg.satp_mode.map & (1 << i)) && (cpu->cfg.satp_mode.init & (1 << i)) && - valid_vm[i]) { + (cpu->cfg.satp_mode.supported & (1 << i))) { for (int j = i - 1; j >= 0; --j) { - if (valid_vm[j]) { + if (cpu->cfg.satp_mode.supported & (1 << j)) { cpu->cfg.satp_mode.map |= (1 << j); break; } @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) } } - /* Make sure the configuration asked is supported by qemu */ - for (int i = 0; i < 16; ++i) { - if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) { - error_setg(errp, "satp_mode %s is not valid", - satp_mode_str(i, rv32)); - return; - } + /* Make sure the user asked for a supported configuration (HW and qemu) */ + if (satp_mode_map_max > satp_mode_supported_max) { + error_setg(errp, "satp_mode %s is higher than hw max capability %s", + satp_mode_str(satp_mode_map_max, rv32), + satp_mode_str(satp_mode_supported_max, rv32)); + return; } /* @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp) * the specification. */ if (!rv32) { - uint8_t satp_mode_max; - - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map); - - for (int i = satp_mode_max - 1; i >= 0; --i) { + for (int i = satp_mode_map_max - 1; i >= 0; --i) { if (!(cpu->cfg.satp_mode.map & (1 << i)) && (cpu->cfg.satp_mode.init & (1 << i)) && - valid_vm[i]) { + (cpu->cfg.satp_mode.supported & (1 << i))) { error_setg(errp, "cannot disable %s satp mode if %s " "is enabled", satp_mode_str(i, false), - satp_mode_str(satp_mode_max, false)); + satp_mode_str(satp_mode_map_max, false)); return; } } diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index e37177db5c..b591122099 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -416,13 +416,17 @@ struct RISCVCPUClass { /* * map is a 16-bit bitmap: the most significant set bit in map is the maximum - * satp mode that is supported. + * satp mode that is supported. It may be chosen by the user and must respect + * what qemu implements (valid_1_10_32/64) and what the hw is capable of + * (supported bitmap below). * * init is a 16-bit bitmap used to make sure the user selected a correct * configuration as per the specification. + * + * supported is a 16-bit bitmap used to reflect the hw capabilities. */ typedef struct { - uint16_t map, init; + uint16_t map, init, supported; } RISCVSATPMap; struct RISCVCPUConfig {
Currently, the max satp mode is set with the only constraint that it must be implemented in qemu, i.e. set in valid_vm_1_10_[32|64]. But we actually need to add another level of constraint: what the hw is actually capable of, because currently, a linux booting on a sifive-u54 boots in sv57 mode which is incompatible with the cpu's sv39 max capability. So add a new bitmap to RISCVSATPMap which contains this capability and initialize it in every XXX_cpu_init. Finally, we have the following chain of constraints: Qemu capability > HW capability > User choice > Software capability Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> --- target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++--------------- target/riscv/cpu.h | 8 +++-- 2 files changed, 59 insertions(+), 27 deletions(-)