| Message ID | 1369950320-22784-1-git-send-email-stepanm@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 30, 2013 at 02:45:20PM -0700, Stepan Moskovchenko wrote: > void __init early_init_dt_add_memory_arch(u64 base, u64 size) > { > +#ifndef CONFIG_ARM_LPAE > + if (base > ((phys_addr_t)~0)) { The #ifdef is probably not necessary here, simply checking that base/size can be represented in a phys_addr_t is enough. > + pr_crit("Ignoring memory at 0x%08llx due to lack of LPAE support\n", > + base); > + return; > + } > + > + if (size > ((phys_addr_t)~0)) > + size = ((phys_addr_t)~0); A similar printk as arm_add_memory for this one too? printk(KERN_CRIT "Truncating memory at 0x%08llx to fit in " "32-bit physical address space\n", (long long)start); Regards, Jason
On Thursday 30 May 2013 14:45:20 Stepan Moskovchenko wrote: > > void __init early_init_dt_add_memory_arch(u64 base, u64 size) > { > +#ifndef CONFIG_ARM_LPAE > + if (base > ((phys_addr_t)~0)) { > + pr_crit("Ignoring memory at 0x%08llx due to lack of LPAE support\n", > + base); > + return; > + } > + > + if (size > ((phys_addr_t)~0)) > + size = ((phys_addr_t)~0); > + > + /* arm_add_memory() already checks for the case of base + size > 4GB */ > +#endif > arm_add_memory(base, size); > } This looks wrong for the case where 'base' is between >0 and 4GB and 'size' makes it spill over the 4GB boundary. You need to set 'size = (phys_addr_t)~0 - base' then. Arnd
On 5/30/2013 3:24 PM, Arnd Bergmann wrote: >> + if (size > ((phys_addr_t)~0)) >> + size = ((phys_addr_t)~0); >> + >> + /* arm_add_memory() already checks for the case of base + size > 4GB */ >> +#endif >> arm_add_memory(base, size); >> } > > This looks wrong for the case where 'base' is between >0 and 4GB and 'size' > makes it spill over the 4GB boundary. You need to set > 'size = (phys_addr_t)~0 - base' then. > Ah. I believe arm_add_memory() already has the logic to handle this case. Here, we are just trying to shrink 'size' to fit into phys_addr_t, since it is currently u64 but arm_add_memory() uses phys_addr_t for its arguments. I did not want to have this logic in two places, but I can do what you said if you like.
Hello Stepan, On Fri, May 31, 2013 at 6:45 AM, Stepan Moskovchenko <stepanm@codeaurora.org> wrote: > Some LPAE-capable systems may use a Device Tree containing > memory nodes that describe memory extending beyond the 4GB > physical address boundary. Ignore or truncate these memory > nodes on kernels that have not been built with LPAE > support, to prevent the extended physical addresses from > being truncated and aliasing with physical addresses below > the 4GB boundary. > > Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org> > --- > arch/arm/kernel/devtree.c | 12 ++++++++++++ > 1 files changed, 12 insertions(+), 0 deletions(-) Thanks for your efforts on fixing this issue. Before I was aware of this patch I wrote a different implementation to solve most likely the same issue, please see the following patches for more information. Thanks to Arnd for pointing me in the right direction. [PATCH 00/03] ARM: 64-bit memory fixes, APE6EVM second memory bank [PATCH 01/03] ARM: Let arm_add_memory() always use 64-bit arguments [PATCH 02/03] ARM: Handle 64-bit memory in case of 32-bit phys_addr_t [PATCH 03/03] ARM: shmobile: Add second memory bank to DTS for APE6EVM Regarding this patch, I have now tested it on my APE6EVM board together with this patch: [PATCH 03/03] ARM: shmobile: Add second memory bank to DTS for APE6EVM Without your patch the situation is as follows: HIGHMEM=n, LPAE=n - OK (busted, second bank ignored with message [1]) HIGHMEM=y, LPAE=n - NG (busted, board hangs on boot) HIGHMEM=n, LPAE=y - OK HIGHMEM=y, LPAE=y - OK [1] Ignoring RAM at 00000000-3fffffff (vmalloc region overlap). With your patch applied I get the following: HIGHMEM=n, LPAE=n - OK (with message [2]) HIGHMEM=y, LPAE=n - OK (with message [2]) HIGHMEM=n, LPAE=y - OK HIGHMEM=y, LPAE=y - OK [2] Ignoring memory at 0x200000000 due to lack of LPAE support So your patch unbreaks the second memory on my board perfectly well, thank you! Regarding implementation details, I wonder if we only need to cover the DT memory banks by performing the check inside early_init_dt_add_memory_arch()? To me the root cause of this issue seems to be how phys_addr_t is configured when LPAE=n. It is understandable that the kernel cannot handle 64-bit addresses when phys_addr_t is 32-bit, but I believe we need some sane way to omit those memory banks. Your patch handles the non-LPAE case before phys_addr_t is involved which seems to work well. Your approach is much better compared to as-is today with potentially wrapping phys_addr_t parameters to arm_add_memory(). The only question in my mind is about the location for this kind of test, shall it be done in early_init_dt_add_memory_arch() or arm_add_memory()? If we care about adding some bounds checking for the kernel command line mem=xxx option then arm_add_memory() seems to be the best location from my point of view. Any ideas? Please add me to CC if you respin your patch. I will give it a go on my board. Thanks, / magnus
Hi Stepan, What is the status of this fix? I am looking forward it to be merged, but it seems not have happend, yet. Are you waiting for it to be merged, too? or planning to post v2? # IMHO, v2 is not needed. Checked conditions and places are reasonable. I just confirmed on 3.11-rc7, that - The issue still exists. - This patch can be applied cleanly, and fixes the issue. Thanks, /yoshii
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c index bee7f9d..24bc80b 100644 --- a/arch/arm/kernel/devtree.c +++ b/arch/arm/kernel/devtree.c @@ -26,6 +26,18 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size) { +#ifndef CONFIG_ARM_LPAE + if (base > ((phys_addr_t)~0)) { + pr_crit("Ignoring memory at 0x%08llx due to lack of LPAE support\n", + base); + return; + } + + if (size > ((phys_addr_t)~0)) + size = ((phys_addr_t)~0); + + /* arm_add_memory() already checks for the case of base + size > 4GB */ +#endif arm_add_memory(base, size); }
Some LPAE-capable systems may use a Device Tree containing memory nodes that describe memory extending beyond the 4GB physical address boundary. Ignore or truncate these memory nodes on kernels that have not been built with LPAE support, to prevent the extended physical addresses from being truncated and aliasing with physical addresses below the 4GB boundary. Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org> --- arch/arm/kernel/devtree.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation