riscv_cpu_disable_priv_spec_isa_exts(), at the end of
riscv_cpu_validate_set_extensions(), will disable cpu->cfg.ext_h and
cpu->cfg.ext_v if priv_ver check fails.

This check can be done in riscv_cpu_validate_misa_ext(). The difference
here is that we're not silently disable it: we'll error out. Silently
disabling a MISA extension after all the validation is completed can can
cause inconsistencies that we don't have to deal with. Verify ealier and
fail faster.

Note that we're ignoring RVV priv_ver validation since its minimal priv
is also the minimal value we support. RVH will error out if enabled
under priv_ver under 1_12_0.

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 399f63b42f..d2eb2b3ba1 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1055,6 +1055,20 @@ static void riscv_cpu_validate_misa_ext(RISCVCPU *cpu, Error **errp)
         return;
     }
 
+    /*
+     * Check for priv spec version. RVH is 1_12_0, RVV is 1_10_0.
+     * We don't support anything under 1_10_0 so skip checking
+     * priv for RVV.
+     *
+     * We're hardcoding it here to avoid looping into the
+     * 50+ entries of isa_edata_arr[] just to check the RVH
+     * entry.
+     */
+    if (cpu->cfg.ext_h && env->priv_ver < PRIV_VERSION_1_12_0) {
+        error_setg(errp, "H extension requires priv spec 1.12.0");
+        return;
+    }
+
     if (cpu->cfg.ext_v) {
         riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
         if (local_err != NULL) {
-- 
2.39.2

On 2023/3/23 06:19, Daniel Henrique Barboza wrote:
> riscv_cpu_disable_priv_spec_isa_exts(), at the end of
> riscv_cpu_validate_set_extensions(), will disable cpu->cfg.ext_h and
> cpu->cfg.ext_v if priv_ver check fails.
>
> This check can be done in riscv_cpu_validate_misa_ext(). The difference
> here is that we're not silently disable it: we'll error out. Silently
> disabling a MISA extension after all the validation is completed can can
> cause inconsistencies that we don't have to deal with. Verify ealier and
> fail faster.
>
> Note that we're ignoring RVV priv_ver validation since its minimal priv
> is also the minimal value we support. RVH will error out if enabled
> under priv_ver under 1_12_0.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 399f63b42f..d2eb2b3ba1 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1055,6 +1055,20 @@ static void riscv_cpu_validate_misa_ext(RISCVCPU *cpu, Error **errp)
>           return;
>       }
>   
> +    /*
> +     * Check for priv spec version. RVH is 1_12_0, RVV is 1_10_0.
> +     * We don't support anything under 1_10_0 so skip checking
> +     * priv for RVV.
> +     *
> +     * We're hardcoding it here to avoid looping into the
> +     * 50+ entries of isa_edata_arr[] just to check the RVH
> +     * entry.
> +     */
> +    if (cpu->cfg.ext_h && env->priv_ver < PRIV_VERSION_1_12_0) {
> +        error_setg(errp, "H extension requires priv spec 1.12.0");
> +        return;
> +    }
The other multi-letter extensions are directly disabled for lower priv 
version with warning message.

Whether we should do the similar action here?

Regards,

Weiwei Li

> +
>       if (cpu->cfg.ext_v) {
>           riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
>           if (local_err != NULL) {

On 3/24/23 11:56, liweiwei wrote:
> 
> On 2023/3/23 06:19, Daniel Henrique Barboza wrote:
>> riscv_cpu_disable_priv_spec_isa_exts(), at the end of
>> riscv_cpu_validate_set_extensions(), will disable cpu->cfg.ext_h and
>> cpu->cfg.ext_v if priv_ver check fails.
>>
>> This check can be done in riscv_cpu_validate_misa_ext(). The difference
>> here is that we're not silently disable it: we'll error out. Silently
>> disabling a MISA extension after all the validation is completed can can
>> cause inconsistencies that we don't have to deal with. Verify ealier and
>> fail faster.
>>
>> Note that we're ignoring RVV priv_ver validation since its minimal priv
>> is also the minimal value we support. RVH will error out if enabled
>> under priv_ver under 1_12_0.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 399f63b42f..d2eb2b3ba1 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1055,6 +1055,20 @@ static void riscv_cpu_validate_misa_ext(RISCVCPU *cpu, Error **errp)
>>           return;
>>       }
>> +    /*
>> +     * Check for priv spec version. RVH is 1_12_0, RVV is 1_10_0.
>> +     * We don't support anything under 1_10_0 so skip checking
>> +     * priv for RVV.
>> +     *
>> +     * We're hardcoding it here to avoid looping into the
>> +     * 50+ entries of isa_edata_arr[] just to check the RVH
>> +     * entry.
>> +     */
>> +    if (cpu->cfg.ext_h && env->priv_ver < PRIV_VERSION_1_12_0) {
>> +        error_setg(errp, "H extension requires priv spec 1.12.0");
>> +        return;
>> +    }
> The other multi-letter extensions are directly disabled for lower priv version with warning message.
> 
> Whether we should do the similar action here?

I'd rather error out in all cases, to be honest. cpu_init() functions should be
responsible for choosing an adequate priv spec level for the extensions it wants
to use.


Thanks,


Daniel

> 
> Regards,
> 
> Weiwei Li
> 
>> +
>>       if (cpu->cfg.ext_v) {
>>           riscv_cpu_validate_v(env, &cpu->cfg, &local_err);
>>           if (local_err != NULL) {
>