Re: [PATCH v2] target/riscv: Don't expose the CPU properties on names CPUs

[Alistair Francis]

On Wed, Jun 8, 2022 at 9:27 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Jun 1, 2022 at 9:37 AM Alistair Francis
> <alistair.francis@opensource.wdc.com> wrote:
> >
> > From: Alistair Francis <alistair.francis@wdc.com>
> >
> > There are currently two types of RISC-V CPUs:
> >  - Generic CPUs (base or any) that allow complete custimisation
> >  - "Named" CPUs that match existing hardware
> >
> > Users can use the base CPUs to custimise the extensions that they want, for
> > example -cpu rv64,v=true.
> >
> > We originally exposed these as part of the named CPUs as well, but that was
> > by accident.
> >
> > Exposing the CPU properties to named CPUs means that we accidently
> > enable extensinos that don't exist on the CPUs by default. For example
>
> typo: extensions
>
> > the SiFive E CPU currently support the zba extension, which is a bug.
> >
> > This patch instead only expose the CPU extensions to the generic CPUs.
>
> exposes
>
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/cpu.c | 57 +++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 46 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index a91253d4bd..c0c0f7e71f 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -118,6 +118,8 @@ static const char * const riscv_intr_names[] = {
> >      "reserved"
> >  };
> >
> > +static void register_cpu_props(DeviceState *dev);
>
> nits: please move the whole static function implementation here to
> avoid the forward declaration

I did try that but the function relies on the `riscv_cpu_extensions`
array, which is defined later so unless we move the properties up to
the top of this file we need to have a declaration here. I think
keeping the properties where they are makes sense as that follows the
usual QEMU layout.

Alistair

>
> > +
> >  const char *riscv_cpu_get_trap_name(target_ulong cause, bool async)
> >  {
> >      if (async) {
> > @@ -161,6 +163,7 @@ static void riscv_any_cpu_init(Object *obj)
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> >  #endif
> >      set_priv_version(env, PRIV_VERSION_1_12_0);
> > +    register_cpu_props(DEVICE(obj));
> >  }
> >
> >  #if defined(TARGET_RISCV64)
> > @@ -169,6 +172,7 @@ static void rv64_base_cpu_init(Object *obj)
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> >      /* We set this in the realise function */
> >      set_misa(env, MXL_RV64, 0);
> > +    register_cpu_props(DEVICE(obj));
> >  }
> >
> >  static void rv64_sifive_u_cpu_init(Object *obj)
> > @@ -181,9 +185,11 @@ static void rv64_sifive_u_cpu_init(Object *obj)
> >  static void rv64_sifive_e_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    qdev_prop_set_bit(DEVICE(obj), "mmu", false);
> > +    cpu->cfg.mmu = false;
> >  }
> >
> >  static void rv128_base_cpu_init(Object *obj)
> > @@ -197,6 +203,7 @@ static void rv128_base_cpu_init(Object *obj)
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> >      /* We set this in the realise function */
> >      set_misa(env, MXL_RV128, 0);
> > +    register_cpu_props(DEVICE(obj));
> >  }
> >  #else
> >  static void rv32_base_cpu_init(Object *obj)
> > @@ -204,6 +211,7 @@ static void rv32_base_cpu_init(Object *obj)
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> >      /* We set this in the realise function */
> >      set_misa(env, MXL_RV32, 0);
> > +    register_cpu_props(DEVICE(obj));
> >  }
> >
> >  static void rv32_sifive_u_cpu_init(Object *obj)
> > @@ -216,27 +224,33 @@ static void rv32_sifive_u_cpu_init(Object *obj)
> >  static void rv32_sifive_e_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    qdev_prop_set_bit(DEVICE(obj), "mmu", false);
> > +    cpu->cfg.mmu = false;
> >  }
> >
> >  static void rv32_ibex_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> > -    qdev_prop_set_bit(DEVICE(obj), "mmu", false);
> > -    qdev_prop_set_bit(DEVICE(obj), "x-epmp", true);
> > +    cpu->cfg.mmu = false;
> > +    cpu->cfg.epmp = true;
> >  }
> >
> >  static void rv32_imafcu_nommu_cpu_init(Object *obj)
> >  {
> >      CPURISCVState *env = &RISCV_CPU(obj)->env;
> > +    RISCVCPU *cpu = RISCV_CPU(obj);
> > +
> >      set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
> >      set_priv_version(env, PRIV_VERSION_1_10_0);
> >      set_resetvec(env, DEFAULT_RSTVEC);
> > -    qdev_prop_set_bit(DEVICE(obj), "mmu", false);
> > +    cpu->cfg.mmu = false;
> >  }
> >  #endif
> >
> > @@ -249,6 +263,7 @@ static void riscv_host_cpu_init(Object *obj)
> >  #elif defined(TARGET_RISCV64)
> >      set_misa(env, MXL_RV64, 0);
> >  #endif
> > +    register_cpu_props(DEVICE(obj));
> >  }
> >  #endif
> >
> > @@ -831,6 +846,12 @@ static void riscv_cpu_init(Object *obj)
> >  {
> >      RISCVCPU *cpu = RISCV_CPU(obj);
> >
> > +    cpu->cfg.ext_counters = true;
> > +    cpu->cfg.ext_ifencei = true;
> > +    cpu->cfg.ext_icsr = true;
> > +    cpu->cfg.mmu = true;
> > +    cpu->cfg.pmp = true;
> > +
> >      cpu_set_cpustate_pointers(cpu);
> >
> >  #ifndef CONFIG_USER_ONLY
> > @@ -839,7 +860,7 @@ static void riscv_cpu_init(Object *obj)
> >  #endif /* CONFIG_USER_ONLY */
> >  }
> >
> > -static Property riscv_cpu_properties[] = {
> > +static Property riscv_cpu_extensions[] = {
> >      /* Defaults for standard extensions */
> >      DEFINE_PROP_BOOL("i", RISCVCPU, cfg.ext_i, true),
> >      DEFINE_PROP_BOOL("e", RISCVCPU, cfg.ext_e, false),
> > @@ -862,17 +883,12 @@ static Property riscv_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("Zve64f", RISCVCPU, cfg.ext_zve64f, false),
> >      DEFINE_PROP_BOOL("mmu", RISCVCPU, cfg.mmu, true),
> >      DEFINE_PROP_BOOL("pmp", RISCVCPU, cfg.pmp, true),
> > -    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
> >
> >      DEFINE_PROP_STRING("priv_spec", RISCVCPU, cfg.priv_spec),
> >      DEFINE_PROP_STRING("vext_spec", RISCVCPU, cfg.vext_spec),
> >      DEFINE_PROP_UINT16("vlen", RISCVCPU, cfg.vlen, 128),
> >      DEFINE_PROP_UINT16("elen", RISCVCPU, cfg.elen, 64),
> >
> > -    DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0),
> > -    DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, 
> > RISCV_CPU_MARCHID),
> > -    DEFINE_PROP_UINT64("mimpid", RISCVCPU, cfg.mimpid, RISCV_CPU_MIMPID),
> > -
> >      DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
> >      DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
> >      DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
> > @@ -909,6 +925,25 @@ static Property riscv_cpu_properties[] = {
> >      DEFINE_PROP_BOOL("x-epmp", RISCVCPU, cfg.epmp, false),
> >      DEFINE_PROP_BOOL("x-aia", RISCVCPU, cfg.aia, false),
> >
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void register_cpu_props(DeviceState *dev)
> > +{
> > +    Property *prop;
> > +
> > +    for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
> > +        qdev_property_add_static(dev, prop);
> > +    }
> > +}
> > +
> > +static Property riscv_cpu_properties[] = {
> > +    DEFINE_PROP_BOOL("debug", RISCVCPU, cfg.debug, true),
> > +
> > +    DEFINE_PROP_UINT32("mvendorid", RISCVCPU, cfg.mvendorid, 0),
> > +    DEFINE_PROP_UINT64("marchid", RISCVCPU, cfg.marchid, 
> > RISCV_CPU_MARCHID),
> > +    DEFINE_PROP_UINT64("mimpid", RISCVCPU, cfg.mimpid, RISCV_CPU_MIMPID),
> > +
> >      DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC),
> >
> >      DEFINE_PROP_BOOL("short-isa-string", RISCVCPU, cfg.short_isa_string, 
> > false),
> > --
>
> Otherwise LGTM.
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> Regards,
> Bin