On 2023/05/08 21:07, Rongwei Wang wrote:
>
> On 2023/5/8 15:46, HAGIO KAZUHITO(萩尾 一仁) wrote:
>> 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 reply.
>
> I will continue to finish this patch for other arches. ;)
>
>>>
>>> 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@xxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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?
> Good idea, it looks better than arm64_init(). Next version will fix here.
>>
>> And zero_paddr might be better than zero_pfn to reduce the calculation
>> on printing, e.g. vt->zero_paddr = zero_pfn << PAGESHIFT().
> OK, next version will fix here.
>>
>>>> +
>>>> 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.
>
> Actually, I'm not sure this 'zero_paddr' has possibility of equal to 0
> (seems unlikely).
>
> Anyway, I will try to make sure that.
Thanks!
If 0 cannot be used, ~0 might be good. And you can also use macros if
possible. For example,
#define IS_ZEROPAGE(paddr) (vt->zero_paddr != ~0UL && ...
or something. Just an idea, not tried.
One more thing, it's hard for us to test all of arches that crash
supports, so it would be ok to implement it first for arches that you
can test. For the other arches, each arch user can reuse the patch if
they want it.
Thanks,
Kazu
>
>>
>> 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)" : "");
>
> Nice, I'm also think my above style is verbose, but have no idea at that
> time.
>
> I will update here at next version.
>
>>
>> Thanks,
>> Kazu
>
> Thanks for your time.
>
> -wrw
>
>>>> 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
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
