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(+)

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);
 }
 
-- 
2.41.0

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