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

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

[Stepan Moskovchenko]

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(-)

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);
 }

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

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

[Jason Gunthorpe]

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

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

[Arnd Bergmann]

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

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

[Stepan Moskovchenko]

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.


-- 
  The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
  hosted by The Linux Foundation

[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@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

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

[takashi.yoshii.zj at renesas.com]

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