Re: [Crash-utility] [PATCH] ppc64: do page traversal if vmemmap_list not populated

On 21/09/23 11:21, lijiang wrote: On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat. com> <lijiang(a)%E2%80%8Aredhat.%E2%80%8Acom&gt; wrote: Hi, Aditya Thank you for the patch. On Mon, Sep 11, 2023 at 8: 00 PM <crash-utility-request@ redhat. com> <crash-utility-request(a)%E2%80%8Aredhat.%E2%80%8Acom&gt; wrote: From: Aditya Gupta <adityag@ linux. ibm. com> <adityag(a)%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom&gt; To: On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com&gt; wrote: > Hi, Aditya > Thank you for the patch. > > On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com&gt; wrote: > >> From: Aditya Gupta <adityag(a)linux.ibm.com&gt; >> To: crash-utility(a)redhat.com >> Cc: Hari Bathini <hbathini(a)linux.ibm.com&gt;, Mahesh J Salgaonkar >> <mahesh(a)linux.ibm.com&gt;, Sourabh Jain <sourabhjain(a)linux.ibm.com >> >, >> "Aneesh Kumar K.V" <aneesh.kumar(a)linux.ibm.com&gt; >> Subject: [Crash-utility] [PATCH] ppc64: do page traversal if >> vmemmap_list not populated >> Message-ID: <20230911093032.419388-1-adityag(a)linux.ibm.com&gt; >> Content-Type: text/plain; charset="US-ASCII"; x-default=true >> >> Currently 'crash-tool' fails on vmcore collected on upstream kernel on >> PowerPC64 with the error: >> >> crash: invalid kernel virtual address: 0 type: "first list entry >> >> Presently the address translation for vmemmap addresses is done using >> the vmemmap_list. But with the below commit in Linux, vmemmap_list can >> be empty, in case of Radix MMU on PowerPC64 >> >> 368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a >> different vmemmap handling function) >> >> In case vmemmap_list is empty, then it's head is NULL, which crash tries >> to access and fails due to accessing NULL. >> >> Instead of depending on 'vmemmap_list' for address translation for >> vmemmap addresses, do a kernel pagetable walk to get the physical >> address associated with given virtual address >> >> Reviewed-by: Hari Bathini <hbathini(a)linux.ibm.com&gt; >> Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com&gt; >> >> --- >> >> Testing >> ======= >> >> Git tree with patch applied: >> https://github.com/adi-g15-ibm/crash/tree/bugzilla-203296-list-v1 >> >> This can be tested with '/proc/vmcore' as the vmcore, since makedumpfile > > also fails in absence of 'vmemmap_list' in upstream linux >> >> The fix for makedumpfile will also been sent to upstream >> >> --- >> --- >> ppc64.c | 47 +++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 33 insertions(+), 14 deletions(-) >> >> diff --git a/ppc64.c b/ppc64.c >> index fc34006f4863..1e84c5f56773 100644 >> --- a/ppc64.c >> +++ b/ppc64.c >> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void) >> long backing_size, virt_addr_offset, phys_offset, list_offset; >> ulong *vmemmap_list; >> char *vmemmap_buf; >> - struct machine_specific *ms; >> - >> + struct machine_specific *ms = machdep->machspec; >> + >> + ld = &list_data; >> + BZERO(ld, sizeof(struct list_data)); >> + >> + /* >> + * vmemmap_list is missing or set to 0 in the kernel would imply >> + * vmemmap region is mapped in the kernel pagetable. So, read >> vmemmap_list >> + * anyway and use the translation method accordingly. >> + */ >> + readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start, >> sizeof(void *), >> + "vmemmap_list", RETURN_ON_ERROR); >> + if (!ld->start) { >> + /* >> + * vmemmap_list is set to 0 or missing. Do kernel >> pagetable walk >> + * for vmemmamp address translation. >> + */ >> + ms->vmemmap_list = NULL; >> + ms->vmemmap_cnt = 0; >> + >> + machdep->flags |= VMEMMAP_AWARE; >> + return; >> + } >> + >> > I would suggest putting the 'readmem(symbol_value("vmemmap_list"),...)' after the 'kernel_symbol_exists("vmemmap_list")', and also check the returned value of readmem(). Yes, I considered that, but the if condition checking for 'kernel_symbol_exists("vmemmap_list")' returns from the function if the symbol is not found, same with the readmem return value check earlier, we wanted to initialise the ms->vmemmap_list and machdep->flags before that.

failed: - if (!readmem(symbol_value("vmemmap_list"), - KVADDR, &ld->start, sizeof(void *), "vmemmap_list", - RETURN_ON_ERROR)) - return; So, intentionally I put it above the if and removed the return value check, since we don't want to depend on vmemmap_list symbol being there for newer kernels anymore. And to be future proof in case the address mapping moves to page table for Hash MMU case also, and since vmemmap_list is PowerPC specific, the symbol might also be removed.

But, if it's a coding guidelines requirement to handle the return values, I can think of how to handle the return value of readmem, any suggestions ?

And other changes are fine to me. Thank you for your reviews Lianbo :)

Thanks, - Aditya Gupta