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

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 ?