Re: [PATCH] target/loongarch: Set CSR_PRCFG1 and CSR_PRCFG2 values

[gaosong]

在 2024/7/4 下午8:47, maobibo 写道:
> On 2024/7/4 下午7:12, Song Gao wrote:
> > We set the value of register CSR_PRCFG3, but left out CSR_PRCFG1
> > and CSR_PRCFG2. Set CSR_PRCFG1 and CSR_PRCFG2 according to the
> > default values of the physical machine.
> >
> > Signed-off-by: Song Gao <gaosong@loongson.cn>
> > ---
> >   target/loongarch/cpu.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> > index 270f711f11..ad40750701 100644
> > --- a/target/loongarch/cpu.c
> > +++ b/target/loongarch/cpu.c
> > @@ -538,6 +538,12 @@ static void loongarch_cpu_reset_hold(Object 
> > *obj, ResetType type)
> >       env->CSR_MERRCTL = FIELD_DP64(env->CSR_MERRCTL, CSR_MERRCTL, 
> > ISMERR, 0);
> >       env->CSR_TID = cs->cpu_index;
> >   +    env->CSR_PRCFG1 = FIELD_DP64(env->CSR_PRCFG1, CSR_PRCFG1, 
> > SAVE_NUM, 8);
> > +    env->CSR_PRCFG1 = FIELD_DP64(env->CSR_PRCFG1, CSR_PRCFG1, 
> > TIMER_BITS, 0x2f);
> > +    env->CSR_PRCFG1 = FIELD_DP64(env->CSR_PRCFG1, CSR_PRCFG1, VSMAX, 
> > 7);
> > +
> > +    env->CSR_PRCFG2 = 0x3ffff000;
> > +
> >       env->CSR_PRCFG3 = FIELD_DP64(env->CSR_PRCFG3, CSR_PRCFG3, 
> > TLB_TYPE, 2);
> >       env->CSR_PRCFG3 = FIELD_DP64(env->CSR_PRCFG3, CSR_PRCFG3, 
> > MTLB_ENTRY, 63);
> >       env->CSR_PRCFG3 = FIELD_DP64(env->CSR_PRCFG3, CSR_PRCFG3, 
> > STLB_WAYS, 7);
> For the function, it looks good to me. There are some nits:
> For PRCFG1-PRCFG3, it is constant. I thinks it had better be put at 
> cpu instance_init() or instance_finalize().
You are right, we should put them in instance_init().
> Also it is strange that most of cpu_state should be cleared with 0 
> besides cpucfg/prcfg etc, such as end_reset_fields marker in other 
> architectures.
>
> Maybe it will better if there is double check with cpu state change 
> callback such as init/reset etc :)
The CSRs that need to be reset are from chapter 6, section 4 of the 
manual, and they are not always reset to 0.
I think maybe that's why end_reset_fields was not used at the time. We 
should add some comments to loongarch_cpu_reset_hold().

Thanks.
Song Gao
> Regards
> Bibo Mao