[for-8.1,v4,23/25] target/riscv: rework write_misa()

write_misa() must use as much common logic as possible. We want to open
code just the bits that are exclusive to the CSR write operation and TCG
internals.

Rewrite write_misa() to work as follows:

- mask the write using misa_ext_mask to avoid enabling unsupported
  extensions;

- suppress RVC if the next insn isn't aligned;

- handle RVE. This is done by filtering all bits but RVE from 'val'.
  Setting RVE will forcefully set only RVE - assuming it gets
  validated afterwards;

- emulate the steps done by realize(): validate the candidate misa_ext
  val, then validate the configuration with the candidate misa_ext val,
  and finally commit the changes to cpu->cfg.

If any of the validation steps fails, the write operation is a no-op.

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 | 12 ++++------
 target/riscv/cpu.h |  6 +++++
 target/riscv/csr.c | 59 ++++++++++++++++++++++++++--------------------
 3 files changed, 45 insertions(+), 32 deletions(-)

On 2023/3/23 06:20, Daniel Henrique Barboza wrote:
> write_misa() must use as much common logic as possible. We want to open
> code just the bits that are exclusive to the CSR write operation and TCG
> internals.
>
> Rewrite write_misa() to work as follows:
>
> - mask the write using misa_ext_mask to avoid enabling unsupported
>    extensions;
>
> - suppress RVC if the next insn isn't aligned;
>
> - handle RVE. This is done by filtering all bits but RVE from 'val'.
>    Setting RVE will forcefully set only RVE - assuming it gets
>    validated afterwards;
>
> - emulate the steps done by realize(): validate the candidate misa_ext
>    val, then validate the configuration with the candidate misa_ext val,
>    and finally commit the changes to cpu->cfg.
>
> If any of the validation steps fails, the write operation is a no-op.
>
> 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 | 12 ++++------
>   target/riscv/cpu.h |  6 +++++
>   target/riscv/csr.c | 59 ++++++++++++++++++++++++++--------------------
>   3 files changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 0e6b8fb45e..41b17ba0c3 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1018,9 +1018,8 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>   }
>   
>   
> -static void riscv_cpu_validate_misa_ext(CPURISCVState *env,
> -                                        uint32_t misa_ext,
> -                                        Error **errp)
> +void riscv_cpu_validate_misa_ext(CPURISCVState *env, uint32_t misa_ext,
> +                                 Error **errp)
>   {
>       Error *local_err = NULL;
>   
> @@ -1113,9 +1112,8 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>    * candidate misa_ext value. No changes in env->misa_ext
>    * are made.
>    */
> -static void riscv_cpu_validate_extensions(RISCVCPU *cpu,
> -                                          uint32_t misa_ext,
> -                                          Error **errp)
> +void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
> +                                   Error **errp)
>   {
>       if (cpu->cfg.epmp && !cpu->cfg.pmp) {
>           /*
> @@ -1206,7 +1204,7 @@ static void riscv_cpu_validate_extensions(RISCVCPU *cpu,
>       }
>   }
>   
> -static void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu)
> +void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu)
>   {
>       if (cpu->cfg.ext_zk) {
>           cpu->cfg.ext_zkn = true;
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index dbb4df9df0..ca2ba6a647 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -593,6 +593,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>   char *riscv_isa_string(RISCVCPU *cpu);
>   void riscv_cpu_list(void);
>   
> +void riscv_cpu_validate_misa_ext(CPURISCVState *env, uint32_t misa_ext,
> +                                 Error **errp);
> +void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t misa_ext,
> +                                   Error **errp);
> +void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu);
> +
>   #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 d522efc0b6..8d5e8f9ad1 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1343,39 +1343,17 @@ static RISCVException read_misa(CPURISCVState *env, int csrno,
>   static RISCVException write_misa(CPURISCVState *env, int csrno,
>                                    target_ulong val)
>   {
> +    RISCVCPU *cpu = env_archcpu(env);
> +    Error *local_err = NULL;
> +
>       if (!riscv_cpu_cfg(env)->misa_w) {
>           /* drop write to misa */
>           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
> @@ -1389,6 +1367,37 @@ static RISCVException write_misa(CPURISCVState *env, int csrno,
>           return RISCV_EXCP_NONE;
>       }
>   
> +    /*
> +     * We'll handle special cases in separate. If one
> +     * of these bits are enabled we'll handle them and
> +     * end the CSR write.
> +     */
> +    if (val & RVE && !(env->misa_ext & RVE)) {
> +        /*
> +         * RVE must be enabled by itself. Clear all other
> +         * misa_env bits and let the validation do its
> +         * job.
> +         */
> +        val &= RVE;

It can just be val = RVE here.

Regards,

Weiwei Li

> +    }
> +
> +    /*
> +     * This flow is similar to what riscv_cpu_realize() does,
> +     * with the difference that we will update env->misa_ext
> +     * value if everything is ok.
> +     */
> +    riscv_cpu_validate_misa_ext(env, val, &local_err);
> +    if (local_err != NULL) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    riscv_cpu_validate_extensions(cpu, val, &local_err);
> +    if (local_err != NULL) {
> +        return RISCV_EXCP_NONE;
> +    }
> +
> +    riscv_cpu_commit_cpu_cfg(cpu);
> +
>       if (!(val & RVF)) {
>           env->mstatus &= ~MSTATUS_FS;
>       }