On Fri, Sep 22, 2023 at 5:26 PM Aditya Gupta <adityag@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxx> wrote:
> > > >
> > > >> Hi, Aditya
> > > >> Thank you for the patch.
> > > >>
> > > >> On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request@xxxxxxxxxx>
> > 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
> > > >
> >
> >
> >
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
