Hello Michael, >Hello Atsushi, > >On Thu, 23 Oct 2014 06:56:47 +0000 >Atsushi Kumagai <kumagai-atsushi at mxc.nes.nec.co.jp> wrote: > >> >On Tue, 14 Oct 2014 07:19:13 +0000 >> >Atsushi Kumagai <kumagai-atsushi at mxc.nes.nec.co.jp> wrote: > >[snip] > >> I noticed that is_vmalloc_addr_x86_64() can't be used as is_phys_addr() >> due to the "kaslr issue". I fixed it in the common path as below, but >> --mem-usage still has the same issue since initial() will be invoked >> after get_kcore_dump_loads(). >> >> http://lists.infradead.org/pipermail/kexec/2014-October/012743.html >> >> I know it's so late, but I began to think the current implementation >> that invokes vaddr_to_paddr_XXX() before initial() is bad: >> >> show_mem_usage() >> + get_kcore_dump_loads() >> + process_dump_load() >> + vaddr_to_paddr_XXX() >> + initial() >> ... >> vaddr_to_paddr_XXX() does VtoP translation *properly*, so it needs >> several symbols. This design is OK since it's a general method. > >So the current implementation that uses vaddr_to_paddr_xxx() >for --mem-usage *before* initial() is broken? Or does it currently >works only by chance? Luckily v1.5.7 works if kaslr is disabled, but it can be broken easily in the future due to dependence on other symbols. vaddr_to_paddr_xxx() should be called after initial() in principle. >> OTOH, we need a kludge which can be used under any situations and >> should use it for --mem-usage: >> >> VtoP_kludge_s390x(unsigned long vaddr) >> { >> /* s390 has 1:1 VtoP mapping that starts with zero. */ >> return vaddr; >> } >> >> also x86_64's can be implemented like below: >> >> VtoP_kludge_x86_64(unsigned long vaddr) >> { >> /* if the address is direct mapped, it's easy to translate. */ >> unsigned long paddr = vaddr - PAGE_OFFSET; >> return paddr; >> } > >What about using the kernel name __pa(vaddr)? Sounds good, but some symbols are still necessary for __pa() in some architectures according to the kernel, it seems difficult to introduce __pa() as a general method. So now I think is_iomem_phys_addr() isn't suitable for other architectures since it depends on __pa(). Anyway, s390x has the simple __pa(), let's continue to use is_iomem_phys_addr() even though it's only for s390x. >[snip] > >> >--- a/elf_info.c >> >+++ b/elf_info.c >> >@@ -854,7 +854,7 @@ int get_kcore_dump_loads(void) >> > >> > for (i = 0; i < num_pt_loads; ++i) { >> > struct pt_load_segment *p = &pt_loads[i]; >> >- if (is_vmalloc_addr(p->virt_start)) >> >+ if (!is_phys_addr(p->virt_start)) >> >> This part implicitly does VtoP translation. >> Actually it works for s390x but it's not suitable as a general code, >> so we should use VtoP_kludge(should be better name!) too. >> Then we can use is_iomem_phys_addr() also for other architecture. > >So how exactly should the code look like for non s390x architectures? > >Michael By early vmcoreinfo initialization, x86_64 will be possible to use is_vmalloc_addr_x86_64() as is_phys_addr(), so your last patch has no problem except missing __pa() in is_iomem_phys_addr(). Now I don't plan to use is_iomem_phys_addr() for non s390x architectures, I prefer putting is_iomem_phys_addr() into arch/s390x.c as Baoquan commented before. Could you modify the patch ? or any questions ? Thanks, Atsushi Kumagai
