On Mon, Jan 23, 2023 at 2:31 PM Andrew Jones <ajo...@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 <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? > > 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