On Fri, May 12, 2023 at 10:42 PM Daniel Henrique Barboza
<dbarb...@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 <dbarb...@ventanamicro.com>
> > Reviewed-by: Weiwei Li <liwei...@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;
> > }
>
