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

On 2023/05/12 0:32, Rongwei Wang wrote: >> btw, how can I create this situation seeing zero page easily? >> I also would like to test the patch. > Here a testcase that I used to allocate zero page: Thanks! I could see it on x86_64. crash> vtop -c 638970 7f018fb11000 VIRTUAL PHYSICAL 7f018fb11000 3af648000 PGD: 4646ca7f0 => 5b1ba7067 PUD: 5b1ba7030 => 5f9e63067 PMD: 5f9e633e8 => b45b7d067 PTE: b45b7d888 => 80000003af648225 PAGE: 3af648000 (ZERO PAGE) > z.c: > > #include <string.h> > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > #include <errno.h> > #include <ctype.h> > > unsigned long *zero_data; > > void read_only(void) > { >         unsigned long i, val, count = 0; > >         printf("hello\n"); >         while (count++ < 10) { >                 for (i=0; i<(4 * 1024 * 1024) / sizeof(unsigned long); i+=1024) { >                         val = zero_data[i]; >                 } >         } > >         return; > } > > int main(int argc, char *argv[]) > { >         char c; > >         zero_data = (unsigned long *)malloc(1024 * 1024 * 4); >         if (!zero_data) { >                 printf("malloc fail: exit\n"); >                 return -1; >         } >         printf("zero: %lx\n", zero_data); > >         read_only(); > >         c = getchar(); > >         free(zero_data); >         exit(EXIT_SUCCESS); > } > > When you test in arm64 with 64k base pagesize, the size of allocating memory is needed to set more than 512MB. I see. >>>>         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 >> a bit verbose, please replace these with "..." > Your mean comment as below is OK? Yes. > 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) > > ... > >>>> +        if (readmem(symbol_value("huge_zero_pfn"), KVADDR, >>>> +                &huge_zero_pfn, sizeof(huge_zero_pfn), >>>> +                "read huge_zero_pfn", QUIET|RETURN_ON_ERROR)) >>>> +            vt->huge_zero_paddr = huge_zero_pfn << PAGESHIFT(); >> In kernel, huge_zero_pfn is initialized as ~0UL and set to a pfn when >> get_huge_zero_page() is called. >> >>     crash> p -x huge_zero_pfn >>     huge_zero_pfn = $4 = 0xffffffffffffffff >> >> so if huge_zero_pfn is ~0UL, vt->huge_zero_paddr should not be set. > Good catch. > > How about following modification: > > +        if (readmem(symbol_value("huge_zero_pfn"), KVADDR, > +                &huge_zero_pfn, sizeof(huge_zero_pfn), > +                "read huge_zero_pfn", QUIET|RETURN_ON_ERROR)) > +            vt->huge_zero_paddr = (huge_zero_pfn != ~0UL) ? (huge_zero_pfn << PAGESHIFT()); hmm, this looks wrong, needs ':'?

I would prefer this simply: if (readmem(symbol_value("huge_zero_pfn"), KVADDR, &huge_zero_pfn, sizeof(huge_zero_pfn), - "read huge_zero_pfn", QUIET|RETURN_ON_ERROR)) + "read huge_zero_pfn", QUIET|RETURN_ON_ERROR) && + huge_zero_pfn != ~0UL) vt->huge_zero_paddr = huge_zero_pfn << PAGESHIFT();

>>>> +    } >>>> + >>>>        if (vt->flags & PERCPU_KMALLOC_V1) >>>>                    vt->dump_kmem_cache = dump_kmem_cache_percpu_v1; >>>>        else if (vt->flags & PERCPU_KMALLOC_V2) >>>> diff --git a/x86_64.c b/x86_64.c >>>> index 5019c69..693a08b 100644 >>>> --- a/x86_64.c >>>> +++ b/x86_64.c >>>> @@ -2114,8 +2114,9 @@ x86_64_uvtop_level4(struct task_context *tc, ulong uvaddr, physaddr_t *paddr, in >>>>            goto no_upage; >>>>            if (pmd_pte & _PAGE_PSE) { >>>>            if (verbose) { >>>> -                        fprintf(fp, "  PAGE: %lx  (2MB)\n\n", >>>> -                PAGEBASE(pmd_pte) & PHYSICAL_PAGE_MASK); >>>> +            fprintf(fp, "  PAGE: %lx  (2MB%s)\n\n", >>>> +                PAGEBASE(pmd_pte) & PHYSICAL_PAGE_MASK, >>>> +                IS_ZEROPAGE(PAGEBASE(pmd_pte) & PHYSICAL_PAGE_MASK) ? ", ZERO PAGE" : ""); >>>>                            x86_64_translate_pte(pmd_pte, 0, 0); >>>>                    } >>>> @@ -2143,8 +2144,8 @@ x86_64_uvtop_level4(struct task_context *tc, ulong uvaddr, physaddr_t *paddr, in >>>>        *paddr = (PAGEBASE(pte) & PHYSICAL_PAGE_MASK) + PAGEOFFSET(uvaddr); >>>>        if (verbose) { >>>> -        fprintf(fp, "  PAGE: %lx\n\n", >>>> -            PAGEBASE(*paddr) & PHYSICAL_PAGE_MASK); >>>> +        fprintf(fp, "  PAGE: %lx  %s\n\n", PAGEBASE(*paddr) & PHYSICAL_PAGE_MASK, >>>> +            IS_ZEROPAGE(PAGEBASE(*paddr) & PHYSICAL_PAGE_MASK) ? "(ZERO PAGE)" : ""); >>>>            x86_64_translate_pte(pte, 0, 0); >>>>        } >> why only for uvtop_level4? > That because I have no idea about xyz_xen_wpt and similar target. In addition, I have no environment to test that, so... > > How about doing this when someone need it in these environment? Or I can do it if you think just complete it directly is ok. Ah ok, I see. Your patch is good enough for now. Thanks, Kazu