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

On Fri, Sep 22, 2023 at 02:13:31PM +0800, lijiang wrote: > On Thu, Sep 21, 2023 at 7:26 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: > > > Hi Lianbo, > > Resending this, since skipped your mail from "To:" field by mistake. > > > > > No worries. I can also get your reply from the mail list. > > > > On Thu, Sep 21, 2023 at 05:46:32PM +0800, lijiang wrote: > > > On Thu, Sep 21, 2023 at 3:59 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; > > wrote: > > > > > > > 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 > > > wrote: > > > >> > > > >> > > > >> ... > > > >> > > > >>> 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. > > > > > > > > > > > It's true. > > > > > > For the current patch, if the symbol 'vmemmap_list' is not found, the > > > symbol_value() will invoke the error(FATAL, ...) and ultimately exit from > > > this function. It doesn't have any chance to execute the initialising > > code. > > > > > > > Got it, this would be an issue when 'vmemmap_list' is missing. > > Made a mistake here, Thanks, will add an if around the readmem, that should > > solve it, like this ? > > > > if (kernel_symbol_exists("vmemmap_list")) > > readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start, > > sizeof(void *), > > "vmemmap_list", RETURN_ON_ERROR | QUIET); > > > > Will fix this in V2. > > > > > Ok, Thanks. For the ppc64_vmemmap_init() changes, does this work for you? > > diff --git a/ppc64.c b/ppc64.c > index fc34006..3e155a0 100644 > --- a/ppc64.c > +++ b/ppc64.c > @@ -1280,10 +1280,34 @@ 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; > - > - if (!(kernel_symbol_exists("vmemmap_list")) || > - !(kernel_symbol_exists("mmu_psize_defs")) || > + struct machine_specific *ms = machdep->machspec; > + > + if (!kernel_symbol_exists("vmemmap_list")) > + return; Returning here is an issue, since 'machdep->flags |= VMEMMAP_AWARE' won't be run, and since 'ppc64_vmemmap_to_phys' checks for the flag before calling 'ppc64_vtop_level4' for page traversal.

So page traversal will not happen if the flag is not set. Ideally I will want the if (!ld->start) codeblock to run if vmemmap_list symbol is missing or the list is empty

diff --git a/ppc64.c b/ppc64.c index fc34006f4863..cd98a679c45e 100644 --- a/ppc64.c +++ b/ppc64.c @@ -1280,10 +1280,32 @@ 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; - - if (!(kernel_symbol_exists("vmemmap_list")) || - !(kernel_symbol_exists("mmu_psize_defs")) || + 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. + */ + if (kernel_symbol_exists("vmemmap_list")) + readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start, + sizeof(void *), "vmemmap_list", RETURN_ON_ERROR|QUIET); Here I add a check for 'kernel_symbol_exists' before 'symbol_value("vmemmap_list") is done + if (!ld->start) { + /* + * vmemmap_list is set to 0 or missing. Do kernel pagetable walk + * for vmemmap address translation. + */ + ms->vmemmap_list = NULL; + ms->vmemmap_cnt = 0; + + machdep->flags |= VMEMMAP_AWARE; + return; + } + + if (!(kernel_symbol_exists("mmu_psize_defs")) || !(kernel_symbol_exists("mmu_vmemmap_psize")) || ... Here I removed the kernel_symbol_exists("vmemmap_list") > + > + 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|QUIET); > + 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; > + } > + > + if (!(kernel_symbol_exists("mmu_psize_defs")) || > !(kernel_symbol_exists("mmu_vmemmap_psize")) || > !STRUCT_EXISTS("vmemmap_backing") || > !STRUCT_EXISTS("mmu_psize_def") || > @@ -1293,8 +1317,6 @@ void ppc64_vmemmap_init(void) > !MEMBER_EXISTS("vmemmap_backing", "list")) > return; > Yup, this is good, I will remove the kernel_symbol_exists check for vmemmap_list from if in next version, since it is handled earlier OK. Looks good to me.

Thanks, Aditya Gupta > > BTW: I just got a system with the radix-mmu, and reproduced this issue. > # dmesg|grep mmu > [ 0.000000] dt-cpu-ftrs: final cpu/mmu features = 0x0001b8eb8f5fb187 > 0x3c007041 > [ 0.000000] radix-mmu: Page sizes from device-tree: > [ 0.000000] radix-mmu: Page size shift = 12 AP=0x0 > [ 0.000000] radix-mmu: Page size shift = 16 AP=0x5 > [ 0.000000] radix-mmu: Page size shift = 21 AP=0x1 > [ 0.000000] radix-mmu: Page size shift = 30 AP=0x2 > [ 0.000000] radix-mmu: Mapped 0x0000000000000000-0x0000000002800000 with > 2.00 MiB pages (exec) > [ 0.000000] radix-mmu: Mapped 0x0000000002800000-0x0000000040000000 with > 2.00 MiB pages > ... > > Thanks > Lianbo > > > And, the initialization code relies on the result of ld->start, once the > > > readmem() fails due to some reasons, it may also enter the > > > initialization code. I'm not sure if that is expected behavior(if yes, > > the > > > "RETURN_ON_ERROR|QUIET " would be good). > > > > Okay, even if readmem fails, we still go ahead and since ld was all zeros, > > 'if (!ld->start)' should remain true, for both cases when vmemmap_list is > > empty > > (head is NULL, hence ld->start=0), vmemmap_list symbol is not found or > > readmem > > fails (ld->start=0 since ld was all zeros due to BZERO). > > > > Will add "RETURN_ON_ERROR|QUIET" also in V2. > > > > > > > > If so, the kernel_symbol_exists("vmemmap_list") may be redundant. That > > can > > > be removed this time. What do you think? > > > > > > > True. Since the case of 'vmemmap_list' being empty, or symbol not being > > there, > > both get handled before this, that can be removed. > > Will do it in V2. > > > > Thanks, > > - Aditya Gupta > > > > > > The readmem used to be like this, which returns from function if > > readmem > > > > 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. > > > > > > > > > > > Ok, got it, thanks for the information. > > > > > > > > > > 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 ? > > > > > > > > > > > No, it's not a coding guidelines requirement. :-) > > > > > > > > > > > And other changes are fine to me. > > > > > > > > Thank you for your reviews Lianbo :) > > > > > > > > > > > No problem. > > > > > > Thanks. > > > Lianbo > > > > > > > > > > Thanks, > > > > - Aditya Gupta > > > > > > > > > >