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

[Suzuki K. Poulose]

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 :(.

>
> 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.

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.

Thanks
Suzuki