On Mon, Jan 23, 2023 at 11:51 AM Andrew Jones <ajo...@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 <alexgh...@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