Re: [PATCH for-8.1 v2 16/26] target/riscv/cpu.c: split RVG code from validate_set_extensions()

From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
To: liweiwei <liweiwei@iscas.ac.cn>, qemu-devel@nongnu.org
Cc: qemu-riscv@nongnu.org, alistair.francis@wdc.com,
	bmeng@tinylab.org, zhiwei_liu@linux.alibaba.com,
	palmer@rivosinc.com
Subject: Re: [PATCH for-8.1 v2 16/26] target/riscv/cpu.c: split RVG code from validate_set_extensions()
Date: Wed, 15 Mar 2023 10:50:57 -0300	[thread overview]
Message-ID: <2a13891e-9b5e-fc3c-308c-7d5b6eb0cfa0@ventanamicro.com> (raw)
In-Reply-To: <dbb76e1d-112f-1de0-211d-8d0762757ac3@iscas.ac.cn>

On 3/15/23 01:43, liweiwei wrote:
> 
> On 2023/3/15 00:49, Daniel Henrique Barboza wrote:
>> We can set all RVG related extensions during realize time, before
>> validate_set_extensions() itself. It will also avoid re-enabling
>> RVG via write_misa() when the CSR start to using the same validation
>> code realize() does.
>>
>> Note that we're setting both cfg->ext_N and env->misa_ext bits, instead
>> of just setting cfg->ext_N. The intention here is to start syncing all
>> misa_ext operations with its cpu->cfg flags, in preparation to allow for
>> the validate function to operate using a misa_ext. This doesn't make any
>> difference for the current code state, but will be a requirement for
>> write_misa() later on.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c | 55 +++++++++++++++++++++++++++++++++-------------
>>   1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 48ad7372b9..133807e39f 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -281,6 +281,42 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
>>       return ext;
>>   }
>> +static void riscv_set_G_virt_ext(RISCVCPU *cpu)
>> +{
>> +    CPURISCVState *env = &cpu->env;
>> +    RISCVCPUConfig *cfg = &cpu->cfg;
>> +
>> +    if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a &&
>> +          cfg->ext_f && cfg->ext_d &&
>> +          cfg->ext_icsr && cfg->ext_ifencei)) {
>> +
>> +        warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
>> +        cfg->ext_i = true;
>> +        env->misa_ext |= RVI;
>> +
>> +        cfg->ext_m = true;
>> +        env->misa_ext |= RVM;
>> +
>> +        cfg->ext_a = true;
>> +        env->misa_ext |= RVA;
>> +
>> +        cfg->ext_f = true;
>> +        env->misa_ext |= RVF;
>> +
>> +        cfg->ext_d = true;
>> +        env->misa_ext |= RVD;
>> +
>> +        cfg->ext_icsr = true;
>> +        cfg->ext_ifencei = true;
>> +
>> +        /*
>> +         * Update misa_ext_mask since this is called
>> +         * only during riscv_cpu_realize().
>> +         */
>> +        env->misa_ext_mask = env->misa_ext;
>> +    }
> 
> Another two question:
> 
> - whether we should set 'G' when all these extensions are supported?

As far as I can tell, no. RVG is being treated as a shortcut to enabled this set of
extensions, but it doesn't mean that if the user happens to chose them manually we
should enable RVG.

> 
> - whether 'G'should be disabled if some of the extensions are disabled by write_misa?

Good point. Yes, we would need to disable RVG if RVG is enabled in the hart but
then another letter extension to the group (I,M,A F or D) is disabled. Which is
something that isn't being handled now.

A simple solution is, in patch 15,  forbid IMAFD to be disabled if RVG is already
set.

In fact, this kind of logic is something that we would need to do for the future
profile extension, so in a way RVG is being handled almost like a profile now.

Thanks,

Daniel

> 
> Regards,
> 
> Weiwei Li
> 
>> +}
>> +
>>   static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>>                                          uint32_t misa_ext)
>>   {
>> @@ -1036,21 +1072,6 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>>           return;
>>       }
>> -    /* Do some ISA extension error checking */
>> -    if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
>> -                            cpu->cfg.ext_a && cpu->cfg.ext_f &&
>> -                            cpu->cfg.ext_d &&
>> -                            cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
>> -        warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
>> -        cpu->cfg.ext_i = true;
>> -        cpu->cfg.ext_m = true;
>> -        cpu->cfg.ext_a = true;
>> -        cpu->cfg.ext_f = true;
>> -        cpu->cfg.ext_d = true;
>> -        cpu->cfg.ext_icsr = true;
>> -        cpu->cfg.ext_ifencei = true;
>> -    }
>> -
>>       if (cpu->cfg.ext_i && cpu->cfg.ext_e) {
>>           error_setg(errp,
>>                      "I and E extensions are incompatible");
>> @@ -1313,6 +1334,10 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>>           return;
>>       }
>> +    if (cpu->cfg.ext_g) {
>> +        riscv_set_G_virt_ext(cpu);
>> +    }
>> +
>>       riscv_cpu_validate_set_extensions(cpu, &local_err);
>>       if (local_err != NULL) {
>>           error_propagate(errp, local_err);
> 