Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode

[Andrew Jones]

On Wed, Jan 18, 2023 at 05:29:43PM +0100, Alexandre Ghiti wrote:
> Hey Andrew,
> 
> On Tue, Jan 17, 2023 at 5:31 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Fri, Jan 13, 2023 at 11:34:53AM +0100, Alexandre Ghiti wrote:
> > > RISC-V specifies multiple sizes for addressable memory and Linux probes 
> > > for
> > > the machine's support at startup via the satp CSR register (done in
> > > csr.c:validate_vm).
> > >
> > > As per the specification, sv64 must support sv57, which in turn must
> > > support sv48...etc. So we can restrict machine support by simply setting 
> > > the
> > > "highest" supported mode and the bare mode is always supported.
> > >
> > > You can set the satp mode using the new properties "sv32", "sv39", "sv48",
> > > "sv57" and "sv64" as follows:
> > > -cpu rv64,sv57=on  # Linux will boot using sv57 scheme
> > > -cpu rv64,sv39=on  # Linux will boot using sv39 scheme
> > > -cpu rv64,sv57=off # Linux will boot using sv48 scheme
> > > -cpu rv64          # Linux will boot using sv57 scheme by default
> > >
> > > We take the highest level set by the user:
> > > -cpu rv64,sv48=on,sv57=on # Linux will boot using sv57 scheme
> > >
> > > We make sure that invalid configurations are rejected:
> > > -cpu rv64,sv32=on # Can't enable 32-bit satp mode in 64-bit
> > > -cpu rv64,sv39=off,sv48=on # sv39 must be supported if higher modes are
> > >                            # enabled
> > >
> > > We accept "redundant" configurations:
> > > -cpu rv64,sv48=on,sv57=off # Linux will boot using sv48 scheme
> > > -cpu rv64,sv32=on,sv32=off # Linux will boot using sv57 scheme (the 
> > > default)
> > >
> > > In addition, we now correctly set the device-tree entry 'mmu-type' using
> > > those new properties.
> > >
> > > Co-Developed-by: Ludovic Henry <ludovic@rivosinc.com>
> > > Signed-off-by: Ludovic Henry <ludovic@rivosinc.com>
> > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > ---
> > >  hw/riscv/virt.c    |  19 ++--
> > >  target/riscv/cpu.c | 221 +++++++++++++++++++++++++++++++++++++++++++++
> > >  target/riscv/cpu.h |  19 ++++
> > >  target/riscv/csr.c |  17 +++-
> > >  4 files changed, 262 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > index 94ff2a1584..48d034a5f7 100644
> > > --- a/hw/riscv/virt.c
> > > +++ b/hw/riscv/virt.c
> > > @@ -228,7 +228,8 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, 
> > > int socket,
> > >      int cpu;
> > >      uint32_t cpu_phandle;
> > >      MachineState *mc = MACHINE(s);
> > > -    char *name, *cpu_name, *core_name, *intc_name;
> > > +    uint8_t satp_mode_max;
> > > +    char *name, *cpu_name, *core_name, *intc_name, *sv_name;
> > >
> > >      for (cpu = s->soc[socket].num_harts - 1; cpu >= 0; cpu--) {
> > >          cpu_phandle = (*phandle)++;
> > > @@ -236,14 +237,14 @@ static void create_fdt_socket_cpus(RISCVVirtState 
> > > *s, int socket,
> > >          cpu_name = g_strdup_printf("/cpus/cpu@%d",
> > >              s->soc[socket].hartid_base + cpu);
> > >          qemu_fdt_add_subnode(mc->fdt, cpu_name);
> > > -        if (riscv_feature(&s->soc[socket].harts[cpu].env,
> > > -                          RISCV_FEATURE_MMU)) {
> > > -            qemu_fdt_setprop_string(mc->fdt, cpu_name, "mmu-type",
> > > -                                    (is_32_bit) ? "riscv,sv32" : 
> > > "riscv,sv48");
> >
> > I just noticed that for the virt machine type, when the user doesn't
> > provide a satp mode cpu property on the command line, and hence gets
> > the default mode, they'll be silently changed from sv48 to sv57. That
> > default change should be a separate patch which comes after this one.
> > BTW, why sv57 and not sv48 or sv64?
> 
> The device tree entry should match the max available satp mode even
> though it makes little sense to have this entry in the first place:
> the max satp mode is easily discoverable at runtime (the kernel does
> that and does not care about the device tree entry).
> 
> But yes, this fix was mentioned at the very end of the commit log,
> which was weird anyway, so I'll move that to its own patch.

Ah, I interpreted that part of the commit message as simply pointing
out that the mmu-type is getting set per the user's input. Thanks for
moving this change to another patch.

...
> > > +
> > > +    if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> > > +        set_satp_mode(cpu, is_32_bit ? "sv32" : "sv57");
> > > +    } else {
> > > +        set_satp_mode(cpu, "mbare");
> >
> > nit: Could probably integrate set_satp_mode() into this function since
> > this function is the only place it's used.
> 
> At the moment yes, but this was a request from Frank to have a helper
> set the default satp mode in the cpu init functions, which I did not
> do here because I was unsure: @Frank Chang What should I use for
> sifive_e and sifive_u? rv64 will use the default mode.

The sifive stuff should probably be a separate patch. If that patch will
be part of this series then the proactive refactoring makes sense as we
can immediately see the users.

...
> > Why isn't all this 'if (cpu->cfg.satp_mode.map == 0)' block above at the
> > top of riscv_cpu_satp_mode_finalize() instead of here?
> >
> 
> Because the realize function seemed to do the properties processing
> and I thought the finalize one was meant to check the consistency of
> the configuration that resulted: I can change that if you don't agree.

finalize should do all the processing and checking, basically everything
not done in the property's set function. realize should call
finalize_features, which then calls each feature's finalize. Take a look
at arm's call chain, for example

 arm_cpu_realizefn
   arm_cpu_finalize_features
     arm_cpu_sve_finalize

Thanks,
drew