[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode
From: |
Alexandre Ghiti |
Subject: |
Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode |
Date: |
Fri, 20 Jan 2023 13:44:41 +0100 |
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...
> >
> > 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
- Re: [PATCH v5 1/2] riscv: Pass Object to register_cpu_props instead of DeviceState, (continued)
- [PATCH v5 2/2] riscv: Allow user to set the satp mode, Alexandre Ghiti, 2023/01/13
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Andrew Jones, 2023/01/17
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Alistair Francis, 2023/01/17
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Andrew Jones, 2023/01/18
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Alistair Francis, 2023/01/18
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Alexandre Ghiti, 2023/01/19
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Andrew Jones, 2023/01/19
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Alistair Francis, 2023/01/19
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Andrew Jones, 2023/01/20
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode,
Alexandre Ghiti <=
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Andrew Jones, 2023/01/20
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Alexandre Ghiti, 2023/01/18
- Re: [PATCH v5 2/2] riscv: Allow user to set the satp mode, Andrew Jones, 2023/01/18