target/loongarch: Set CSR_PRCFG1 and CSR_PRCFG2 values

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

Posted by Song Gao 4 months, 3 weeks ago

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);
-- 
2.33.0

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

Posted by maobibo 4 months, 3 weeks ago


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().

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 :)

Regards
Bibo Mao

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

Posted by gaosong 4 months, 3 weeks ago

在 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