| Message ID | 20231102224445.527355-4-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | rv64i and rva22u64 CPUs, RVA22U64 profile support | expand |
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. 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
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
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. + */ + cpu_validate_multi_ext_priv_ver(&cpu->env, multi_ext_cfg->offset); + } + isa_ext_update_enabled(cpu, multi_ext_cfg->offset, value); }
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. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/tcg/tcg-cpu.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)