[PATCH] makedumpfile: Enable --mem-usage for s390x

[Atsushi Kumagai]

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