On Wed, May 17, 2023 at 11:58 PM Daniel Henrique Barboza
<dbarb...@ventanamicro.com> 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.
>
> Our validation is done with riscv_cpu_validate_set_extensions(), but we
> need a small tweak first. When enabling RVG we're doing:
>
> env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> env->misa_ext_mask = env->misa_ext;
>
> This works fine for realize() time but this can potentially overwrite
> env->misa_ext_mask if we reutilize the function for write_misa().
>
> Instead of doing misa_ext_mask = misa_ext, sum up the RVG extensions in
> misa_ext_mask as well. This won't change realize() time behavior
> (misa_ext_mask will be == misa_ext) and will ensure that write_misa()
> won't change misa_ext_mask by accident.
>
> After that, 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;
>
> - disable RVG if any of RVG dependencies are being disabled by the user;
>
> - assign env->misa_ext and run riscv_cpu_validate_set_extensions(). On
> error, rollback env->misa_ext to its original value, logging a
> GUEST_ERROR to inform the user about the failed write;
>
> - handle RVF and MSTATUS_FS and continue as usual.
>
> Let's keep write_misa() as experimental for now until this logic gains
> enough mileage.
>
> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
> Reviewed-by: Weiwei Li <liwei...@iscas.ac.cn>
> Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
> ---
> target/riscv/cpu.c | 4 ++--
> target/riscv/cpu.h | 1 +
> target/riscv/csr.c | 51 ++++++++++++++++++++++------------------------
> 3 files changed, 27 insertions(+), 29 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 52f91ff8cf..6d4748bf24 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -981,7 +981,7 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu,
> Error **errp)
> * Check consistency between chosen extensions while setting
> * cpu->cfg accordingly.
> */
> -static void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> +void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
> {
> CPURISCVState *env = &cpu->env;
> Error *local_err = NULL;
> @@ -997,7 +997,7 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU
> *cpu, Error **errp)
> cpu->cfg.ext_ifencei = true;
>
> env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
> - env->misa_ext_mask = env->misa_ext;
> + env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> }
>
> if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 15423585d0..1f39edc687 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -548,6 +548,7 @@ 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 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 4451bd1263..cf7da4f87f 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1387,39 +1387,18 @@ 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 orig_misa_ext = env->misa_ext;
> + 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
> @@ -1428,18 +1407,36 @@ static RISCVException write_misa(CPURISCVState *env,
> int csrno,
> val &= ~RVC;
> }
>
> + /* Disable RVG if any of its dependencies are disabled */
> + if (!(val & RVI && val & RVM && val & RVA &&
> + val & RVF && val & RVD)) {
> + val &= ~RVG;
> + }
> +
> /* If nothing changed, do nothing. */
> if (val == env->misa_ext) {
> return RISCV_EXCP_NONE;
> }
>
> - if (!(val & RVF)) {
> + env->misa_ext = val;
> + riscv_cpu_validate_set_extensions(cpu, &local_err);
> + if (local_err != NULL) {
> + /* Rollback on validation error */
> + qemu_log_mask(LOG_GUEST_ERROR, "Unable to write MISA ext value "
> + "0x%x, keeping existing MISA ext 0x%x\n",
> + env->misa_ext, orig_misa_ext);
> +
> + env->misa_ext = orig_misa_ext;
> +
> + return RISCV_EXCP_NONE;
> + }
> +
> + if (!(env->misa_ext & 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;
> }
> --
> 2.40.1
>
>
