Re: [PATCH RESEND v7 12/12] target/riscv: forbid write_misa() for static CPUs

[Daniel Henrique Barboza]

On 4/20/23 20:48, Alistair Francis wrote:
> On Thu, Apr 20, 2023 at 7:22 PM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> > Static CPUs don't want their extensions changed by user interaction. We
> > can prevent it during init by not exposing user facing properties, but
> > write_misa() is also capable of disabling/enabling extension during
> > runtime.
> >
> > We have a way of telling whether a CPU is static or not by checking for
> > TYPE_RISCV_DYNAMIC_CPU. Use it to make write_misa() a no-op for these
> > CPUs.
> >
> > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > ---
> >   target/riscv/cpu.c | 5 +++++
> >   target/riscv/cpu.h | 2 ++
> >   target/riscv/csr.c | 5 +++++
> >   3 files changed, 12 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 4fa720a39d..3cbcf6d320 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -1452,6 +1452,11 @@ static void riscv_cpu_init(Object *obj)
> >   #endif /* CONFIG_USER_ONLY */
> >   }
> >
> > +bool riscv_cpu_is_static(RISCVCPU *cpu)
> > +{
> > +    return object_dynamic_cast(OBJECT(cpu), TYPE_RISCV_DYNAMIC_CPU) == NULL;
> > +}
> > +
> >   typedef struct RISCVCPUMisaExtConfig {
> >       const char *name;
> >       const char *description;
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 1f39edc687..1913ab9d8d 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -587,6 +587,8 @@ G_NORETURN void riscv_raise_exception(CPURISCVState *env,
> >   target_ulong riscv_cpu_get_fflags(CPURISCVState *env);
> >   void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
> >
> > +bool riscv_cpu_is_static(RISCVCPU *cpu);
> > +
> >   #include "exec/cpu-all.h"
> >
> >   FIELD(TB_FLAGS, MEM_IDX, 0, 3)
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index d449da2657..929c5477dd 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -1391,6 +1391,11 @@ static RISCVException write_misa(CPURISCVState *env, int 
> > csrno,
> >       uint32_t orig_misa_ext = env->misa_ext;
> >       Error *local_err = NULL;
> >
> > +    if (riscv_cpu_is_static(cpu)) {
> > +        /* never write MISA for static CPUs */
> > +        return RISCV_EXCP_NONE;
> > +    }
>
> Do we actually need this? We already check misa_w which would be
> disabled. What's the harm in allowing someone to manually enable
> misa_w and then change the MISA?
>
> Also, it's possible that static CPUs actually support changing the
> MISA value at run time.

That's all valid points. I believe it's ok to drop this patch to allow
for static CPUs to have more flexibility in the future. We're still have
misa_w.


Thanks,

Daniel

> Alistair
>
> > +
> >       if (!riscv_cpu_cfg(env)->misa_w) {
> >           /* drop write to misa */
> >           return RISCV_EXCP_NONE;
> > --
> > 2.40.0