Re: [PATCH v8 11/11] target/riscv: rework write_misa()

[Alistair Francis]

On Fri, May 12, 2023 at 10:42 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Alistair,
>
>
> Since this is the only patch that is being contested is it possible to apply 
> all
> the other ones to riscv-to-apply.next?
>
> I'm asking because I'm going to send more code that will affect cpu_init() and
> riscv_cpu_realize() functions, and it would be easier if the cleanups were 
> already
> in 'next'. Otherwise I'll either have to base the new code on top of this 
> pending
> series or I'll base them under riscv-to-apply.next and we'll have to deal with
> conflicts.

Urgh, that's fair.

I just replied to your other thread. Let's throw a guest error print
in here and then we can merge it

Alistair

>
>
> Thanks,
>
> Daniel
>
> On 4/21/23 10:27, 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.
> >
> > 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;
> >
> > - 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 <dbarboza@ventanamicro.com>
> > Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > ---
> >   target/riscv/cpu.c |  4 ++--
> >   target/riscv/cpu.h |  1 +
> >   target/riscv/csr.c | 47 ++++++++++++++++++++--------------------------
> >   3 files changed, 23 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 7d407321aa..4fa720a39d 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -944,7 +944,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;
> > @@ -960,7 +960,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..4a3c57ea6f 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,32 @@ 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 */
> > +        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;
> >   }
>