[RESEND,v8,20/20] target/riscv/cpu.c: consider user option with RVG

Enabling RVG will enable a set of extensions that we're not checking if
the user was okay enabling or not. And in this case we want to error
out, instead of ignoring, otherwise we will be inconsistent enabling RVG
without all its extensions.

After this patch, disabling ifencei or icsr while enabling RVG will
result in error:

$ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic
qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 target/riscv/cpu.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

On Thu, Aug 24, 2023 at 07:14:40PM -0300, Daniel Henrique Barboza wrote:
> Enabling RVG will enable a set of extensions that we're not checking if
> the user was okay enabling or not. And in this case we want to error
> out, instead of ignoring, otherwise we will be inconsistent enabling RVG
> without all its extensions.
> 
> After this patch, disabling ifencei or icsr while enabling RVG will
> result in error:
> 
> $ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false

The warning makes it sound like Zifencei is getting overridden, but then
the error says "nope". So I think...

> 
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  target/riscv/cpu.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e07b2c73e7..21ebdbf084 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1155,8 +1155,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>            riscv_has_ext(env, RVD) &&
>            cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
>          warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");

...we want to move this warning down below the checks and we should even
check if it should be issued with

 if (!cpu->cfg.ext_icsr || !cpu->cfg.ext_ifencei || !cpu->cfg.a || ...)
    warn_report(...)

> -        cpu->cfg.ext_icsr = true;
> -        cpu->cfg.ext_ifencei = true;
> +
> +        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_icsr)) &&
> +            !cpu->cfg.ext_icsr) {
> +            error_setg(errp, "RVG requires Zicsr but user set Zicsr to false");
> +            return;
> +        }
> +
> +        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_ifencei)) &&
> +            !cpu->cfg.ext_ifencei) {
> +            error_setg(errp, "RVG requires Zifencei but user set "
> +                       "Zifencei to false");
> +            return;
> +        }
> +
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>  
>          env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>          env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> -- 
> 2.41.0
> 
>

Thanks,
drew

On 8/31/23 11:02, Andrew Jones wrote:
> On Thu, Aug 24, 2023 at 07:14:40PM -0300, Daniel Henrique Barboza wrote:
>> Enabling RVG will enable a set of extensions that we're not checking if
>> the user was okay enabling or not. And in this case we want to error
>> out, instead of ignoring, otherwise we will be inconsistent enabling RVG
>> without all its extensions.
>>
>> After this patch, disabling ifencei or icsr while enabling RVG will
>> result in error:
>>
>> $ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false --nographic
>> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
>> qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false
> 
> The warning makes it sound like Zifencei is getting overridden, but then
> the error says "nope". So I think...
> 
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> ---
>>   target/riscv/cpu.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index e07b2c73e7..21ebdbf084 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1155,8 +1155,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>             riscv_has_ext(env, RVD) &&
>>             cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
>>           warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> 
> ...we want to move this warning down below the checks and we should even
> check if it should be issued with
> 
>   if (!cpu->cfg.ext_icsr || !cpu->cfg.ext_ifencei || !cpu->cfg.a || ...)
>      warn_report(...)

Moving the warning makes sense. I'll do that.

About the check, it's not visible in the patch but right before this code we have:


     /* Do some ISA extension error checking */
     if (riscv_has_ext(env, RVG) &&
         !(riscv_has_ext(env, RVI) && riscv_has_ext(env, RVM) &&
           riscv_has_ext(env, RVA) && riscv_has_ext(env, RVF) &&
           riscv_has_ext(env, RVD) &&
           cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
         warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");

So we're already checking if we have at least one G extension disabled
before issuing the warning.



Thanks,

Daniel


> 
>> -        cpu->cfg.ext_icsr = true;
>> -        cpu->cfg.ext_ifencei = true;
>> +
>> +        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_icsr)) &&
>> +            !cpu->cfg.ext_icsr) {
>> +            error_setg(errp, "RVG requires Zicsr but user set Zicsr to false");
>> +            return;
>> +        }
>> +
>> +        if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_ifencei)) &&
>> +            !cpu->cfg.ext_ifencei) {
>> +            error_setg(errp, "RVG requires Zifencei but user set "
>> +                       "Zifencei to false");
>> +            return;
>> +        }
>> +
>> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
>> +        cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>>   
>>           env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>>           env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>> -- 
>> 2.41.0
>>
>>
> 
> Thanks,
> drew