Re: [PATCH for-9.0 2/7] target/riscv: add priv ver restriction to profiles

[Daniel Henrique Barboza]

On 11/24/23 06:57, Andrew Jones wrote:
> On Thu, Nov 23, 2023 at 04:15:27PM -0300, Daniel Henrique Barboza wrote:
> > Some profiles, like RVA22S64, has a priv_spec requirement.
> >
> > Make this requirement explicit for all profiles. We'll validate this
> > requirement finalize() time and, in case the user chooses an
> > incompatible priv_spec while activating a profile, a warning will be
> > shown.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/cpu.c         |  1 +
> >   target/riscv/cpu.h         |  2 ++
> >   target/riscv/tcg/tcg-cpu.c | 30 ++++++++++++++++++++++++++++++
> >   3 files changed, 33 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 59b131c1fc..29a9f77702 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1537,6 +1537,7 @@ Property riscv_cpu_options[] = {
> >   static RISCVCPUProfile RVA22U64 = {
> >       .name = "rva22u64",
> >       .misa_ext = RVI | RVM | RVA | RVF | RVD | RVC | RVU,
> > +    .priv_spec = RISCV_PROFILE_ATTR_UNUSED,
> >       .ext_offsets = {
> >           CPU_CFG_OFFSET(ext_zicsr), CPU_CFG_OFFSET(ext_zihintpause),
> >           CPU_CFG_OFFSET(ext_zba), CPU_CFG_OFFSET(ext_zbb),
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 5ff629650d..1f34eda1e4 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -81,10 +81,12 @@ typedef struct riscv_cpu_profile {
> >       uint32_t misa_ext;
> >       bool enabled;
> >       bool user_set;
> > +    int priv_spec;
> >       const int32_t ext_offsets[];
> >   } RISCVCPUProfile;
> >   
> >   #define RISCV_PROFILE_EXT_LIST_END -1
> > +#define RISCV_PROFILE_ATTR_UNUSED -1
> >   
> >   extern RISCVCPUProfile *riscv_profiles[];
> >   
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index ddf37b25f3..a26cc6f093 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -74,6 +74,20 @@ static void riscv_cpu_write_misa_bit(RISCVCPU *cpu, uint32_t 
> > bit,
> >       }
> >   }
> >   
> > +static const char *cpu_priv_ver_to_str(int priv_ver)
> > +{
> > +    switch (priv_ver) {
> > +    case PRIV_VERSION_1_10_0:
> > +        return "v1.10.0";
> > +    case PRIV_VERSION_1_11_0:
> > +        return "v1.11.0";
> > +    case PRIV_VERSION_1_12_0:
> > +        return "v1.12.0";
> > +    }
> > +
> > +    g_assert_not_reached();
> > +}
> > +
> >   static void riscv_cpu_synchronize_from_tb(CPUState *cs,
> >                                             const TranslationBlock *tb)
> >   {
> > @@ -764,11 +778,23 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> > Error **errp)
> >   static void riscv_cpu_validate_profile(RISCVCPU *cpu,
> >                                          RISCVCPUProfile *profile)
> >   {
> > +    CPURISCVState *env = &cpu->env;
> >       const char *warn_msg = "Profile %s mandates disabled extension %s";
> >       bool send_warn = profile->user_set && profile->enabled;
> >       bool profile_impl = true;
> >       int i;
> >   
> > +    if (profile->priv_spec != env->priv_ver) {
>
> Shouldn't this be something like
>
>   if (profile->priv_spec != RISCV_PROFILE_ATTR_UNUSED &&
>       profile->priv_spec != env->priv_ver)

Yeah it should. Otherwise we might send warnings for cases in which the profile
doesn't care about priv_ver. I'll fix it in v2.


Thanks,


Daniel

> > +        profile_impl = false;
> > +
> > +        if (send_warn) {
> > +            warn_report("Profile %s requires priv spec %s, "
> > +                        "but priv ver %s was set", profile->name,
> > +                        cpu_priv_ver_to_str(profile->priv_spec),
> > +                        cpu_priv_ver_to_str(env->priv_ver));
> > +        }
> > +    }
> > +
> >       for (i = 0; misa_bits[i] != 0; i++) {
> >           uint32_t bit = misa_bits[i];
> >   
> > @@ -1057,6 +1083,10 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name,
> >       profile->user_set = true;
> >       profile->enabled = value;
> >   
> > +    if (profile->enabled) {
> > +        cpu->env.priv_ver = profile->priv_spec;
> > +    }
> > +
> >       for (i = 0; misa_bits[i] != 0; i++) {
> >           uint32_t bit = misa_bits[i];
> >   
> > --
> > 2.41.0
>
> Thanks,
> drew