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

From: liweiwei <liweiwei@iscas.ac.cn>
To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>,
	qemu-devel@nongnu.org
Cc: liweiwei@iscas.ac.cn, 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 26/26] target/riscv: allow write_misa() to enable RVV
Date: Tue, 21 Mar 2023 11:41:27 +0800	[thread overview]
Message-ID: <c8f3683c-24b8-ef46-b074-aabeee98e0ec@iscas.ac.cn> (raw)
In-Reply-To: <20230318200436.299464-27-dbarboza@ventanamicro.com>

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.

Regards,

Weiwei Li

>       /*
>        * This flow is similar to what riscv_cpu_realize() does,
>        * with the difference that we will update env->misa_ext