arm64: mvebu: fix crash in EFI memory allocation

Am 22.11.2018 um 11:42 schrieb Baruch Siach <[email protected]>:

EFI memory allocation code return values that are not mapped by U-Boot MMU. This causes a crash when tftp calls efi_set_bootdev(). Fix this similarly to commit 0797f7f0b7e1 ("ARM: tegra: reserve unmapped RAM so EFI doesn't use it").

Signed-off-by Baruch Siach <[email protected]>

---

arch/arm/mach-mvebu/arm64-common.c     | 13 ++-----------
arch/arm/mach-mvebu/include/mach/cpu.h |  9 +++++++++
board/Marvell/mvebu_armada-8k/board.c  | 17 +++++++++++++++++
3 files changed, 28 insertions(+), 11 deletions(-)

arch/arm/mach-mvebu/arm64-common.c +2 -11

index 47bbf69944ec..b82f33093a2c 100644
@@ -17,19 +17,10 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/*
- * Not all memory is mapped in the MMU. So we need to restrict the
- * memory size so that U-Boot does not try to access it. Also, the
- * internal registers are located at 0xf000.0000 - 0xffff.ffff.
- * Currently only 2GiB are mapped for system memory. This is what
- * we pass to the U-Boot subsystem here.
- */
-#define USABLE_RAM_SIZE        0x80000000
-
 ulong board_get_usable_ram_top(ulong total_size)
 {
-    if (gd->ram_size > USABLE_RAM_SIZE)
-        return USABLE_RAM_SIZE;
+    if (gd->ram_size > MVEBU_ARM64_USABLE_RAM_SIZE)
+        return MVEBU_ARM64_USABLE_RAM_SIZE;
 
   return gd->ram_size;
 }

arch/arm/mach-mvebu/include/mach/cpu.h +9 -0

index 85d7dd1610a8..e22a14977859 100644
@@ -177,5 +177,14 @@ int mvebu_lcd_register_init(struct mvebu_lcd_info *lcd_info);
 */
 u32 get_ref_clk(void);
 
+/*
+ * Not all memory is mapped in the MMU. So we need to restrict the
+ * memory size so that U-Boot does not try to access it. Also, the
+ * internal registers are located at 0xf000.0000 - 0xffff.ffff.
+ * Currently only 2GiB are mapped for system memory. This is what
+ * we pass to the U-Boot subsystem in board_get_usable_ram_top().
+ */
+#define MVEBU_ARM64_USABLE_RAM_SIZE    0x80000000
+
 #endif /* __ASSEMBLY__ */
 #endif /* _MVEBU_CPU_H */

board/Marvell/mvebu_armada-8k/board.c +17 -0

index e927e338ea6e..2fffe05c4958 100644
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <dm.h>
 #include <i2c.h>
+#include <efi_loader.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>

@@ -154,6 +155,22 @@ int board_init(void)
 
 int board_late_init(void)
 {
+    uint64_t unmapped_ram_start = gd->bd->bi_dram[0].start
+        + MVEBU_ARM64_USABLE_RAM_SIZE;
+    uint64_t unmapped_ram_size = gd->bd->bi_dram[0].size
+        - MVEBU_ARM64_USABLE_RAM_SIZE;
+
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+    if (gd->bd->bi_dram[0].size > MVEBU_ARM64_USABLE_RAM_SIZE)
+        efi_add_memory_map(unmapped_ram_start,
+                unmapped_ram_size >> EFI_PAGE_SHIFT,
+                EFI_BOOT_SERVICES_DATA, false);
+    if (gd->bd->bi_dram[1].start)
+        efi_add_memory_map(gd->bd->bi_dram[1].start,
+                gd->bd->bi_dram[1].size >> EFI_PAGE_SHIFT,
+                EFI_BOOT_SERVICES_DATA, false);
+#endif
+
   /* Pre-configure the USB ports (overcurrent, VBus) */
   board_xhci_config();
 

Signature · 1 line

2.19.1

Quoted 8 lines

Am 22.11.2018 um 11:42 schrieb Baruch Siach <[email protected]>:

EFI memory allocation code return values that are not mapped by U-Boot MMU. This causes a crash when tftp calls efi_set_bootdev(). Fix this similarly to commit 0797f7f0b7e1 ("ARM: tegra: reserve unmapped RAM so EFI doesn't use it").

Signed-off-by Baruch Siach <[email protected]>

Hm, how about we fox this up in the common memory registration function and add addresses > board_get_usable_ram_top() as boot time data?

As a side note, why do we not map everything in the first place? Is it just to prevent 32bit DMAs from failing?

Quoted 85 lines

---

arch/arm/mach-mvebu/arm64-common.c     | 13 ++-----------
arch/arm/mach-mvebu/include/mach/cpu.h |  9 +++++++++
board/Marvell/mvebu_armada-8k/board.c  | 17 +++++++++++++++++
3 files changed, 28 insertions(+), 11 deletions(-)

arch/arm/mach-mvebu/arm64-common.c +2 -11

index 47bbf69944ec..b82f33093a2c 100644
@@ -17,19 +17,10 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-/*
- * Not all memory is mapped in the MMU. So we need to restrict the
- * memory size so that U-Boot does not try to access it. Also, the
- * internal registers are located at 0xf000.0000 - 0xffff.ffff.
- * Currently only 2GiB are mapped for system memory. This is what
- * we pass to the U-Boot subsystem here.
- */
-#define USABLE_RAM_SIZE        0x80000000
-
 ulong board_get_usable_ram_top(ulong total_size)
 {
-    if (gd->ram_size > USABLE_RAM_SIZE)
-        return USABLE_RAM_SIZE;
+    if (gd->ram_size > MVEBU_ARM64_USABLE_RAM_SIZE)
+        return MVEBU_ARM64_USABLE_RAM_SIZE;
 
   return gd->ram_size;
 }

arch/arm/mach-mvebu/include/mach/cpu.h +9 -0

index 85d7dd1610a8..e22a14977859 100644
@@ -177,5 +177,14 @@ int mvebu_lcd_register_init(struct mvebu_lcd_info *lcd_info);
 */
 u32 get_ref_clk(void);
 
+/*
+ * Not all memory is mapped in the MMU. So we need to restrict the
+ * memory size so that U-Boot does not try to access it. Also, the
+ * internal registers are located at 0xf000.0000 - 0xffff.ffff.
+ * Currently only 2GiB are mapped for system memory. This is what
+ * we pass to the U-Boot subsystem in board_get_usable_ram_top().
+ */
+#define MVEBU_ARM64_USABLE_RAM_SIZE    0x80000000
+
 #endif /* __ASSEMBLY__ */
 #endif /* _MVEBU_CPU_H */

board/Marvell/mvebu_armada-8k/board.c +17 -0

index e927e338ea6e..2fffe05c4958 100644
@@ -6,6 +6,7 @@
 #include <common.h>
 #include <dm.h>
 #include <i2c.h>
+#include <efi_loader.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>

@@ -154,6 +155,22 @@ int board_init(void)
 
 int board_late_init(void)
 {
+    uint64_t unmapped_ram_start = gd->bd->bi_dram[0].start
+        + MVEBU_ARM64_USABLE_RAM_SIZE;
+    uint64_t unmapped_ram_size = gd->bd->bi_dram[0].size
+        - MVEBU_ARM64_USABLE_RAM_SIZE;
+
+#if CONFIG_IS_ENABLED(EFI_LOADER)
+    if (gd->bd->bi_dram[0].size > MVEBU_ARM64_USABLE_RAM_SIZE)
+        efi_add_memory_map(unmapped_ram_start,
+                unmapped_ram_size >> EFI_PAGE_SHIFT,
+                EFI_BOOT_SERVICES_DATA, false);
+    if (gd->bd->bi_dram[1].start)
+        efi_add_memory_map(gd->bd->bi_dram[1].start,
+                gd->bd->bi_dram[1].size >> EFI_PAGE_SHIFT,
+                EFI_BOOT_SERVICES_DATA, false);
+#endif
+
   /* Pre-configure the USB ports (overcurrent, VBus) */
   board_xhci_config();

Hi Alex,

Adding Stephen Warren to Cc.

Quoted 9 lines · Alexander Graf

Quoted 8 lines

Am 22.11.2018 um 11:42 schrieb Baruch Siach <[email protected]>:

EFI memory allocation code return values that are not mapped by U-Boot MMU. This causes a crash when tftp calls efi_set_bootdev(). Fix this similarly to commit 0797f7f0b7e1 ("ARM: tegra: reserve unmapped RAM so EFI doesn't use it").

Signed-off-by Baruch Siach <[email protected]>

Quoted 2 lines

Hm, how about we fox this up in the common memory registration function and add addresses > board_get_usable_ram_top() as boot time data?

That would be much nicer indeed. Where should I look in lib/efi_loader/ to implement that?

With that in place we can revert commit 0797f7f0b7e1, right? Stephen, do you concur?

Quoted 15 lines · Baruch Siach

Adding Stephen Warren to Cc.

Quoted 11 lines · Alexander Graf

Quoted 8 lines

Am 22.11.2018 um 11:42 schrieb Baruch Siach <[email protected]>:

EFI memory allocation code return values that are not mapped by U-Boot MMU. This causes a crash when tftp calls efi_set_bootdev(). Fix this similarly to commit 0797f7f0b7e1 ("ARM: tegra: reserve unmapped RAM so EFI doesn't use it").

Signed-off-by Baruch Siach <[email protected]>

Hm, how about we fox this up in the common memory registration function and add addresses > board_get_usable_ram_top() as boot time data?

Quoted 5 lines

That would be much nicer indeed. Where should I look in lib/efi_loader/ to implement that?

With that in place we can revert commit 0797f7f0b7e1, right? Stephen, do you concur?

I believe so, yes. I'm not sure why it's acceptable to fix the EFI allocator to solve this now, when it wasn't acceptable before though.

Hi Stephen,

Adding Heinrich Schuchardt.

Quoted 16 lines · Stephen Warren

Quoted 15 lines · Baruch Siach

Adding Stephen Warren to Cc.

Quoted 11 lines · Alexander Graf

Quoted 8 lines

Am 22.11.2018 um 11:42 schrieb Baruch Siach <[email protected]>:

EFI memory allocation code return values that are not mapped by U-Boot MMU. This causes a crash when tftp calls efi_set_bootdev(). Fix this similarly to commit 0797f7f0b7e1 ("ARM: tegra: reserve unmapped RAM so EFI doesn't use it").

Signed-off-by Baruch Siach <[email protected]>

Hm, how about we fox this up in the common memory registration function and add addresses > board_get_usable_ram_top() as boot time data?

Quoted 8 lines

Quoted 5 lines

That would be much nicer indeed. Where should I look in lib/efi_loader/ to implement that?

With that in place we can revert commit 0797f7f0b7e1, right? Stephen, do you concur?

I believe so, yes. I'm not sure why it's acceptable to fix the EFI allocator to solve this now, when it wasn't acceptable before though.

I read the previous discussion[1] from July/August. If I understand the conclusion correctly, efi_add_memory_map() provides information that is vital to the correctness of the EFI memory map, and is platform specific. Is that correct?

If so, then this patch (actually, its v2) is the correct solution for Armada 8K. A possible improvement to the EFI memory allocation code would be a warning instead of crash. Would that be acceptable?