| Message ID | 20240718020935.12803-1-cuiyunhui@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv/mm/fault: add show_pte() before die() | expand |
On Thu, Jul 18, 2024 at 10:09:35AM GMT, Yunhui Cui wrote: > When the kernel displays "Unable to handle kernel paging request at > virtual address", we would like to confirm the status of the virtual > address in the page table. So add show_pte() before die(). > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/mm/fault.c | 47 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > index 90d4ba36d1d0..dfe3ce38e16b 100644 > --- a/arch/riscv/mm/fault.c > +++ b/arch/riscv/mm/fault.c > @@ -22,6 +22,52 @@ > > #include "../kernel/head.h" > > +static void show_pte(unsigned long addr) > +{ > + pgd_t *pgdp, pgd; > + p4d_t *p4dp, p4d; > + pud_t *pudp, pud; > + pmd_t *pmdp, pmd; > + pte_t *ptep, pte; > + struct mm_struct *mm = current->mm; > + > + if (!mm) > + mm = &init_mm; arm64 show_pte starts with a summary line where the pgtable type (swapper vs. user), number of VA bits, and physical address of the pgd are displayed. Should we add that too? Thanks, drew > + pgdp = pgd_offset(mm, addr); > + pgd = READ_ONCE(*pgdp); > + pr_alert("[%016lx] pgd=%016lx", addr, pgd_val(pgd)); > + if (pgd_none(pgd) || pgd_bad(pgd)) > + goto out; > + > + p4dp = p4d_offset(pgdp, addr); > + p4d = READ_ONCE(*p4dp); > + pr_cont(", p4d=%016lx", p4d_val(p4d)); > + if (p4d_none(p4d) || p4d_bad(p4d)) > + goto out; > + > + pudp = pud_offset(p4dp, addr); > + pud = READ_ONCE(*pudp); > + pr_cont(", pud=%016lx", pud_val(pud)); > + if (pud_none(pud) || pud_bad(pud)) > + goto out; > + > + pmdp = pmd_offset(pudp, addr); > + pmd = READ_ONCE(*pmdp); > + pr_cont(", pmd=%016lx", pmd_val(pmd)); > + if (pmd_none(pmd) || pmd_bad(pmd)) > + goto out; > + > + ptep = pte_offset_map(pmdp, addr); > + if (!ptep) > + goto out; > + > + pte = READ_ONCE(*ptep); > + pr_cont(", pte=%016lx", pte_val(pte)); > + pte_unmap(ptep); > +out: > + pr_cont("\n"); > +} > + > static void die_kernel_fault(const char *msg, unsigned long addr, > struct pt_regs *regs) > { > @@ -31,6 +77,7 @@ static void die_kernel_fault(const char *msg, unsigned long addr, > addr); > > bust_spinlocks(0); > + show_pte(addr); > die(regs, "Oops"); > make_task_dead(SIGKILL); > } > -- > 2.39.2 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Yunhui, On Thu, Jul 18, 2024 at 4:20 AM Yunhui Cui <cuiyunhui@bytedance.com> wrote: > > When the kernel displays "Unable to handle kernel paging request at > virtual address", we would like to confirm the status of the virtual > address in the page table. So add show_pte() before die(). > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/mm/fault.c | 47 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > index 90d4ba36d1d0..dfe3ce38e16b 100644 > --- a/arch/riscv/mm/fault.c > +++ b/arch/riscv/mm/fault.c > @@ -22,6 +22,52 @@ > > #include "../kernel/head.h" > > +static void show_pte(unsigned long addr) > +{ > + pgd_t *pgdp, pgd; > + p4d_t *p4dp, p4d; > + pud_t *pudp, pud; > + pmd_t *pmdp, pmd; > + pte_t *ptep, pte; > + struct mm_struct *mm = current->mm; > + > + if (!mm) > + mm = &init_mm; > + pgdp = pgd_offset(mm, addr); > + pgd = READ_ONCE(*pgdp); > + pr_alert("[%016lx] pgd=%016lx", addr, pgd_val(pgd)); > + if (pgd_none(pgd) || pgd_bad(pgd)) > + goto out; > + > + p4dp = p4d_offset(pgdp, addr); > + p4d = READ_ONCE(*p4dp); > + pr_cont(", p4d=%016lx", p4d_val(p4d)); > + if (p4d_none(p4d) || p4d_bad(p4d)) > + goto out; > + > + pudp = pud_offset(p4dp, addr); > + pud = READ_ONCE(*pudp); > + pr_cont(", pud=%016lx", pud_val(pud)); > + if (pud_none(pud) || pud_bad(pud)) > + goto out; > + > + pmdp = pmd_offset(pudp, addr); > + pmd = READ_ONCE(*pmdp); > + pr_cont(", pmd=%016lx", pmd_val(pmd)); > + if (pmd_none(pmd) || pmd_bad(pmd)) > + goto out; > + > + ptep = pte_offset_map(pmdp, addr); > + if (!ptep) > + goto out; > + > + pte = READ_ONCE(*ptep); All the READ_ONCE() can be replaced by pXXp_get() macros defined here: https://elixir.bootlin.com/linux/v6.10/source/include/linux/pgtable.h#L315 And we should stop as soon as we encounter a leaf entry using pXd_leaf(). Thanks, Alex > + pr_cont(", pte=%016lx", pte_val(pte)); > + pte_unmap(ptep); > +out: > + pr_cont("\n"); > +} > + > static void die_kernel_fault(const char *msg, unsigned long addr, > struct pt_regs *regs) > { > @@ -31,6 +77,7 @@ static void die_kernel_fault(const char *msg, unsigned long addr, > addr); > > bust_spinlocks(0); > + show_pte(addr); > die(regs, "Oops"); > make_task_dead(SIGKILL); > } > -- > 2.39.2 > Otherwise, that's a good idea to print the page table content on fault so: Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Thanks, Alex
Hi drew, On Fri, Jul 19, 2024 at 10:33 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Thu, Jul 18, 2024 at 10:09:35AM GMT, Yunhui Cui wrote: > > When the kernel displays "Unable to handle kernel paging request at > > virtual address", we would like to confirm the status of the virtual > > address in the page table. So add show_pte() before die(). > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > --- > > arch/riscv/mm/fault.c | 47 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > > index 90d4ba36d1d0..dfe3ce38e16b 100644 > > --- a/arch/riscv/mm/fault.c > > +++ b/arch/riscv/mm/fault.c > > @@ -22,6 +22,52 @@ > > > > #include "../kernel/head.h" > > > > +static void show_pte(unsigned long addr) > > +{ > > + pgd_t *pgdp, pgd; > > + p4d_t *p4dp, p4d; > > + pud_t *pudp, pud; > > + pmd_t *pmdp, pmd; > > + pte_t *ptep, pte; > > + struct mm_struct *mm = current->mm; > > + > > + if (!mm) > > + mm = &init_mm; > > arm64 show_pte starts with a summary line where the pgtable type (swapper > vs. user), number of VA bits, and physical address of the pgd are > displayed. Should we add that too? Oaky， I will add it in v2. > > Thanks, > drew > > > + pgdp = pgd_offset(mm, addr); > > + pgd = READ_ONCE(*pgdp); > > + pr_alert("[%016lx] pgd=%016lx", addr, pgd_val(pgd)); > > + if (pgd_none(pgd) || pgd_bad(pgd)) > > + goto out; > > + > > + p4dp = p4d_offset(pgdp, addr); > > + p4d = READ_ONCE(*p4dp); > > + pr_cont(", p4d=%016lx", p4d_val(p4d)); > > + if (p4d_none(p4d) || p4d_bad(p4d)) > > + goto out; > > + > > + pudp = pud_offset(p4dp, addr); > > + pud = READ_ONCE(*pudp); > > + pr_cont(", pud=%016lx", pud_val(pud)); > > + if (pud_none(pud) || pud_bad(pud)) > > + goto out; > > + > > + pmdp = pmd_offset(pudp, addr); > > + pmd = READ_ONCE(*pmdp); > > + pr_cont(", pmd=%016lx", pmd_val(pmd)); > > + if (pmd_none(pmd) || pmd_bad(pmd)) > > + goto out; > > + > > + ptep = pte_offset_map(pmdp, addr); > > + if (!ptep) > > + goto out; > > + > > + pte = READ_ONCE(*ptep); > > + pr_cont(", pte=%016lx", pte_val(pte)); > > + pte_unmap(ptep); > > +out: > > + pr_cont("\n"); > > +} > > + > > static void die_kernel_fault(const char *msg, unsigned long addr, > > struct pt_regs *regs) > > { > > @@ -31,6 +77,7 @@ static void die_kernel_fault(const char *msg, unsigned long addr, > > addr); > > > > bust_spinlocks(0); > > + show_pte(addr); > > die(regs, "Oops"); > > make_task_dead(SIGKILL); > > } > > -- > > 2.39.2 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv Thanks， Yunhui
Hi Alex， On Sat, Jul 20, 2024 at 12:26 AM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > Hi Yunhui, > > On Thu, Jul 18, 2024 at 4:20 AM Yunhui Cui <cuiyunhui@bytedance.com> wrote: > > > > When the kernel displays "Unable to handle kernel paging request at > > virtual address", we would like to confirm the status of the virtual > > address in the page table. So add show_pte() before die(). > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > --- > > arch/riscv/mm/fault.c | 47 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > > index 90d4ba36d1d0..dfe3ce38e16b 100644 > > --- a/arch/riscv/mm/fault.c > > +++ b/arch/riscv/mm/fault.c > > @@ -22,6 +22,52 @@ > > > > #include "../kernel/head.h" > > > > +static void show_pte(unsigned long addr) > > +{ > > + pgd_t *pgdp, pgd; > > + p4d_t *p4dp, p4d; > > + pud_t *pudp, pud; > > + pmd_t *pmdp, pmd; > > + pte_t *ptep, pte; > > + struct mm_struct *mm = current->mm; > > + > > + if (!mm) > > + mm = &init_mm; > > + pgdp = pgd_offset(mm, addr); > > + pgd = READ_ONCE(*pgdp); > > + pr_alert("[%016lx] pgd=%016lx", addr, pgd_val(pgd)); > > + if (pgd_none(pgd) || pgd_bad(pgd)) > > + goto out; > > + > > + p4dp = p4d_offset(pgdp, addr); > > + p4d = READ_ONCE(*p4dp); > > + pr_cont(", p4d=%016lx", p4d_val(p4d)); > > + if (p4d_none(p4d) || p4d_bad(p4d)) > > + goto out; > > + > > + pudp = pud_offset(p4dp, addr); > > + pud = READ_ONCE(*pudp); > > + pr_cont(", pud=%016lx", pud_val(pud)); > > + if (pud_none(pud) || pud_bad(pud)) > > + goto out; > > + > > + pmdp = pmd_offset(pudp, addr); > > + pmd = READ_ONCE(*pmdp); > > + pr_cont(", pmd=%016lx", pmd_val(pmd)); > > + if (pmd_none(pmd) || pmd_bad(pmd)) > > + goto out; > > + > > + ptep = pte_offset_map(pmdp, addr); > > + if (!ptep) > > + goto out; > > + > > + pte = READ_ONCE(*ptep); > > All the READ_ONCE() can be replaced by pXXp_get() macros defined here: > https://elixir.bootlin.com/linux/v6.10/source/include/linux/pgtable.h#L315 > > And we should stop as soon as we encounter a leaf entry using pXd_leaf(). > Okay, I'll update it in v2. > Thanks, > > Alex > > > + pr_cont(", pte=%016lx", pte_val(pte)); > > + pte_unmap(ptep); > > +out: > > + pr_cont("\n"); > > +} > > + > > static void die_kernel_fault(const char *msg, unsigned long addr, > > struct pt_regs *regs) > > { > > @@ -31,6 +77,7 @@ static void die_kernel_fault(const char *msg, unsigned long addr, > > addr); > > > > bust_spinlocks(0); > > + show_pte(addr); > > die(regs, "Oops"); > > make_task_dead(SIGKILL); > > } > > -- > > 2.39.2 > > > > Otherwise, that's a good idea to print the page table content on fault so: > > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > Thanks, > > Alex Thanks, Yunhui
diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c index 90d4ba36d1d0..dfe3ce38e16b 100644 --- a/arch/riscv/mm/fault.c +++ b/arch/riscv/mm/fault.c @@ -22,6 +22,52 @@ #include "../kernel/head.h" +static void show_pte(unsigned long addr) +{ + pgd_t *pgdp, pgd; + p4d_t *p4dp, p4d; + pud_t *pudp, pud; + pmd_t *pmdp, pmd; + pte_t *ptep, pte; + struct mm_struct *mm = current->mm; + + if (!mm) + mm = &init_mm; + pgdp = pgd_offset(mm, addr); + pgd = READ_ONCE(*pgdp); + pr_alert("[%016lx] pgd=%016lx", addr, pgd_val(pgd)); + if (pgd_none(pgd) || pgd_bad(pgd)) + goto out; + + p4dp = p4d_offset(pgdp, addr); + p4d = READ_ONCE(*p4dp); + pr_cont(", p4d=%016lx", p4d_val(p4d)); + if (p4d_none(p4d) || p4d_bad(p4d)) + goto out; + + pudp = pud_offset(p4dp, addr); + pud = READ_ONCE(*pudp); + pr_cont(", pud=%016lx", pud_val(pud)); + if (pud_none(pud) || pud_bad(pud)) + goto out; + + pmdp = pmd_offset(pudp, addr); + pmd = READ_ONCE(*pmdp); + pr_cont(", pmd=%016lx", pmd_val(pmd)); + if (pmd_none(pmd) || pmd_bad(pmd)) + goto out; + + ptep = pte_offset_map(pmdp, addr); + if (!ptep) + goto out; + + pte = READ_ONCE(*ptep); + pr_cont(", pte=%016lx", pte_val(pte)); + pte_unmap(ptep); +out: + pr_cont("\n"); +} + static void die_kernel_fault(const char *msg, unsigned long addr, struct pt_regs *regs) { @@ -31,6 +77,7 @@ static void die_kernel_fault(const char *msg, unsigned long addr, addr); bust_spinlocks(0); + show_pte(addr); die(regs, "Oops"); make_task_dead(SIGKILL); }
When the kernel displays "Unable to handle kernel paging request at virtual address", we would like to confirm the status of the virtual address in the page table. So add show_pte() before die(). Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> --- arch/riscv/mm/fault.c | 47 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)