diff mbox series

[PULL,2/3] target/loongarch: Fix tlb huge page loading issue

Message ID 20240320024010.1659193-3-gaosong@loongson.cn (mailing list archive)
State New, archived
Headers show
Series [PULL,1/3] hw/intc/loongarch_extioi: Fix interrupt routing update | expand

Commit Message

gaosong March 20, 2024, 2:40 a.m. UTC 
From: Xianglai Li <lixianglai@loongson.cn>

When we use qemu tcg simulation, the page size of bios is 4KB.
When using the level 2 super huge page (page size is 1G) to create the page table,
it is found that the content of the corresponding address space is abnormal,
resulting in the bios can not start the operating system and graphical interface normally.

The lddir and ldpte instruction emulation has
a problem with the use of super huge page processing above level 2.
The page size is not correctly calculated,
resulting in the wrong page size of the table entry found by tlb.

Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Song Gao <gaosong@loongson.cn>
Message-Id: <20240318070332.1273939-1-lixianglai@loongson.cn>
---
 target/loongarch/cpu-csr.h        |   3 +
 target/loongarch/internals.h      |   5 --
 target/loongarch/tcg/tlb_helper.c | 113 +++++++++++++++++++++---------
 3 files changed, 82 insertions(+), 39 deletions(-)

Comments

Michael Tokarev March 21, 2024, 5:10 p.m. UTC | #1 
20.03.2024 05:40, Song Gao :
> From: Xianglai Li <lixianglai@loongson.cn>


> +    if (unlikely((level == 0) || (level > 4))) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Attepted LDDIR with level %"PRId64"\n", level);

Attempted.  FWIW, it's applied already.

/mjt
Peter Maydell July 23, 2024, 3:47 p.m. UTC | #2 
On Wed, 20 Mar 2024 at 02:40, Song Gao <gaosong@loongson.cn> wrote:
>
> From: Xianglai Li <lixianglai@loongson.cn>
>
> When we use qemu tcg simulation, the page size of bios is 4KB.
> When using the level 2 super huge page (page size is 1G) to create the page table,
> it is found that the content of the corresponding address space is abnormal,
> resulting in the bios can not start the operating system and graphical interface normally.
>
> The lddir and ldpte instruction emulation has
> a problem with the use of super huge page processing above level 2.
> The page size is not correctly calculated,
> resulting in the wrong page size of the table entry found by tlb.
>
> Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> Message-Id: <20240318070332.1273939-1-lixianglai@loongson.cn>

Hi; Coverity points out an issue with this patch (Coverity
CID 1547717):


> @@ -485,7 +513,25 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
>      target_ulong badvaddr, index, phys, ret;
>      int shift;
>      uint64_t dir_base, dir_width;
> -    bool huge = (base >> LOONGARCH_PAGE_HUGE_SHIFT) & 0x1;
> +
> +    if (unlikely((level == 0) || (level > 4))) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "Attepted LDDIR with level %"PRId64"\n", level);
> +        return base;
> +    }
> +
> +    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
> +        if (unlikely(level == 4)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "Attempted use of level 4 huge page\n");

Here we log level == 4 as being a guest error, but there is no
early "return <something>" the way the previous error-exit
codepath did above...

> +        }
> +
> +        if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
> +            return base;
> +        } else {
> +            return FIELD_DP64(base, TLBENTRY, LEVEL, level);

...so Coverity complains that here we will try to put that value 4
into a field in the TLBENTRY that is only 2 bits wide.

> +        }
> +    }

Should the level == 4 if() do a "return base" like the
error cases for level == 0 or > 4 ?

thanks
-- PMM
gaosong July 24, 2024, 1 a.m. UTC | #3 
在 2024/7/23 下午11:47, Peter Maydell 写道:
> On Wed, 20 Mar 2024 at 02:40, Song Gao <gaosong@loongson.cn> wrote:
>> From: Xianglai Li <lixianglai@loongson.cn>
>>
>> When we use qemu tcg simulation, the page size of bios is 4KB.
>> When using the level 2 super huge page (page size is 1G) to create the page table,
>> it is found that the content of the corresponding address space is abnormal,
>> resulting in the bios can not start the operating system and graphical interface normally.
>>
>> The lddir and ldpte instruction emulation has
>> a problem with the use of super huge page processing above level 2.
>> The page size is not correctly calculated,
>> resulting in the wrong page size of the table entry found by tlb.
>>
>> Signed-off-by: Xianglai Li <lixianglai@loongson.cn>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>> Message-Id: <20240318070332.1273939-1-lixianglai@loongson.cn>
> Hi; Coverity points out an issue with this patch (Coverity
> CID 1547717):
>
>
>> @@ -485,7 +513,25 @@ target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
>>       target_ulong badvaddr, index, phys, ret;
>>       int shift;
>>       uint64_t dir_base, dir_width;
>> -    bool huge = (base >> LOONGARCH_PAGE_HUGE_SHIFT) & 0x1;
>> +
>> +    if (unlikely((level == 0) || (level > 4))) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "Attepted LDDIR with level %"PRId64"\n", level);
>> +        return base;
>> +    }
>> +
>> +    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
>> +        if (unlikely(level == 4)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "Attempted use of level 4 huge page\n");
> Here we log level == 4 as being a guest error, but there is no
> early "return <something>" the way the previous error-exit
> codepath did above...
>
>> +        }
>> +
>> +        if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
>> +            return base;
>> +        } else {
>> +            return FIELD_DP64(base, TLBENTRY, LEVEL, level);
> ...so Coverity complains that here we will try to put that value 4
> into a field in the TLBENTRY that is only 2 bits wide.
>
>> +        }
>> +    }
> Should the level == 4 if() do a "return base" like the
> error cases for level == 0 or > 4 ?
Yes,  we should,  I will send a patch to fix it.

Thanks.
Song Gao
diff mbox series

Patch

diff --git a/target/loongarch/cpu-csr.h b/target/loongarch/cpu-csr.h
index c59d7a9fcb..0834e91f30 100644
--- a/target/loongarch/cpu-csr.h
+++ b/target/loongarch/cpu-csr.h
@@ -67,6 +67,9 @@  FIELD(TLBENTRY, D, 1, 1)
 FIELD(TLBENTRY, PLV, 2, 2)
 FIELD(TLBENTRY, MAT, 4, 2)
 FIELD(TLBENTRY, G, 6, 1)
+FIELD(TLBENTRY, HUGE, 6, 1)
+FIELD(TLBENTRY, HGLOBAL, 12, 1)
+FIELD(TLBENTRY, LEVEL, 13, 2)
 FIELD(TLBENTRY_32, PPN, 8, 24)
 FIELD(TLBENTRY_64, PPN, 12, 36)
 FIELD(TLBENTRY_64, NR, 61, 1)
diff --git a/target/loongarch/internals.h b/target/loongarch/internals.h
index a2fc54c8a7..944153b180 100644
--- a/target/loongarch/internals.h
+++ b/target/loongarch/internals.h
@@ -16,11 +16,6 @@ 
 #define TARGET_PHYS_MASK MAKE_64BIT_MASK(0, TARGET_PHYS_ADDR_SPACE_BITS)
 #define TARGET_VIRT_MASK MAKE_64BIT_MASK(0, TARGET_VIRT_ADDR_SPACE_BITS)
 
-/* Global bit used for lddir/ldpte */
-#define LOONGARCH_PAGE_HUGE_SHIFT   6
-/* Global bit for huge page */
-#define LOONGARCH_HGLOBAL_SHIFT     12
-
 void loongarch_translate_init(void);
 
 void loongarch_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
diff --git a/target/loongarch/tcg/tlb_helper.c b/target/loongarch/tcg/tlb_helper.c
index 22be031ac7..57f5308632 100644
--- a/target/loongarch/tcg/tlb_helper.c
+++ b/target/loongarch/tcg/tlb_helper.c
@@ -17,6 +17,34 @@ 
 #include "exec/log.h"
 #include "cpu-csr.h"
 
+static void get_dir_base_width(CPULoongArchState *env, uint64_t *dir_base,
+                               uint64_t *dir_width, target_ulong level)
+{
+    switch (level) {
+    case 1:
+        *dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_BASE);
+        *dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_WIDTH);
+        break;
+    case 2:
+        *dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_BASE);
+        *dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_WIDTH);
+        break;
+    case 3:
+        *dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_BASE);
+        *dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_WIDTH);
+        break;
+    case 4:
+        *dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_BASE);
+        *dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_WIDTH);
+        break;
+    default:
+        /* level may be zero for ldpte */
+        *dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTBASE);
+        *dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTWIDTH);
+        break;
+    }
+}
+
 static void raise_mmu_exception(CPULoongArchState *env, target_ulong address,
                                 MMUAccessType access_type, int tlb_error)
 {
@@ -485,7 +513,25 @@  target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
     target_ulong badvaddr, index, phys, ret;
     int shift;
     uint64_t dir_base, dir_width;
-    bool huge = (base >> LOONGARCH_PAGE_HUGE_SHIFT) & 0x1;
+
+    if (unlikely((level == 0) || (level > 4))) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "Attepted LDDIR with level %"PRId64"\n", level);
+        return base;
+    }
+
+    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
+        if (unlikely(level == 4)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "Attempted use of level 4 huge page\n");
+        }
+
+        if (FIELD_EX64(base, TLBENTRY, LEVEL)) {
+            return base;
+        } else {
+            return FIELD_DP64(base, TLBENTRY, LEVEL, level);
+        }
+    }
 
     badvaddr = env->CSR_TLBRBADV;
     base = base & TARGET_PHYS_MASK;
@@ -494,30 +540,7 @@  target_ulong helper_lddir(CPULoongArchState *env, target_ulong base,
     shift = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTEWIDTH);
     shift = (shift + 1) * 3;
 
-    if (huge) {
-        return base;
-    }
-    switch (level) {
-    case 1:
-        dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_BASE);
-        dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR1_WIDTH);
-        break;
-    case 2:
-        dir_base = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_BASE);
-        dir_width = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, DIR2_WIDTH);
-        break;
-    case 3:
-        dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_BASE);
-        dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR3_WIDTH);
-        break;
-    case 4:
-        dir_base = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_BASE);
-        dir_width = FIELD_EX64(env->CSR_PWCH, CSR_PWCH, DIR4_WIDTH);
-        break;
-    default:
-        do_raise_exception(env, EXCCODE_INE, GETPC());
-        return 0;
-    }
+    get_dir_base_width(env, &dir_base, &dir_width, level);
     index = (badvaddr >> dir_base) & ((1 << dir_width) - 1);
     phys = base | index << shift;
     ret = ldq_phys(cs->as, phys) & TARGET_PHYS_MASK;
@@ -530,20 +553,42 @@  void helper_ldpte(CPULoongArchState *env, target_ulong base, target_ulong odd,
     CPUState *cs = env_cpu(env);
     target_ulong phys, tmp0, ptindex, ptoffset0, ptoffset1, ps, badv;
     int shift;
-    bool huge = (base >> LOONGARCH_PAGE_HUGE_SHIFT) & 0x1;
     uint64_t ptbase = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTBASE);
     uint64_t ptwidth = FIELD_EX64(env->CSR_PWCL, CSR_PWCL, PTWIDTH);
+    uint64_t dir_base, dir_width;
 
+    /*
+     * The parameter "base" has only two types,
+     * one is the page table base address,
+     * whose bit 6 should be 0,
+     * and the other is the huge page entry,
+     * whose bit 6 should be 1.
+     */
     base = base & TARGET_PHYS_MASK;
+    if (FIELD_EX64(base, TLBENTRY, HUGE)) {
+        /*
+         * Gets the huge page level and Gets huge page size.
+         * Clears the huge page level information in the entry.
+         * Clears huge page bit.
+         * Move HGLOBAL bit to GLOBAL bit.
+         */
+        get_dir_base_width(env, &dir_base, &dir_width,
+                           FIELD_EX64(base, TLBENTRY, LEVEL));
+
+        base = FIELD_DP64(base, TLBENTRY, LEVEL, 0);
+        base = FIELD_DP64(base, TLBENTRY, HUGE, 0);
+        if (FIELD_EX64(base, TLBENTRY, HGLOBAL)) {
+            base = FIELD_DP64(base, TLBENTRY, HGLOBAL, 0);
+            base = FIELD_DP64(base, TLBENTRY, G, 1);
+        }
 
-    if (huge) {
-        /* Huge Page. base is paddr */
-        tmp0 = base ^ (1 << LOONGARCH_PAGE_HUGE_SHIFT);
-        /* Move Global bit */
-        tmp0 = ((tmp0 & (1 << LOONGARCH_HGLOBAL_SHIFT))  >>
-                LOONGARCH_HGLOBAL_SHIFT) << R_TLBENTRY_G_SHIFT |
-                (tmp0 & (~(1 << LOONGARCH_HGLOBAL_SHIFT)));
-        ps = ptbase + ptwidth - 1;
+        ps = dir_base + dir_width - 1;
+        /*
+         * Huge pages are evenly split into parity pages
+         * when loaded into the tlb,
+         * so the tlb page size needs to be divided by 2.
+         */
+        tmp0 = base;
         if (odd) {
             tmp0 += MAKE_64BIT_MASK(ps, 1);
         }