Re: [Crash-utility] [PATCH RFC] arm64: show zero pfn information when using vtop

On 2023/05/01 1:16, Rongwei Wang wrote: > Hello, > > Recently I handle some stuff about zero page with crash. I'm always use > vtop to check > > whether an virtual address has been set the zero page. But, it can not > directly show whether a page > > is zero page or not. So I try to add this patch to support this. > > And I'm not sure this idea is nice to be accepted, so just realize it on > arm64 platform. I can finish others > > arch if it accepted. Sorry for the delay, I had been on holidays last week. I think this is a nice suggestion and would like to accept it also for other architectures.

> > Thanks for your time. > > On 2023/5/1 00:02, Rongwei Wang wrote: >> Now vtop can not show us the page is zero pfn >> when PTE or PMD has attached ZERO PAGE. This >> patch supports show this information directly >> when using vtop, likes: >> >> crash> vtop -c 13674 ffff8917e000 >> VIRTUAL     PHYSICAL >> ffff8917e000  836e71000 >> >> PAGE DIRECTORY: ffff000802f8d000 >>     PGD: ffff000802f8dff8 => 884e29003 >>     PUD: ffff000844e29ff0 => 884e93003 >>     PMD: ffff000844e93240 => 840413003 >>     PTE: ffff000800413bf0 => 160000836e71fc3 >>    PAGE: 836e71000  (ZERO PAGE) >> >>        PTE        PHYSICAL   FLAGS >> 160000836e71fc3  836e71000 >> (VALID|USER|RDONLY|SHARED|AF|NG|PXN|UXN|SPECIAL) >> >>        VMA           START       END     FLAGS FILE >> ffff000844f51860 ffff8917c000 ffff8957d000 100073 >> >>        PAGE         PHYSICAL      MAPPING       INDEX CNT FLAGS >> fffffe001fbb9c40  836e71000                0        0  1 >> 2ffffc000001000 reserved >> >> If huge page found: >> >> crash> vtop -c 14538 ffff95800000 >> VIRTUAL     PHYSICAL >> ffff95800000  910c00000 >> >> PAGE DIRECTORY: ffff000801fa0000 >>     PGD: ffff000801fa0ff8 => 884f53003 >>     PUD: ffff000844f53ff0 => 8426cb003 >>     PMD: ffff0008026cb560 => 60000910c00fc1 >>    PAGE: 910c00000  (2MB, ZERO PAGE) >> >>       PTE        PHYSICAL   FLAGS >> 60000910c00fc1  910c00000  (VALID|USER|RDONLY|SHARED|AF|NG|PXN|UXN) >> >>        VMA           START       END     FLAGS FILE >> ffff0000caa711e0 ffff956a9000 ffff95aaa000 100073 >> >>        PAGE         PHYSICAL      MAPPING       INDEX CNT FLAGS >> fffffe0023230000  910c00000                0        0  1 >> 6ffffc000010000 head >> >> That seems be sensible with this patch. >> >> Signed-off-by: Rongwei Wang <rongwei.wang(a)linux.alibaba.com&gt; >> --- >>   arm64.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++------ >>   defs.h  |  5 ++++ >>   2 files changed, 88 insertions(+), 9 deletions(-) >> >> diff --git a/arm64.c b/arm64.c >> index 56fb841..264572d 100644 >> --- a/arm64.c >> +++ b/arm64.c >> @@ -419,7 +419,25 @@ arm64_init(int when) >>           /* use machdep parameters */ >>           arm64_calc_phys_offset(); >>           arm64_calc_physvirt_offset(); >> - >> + >> +        if (kernel_symbol_exists("zero_pfn")) { >> +                ulong zero_pfn = 0; >> + >> +                if (readmem(symbol_value("zero_pfn"), KVADDR, >> +                          &zero_pfn, sizeof(zero_pfn), >> +                          "read zero_pfn", QUIET|RETURN_ON_ERROR)) >> +                    machdep->zero_pfn = zero_pfn; >> +        } >> + >> +        if (kernel_symbol_exists("huge_zero_pfn")) { >> +                ulong huge_zero_pfn = 0; >> + >> +                if (readmem(symbol_value("huge_zero_pfn"), KVADDR, >> +                          &huge_zero_pfn, sizeof(huge_zero_pfn), >> +                          "read huge_zero_pfn", QUIET|RETURN_ON_ERROR)) >> +                    machdep->huge_zero_pfn = huge_zero_pfn; >> +        } so, for other architectures, are you going to move these to vm_init() or somewhere?

And zero_paddr might be better than zero_pfn to reduce the calculation on printing, e.g. vt->zero_paddr = zero_pfn << PAGESHIFT().

>> + >>           if (CRASHDEBUG(1)) { >>               if (machdep->flags & NEW_VMEMMAP) >>                   fprintf(fp, "kimage_voffset: %lx\n", >> @@ -1787,7 +1805,14 @@ arm64_vtop_2level_64k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if ((pgd_val & PMD_TYPE_MASK) == PMD_TYPE_SECT) { >>           ulong sectionbase = (pgd_val & SECTION_PAGE_MASK_512MB) & >> PHYS_MASK; >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx  (512MB)\n\n", sectionbase); >> +            if (kernel_symbol_exists("huge_zero_pfn")) { >> +                if (sectionbase == (HUGE_ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (512MB, ZERO PAGE)\n\n", >> +                        HUGE_ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx  (512MB)\n\n", >> sectionbase); >> +            } else >> +                fprintf(fp, "  PAGE: %lx  (512MB)\n\n", sectionbase); >>               arm64_translate_pte(pgd_val, 0, 0); >>           } >>           *paddr = sectionbase + (vaddr & ~SECTION_PAGE_MASK_512MB); >> @@ -1806,7 +1831,14 @@ arm64_vtop_2level_64k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if (pte_val & PTE_VALID) { >>           *paddr = (PAGEBASE(pte_val) & PHYS_MASK) + PAGEOFFSET(vaddr); >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            if (kernel_symbol_exists("zero_pfn")) { >> +                if (PAGEBASE(*paddr) == (ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (ZERO PAGE)\n\n", >> +                        ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            } else >> +                fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); These blocks look a bit verbose and it would be better to not call kernel_symbol_exists() each time, if possible.

Can it be changed to something like this? if (vt->zero_paddr && PAGEBASE(*paddr) == vt->zero_paddr) { or this. fprintf(fp, " PAGE: %lx %s\n\n", PAGEBASE(*paddr), vt->zero_paddr && PAGEBASE(*paddr) == vt->zero_paddr ? "(ZERO PAGE)" : "");

Thanks, Kazu

>>               arm64_translate_pte(pte_val, 0, 0); >>           } >>       } else { >> @@ -1859,7 +1891,14 @@ arm64_vtop_3level_64k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if ((pmd_val & PMD_TYPE_MASK) == PMD_TYPE_SECT) { >>           ulong sectionbase = PTE_TO_PHYS(pmd_val) & >> SECTION_PAGE_MASK_512MB; >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx  (512MB)\n\n", sectionbase); >> +            if (kernel_symbol_exists("huge_zero_pfn")) { >> +                if (sectionbase == (HUGE_ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (512MB, ZERO PAGE)\n\n", >> +                        HUGE_ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx  (512MB)\n\n", >> sectionbase); >> +            } else >> +                fprintf(fp, "  PAGE: %lx  (512MB)\n\n", sectionbase); >>               arm64_translate_pte(pmd_val, 0, 0); >>           } >>           *paddr = sectionbase + (vaddr & ~SECTION_PAGE_MASK_512MB); >> @@ -1878,7 +1917,14 @@ arm64_vtop_3level_64k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if (pte_val & PTE_VALID) { >>           *paddr = PTE_TO_PHYS(pte_val) + PAGEOFFSET(vaddr); >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            if (kernel_symbol_exists("zero_pfn")) { >> +                if (PAGEBASE(*paddr) == (ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (ZERO PAGE)\n\n", >> +                        ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            } else >> +                fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >>               arm64_translate_pte(pte_val, 0, 0); >>           } >>       } else { >> @@ -1940,7 +1986,14 @@ arm64_vtop_3level_4k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if ((pmd_val & PMD_TYPE_MASK) == PMD_TYPE_SECT) { >>           ulong sectionbase = (pmd_val & SECTION_PAGE_MASK_2MB) & >> PHYS_MASK; >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx  (2MB)\n\n", sectionbase); >> +            if (kernel_symbol_exists("huge_zero_pfn")) { >> +                if (sectionbase == (HUGE_ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (2MB, ZERO PAGE)\n\n", >> +                        HUGE_ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx  (2MB)\n\n", sectionbase); >> +            } else >> +                fprintf(fp, "  PAGE: %lx  (2MB)\n\n", sectionbase); >>               arm64_translate_pte(pmd_val, 0, 0); >>           } >>           *paddr = sectionbase + (vaddr & ~SECTION_PAGE_MASK_2MB); >> @@ -1959,7 +2012,14 @@ arm64_vtop_3level_4k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if (pte_val & PTE_VALID) { >>           *paddr = (PAGEBASE(pte_val) & PHYS_MASK) + PAGEOFFSET(vaddr); >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            if (kernel_symbol_exists("zero_pfn")) { >> +                if (PAGEBASE(*paddr) == (ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (ZERO PAGE)\n\n", >> +                        ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            } else >> +                fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >>               arm64_translate_pte(pte_val, 0, 0); >>           } >>       } else { >> @@ -2029,7 +2089,14 @@ arm64_vtop_4level_4k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if ((pmd_val & PMD_TYPE_MASK) == PMD_TYPE_SECT) { >>           ulong sectionbase = (pmd_val & SECTION_PAGE_MASK_2MB) & >> PHYS_MASK; >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx  (2MB)\n\n", sectionbase); >> +            if (kernel_symbol_exists("huge_zero_pfn")) { >> +                if (sectionbase == (HUGE_ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (2MB, ZERO PAGE)\n\n", >> +                        HUGE_ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx  (2MB)\n\n", sectionbase); >> +            } else >> +                fprintf(fp, "  PAGE: %lx  (2MB)\n\n", sectionbase); >>               arm64_translate_pte(pmd_val, 0, 0); >>           } >>           *paddr = sectionbase + (vaddr & ~SECTION_PAGE_MASK_2MB); >> @@ -2048,7 +2115,14 @@ arm64_vtop_4level_4k(ulong pgd, ulong vaddr, >> physaddr_t *paddr, int verbose) >>       if (pte_val & PTE_VALID) { >>           *paddr = (PAGEBASE(pte_val) & PHYS_MASK) + PAGEOFFSET(vaddr); >>           if (verbose) { >> -            fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            if (kernel_symbol_exists("zero_pfn")) { >> +                if (PAGEBASE(*paddr) == (ZEROPFN() << PAGESHIFT())) >> +                    fprintf(fp, "  PAGE: %lx  (ZERO PAGE)\n\n", >> +                        ZEROPFN() << PAGESHIFT()); >> +                else >> +                    fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >> +            } else >> +                fprintf(fp, "  PAGE: %lx\n\n", PAGEBASE(*paddr)); >>               arm64_translate_pte(pte_val, 0, 0); >>           } >>       } else { >> diff --git a/defs.h b/defs.h >> index 12ad6aa..4ed2d0a 100644 >> --- a/defs.h >> +++ b/defs.h >> @@ -1071,6 +1071,8 @@ struct machdep_table { >>           void (*show_interrupts)(int, ulong *); >>       int (*is_page_ptr)(ulong, physaddr_t *); >>       int (*get_cpu_reg)(int, int, const char *, int, void *); >> +    ulong zero_pfn; >> +    ulong huge_zero_pfn; >>   }; >>   /* >> @@ -2999,6 +3001,9 @@ struct load_module { >>   #define VIRTPAGEBASE(X)  (((ulong)(X)) & (ulong)machdep->pagemask) >>   #define PHYSPAGEBASE(X)  (((physaddr_t)(X)) & >> (physaddr_t)machdep->pagemask) >> +#define ZEROPFN()    (machdep->zero_pfn) >> +#define HUGE_ZEROPFN()   (machdep->huge_zero_pfn) >> + >>   /* >>    * Sparse memory stuff >>    *  These must follow the definitions in the kernel mmzone.h