Re: [PATCH] target/riscv: Don't wrongly overide isa version

[LIU Zhiwei]

On 2021/8/11 下午5:26, Bin Meng wrote:
> On Tue, Aug 10, 2021 at 11:35 AM LIU Zhiwei <zhiwei_liu@c-sky.com> wrote:
> > For some cpu, the isa version has already been set in cpu init function.
> > Thus only overide the isa version when isa version is not set, or
> typo: override, please fix the commit title as well
OK
> > users set different isa version explicitly by cpu parameters.
> >
> > Signed-off-by: LIU Zhiwei <zhiwei_liu@c-sky.com>
> > ---
> >   target/riscv/cpu.c | 14 ++++++++------
> >   1 file changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 991a6bb760..425efba0c8 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -392,9 +392,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> > **errp)
> >       RISCVCPU *cpu = RISCV_CPU(dev);
> >       CPURISCVState *env = &cpu->env;
> >       RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
> > -    int priv_version = PRIV_VERSION_1_11_0;
> > -    int bext_version = BEXT_VERSION_0_93_0;
> > -    int vext_version = VEXT_VERSION_0_07_1;
> > +    int priv_version = env->priv_ver;
> >       target_ulong target_misa = env->misa;
> >       Error *local_err = NULL;
> >
> > @@ -417,9 +415,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> > **errp)
> >           }
> >       }
> >
> > -    set_priv_version(env, priv_version);
> > -    set_bext_version(env, bext_version);
> > -    set_vext_version(env, vext_version);
> > +    if (!env->priv_ver) {
> > +        set_priv_version(env, PRIV_VERSION_1_11_0);
> > +    } else if (env->priv_ver != priv_version) {
> > +        set_priv_version(env, priv_version);
> > +    }
> This logic seems incorrect to me. So if cpu init function does not set
> the priv, and cfg set it to v1.10, v1.11 will be set in the new logic.

Yes,  it's also here.

Thanks,
Zhiwei

> The previous logic makes sure the cfg value overrides the cpu init
> value which seems to be intended.
>
> >       if (cpu->cfg.mmu) {
> >           set_feature(env, RISCV_FEATURE_MMU);
> > @@ -497,6 +497,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> > **errp)
> >               target_misa |= RVH;
> >           }
> >           if (cpu->cfg.ext_b) {
> > +            int bext_version = BEXT_VERSION_0_93_0;
> >               target_misa |= RVB;
> >
> >               if (cpu->cfg.bext_spec) {
> > @@ -515,6 +516,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
> > **errp)
> >               set_bext_version(env, bext_version);
> >           }
> >           if (cpu->cfg.ext_v) {
> > +            int vext_version = VEXT_VERSION_0_07_1;
> >               target_misa |= RVV;
> >               if (!is_power_of_2(cpu->cfg.vlen)) {
> >                   error_setg(errp,
> > --
> Regards,
> Bin