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

[lijiang <lijiang@xxxxxxxxxx>]

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;
+
+ 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;
 
- ms = machdep->machspec;
-
  backing_size = STRUCT_SIZE("vmemmap_backing");
  virt_addr_offset = MEMBER_OFFSET("vmemmap_backing", "virt_addr");
  phys_offset = MEMBER_OFFSET("vmemmap_backing", "phys");
@@ -1313,12 +1335,6 @@ void ppc64_vmemmap_init(void)
 
  ms->vmemmap_psize = 1 << shift;
 
-        ld =  &list_data;
-        BZERO(ld, sizeof(struct list_data));
- if (!readmem(symbol_value("vmemmap_list"),
-    KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
-    RETURN_ON_ERROR))
- return;
         ld->end = symbol_value("vmemmap_list");
         ld->list_head_offset = list_offset;
 

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