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

[liweiwei <liweiwei@iscas.ac.cn>]

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.

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)) {