[Crash-utility] [PATCH] ppc64: do page traversal if vmemmap_list not populated
lijiang
lijiang at redhat.com
Fri Sep 22 10:22:15 UTC 2023
On Fri, Sep 22, 2023 at 5:26 PM Aditya Gupta <adityag at linux.ibm.com> wrote:
> On Fri, Sep 22, 2023 at 02:13:31PM +0800, lijiang wrote:
> > On Thu, Sep 21, 2023 at 7:26 PM Aditya Gupta <adityag at linux.ibm.com>
> 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 at linux.ibm.com>
> > > wrote:
> > > >
> > > > > On 21/09/23 11:21, lijiang wrote:
> > > > >
> > > > > On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat. com>
> > > > > <lijiang@%E2%80%8Aredhat.%E2%80%8Acom> 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@%E2%80%8Aredhat.%E2%80%8Acom>
> > > wrote:
> > > > > From: Aditya Gupta <adityag@ linux. ibm. com>
> > > > > <adityag@%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom> To:
> > > > > On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang at redhat.com>
> wrote:
> > > > >
> > > > >> Hi, Aditya
> > > > >> Thank you for the patch.
> > > > >>
> > > > >> On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request at 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.
>
Understood.
> 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
>
Now this is the point. I have no other issues, thank you for the
explanation, Aditya.
Currently I am planning this diff for V2:
>
> 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.
Lianbo
> 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
> > > > >
> > >
> > >
> > >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20230922/0c4d0956/attachment-0001.htm>
More information about the Crash-utility
mailing list