Re: [PATCH for-8.1 v3 25/26] target/riscv: allow write_misa() to enable RVG

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 v3 25/26] target/riscv: allow write_misa() to enable RVG
Date: Wed, 22 Mar 2023 14:42:14 -0300	[thread overview]
Message-ID: <dcc3ce9a-3714-05c5-c751-ad2f8f8803ff@ventanamicro.com> (raw)
In-Reply-To: <8b219f0e-cfd8-390e-405c-2fd9b1e784b5@iscas.ac.cn>

On 3/21/23 00:25, liweiwei wrote:
> 
> On 2023/3/19 04:04, Daniel Henrique Barboza wrote:
>> Allow write_misa() to enable RVG by changing riscv_cpu_enable_g()
>> slighty: instead of returning void, return the current env->misa_ext
>> value. This is then retrieved by 'val', which will add the RVG flag
>> itself, and then we'll skip validation and go right into commiting the
>> changes.
>>
>> The reason why it's ok to skip validation in this case is because we're
>> only allowing RVG (and its associated extensions/Z extensions) to be
>> enabled in the hart, and riscv_cpu_enable_g() already does its own
>> validation before enabling itself. Everything else is considered to be
>> already validated beforehand, so we don't need to repeat ourselves.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>>   target/riscv/cpu.c |  7 +++++--
>>   target/riscv/cpu.h |  2 ++
>>   target/riscv/csr.c | 15 +++++++++++++++
>>   3 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 2d2a354af3..73a5fa46ee 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -281,7 +281,8 @@ static uint32_t riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
>>       return ext;
>>   }
>> -static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
>> +
>> +target_ulong riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
>>   {
>>       CPURISCVState *env = &cpu->env;
>>       RISCVCPUConfig *cfg = &cpu->cfg;
>> @@ -289,7 +290,7 @@ static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
>>       if (cpu->cfg.ext_zfinx) {
>>           error_setg(errp, "Unable to enable G: Zfinx is enabled, "
>>                            "so F can not be enabled");
>> -        return;
>> +        return 0;
>>       }
>>       if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a &&
>> @@ -315,6 +316,8 @@ static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
>>           cfg->ext_icsr = true;
>>           cfg->ext_ifencei = true;
>>       }
>> +
>> +    return env->misa_ext;
>>   }
>>   static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index befc3b8fff..3ca1d4903c 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -599,6 +599,8 @@ void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
>>                                      Error **errp);
>>   void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu, uint32_t misa_ext);
>> +target_ulong riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp);
>> +
>>   #define cpu_list riscv_cpu_list
>>   #define cpu_mmu_index riscv_cpu_mmu_index
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 839862f1a8..4335398c19 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1381,6 +1381,20 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>           val &= RVE;
>>       }
>> +    if (val & RVG && !(env->misa_ext & RVG)) {
>> +        /*
>> +         * If the write wants to enable RVG, only RVG and
>> +         * its dependencies will be updated in the CSR.
>> +         */
>> +        val = riscv_cpu_enable_g(cpu, &local_err);
>> +        if (local_err != NULL) {
>> +            return RISCV_EXCP_NONE;
>> +        }
>> +
>> +        val |= RVG;
> 
> This assignment is not necessary, since RVG is already set in val.
> 
> By the way, RVG is still not disabled if any some of included extensions are disabled by write_misa.

I'll include this use case in v4. And I'll also treat RVG as a regular extension since
all checks were already done in realize() time.

Daniel

> 
> Regards,
> 
> Weiwei Li
> 
>> +        goto commit;
>> +    }
>> +
>>       /*
>>        * This flow is similar to what riscv_cpu_realize() does,
>>        * with the difference that we will update env->misa_ext
>> @@ -1396,6 +1410,7 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>>           return RISCV_EXCP_NONE;
>>       }
>> +commit:
>>       riscv_cpu_commit_cpu_cfg(cpu, val);
>>       if (!(val & RVF)) {
> 