On 3/21/23 00:41, liweiwei wrote:
>
> On 2023/3/19 04:04, Daniel Henrique Barboza wrote:
>> Allow write_misa() to enable RVV like we did with RVG. We'll need a
>> riscv_cpu_enable_v() to enable all related misa bits and Z extensions.
>> This new helper validates the existing 'env' conf by using the existing
>> riscv_cpu_validate_v(). We'll also check if we'll be able to enable 'F'
>> by checking for ext_zfinx.
>>
>> As with RVG, enabling RVV is considered to be a standalone operation in
>> write_misa(). This means that we'll guarantee that we're not being
>> inconsistent in riscv_cpu_enable_v() and that we're okay with skipping
>> regular validation.
>>
>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>> ---
>> target/riscv/cpu.c | 35 +++++++++++++++++++++++++++++++++++
>> target/riscv/cpu.h | 1 +
>> target/riscv/csr.c | 14 ++++++++++++++
>> 3 files changed, 50 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 73a5fa46ee..9c16b29f27 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -983,6 +983,41 @@ static void riscv_cpu_validate_v(CPURISCVState *env,
>> env->vext_ver = vext_version;
>> }
>> +target_ulong riscv_cpu_enable_v(RISCVCPU *cpu, Error **errp)
>> +{
>> + CPURISCVState *env = &cpu->env;
>> + RISCVCPUConfig *cfg = &cpu->cfg;
>> + Error *local_err = NULL;
>> +
>> + riscv_cpu_validate_v(env, cfg, &local_err);
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>> + return 0;
>> + }
>
> This check is not necessary, we call this function only when we enable v by write_misa, which also have a prerequisite:
>
> V is enabled at the very first. So this check will always be true, since the parameter for vector cannot be changed dynamically.
>
> Similar to following check.
>
>> +
>> + if (cpu->cfg.ext_zfinx) {
>> + error_setg(errp, "Unable to enable V: Zfinx is enabled, "
>> + "so F can not be enabled");
>> + return 0;
>> + }
>> +
>> + cfg->ext_f = true;
>> + env->misa_ext |= RVF;
>> +
>> + cfg->ext_d = true;
>> + env->misa_ext |= RVD;
>
> We do check V against F/D at first. Why we do this when enable V?
>
> And if we do this, whether we should also enable F when enable D?
>
>
>> +
>> + /*
>> + * The V vector extension depends on the
>> + * Zve32f, Zve64f and Zve64d extensions.
>> + */
>> + cpu->cfg.ext_zve64d = true;
>> + cpu->cfg.ext_zve64f = true;
>> + cpu->cfg.ext_zve32f = true;
>
> This is right, but not necessary in current implementation, since they will not be disabled when we disable V.
>
> So we needn't enable them when we re-enable V.
>
>> +
>> + return env->misa_ext;
>> +}
>> +
>> static void riscv_cpu_validate_priv_spec(RISCVCPU *cpu, Error **errp)
>> {
>> CPURISCVState *env = &cpu->env;
>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
>> index 3ca1d4903c..45e801d926 100644
>> --- a/target/riscv/cpu.h
>> +++ b/target/riscv/cpu.h
>> @@ -600,6 +600,7 @@ void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
>> void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu, uint32_t misa_ext);
>> target_ulong riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp);
>> +target_ulong riscv_cpu_enable_v(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 4335398c19..e9e1afc57e 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1395,6 +1395,20 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>> goto commit;
>> }
>> + if (val & RVV && !(env->misa_ext & RVV)) {
>> + /*
>> + * If the write wants to enable RVV, only RVV and
>> + * its dependencies will be updated in the CSR.
>> + */
>> + val = riscv_cpu_enable_v(cpu, &local_err);
>> + if (local_err != NULL) {
>> + return RISCV_EXCP_NONE;
>> + }
>> +
>> + val |= RVV;
>> + goto commit;
>> + }
>> +
>
> So, I think we can just treat V as common extension, and do nothing additionally for disabling/re-enabling it.
In fact I think the same can be said about RVG, since both extensions - and in fact,
all extensions - would have to be enabled once during cpu_init() anyway. If not,
env->misa_ext_mask wouldn't allow it be enabled in write_misa() time later on.
I believe I've over-complicated things a bit in these last patches. I'll simplify
things in v4.
Thanks,
Daniel
>
> Regards,
>
> Weiwei Li
>
>> /*
>> * This flow is similar to what riscv_cpu_realize() does,
>> * with the difference that we will update env->misa_ext
>