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

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&gt; > 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;

+ + 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;

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 > > > > > >