Re: [PATCH v9 03/19] target/riscv/tcg: update priv_ver on user_set extensions

[Daniel Henrique Barboza]

On 11/3/23 05:33, Andrew Jones wrote:
> On Thu, Nov 02, 2023 at 07:44:29PM -0300, Daniel Henrique Barboza wrote:
> > We'll add a new bare CPU type that won't have any default priv_ver. This
> > means that the CPU will default to priv_ver = 0, i.e. 1.10.0.
> >
> > At the same we'll allow these CPUs to enable extensions at will, but
> > then, if the extension has a priv_ver newer than 1.10, we'll end up
> > disabling it. Users will then need to manually set priv_ver to something
> > other than 1.10 to enable the extensions they want, which is not ideal.
> >
> > Change the setter() of extensions to allow user enabled extensions to
> > bump the priv_ver of the CPU. This will make it convenient for users to
> > enable extensions for CPUs that doesn't set a default priv_ver.
> >
> > This change does not affect any existing CPU: vendor CPUs does not allow
> > extensions to be enabled, and generic CPUs are already set to priv_ver
> > LATEST.
>
> The only problem I see is that priv_ver will be silently bumped for any
> CPU type which accepts extensions. While generic CPUs currently always
> select LATEST, meaning it doesn't matter, and the new bare CPU type needs
> this feature, we'll also eventually have CPU types that set a priv_ver
> in their definition which should not be changed. For example, when the
> rva22s64 profile is introduced it will set its priv_ver to 1.12, as
> mandated by the profile, if a user then adds an extension which requires
> a later profile, its priv_ver will get silently bumped. Maybe that won't
> matter, though, because later in realize we'll check that Ss1p12 is true
> and when it's false we'll complain that the profile is not compliant?
> Or maybe I'm reading the profile spec too strictly and/or am too
> pessimistic about later specs being backwards compatible. If we believe
> later specs are always compatible with older, then we could advertise
> compliance with a profile which mandates 1.12 as long as its spec is 1.12
> or later

TBH I have no idea if a profile that demands priv spec 1.12 (like rva22s64, the
profile we're adding next) would be compliant with a CPU that runs a newer priv
spec. We'll have to cross that bridge when we come to it I guess ...


Thanks,

Daniel

.
> Anyway, just food for thought. I think we can address this later when
> we get the first CPU type which sets a priv_ver and accepts extensions.
>
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/tcg/tcg-cpu.c | 32 ++++++++++++++++++++++++++++++++
> >   1 file changed, 32 insertions(+)
> >
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index 08f8dded56..0e684ab86f 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -114,6 +114,22 @@ static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
> >       g_assert_not_reached();
> >   }
> >   
> > +static void cpu_validate_multi_ext_priv_ver(CPURISCVState *env,
> > +                                            uint32_t ext_offset)
> > +{
> > +    int ext_priv_ver;
> > +
> > +    if (env->priv_ver == PRIV_VERSION_LATEST) {
> > +        return;
> > +    }
> > +
> > +    ext_priv_ver = cpu_cfg_ext_get_min_version(ext_offset);
> > +
> > +    if (env->priv_ver < ext_priv_ver) {
> > +        env->priv_ver = ext_priv_ver;
> > +    }
> > +}
> > +
> >   static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
> >                                       bool value)
> >   {
> > @@ -742,6 +758,14 @@ static void cpu_set_misa_ext_cfg(Object *obj, Visitor *v, 
> > const char *name,
> >               return;
> >           }
> >   
> > +        if (misa_bit == RVH && env->priv_ver < PRIV_VERSION_1_12_0) {
> > +            /*
> > +             * Note: the 'priv_spec' command line option, if present,
> > +             * will take precedence over this priv_ver bump.
> > +             */
> > +            env->priv_ver = PRIV_VERSION_1_12_0;
> > +        }
> > +
> >           env->misa_ext |= misa_bit;
> >           env->misa_ext_mask |= misa_bit;
> >       } else {
> > @@ -871,6 +895,14 @@ static void cpu_set_multi_ext_cfg(Object *obj, Visitor *v, 
> > const char *name,
> >           return;
> >       }
> >   
> > +    if (value) {
> > +        /*
> > +         * Note: the 'priv_spec' command line option, if present,
> > +         * will take precedence over this priv_ver bump.
> > +         */
>
> The above comment would be better in cpu_validate_multi_ext_priv_ver() at
> the line where the bumping is done.
>
> > +        cpu_validate_multi_ext_priv_ver(&cpu->env, multi_ext_cfg->offset);
> > +    }
> > +
> >       isa_ext_update_enabled(cpu, multi_ext_cfg->offset, value);
> >   }
> >   
> > --
> > 2.41.0
>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>
> Thanks,
> drew