Re: [PATCH] arm: Prevent memory aliasing on non-LPAE kernels

[Magnus Damm]

Hello Stepan,

On Fri, May 31, 2013 at 6:45 AM, Stepan Moskovchenko
<stepanm@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/