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.
Thanks,
drew
