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

[Dave Anderson]

----- Original Message -----
> On 02/17/2012 12:43 AM, Dave Anderson wrote:
> >
> >
> > ----- 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?
> 
> Yep. One more question. Why don't we use  phys_to_page() in the dump_mem_mapXX() to
> get the page struct and then do the rest of the work ?

It probably could.  It's just that when the function is used to display
a single virtual or physical address is passed to the function, it's 
pretty much riding on the back of the function's original design which
reads and caches huge chunks of the mem_map, and then walks through
them.

Dave