Re: [PATCH for-8.1 17/17] target/riscv: rework write_misa()

[LIU Zhiwei]

On 2023/3/9 4:19, Daniel Henrique Barboza wrote:
> write_misa() must use as much common logic as possible, only specifying
> the bits that are exclusive to the CSR write operation and TCG
> internals.
>
> Rewrite write_misa() to work as follows:
>
> - supress RVC right after verifying that we're not updating RVG;
>
> - mask the write using misa_ext_mask to avoid enabling unsupported
>    extensions;
>
> - emulate the steps done by the cpu init() functions: set cpu->cfg using
>    the desired misa value, validate it, and then commit;
>
> - fallback if the validation step fails. We'll need to re-write cpu->cfg
>    with the original misa_ext value for the hart.
>
> Let's keep write_misa() as experimental for now until this logic gains
> enough mileage.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>   target/riscv/cpu.c |  7 +++---
>   target/riscv/cpu.h |  2 ++
>   target/riscv/csr.c | 53 +++++++++++++++++++++-------------------------
>   3 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 7be6a86305..4b2be32de3 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -281,8 +281,7 @@ static uint32_t 
> riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
>       return ext;
>   }
>   
> -static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
> -                                       uint32_t misa_ext)
> +static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg, uint32_t misa_ext)
>   {
>       cfg->ext_i = misa_ext & RVI;
>       cfg->ext_e = misa_ext & RVE;
> @@ -299,7 +298,7 @@ static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
>       cfg->ext_g = misa_ext & RVG;
>   }
>   
> -static void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
> +void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext)
>   {
>       env->misa_mxl_max = env->misa_mxl = mxl;
>       env->misa_ext_mask = env->misa_ext = ext;
> @@ -995,7 +994,7 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU 
> *cpu)
>    * Check consistency between chosen extensions while setting
>    * cpu->cfg accordingly, doing a set_misa() in the end.
>    */
> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>   {
>       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>       CPUClass *cc = CPU_CLASS(mcc);
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 013a1389d6..d64d0f8dd6 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -591,6 +591,8 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
> size,
>                           bool probe, uintptr_t retaddr);
>   char *riscv_isa_string(RISCVCPU *cpu);
>   void riscv_cpu_list(void);
> +void set_misa(CPURISCVState *env, RISCVMXL mxl, uint32_t ext);
> +void riscv_cpu_validate_set_extensions(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 02a5c2a5ca..2e75c75fcc 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1342,6 +1342,11 @@ static RISCVException read_misa(CPURISCVState *env, int 
> csrno,
>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>   {
> +    RISCVCPU *cpu = env_archcpu(env);
> +    uint32_t hart_ext_mask = env->misa_ext_mask;
> +    uint32_t hart_ext = env->misa_ext;
> +    Error *local_err = NULL;
> +
>       if (!riscv_cpu_cfg(env)->misa_w) {
>           /* drop write to misa */
>           return RISCV_EXCP_NONE;
> @@ -1352,34 +1357,6 @@ static RISCVException write_misa(CPURISCVState *env, int 
> csrno,
>           return RISCV_EXCP_NONE;
>       }
>   
> -    /* 'I' or 'E' must be present */
> -    if (!(val & (RVI | RVE))) {
> -        /* It is not, drop write to misa */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /* 'E' excludes all other extensions */
> -    if (val & RVE) {
> -        /*
> -         * when we support 'E' we can do "val = RVE;" however
> -         * for now we just drop writes if 'E' is present.
> -         */
> -        return RISCV_EXCP_NONE;
> -    }
> -
> -    /*
> -     * misa.MXL writes are not supported by QEMU.
> -     * Drop writes to those bits.
> -     */
> -
> -    /* Mask extensions that are not supported by this hart */
> -    val &= env->misa_ext_mask;
> -
> -    /* 'D' depends on 'F', so clear 'D' if 'F' is not present */
> -    if ((val & RVD) && !(val & RVF)) {
> -        val &= ~RVD;
> -    }
> -
>       /*
>        * Suppress 'C' if next instruction is not aligned
>        * TODO: this should check next_pc
> @@ -1388,18 +1365,36 @@ static RISCVException write_misa(CPURISCVState *env, 
> int csrno,
>           val &= ~RVC;
>       }
>   
> +    /* Mask extensions that are not supported by this hart */
> +    val &= hart_ext_mask;
> +
>       /* If nothing changed, do nothing. */
>       if (val == env->misa_ext) {
>           return RISCV_EXCP_NONE;
>       }
>   
> +    /*
> +     * Validate the new configuration. Rollback to previous
> +     * values if something goes wrong.
> +     */
> +    set_misa(env, env->misa_mxl, val);
> +    riscv_cpu_validate_set_extensions(cpu, &local_err);

Can we split the riscv_cpu_validate_set_extensions to validate and set 
functions? If so, we can remove the restore work here.
And I think the fields in misa are WARL, we should be able to write into 
each single field if it is legal.

Zhiwei

> +    if (local_err) {
> +        set_misa(env, env->misa_mxl, hart_ext);
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    /*
> +     * Keep the original misa_ext_mask from the hart.
> +     */
> +    env->misa_ext_mask = hart_ext_mask;
> +
>       if (!(val & RVF)) {
>           env->mstatus &= ~MSTATUS_FS;
>       }
>   
>       /* flush translation cache */
>       tb_flush(env_cpu(env));
> -    env->misa_ext = val;
>       env->xl = riscv_cpu_mxl(env);
>       return RISCV_EXCP_NONE;
>   }