[Crash-utility] [PATCH v3 0/3] vmalloc translation support for PPC

[Dave Anderson]

----- Original Message -----
> On 02/16/2012 09:52 PM, Dave Anderson wrote:
> >
> >
> > ----- Original Message -----
> > ...
> >>> So just do the same thing -- no verbose expanation is required.
> >>
> >> There are two ways to fix this :
> >>
> >> 1) Fix dump_mem_map*() to print the header only when there is
> >> information to dump.
> >>
> >> --- a/memory.c
> >> +++ b/memory.c
> >> @@ -4637,13 +4637,6 @@ dump_mem_map_SPARSEMEM(struct meminfo *mi)
> >>                           continue;
> >>                   }
> >>
> >> -               if (print_hdr) {
> >> -                       if (!(pc->curcmd_flags&  HEADER_PRINTED))
> >> -                               fprintf(fp, "%s", hdr);
> >> -                       print_hdr = FALSE;
> >> -                       pc->curcmd_flags |= HEADER_PRINTED;
> >> -               }
> >> -
> >>                   pp = section_mem_map_addr(section);
> >>                   pp = sparse_decode_mem_map(pp, section_nr);
> >>                   phys = (physaddr_t) section_nr *
> >>                   PAGES_PER_SECTION()
> >>                   * PAGESIZE();
> >> @@ -4854,6 +4847,13 @@ dump_mem_map_SPARSEMEM(struct meminfo *mi)
> >>                           }
> >>
> >>                           if (bufferindex>  buffersize) {
> >> +                               if (print_hdr) {
> >> +                                       if (!(pc->curcmd_flags&
> >>  HEADER_PRINTED))
> >> +                                               fprintf(fp, "%s",
> >> hdr);
> >> +                                       print_hdr = FALSE;
> >> +                                       pc->curcmd_flags |=
> >> HEADER_PRINTED;
> >> +                               }
> >> +
> >>                                   fprintf(fp, "%s", outputbuffer);
> >>                                   bufferindex = 0;
> >>                           }
> >> @@ -4867,6 +4867,13 @@ dump_mem_map_SPARSEMEM(struct meminfo *mi)
> >>           }
> >>
> >>           if (bufferindex>  0) {
> >> +               if (print_hdr) {
> >> +                       if (!(pc->curcmd_flags&  HEADER_PRINTED))
> >> +                               fprintf(fp, "%s", hdr);
> >> +                       print_hdr = FALSE;
> >> +                       pc->curcmd_flags |= HEADER_PRINTED;
> >> +               }
> >> +
> >>                   fprintf(fp, "%s", outputbuffer);
> >>           }
> >>
> >> Similarly for the dump_mem_map().
> >>
> >> 2) Fix ppc_pgd_vtop() to return FALSE if the paddr>
> >>  machdep->memsize
> >>
> >> --- a/ppc.c
> >> +++ b/ppc.c
> >> @@ -438,6 +438,10 @@ ppc_pgd_vtop(ulong *pgd, ulong vaddr,
> >> physaddr_t
> >> *paddr, int verbose)
> >>
> >>           *paddr = PAGEBASE(pte) + PAGEOFFSET(vaddr);
> >>
> >> +       if (*paddr>  machdep->memsize)
> >> +       /* We don't have pages above System RAM */
> >> +               return FALSE;
> >> +
> >>           return TRUE;
> >>
> >>    no_page:
> >>
> >> I prefer the (1). What do you think ?
> >
> > Hi Suzuki,
> >
> > Hmmm -- with respect to (1), I suppose that would work, although
> > given that both x86 and x86_64 pass through dump_mem_map_SPARSEMEM()
> > without printing the header in a non-existent-page case, I don't
> > understand why ppc is different?
> Yep, I digged into that a little, but not deep enough to debug it with
> a dump. Nothing was evident from the code :(.

Right -- I tried debugging it from the x86 and x86_64 perspective,
and couldn't see why ppc would be different!  ;-)

Oh well...
 
> >
> > And I'm thinking that a more general solution might be to change
> > do_vtop() here, and not even bother calling the relevant
> > dump_mem_map()
> > function if there's no page struct associated with it:
> >
> > --- memory.c    10 Feb 2012 16:41:38 -0000      1.273
> > +++ memory.c    16 Feb 2012 14:18:03 -0000
> > @@ -2796,7 +2796,7 @@
> >   do_vtop(ulong vaddr, struct task_context *tc, ulong vtop_flags)
> >   {
> >          physaddr_t paddr;
> > -       ulong vma;
> > +       ulong vma, pageptr;
> >          int page_exists;
> >           struct meminfo meminfo;
> >           char buf1[BUFSIZE];
> > @@ -2930,7 +2930,7 @@
> >
> >          fprintf(fp, "\n");
> >
> > -       if (page_exists) {
> > +       if (page_exists&&  phys_to_page(paddr,&pageptr)) {
> >                  if ((pc->flags&  DEVMEM)&&  (paddr>=
> >                  VTOP(vt->high_memory)))
> >                          return;
> >                  BZERO(&meminfo, sizeof(struct meminfo));
> >
> > And w/respect to (2), wouldn't that just cause the generic kvtop()
> > to fail?  And if so, it kind of re-defines the meaning of kvtop(),
> > even though its current callers pretty much expect to receive
> > a legitimate physical memory address.  But if a virtual address
> > resolves to a PTE with any legitimate address in it, then kvtop()
> > should return whatever's there.
> 
> Yep, I agree.
> 
> >
> > But I'm still wondering what makes ppc behave differently in
> > dump_mem_map_SPARSEMEM()?
> >
> Btw, we don't have SPARSMEM on ppc44x, and end up in dump_mem_map().  I was
> patching both the functions to cover all the platforms.

OK, so do you agree that just patching do_vtop() makes more sense?

> 
> Also, I found out that we need to abstract away the definition of Page flags
> as well, since it differes for different platforms (except for the _PAGE_PRESENT).
> I will include the changes in the next version.

OK.

Thanks,
  Dave