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

[Andrew Jones]

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.
> 
> 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 ...
  }

Thanks,
drew