Re: [PATCH for-8.1 v3 25/26] target/riscv: allow write_misa() to enable RVG

[Daniel Henrique Barboza]

On 3/21/23 00:25, liweiwei wrote:
> On 2023/3/19 04:04, Daniel Henrique Barboza wrote:
> > Allow write_misa() to enable RVG by changing riscv_cpu_enable_g()
> > slighty: instead of returning void, return the current env->misa_ext
> > value. This is then retrieved by 'val', which will add the RVG flag
> > itself, and then we'll skip validation and go right into commiting the
> > changes.
> >
> > The reason why it's ok to skip validation in this case is because we're
> > only allowing RVG (and its associated extensions/Z extensions) to be
> > enabled in the hart, and riscv_cpu_enable_g() already does its own
> > validation before enabling itself. Everything else is considered to be
> > already validated beforehand, so we don't need to repeat ourselves.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/cpu.c |  7 +++++--
> >   target/riscv/cpu.h |  2 ++
> >   target/riscv/csr.c | 15 +++++++++++++++
> >   3 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 2d2a354af3..73a5fa46ee 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -281,7 +281,8 @@ static uint32_t 
> > riscv_get_misa_ext_with_cpucfg(RISCVCPUConfig *cfg)
> >       return ext;
> >   }
> > -static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
> > +
> > +target_ulong riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
> >   {
> >       CPURISCVState *env = &cpu->env;
> >       RISCVCPUConfig *cfg = &cpu->cfg;
> > @@ -289,7 +290,7 @@ static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
> >       if (cpu->cfg.ext_zfinx) {
> >           error_setg(errp, "Unable to enable G: Zfinx is enabled, "
> >                            "so F can not be enabled");
> > -        return;
> > +        return 0;
> >       }
> >       if (!(cfg->ext_i && cfg->ext_m && cfg->ext_a &&
> > @@ -315,6 +316,8 @@ static void riscv_cpu_enable_g(RISCVCPU *cpu, Error **errp)
> >           cfg->ext_icsr = true;
> >           cfg->ext_ifencei = true;
> >       }
> > +
> > +    return env->misa_ext;
> >   }
> >   static void riscv_set_cpucfg_with_misa(RISCVCPUConfig *cfg,
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index befc3b8fff..3ca1d4903c 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -599,6 +599,8 @@ void riscv_cpu_validate_extensions(RISCVCPU *cpu, uint32_t 
> > misa_ext,
> >                                      Error **errp);
> >   void riscv_cpu_commit_cpu_cfg(RISCVCPU *cpu, uint32_t misa_ext);
> > +target_ulong riscv_cpu_enable_g(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 839862f1a8..4335398c19 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1381,6 +1381,20 @@ static RISCVException write_misa(CPURISCVState *env, int 
> > csrno,
> >           val &= RVE;
> >       }
> > +    if (val & RVG && !(env->misa_ext & RVG)) {
> > +        /*
> > +         * If the write wants to enable RVG, only RVG and
> > +         * its dependencies will be updated in the CSR.
> > +         */
> > +        val = riscv_cpu_enable_g(cpu, &local_err);
> > +        if (local_err != NULL) {
> > +            return RISCV_EXCP_NONE;
> > +        }
> > +
> > +        val |= RVG;
>
> This assignment is not necessary, since RVG is already set in val.
>
> By the way, RVG is still not disabled if any some of included extensions are 
> disabled by write_misa.

I'll include this use case in v4. And I'll also treat RVG as a regular 
extension since
all checks were already done in realize() time.


Daniel


> Regards,
>
> Weiwei Li
>
> > +        goto commit;
> > +    }
> > +
> >       /*
> >        * This flow is similar to what riscv_cpu_realize() does,
> >        * with the difference that we will update env->misa_ext
> > @@ -1396,6 +1410,7 @@ static RISCVException write_misa(CPURISCVState *env, int 
> > csrno,
> >           return RISCV_EXCP_NONE;
> >       }
> > +commit:
> >       riscv_cpu_commit_cpu_cfg(cpu, val);
> >       if (!(val & RVF)) {