target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version

[PATCH] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version

Posted by Weiwei Li 1 year ago

Using implicitly enabled extensions such as Zca/Zcf/Zcd instead of their
super extensions can simplify the extension related check. However, they
may have higher priv version than their super extensions. So we should mask
them in the isa_string based on priv version to make them invisible to user
if the specified priv version is lower than their minimal priv version.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index cb68916fce..1a5099382c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1709,6 +1709,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
 
     for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
         if (isa_edata_arr[i].multi_letter &&
+            (cpu->env.priv_ver >= isa_edata_arr[i].min_version) &&
             isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
             new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
             g_free(old);
-- 
2.25.1

Re: [PATCH] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version

Posted by Daniel Henrique Barboza 1 year ago


On 4/7/23 00:30, Weiwei Li wrote:
> Using implicitly enabled extensions such as Zca/Zcf/Zcd instead of their
> super extensions can simplify the extension related check. However, they
> may have higher priv version than their super extensions. So we should mask
> them in the isa_string based on priv version to make them invisible to user
> if the specified priv version is lower than their minimal priv version.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


And I'll fold it into the next version of "[PATCH v6 0/9] target/riscv: rework
CPU extensions validation" to fix the sifive break I'm experiencing there.



Thanks,


Daniel

>   target/riscv/cpu.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cb68916fce..1a5099382c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1709,6 +1709,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>   
>       for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
>           if (isa_edata_arr[i].multi_letter &&
> +            (cpu->env.priv_ver >= isa_edata_arr[i].min_version) &&
>               isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
>               new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
>               g_free(old);

Re: [PATCH] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version

Posted by Daniel Henrique Barboza 1 year ago

On 4/7/23 00:30, Weiwei Li wrote:
> Using implicitly enabled extensions such as Zca/Zcf/Zcd instead of their
> super extensions can simplify the extension related check. However, they
> may have higher priv version than their super extensions. So we should mask
> them in the isa_string based on priv version to make them invisible to user
> if the specified priv version is lower than their minimal priv version.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/cpu.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index cb68916fce..1a5099382c 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1709,6 +1709,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str,
>   
>       for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
>           if (isa_edata_arr[i].multi_letter &&
> +            (cpu->env.priv_ver >= isa_edata_arr[i].min_version) &&

We don't have a way of telling whether an extension was enabled by us or by the user.
This will end up filtering user extensions from the isa_string.

IMHO, the way the logic is working today, we can't enable Z extensions based on enabled
MISA bits alone and disable extensions based on priv_spec at the same time. We would
need to check for priv_version when enabling these extensions implicitly during realize().
Another alternative is to not disable any extensions at all based on priv spec - we send
a warning about the priv mismatch and that's it.

Thanks,

Daniel

>               isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
>               new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
>               g_free(old);

Re: [PATCH] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version

Posted by liweiwei 1 year ago

On 2023/4/7 18:58, Daniel Henrique Barboza wrote:
>
>
> On 4/7/23 00:30, Weiwei Li wrote:
>> Using implicitly enabled extensions such as Zca/Zcf/Zcd instead of their
>> super extensions can simplify the extension related check. However, they
>> may have higher priv version than their super extensions. So we 
>> should mask
>> them in the isa_string based on priv version to make them invisible 
>> to user
>> if the specified priv version is lower than their minimal priv version.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>>   target/riscv/cpu.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index cb68916fce..1a5099382c 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1709,6 +1709,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, 
>> char **isa_str,
>>         for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
>>           if (isa_edata_arr[i].multi_letter &&
>> +            (cpu->env.priv_ver >= isa_edata_arr[i].min_version) &&
>
> We don't have a way of telling whether an extension was enabled by us 
> or by the user.
> This will end up filtering user extensions from the isa_string.

As the method described in the 
https://lists.gnu.org/archive/html/qemu-riscv/2023-04/msg00200.html:

The user specified extension have been disabled in realize(), So what 
needs masked is the implied extensions here.

>
> IMHO, the way the logic is working today, we can't enable Z extensions 
> based on enabled
> MISA bits alone and disable extensions based on priv_spec at the same 
> time. We would
> need to check for priv_version when enabling these extensions 
> implicitly during realize().

If we check this in realize(), we also need check them in support of the 
extension. So this is unnecessary.

Regards,

Weiwei Li

> Another alternative is to not disable any extensions at all based on 
> priv spec - we send
> a warning about the priv mismatch and that's it.
>
>
> Thanks,
>
> Daniel
>
>
>
>>               isa_ext_is_enabled(cpu, &isa_edata_arr[i])) {
>>               new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
>>               g_free(old);