Hi Mika,
Thank you for your patches.
Some comments are below.
From: Mika Westerberg <ext-mika.1.westerberg@xxxxxxxxx>
Subject: [PATCH 2/2] ARM: don't depend on vmalloc_start
Date: Thu, 4 Nov 2010 10:15:17 +0200
> In ARM, modules are placed in vmalloc'ed area right above user stack and the
> rest of the vmalloc area is somewhere after high_memory (this can be different
> on different platforms). Since this value is not stored in vmcoreinfo, we have
> no means to find out the correct value for vmalloc_start.
>
> So we assume that all V->P translations are within the kernel direct mapped
> memory and use translation tables only when '--vtop' option is passed.
>
> Signed-off-by: Mika Westerberg <ext-mika.1.westerberg at nokia.com>
> ---
> arm.c | 40 +++++++++++++++++++---------------------
> 1 files changed, 19 insertions(+), 21 deletions(-)
>
> diff --git a/arm.c b/arm.c
> index 3ea450a..6469f03 100644
> --- a/arm.c
> +++ b/arm.c
> @@ -86,6 +86,9 @@ get_machdep_info_arm(void)
> info->kernel_start = SYMBOL(_stext);
> info->section_size_bits = _SECTION_SIZE_BITS;
>
> + DEBUG_MSG("page_offset : %lx\n", info->page_offset);
> + DEBUG_MSG("kernel_start : %lx\n", info->kernel_start);
> +
> /*
> * For the compatibility, makedumpfile should run without the symbol
> * vmlist and the offset of vm_struct.addr if they are not necessary.
> @@ -95,31 +98,28 @@ get_machdep_info_arm(void)
> return TRUE;
> }
>
> + /*
> + * In ARM the VMALLOC_START can be redefined per platform and we have no
> + * means to read that from the vmcore (there is currently no vmcoreinfo
> + * which specifies that), so we are not going to use vmalloc_start for
> + * anything.
> + */
> if (!readmem(VADDR, SYMBOL(vmlist), &vmlist, sizeof(vmlist))) {
> ERRMSG("Can't get vmlist.\n");
> - return FALSE;
> + return TRUE;
> }
> if (!readmem(VADDR, vmlist + OFFSET(vm_struct.addr), &vmalloc_start,
> sizeof(vmalloc_start))) {
> ERRMSG("Can't get vmalloc_start.\n");
> - return FALSE;
> + return TRUE;
> }
I think this code can be removed.
Because this code is easy to misunderstand for users and
there are no means to check now.
>
> info->vmalloc_start = vmalloc_start;
> -
> - DEBUG_MSG("page_offset : %lx\n", info->page_offset);
> - DEBUG_MSG("kernel_start : %lx\n", info->kernel_start);
> DEBUG_MSG("vmalloc_start: %lx\n", vmalloc_start);
These codes too?
There is no need 'info->vmalloc_start' by removing is_vmalloc_addr_arm() in ARM now.
>
> return TRUE;
> }
>
> -static int
> -is_vmalloc_addr_arm(unsigned long vaddr)
> -{
> - return (info->vmalloc_start && vaddr >= info->vmalloc_start);
> -}
> -
> /*
> * vtop_arm() - translate arbitrary virtual address to physical
> * @vaddr: virtual address to translate
> @@ -184,17 +184,15 @@ vtop_arm(unsigned long vaddr)
> unsigned long long
> vaddr_to_paddr_arm(unsigned long vaddr)
> {
> - unsigned long long paddr = vaddr_to_paddr_general(vaddr);
> -
> - if (paddr != NOT_PADDR)
> - return paddr;
> -
> - if (is_vmalloc_addr_arm(vaddr))
> - paddr = vtop_arm(vaddr);
> - else
> - paddr = __pa(vaddr);
> + /*
> + * Only use translation tables when user has explicitly requested us to
> + * perform translation for a given address. Otherwise we assume that the
> + * translation is done within the kernel direct mapped region.
> + */
> + if (info->vaddr_for_vtop == vaddr)
> + return vtop_arm(vaddr);
>
> - return paddr;
> + return __pa(vaddr);
> }
Why do you use vtop tranlation only when '--vtop' option is passed?
I think if you don't need vtop translation, vtop_arm() can be removed
and vaddr_to_paddr_arm() can be like below.
===
unsigned long long
vaddr_to_paddr_arm(unsigned long vaddr)
{
unsigned long long paddr;
paddr = __pa(vaddr);
if (info->vaddr_for_vtop == vaddr)
MSG(" VADDR : %08lx => PADDR : %08lx\n", vaddr, paddr);
return paddr;
}
===
Thanks.
-- Masayuki Igawa
>
> #endif /* __arm__ */
> --
> 1.5.6.5
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pkcs7-signature
Size: 3578 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kexec/attachments/20101105/295bcfae/attachment.p7s>
