On Fri, Jan 20, 2023 at 10:53 AM Andrew Jones <ajo...@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 <alexgh...@rivosinc.com> > > wrote: > > > > > > Hi Alistair, Andrew, > > > > > > On Thu, Jan 19, 2023 at 1:25 AM Alistair Francis <alistai...@gmail.com> > > > wrote: > > > > > > > > On Wed, Jan 18, 2023 at 10:19 PM Andrew Jones <ajo...@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 > > > > > > <ajo...@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