[PATCH] riscv/mm/fault: add show_pte() before die()

[PATCH] riscv/mm/fault: add show_pte() before die()

[Yunhui Cui]

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

Re: [PATCH] riscv/mm/fault: add show_pte() before die()

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv/mm/fault: add show_pte() before die()

[Alexandre Ghiti]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv/mm/fault: add show_pte() before die()

[yunhui cui]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [External] Re: [PATCH] riscv/mm/fault: add show_pte() before die()

[yunhui cui]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv