diff mbox series

[PULL,2/2] target/loongarch: check tlb_ps

Message ID 20250307021934.1080115-3-gaosong@loongson.cn (mailing list archive)
State New
Headers show
Series [PULL,1/2] target/loongarch: fix 'make check-functional' failed | expand

Commit Message

Song Gao March 7, 2025, 2:19 a.m. UTC 
For LoongArch th min tlb_ps is 12(4KB), for TLB code,
the tlb_ps may be 0,this may case UndefinedBehavior
Add a check-tlb_ps fuction to check tlb_ps,
to make sure the tlb_ps is avalablie. we check tlb_ps
when get the tlb_ps from tlb->misc or CSR bits.
1. cpu reset
   set CSR_PWCL.PTBASE and CSR_STLBPS.PS bits a default value
   from CSR_PRCFG2;
2. tlb instructions.
   some tlb instructions get  the tlb_ps from tlb->misc but the
   value may  has been initialized to 0. we need just check the tlb_ps
   skip the function and write a guest log.
3. csrwr instructions.
   to make sure CSR_PWCL.PTBASE and CSR_STLBPS.PS bits are avalable,
   cheke theses bits and set a default value from CSR_PRCFG2.

Signed-off-by: Song Gao <gaosong@loongson.cn>
Reviewed-by: Bibo Mao <maobibo@loongson.cn>
Message-Id: <20250305063311.830674-3-gaosong@loongson.cn>
---
 target/loongarch/cpu.c                        | 11 +++++---
 target/loongarch/helper.h                     |  1 +
 target/loongarch/internals.h                  |  2 ++
 target/loongarch/tcg/csr_helper.c             | 26 ++++++++++++++++---
 .../tcg/insn_trans/trans_privileged.c.inc     |  1 +
 target/loongarch/tcg/tlb_helper.c             | 23 ++++++++++++++--
 6 files changed, 56 insertions(+), 8 deletions(-)

Comments

Peter Maydell March 18, 2025, 4:23 p.m. UTC | #1 
On Fri, 7 Mar 2025 at 02:42, Song Gao <gaosong@loongson.cn> wrote:
>
> For LoongArch th min tlb_ps is 12(4KB), for TLB code,
> the tlb_ps may be 0,this may case UndefinedBehavior
> Add a check-tlb_ps fuction to check tlb_ps,
> to make sure the tlb_ps is avalablie. we check tlb_ps
> when get the tlb_ps from tlb->misc or CSR bits.
> 1. cpu reset
>    set CSR_PWCL.PTBASE and CSR_STLBPS.PS bits a default value
>    from CSR_PRCFG2;
> 2. tlb instructions.
>    some tlb instructions get  the tlb_ps from tlb->misc but the
>    value may  has been initialized to 0. we need just check the tlb_ps
>    skip the function and write a guest log.
> 3. csrwr instructions.
>    to make sure CSR_PWCL.PTBASE and CSR_STLBPS.PS bits are avalable,
>    cheke theses bits and set a default value from CSR_PRCFG2.
>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> Reviewed-by: Bibo Mao <maobibo@loongson.cn>
> Message-Id: <20250305063311.830674-3-gaosong@loongson.cn>

Hi; Coverity points out what looks like an error in this commit
(CID 1593475):

> +bool check_ps(CPULoongArchState *env, int tlb_ps)
> +{
> +     if (tlb_ps > 64) {
> +         return false;
> +     }
> +     return BIT_ULL(tlb_ps) & (env->CSR_PRCFG2);

BIT_ULL(64) isn't valid, as it would be off the end of a
64-bit value. Should the check be for "tlb_ps >= 64" ?

> +}

thanks
-- PMM
Song Gao March 19, 2025, 2:24 a.m. UTC | #2 
在 2025/3/19 上午12:23, Peter Maydell 写道:
> On Fri, 7 Mar 2025 at 02:42, Song Gao <gaosong@loongson.cn> wrote:
>> For LoongArch th min tlb_ps is 12(4KB), for TLB code,
>> the tlb_ps may be 0,this may case UndefinedBehavior
>> Add a check-tlb_ps fuction to check tlb_ps,
>> to make sure the tlb_ps is avalablie. we check tlb_ps
>> when get the tlb_ps from tlb->misc or CSR bits.
>> 1. cpu reset
>>     set CSR_PWCL.PTBASE and CSR_STLBPS.PS bits a default value
>>     from CSR_PRCFG2;
>> 2. tlb instructions.
>>     some tlb instructions get  the tlb_ps from tlb->misc but the
>>     value may  has been initialized to 0. we need just check the tlb_ps
>>     skip the function and write a guest log.
>> 3. csrwr instructions.
>>     to make sure CSR_PWCL.PTBASE and CSR_STLBPS.PS bits are avalable,
>>     cheke theses bits and set a default value from CSR_PRCFG2.
>>
>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>> Reviewed-by: Bibo Mao <maobibo@loongson.cn>
>> Message-Id: <20250305063311.830674-3-gaosong@loongson.cn>
> Hi; Coverity points out what looks like an error in this commit
> (CID 1593475):
>
>> +bool check_ps(CPULoongArchState *env, int tlb_ps)
>> +{
>> +     if (tlb_ps > 64) {
>> +         return false;
>> +     }
>> +     return BIT_ULL(tlb_ps) & (env->CSR_PRCFG2);
> BIT_ULL(64) isn't valid, as it would be off the end of a
> 64-bit value. Should the check be for "tlb_ps >= 64" ?
yes, I had send a patch to fix.

thanks.
Song Gao
>> +}
> thanks
> -- PMM
diff mbox series

Patch

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index ac514a15fb..0486853048 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -544,6 +544,7 @@  static void loongarch_max_initfn(Object *obj)
 
 static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
 {
+    uint8_t tlb_ps;
     CPUState *cs = CPU(obj);
     LoongArchCPUClass *lacc = LOONGARCH_CPU_GET_CLASS(obj);
     CPULoongArchState *env = cpu_env(cs);
@@ -592,13 +593,17 @@  static void loongarch_cpu_reset_hold(Object *obj, ResetType type)
      */
     env->CSR_PGDH = 0;
     env->CSR_PGDL = 0;
-    env->CSR_PWCL = 0;
     env->CSR_PWCH = 0;
-    env->CSR_STLBPS = 0;
     env->CSR_EENTRY = 0;
     env->CSR_TLBRENTRY = 0;
     env->CSR_MERRENTRY = 0;
-
+    /* set CSR_PWCL.PTBASE and CSR_STLBPS.PS bits from CSR_PRCFG2 */
+    if (env->CSR_PRCFG2 == 0) {
+        env->CSR_PRCFG2 = 0x3fffff000;
+    }
+    tlb_ps = ctz32(env->CSR_PRCFG2);
+    env->CSR_STLBPS = FIELD_DP64(env->CSR_STLBPS, CSR_STLBPS, PS, tlb_ps);
+    env->CSR_PWCL = FIELD_DP64(env->CSR_PWCL, CSR_PWCL, PTBASE, tlb_ps);
     for (n = 0; n < 4; n++) {
         env->CSR_DMW[n] = FIELD_DP64(env->CSR_DMW[n], CSR_DMW, PLV0, 0);
         env->CSR_DMW[n] = FIELD_DP64(env->CSR_DMW[n], CSR_DMW, PLV1, 0);
diff --git a/target/loongarch/helper.h b/target/loongarch/helper.h
index 943517b5f2..1d5cb0198c 100644
--- a/target/loongarch/helper.h
+++ b/target/loongarch/helper.h
@@ -100,6 +100,7 @@  DEF_HELPER_1(rdtime_d, i64, env)
 DEF_HELPER_1(csrrd_pgd, i64, env)
 DEF_HELPER_1(csrrd_cpuid, i64, env)
 DEF_HELPER_1(csrrd_tval, i64, env)
+DEF_HELPER_2(csrwr_stlbps, i64, env, tl)
 DEF_HELPER_2(csrwr_estat, i64, env, tl)
 DEF_HELPER_2(csrwr_asid, i64, env, tl)
 DEF_HELPER_2(csrwr_tcfg, i64, env, tl)
diff --git a/target/loongarch/internals.h b/target/loongarch/internals.h
index 7b254c5f49..1cd959a766 100644
--- a/target/loongarch/internals.h
+++ b/target/loongarch/internals.h
@@ -43,6 +43,8 @@  enum {
     TLBRET_PE = 7,
 };
 
+bool check_ps(CPULoongArchState *ent, int ps);
+
 extern const VMStateDescription vmstate_loongarch_cpu;
 
 void loongarch_cpu_set_irq(void *opaque, int irq, int level);
diff --git a/target/loongarch/tcg/csr_helper.c b/target/loongarch/tcg/csr_helper.c
index 6c95be9910..289d89266e 100644
--- a/target/loongarch/tcg/csr_helper.c
+++ b/target/loongarch/tcg/csr_helper.c
@@ -17,6 +17,22 @@ 
 #include "hw/irq.h"
 #include "cpu-csr.h"
 
+target_ulong helper_csrwr_stlbps(CPULoongArchState *env, target_ulong val)
+{
+    int64_t old_v = env->CSR_STLBPS;
+
+    /*
+     * The real hardware only supports the min tlb_ps is 12
+     * tlb_ps=0 may cause undefined-behavior.
+     */
+    uint8_t tlb_ps = FIELD_EX64(env->CSR_STLBPS, CSR_STLBPS, PS);
+    if (!check_ps(env, tlb_ps)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Attempted set ps %d\n", tlb_ps);
+    }
+    return old_v;
+}
+
 target_ulong helper_csrrd_pgd(CPULoongArchState *env)
 {
     int64_t v;
@@ -99,7 +115,7 @@  target_ulong helper_csrwr_ticlr(CPULoongArchState *env, target_ulong val)
 
 target_ulong helper_csrwr_pwcl(CPULoongArchState *env, target_ulong val)
 {
-    int shift;
+    int shift, ptbase;
     int64_t old_v = env->CSR_PWCL;
 
     /*
@@ -107,12 +123,16 @@  target_ulong helper_csrwr_pwcl(CPULoongArchState *env, target_ulong val)
      * treated as illegal.
      */
     shift = FIELD_EX64(val, CSR_PWCL, PTEWIDTH);
+    ptbase = FIELD_EX64(val, CSR_PWCL, PTBASE);
     if (shift) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "Attempted set pte width with %d bit\n", 64 << shift);
         val = FIELD_DP64(val, CSR_PWCL, PTEWIDTH, 0);
     }
-
-    env->CSR_PWCL = val;
+    if (!check_ps(env, ptbase)) {
+         qemu_log_mask(LOG_GUEST_ERROR,
+                      "Attrmpted set ptbase 2^%d\n", ptbase);
+    }
+    env->CSR_PWCL =val;
     return old_v;
 }
diff --git a/target/loongarch/tcg/insn_trans/trans_privileged.c.inc b/target/loongarch/tcg/insn_trans/trans_privileged.c.inc
index 3afa23af79..ecbfe23b63 100644
--- a/target/loongarch/tcg/insn_trans/trans_privileged.c.inc
+++ b/target/loongarch/tcg/insn_trans/trans_privileged.c.inc
@@ -74,6 +74,7 @@  static bool set_csr_trans_func(unsigned int csr_num, GenCSRRead readfn,
 
 void loongarch_csr_translate_init(void)
 {
+    SET_CSR_FUNC(STLBPS, NULL, gen_helper_csrwr_stlbps);
     SET_CSR_FUNC(ESTAT, NULL, gen_helper_csrwr_estat);
     SET_CSR_FUNC(ASID,  NULL, gen_helper_csrwr_asid);
     SET_CSR_FUNC(PGD,   gen_helper_csrrd_pgd, NULL);
diff --git a/target/loongarch/tcg/tlb_helper.c b/target/loongarch/tcg/tlb_helper.c
index 27c729b5b5..5a426691bc 100644
--- a/target/loongarch/tcg/tlb_helper.c
+++ b/target/loongarch/tcg/tlb_helper.c
@@ -18,6 +18,14 @@ 
 #include "exec/log.h"
 #include "cpu-csr.h"
 
+bool check_ps(CPULoongArchState *env, int tlb_ps)
+{
+     if (tlb_ps > 64) {
+         return false;
+     }
+     return BIT_ULL(tlb_ps) & (env->CSR_PRCFG2);
+}
+
 void get_dir_base_width(CPULoongArchState *env, uint64_t *dir_base,
                                uint64_t *dir_width, target_ulong level)
 {
@@ -191,8 +199,10 @@  static void fill_tlb_entry(CPULoongArchState *env, int index)
         lo1 = env->CSR_TLBELO1;
     }
 
-    if (csr_ps == 0) {
-        qemu_log_mask(CPU_LOG_MMU, "page size is 0\n");
+    /*check csr_ps */
+    if (!check_ps(env, csr_ps)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "csr_ps %d is illegal\n", csr_ps);
+        return;
     }
 
     /* Only MTLB has the ps fields */
@@ -302,7 +312,16 @@  void helper_tlbfill(CPULoongArchState *env)
         pagesize = FIELD_EX64(env->CSR_TLBIDX, CSR_TLBIDX, PS);
     }
 
+    if (!check_ps(env, pagesize)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "pagesize %d is illegal\n", pagesize);
+        return;
+    }
+
     stlb_ps = FIELD_EX64(env->CSR_STLBPS, CSR_STLBPS, PS);
+    if (!check_ps(env, stlb_ps)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "stlb_ps %d is illegal\n", stlb_ps);
+        return;
+    }
 
     if (pagesize == stlb_ps) {
         /* Only write into STLB bits [47:13] */