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

[Andrew Jones <ajones@ventanamicro.com>]

On Fri, Jan 20, 2023 at 01:44:41PM +0100, Alexandre Ghiti wrote:
> On Fri, Jan 20, 2023 at 10:53 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Fri, Jan 20, 2023 at 09:46:05AM +1000, Alistair Francis wrote:
> > > On Thu, Jan 19, 2023 at 11:00 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> > > >
> > > > Hi Alistair, Andrew,
> > > >
> > > > On Thu, Jan 19, 2023 at 1:25 AM Alistair Francis <alistair23@gmail.com> wrote:
> > > > >
> > > > > On Wed, Jan 18, 2023 at 10:19 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 18, 2023 at 10:28:46AM +1000, Alistair Francis wrote:
> > > > > > > On Wed, Jan 18, 2023 at 2:32 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Jan 13, 2023 at 11:34:53AM +0100, Alexandre Ghiti wrote:
> > > > > > ...
> > > > > > > > > +
> > > > > > > > > +    /* Get rid of 32-bit/64-bit incompatibility */
> > > > > > > > > +    for (int i = 0; i < 16; ++i) {
> > > > > > > > > +        if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) {
> > > > > > > >
> > > > > > > > If we ever define mode=1 for rv64, then 'sv32=on' will be incorrectly
> > > > > > > > accepted as an alias. I think we should simply not define the sv32
> > > > > > > > property for rv64 nor the rv64-only modes for rv32. So, down in
> > > > > > > > riscv_add_satp_mode_properties() we can add some
> > > > > > > >
> > > > > > > >   #if defined(TARGET_RISCV32)
> > > > > > > >   ...
> > > > > > > >   #elif defined(TARGET_RISCV64)
> > > > > > > >   ...
> > > > > > > >   #endif
> > > > > > >
> > > > > > > Do not add any #if defined(TARGET_RISCV32) to QEMU.
> > > > > > >
> > > > > > > We are aiming for the riscv64-softmmu to be able to emulate 32-bit
> > > > > > > CPUs and compile time macros are the wrong solution here. Instead you
> > > > > > > can get the xlen of the hart and use that.
> > > > > > >
> > > > > >
> > > > > > Does this mean we want to be able to do the following?
> > > > > >
> > > > > >   qemu-system-riscv64 -cpu rv32,sv32=on ...
> > > > >
> > > > > That's the plan
> > > > >
> > > > > >
> > > > > > If so, then can we move the object_property_add() for sv32 to
> > > > > > rv32_base_cpu_init() and the rest to rv64_base_cpu_init()?
> > >
> > > Wait! Sorry I didn't read this carefully enough. No, that is not what
> > > we want to do. That then won't support the vendor CPUs.
> > >
> > > We just want to add the properties to all CPUs. Then if an invalid
> > > option is set we should return an error.
> 
> Maybe I just don't get this part...

Indeed, I like not adding the property at all over adding it and then
complaining when it's used. Your solution below looks good to me and
would be my preference as well.

Thanks,
drew

> 
> > >
> > > Note that the 64-bit only configs can be hidden behind a #if
> > > defined(TARGET_RISCV64).
> >
> > OK, so we want the original suggestion of putting an
> > 'if defined(TARGET_RISCV64)' in riscv_add_satp_mode_properties(),
> > which is called from register_cpu_props(), for the 64-bit only
> > configs, but to support emulation we can't put sv32 under an
> > 'if defined(TARGET_RISCV32)'. Instead, we need to check the xlen
> > supported by the cpu type. That makes sense to me, and I think
> > it'd be easiest to do in cpu_riscv_set_satp() with something like
> >
> >   if (!strncmp(name, "rv32", 4) &&
> >       RISCV_CPU(obj)->env.misa_mxl != MXL_RV32) {
> >      ... fail with error message ...
> >   }
> >
> 
> ...but what about simply using the runtime check when we add the
> properties? Like this:
> 
> static void riscv_add_satp_mode_properties(Object *obj)
> {
>     RISCVCPU *cpu = RISCV_CPU(obj);
> 
>     if (cpu->env.misa_mxl == MXL_RV32) {
>         object_property_add(obj, "sv32", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>     } else {
>         object_property_add(obj, "sv39", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>         object_property_add(obj, "sv48", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>         object_property_add(obj, "sv57", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>         object_property_add(obj, "sv64", "bool", cpu_riscv_get_satp,
>                             cpu_riscv_set_satp, NULL,
> &cpu->cfg.satp_mode);
>     }
> }
> 
> > Thanks,
> > drew