qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities


From: Alexandre Ghiti
Subject: Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities
Date: Tue, 24 Jan 2023 14:13:35 +0100

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]