* [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue [not found] <cover.1710379781.git.lixianglai@loongson.cn> @ 2024-03-14 1:33 ` Xianglai Li 2024-03-14 2:28 ` chen huacai 2024-03-14 16:30 ` Richard Henderson 0 siblings, 2 replies; 8+ messages in thread From: Xianglai Li @ 2024-03-14 1:33 UTC (permalink / raw) To: qemu-devel Cc: maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang When we use qemu tcg simulation, the page size of bios is 4KB. When using the level 2 super large 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 large 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. Cc: maobibo@loongson.cn Cc: Song Gao <gaosong@loongson.cn> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn> Cc: zhaotianrui@loongson.cn Cc: yijun@loongson.cn Cc: wuruiyang@loongson.cn Signed-off-by: Xianglai Li <lixianglai@loongson.cn> --- target/loongarch/cpu-csr.h | 3 + target/loongarch/internals.h | 5 -- target/loongarch/tcg/tlb_helper.c | 105 ++++++++++++++++++++---------- 3 files changed, 74 insertions(+), 39 deletions(-) Changes log: V3->V4: Optimize the huge page calculation method, use the FIELD macro for bit calculation. V2->V3: Delete the intermediate variable LDDIR_PS, and implement lddir and ldpte huge pages by referring to the latest architecture reference manual. V1->V2: Modified the patch title format and Enrich the commit mesg description diff --git a/target/loongarch/cpu-csr.h b/target/loongarch/cpu-csr.h index c59d7a9fcb..b0775cf6bf 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, HG, 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..b9a8633791 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,23 @@ 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))) { + return base; + } + + if (FIELD_EX64(base, TLBENTRY, HUGE)) { + if (FIELD_EX64(base, TLBENTRY, LEVEL)) { + return base; + } else { + return FIELD_DP64(base, TLBENTRY, LEVEL, level); + } + + if (unlikely(level == 4)) { + qemu_log_mask(LOG_GUEST_ERROR, + "Attempted use of level %lu huge page\n", level); + } + } badvaddr = env->CSR_TLBRBADV; base = base & TARGET_PHYS_MASK; @@ -494,33 +538,12 @@ 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; + return ret; } @@ -530,20 +553,34 @@ 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; 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 address + * Clears huge page bit + */ + get_dir_base_width(env, &dir_base, &dir_width, + FIELD_EX64(base, TLBENTRY, LEVEL)); + + FIELD_DP64(base, TLBENTRY, LEVEL, 0); + FIELD_DP64(base, TLBENTRY, HUGE, 0); + if (FIELD_EX64(base, TLBENTRY, HG)) { + FIELD_DP64(base, TLBENTRY, HG, 0); + 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; + /* + * Huge pages are evenly split into parity pages + * when loaded into the tlb, + * so the tlb page size needs to be divided by 2. + */ + ps = dir_base + dir_width - 1; + tmp0 = base; if (odd) { tmp0 += MAKE_64BIT_MASK(ps, 1); } -- 2.39.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue 2024-03-14 1:33 ` [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue Xianglai Li @ 2024-03-14 2:28 ` chen huacai 2024-03-14 16:30 ` Richard Henderson 1 sibling, 0 replies; 8+ messages in thread From: chen huacai @ 2024-03-14 2:28 UTC (permalink / raw) To: Xianglai Li Cc: qemu-devel, maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang Hi, Xianglai, Generally, the subject should be "Fix tlb huge page loading issue" rather than "Fixed tlb huge page loading issue". On Thu, Mar 14, 2024 at 9:34 AM Xianglai Li <lixianglai@loongson.cn> wrote: > > When we use qemu tcg simulation, the page size of bios is 4KB. > When using the level 2 super large 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 large 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. > > Cc: maobibo@loongson.cn > Cc: Song Gao <gaosong@loongson.cn> > Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn> > Cc: zhaotianrui@loongson.cn > Cc: yijun@loongson.cn > Cc: wuruiyang@loongson.cn > > Signed-off-by: Xianglai Li <lixianglai@loongson.cn> > --- > target/loongarch/cpu-csr.h | 3 + > target/loongarch/internals.h | 5 -- > target/loongarch/tcg/tlb_helper.c | 105 ++++++++++++++++++++---------- > 3 files changed, 74 insertions(+), 39 deletions(-) > > Changes log: > V3->V4: > Optimize the huge page calculation method, > use the FIELD macro for bit calculation. > > V2->V3: > Delete the intermediate variable LDDIR_PS, and implement lddir and ldpte > huge pages by referring to the latest architecture reference manual. > > V1->V2: > Modified the patch title format and Enrich the commit mesg description > > diff --git a/target/loongarch/cpu-csr.h b/target/loongarch/cpu-csr.h > index c59d7a9fcb..b0775cf6bf 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, HG, 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..b9a8633791 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,23 @@ 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))) { > + return base; > + } > + > + if (FIELD_EX64(base, TLBENTRY, HUGE)) { > + if (FIELD_EX64(base, TLBENTRY, LEVEL)) { > + return base; > + } else { > + return FIELD_DP64(base, TLBENTRY, LEVEL, level); > + } > + > + if (unlikely(level == 4)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "Attempted use of level %lu huge page\n", level); > + } > + } > > badvaddr = env->CSR_TLBRBADV; > base = base & TARGET_PHYS_MASK; > @@ -494,33 +538,12 @@ 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; > + > return ret; > } > > @@ -530,20 +553,34 @@ 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; > > 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 address > + * Clears huge page bit > + */ > + get_dir_base_width(env, &dir_base, &dir_width, > + FIELD_EX64(base, TLBENTRY, LEVEL)); > + > + FIELD_DP64(base, TLBENTRY, LEVEL, 0); > + FIELD_DP64(base, TLBENTRY, HUGE, 0); > + if (FIELD_EX64(base, TLBENTRY, HG)) { > + FIELD_DP64(base, TLBENTRY, HG, 0); > + 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; > + /* > + * Huge pages are evenly split into parity pages > + * when loaded into the tlb, > + * so the tlb page size needs to be divided by 2. > + */ > + ps = dir_base + dir_width - 1; > + tmp0 = base; > if (odd) { > tmp0 += MAKE_64BIT_MASK(ps, 1); > } > -- > 2.39.1 > > -- Huacai Chen ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue 2024-03-14 1:33 ` [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue Xianglai Li 2024-03-14 2:28 ` chen huacai @ 2024-03-14 16:30 ` Richard Henderson 2024-03-15 6:22 ` lixianglai 1 sibling, 1 reply; 8+ messages in thread From: Richard Henderson @ 2024-03-14 16:30 UTC (permalink / raw) To: Xianglai Li, qemu-devel Cc: maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang On 3/13/24 15:33, Xianglai Li wrote: > + if (unlikely((level == 0) || (level > 4))) { > + return base; > + } > + > + if (FIELD_EX64(base, TLBENTRY, HUGE)) { > + if (FIELD_EX64(base, TLBENTRY, LEVEL)) { > + return base; > + } else { > + return FIELD_DP64(base, TLBENTRY, LEVEL, level); > + } > + > + if (unlikely(level == 4)) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "Attempted use of level %lu huge page\n", level); > + } This block is unreachable, because you've already returned. Perhaps it would be worthwhile to add another for the level==0 or > 4 case above? > @@ -530,20 +553,34 @@ 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; > > 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 address > + * Clears huge page bit > + */ > + get_dir_base_width(env, &dir_base, &dir_width, > + FIELD_EX64(base, TLBENTRY, LEVEL)); > + > + FIELD_DP64(base, TLBENTRY, LEVEL, 0); > + FIELD_DP64(base, TLBENTRY, HUGE, 0); > + if (FIELD_EX64(base, TLBENTRY, HG)) { > + FIELD_DP64(base, TLBENTRY, HG, 0); > + FIELD_DP64(base, TLBENTRY, G, 1); FIELD_DP64 returns a value. You need base = FIELD_DP64(base, ...); r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue 2024-03-14 16:30 ` Richard Henderson @ 2024-03-15 6:22 ` lixianglai 2024-03-15 7:46 ` Richard Henderson 0 siblings, 1 reply; 8+ messages in thread From: lixianglai @ 2024-03-15 6:22 UTC (permalink / raw) To: Richard Henderson, qemu-devel Cc: maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang Hi Richard: > On 3/13/24 15:33, Xianglai Li wrote: >> + if (unlikely((level == 0) || (level > 4))) { >> + return base; >> + } >> + >> + if (FIELD_EX64(base, TLBENTRY, HUGE)) { >> + if (FIELD_EX64(base, TLBENTRY, LEVEL)) { >> + return base; >> + } else { >> + return FIELD_DP64(base, TLBENTRY, LEVEL, level); >> + } >> + >> + if (unlikely(level == 4)) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "Attempted use of level %lu huge page\n", >> level); >> + } > > This block is unreachable, because you've already returned. > Perhaps it would be worthwhile to add another for the level==0 or > 4 > case above? > A normal level 4 page table should not print an error log, only if a level 4 page is large, so we should put it in if (FIELD_EX64(base, TLBENTRY, HUGE)) { if (unlikely(level == 4)) { qemu_log_mask(LOG_GUEST_ERROR, "Attempted use of level %lu huge page\n", level); } if (FIELD_EX64(base, TLBENTRY, LEVEL)) { return base; } else { return FIELD_DP64(base, TLBENTRY, LEVEL, level); } } Thanks! Xianglai. >> @@ -530,20 +553,34 @@ 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; >> 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 address >> + * Clears huge page bit >> + */ >> + get_dir_base_width(env, &dir_base, &dir_width, >> + FIELD_EX64(base, TLBENTRY, LEVEL)); >> + >> + FIELD_DP64(base, TLBENTRY, LEVEL, 0); >> + FIELD_DP64(base, TLBENTRY, HUGE, 0); >> + if (FIELD_EX64(base, TLBENTRY, HG)) { >> + FIELD_DP64(base, TLBENTRY, HG, 0); >> + FIELD_DP64(base, TLBENTRY, G, 1); > > FIELD_DP64 returns a value. You need > > base = FIELD_DP64(base, ...); > > > r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue 2024-03-15 6:22 ` lixianglai @ 2024-03-15 7:46 ` Richard Henderson 2024-03-15 9:01 ` lixianglai 0 siblings, 1 reply; 8+ messages in thread From: Richard Henderson @ 2024-03-15 7:46 UTC (permalink / raw) To: lixianglai, qemu-devel Cc: maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang On 3/14/24 20:22, lixianglai wrote: > Hi Richard: >> On 3/13/24 15:33, Xianglai Li wrote: >>> + if (unlikely((level == 0) || (level > 4))) { >>> + return base; >>> + } ... >> Perhaps it would be worthwhile to add another for the level==0 or > 4 case above? >> > A normal level 4 page table should not print an error log, > > only if a level 4 page is large, so we should put it in > > if (FIELD_EX64(base, TLBENTRY, HUGE)) { > if (unlikely(level == 4)) { > qemu_log_mask(LOG_GUEST_ERROR, > "Attempted use of level %lu huge page\n", level); > } > > if (FIELD_EX64(base, TLBENTRY, LEVEL)) { > return base; > } else { > return FIELD_DP64(base, TLBENTRY, LEVEL, level); > } > } A level 5 page table is not normal, nor is a level 0 lddir. r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue 2024-03-15 7:46 ` Richard Henderson @ 2024-03-15 9:01 ` lixianglai 2024-03-15 17:06 ` Richard Henderson 0 siblings, 1 reply; 8+ messages in thread From: lixianglai @ 2024-03-15 9:01 UTC (permalink / raw) To: Richard Henderson, qemu-devel Cc: maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang Hi Richard : > On 3/14/24 20:22, lixianglai wrote: >> Hi Richard: >>> On 3/13/24 15:33, Xianglai Li wrote: >>>> + if (unlikely((level == 0) || (level > 4))) { >>>> + return base; >>>> + } > ... >>> Perhaps it would be worthwhile to add another for the level==0 or > >>> 4 case above? >>> >> A normal level 4 page table should not print an error log, >> >> only if a level 4 page is large, so we should put it in >> >> if (FIELD_EX64(base, TLBENTRY, HUGE)) { >> if (unlikely(level == 4)) { >> qemu_log_mask(LOG_GUEST_ERROR, >> "Attempted use of level %lu huge page\n", >> level); >> } >> >> if (FIELD_EX64(base, TLBENTRY, LEVEL)) { >> return base; >> } else { >> return FIELD_DP64(base, TLBENTRY, LEVEL, level); >> } >> } > > A level 5 page table is not normal, nor is a level 0 lddir. > We communicate with the hardware guys that the behavior above level 4 and lddir 0 is undefined behavior. The result of our test on 3A5000 is that it has no any effect on "base", however in future chips the behavior may change since it may support 5-level page table and width for level[13:14] may change also. So in this context,I am not sure which level to use to print logs, which content to print, and where to add these prints, any more detailed advice? Thanks! Xianglai. > > r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue 2024-03-15 9:01 ` lixianglai @ 2024-03-15 17:06 ` Richard Henderson 2024-03-16 1:19 ` lixianglai 0 siblings, 1 reply; 8+ messages in thread From: Richard Henderson @ 2024-03-15 17:06 UTC (permalink / raw) To: lixianglai, qemu-devel Cc: maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang On 3/14/24 23:01, lixianglai wrote: > Hi Richard : > >> On 3/14/24 20:22, lixianglai wrote: >>> Hi Richard: >>>> On 3/13/24 15:33, Xianglai Li wrote: >>>>> + if (unlikely((level == 0) || (level > 4))) { >>>>> + return base; >>>>> + } >> ... >>>> Perhaps it would be worthwhile to add another for the level==0 or > 4 case above? >>>> >>> A normal level 4 page table should not print an error log, >>> >>> only if a level 4 page is large, so we should put it in >>> >>> if (FIELD_EX64(base, TLBENTRY, HUGE)) { >>> if (unlikely(level == 4)) { >>> qemu_log_mask(LOG_GUEST_ERROR, >>> "Attempted use of level %lu huge page\n", level); >>> } >>> >>> if (FIELD_EX64(base, TLBENTRY, LEVEL)) { >>> return base; >>> } else { >>> return FIELD_DP64(base, TLBENTRY, LEVEL, level); >>> } >>> } >> >> A level 5 page table is not normal, nor is a level 0 lddir. >> > > We communicate with the hardware guys that the behavior above level 4 and lddir 0 is > undefined behavior. > > The result of our test on 3A5000 is that it has no any effect on "base", > > however in future chips the behavior may change since it may support 5-level page table > and width for level[13:14] may change also. > > > So in this context,I am not sure which level to use to print logs, > > which content to print, and where to add these prints, > > any more detailed advice? Yes, right there in the IF that I quoted at the top. What I was trying to spell out is if (unlikely(level == 0 || level > 4)) { qemu_log_mask(LOG_GUEST_ERROR, "Attepted LDDIR with level %"PRId64"\n", level); return base; } r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue 2024-03-15 17:06 ` Richard Henderson @ 2024-03-16 1:19 ` lixianglai 0 siblings, 0 replies; 8+ messages in thread From: lixianglai @ 2024-03-16 1:19 UTC (permalink / raw) To: Richard Henderson, qemu-devel Cc: maobibo, Song Gao, Xiaojuan Yang, zhaotianrui, yijun, wuruiyang 在 2024/3/16 上午1:06, Richard Henderson 写道: > On 3/14/24 23:01, lixianglai wrote: >> Hi Richard : >> >>> On 3/14/24 20:22, lixianglai wrote: >>>> Hi Richard: >>>>> On 3/13/24 15:33, Xianglai Li wrote: >>>>>> + if (unlikely((level == 0) || (level > 4))) { >>>>>> + return base; >>>>>> + } >>> ... >>>>> Perhaps it would be worthwhile to add another for the level==0 or > 4 case above? >>>>> >>>> A normal level 4 page table should not print an error log, >>>> >>>> only if a level 4 page is large, so we should put it in >>>> >>>> if (FIELD_EX64(base, TLBENTRY, HUGE)) { >>>> if (unlikely(level == 4)) { >>>> qemu_log_mask(LOG_GUEST_ERROR, >>>> "Attempted use of level %lu huge page\n", level); >>>> } >>>> >>>> if (FIELD_EX64(base, TLBENTRY, LEVEL)) { >>>> return base; >>>> } else { >>>> return FIELD_DP64(base, TLBENTRY, LEVEL, level); >>>> } >>>> } >>> >>> A level 5 page table is not normal, nor is a level 0 lddir. >>> >> >> We communicate with the hardware guys that the behavior above level 4 and lddir 0 is undefined behavior. >> >> The result of our test on 3A5000 is that it has no any effect on "base", >> >> however in future chips the behavior may change since it may support 5-level page table and width for level[13:14] may change also. >> >> >> So in this context,I am not sure which level to use to print logs, >> >> which content to print, and where to add these prints, >> >> any more detailed advice? > > Yes, right there in the IF that I quoted at the top. > What I was trying to spell out is > > if (unlikely(level == 0 || level > 4)) { > qemu_log_mask(LOG_GUEST_ERROR, > "Attepted LDDIR with level %"PRId64"\n", level); > return base; > } > Thank you very much, I will modify it in V6 version of patch. Thanks, Xianglai. > > r~ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-03-16 1:20 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1710379781.git.lixianglai@loongson.cn>
2024-03-14 1:33 ` [PATCH V4 1/1] target/loongarch: Fixed tlb huge page loading issue Xianglai Li
2024-03-14 2:28 ` chen huacai
2024-03-14 16:30 ` Richard Henderson
2024-03-15 6:22 ` lixianglai
2024-03-15 7:46 ` Richard Henderson
2024-03-15 9:01 ` lixianglai
2024-03-15 17:06 ` Richard Henderson
2024-03-16 1:19 ` lixianglai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).