[PATCH v1] armv8: MMU: Fix the memory map for RAM
Marek Bykowski
marek.bykowski at gmail.com
Fri Sep 4 21:05:51 CEST 2020
Hi Patrick,
First of all thanks for your feedback...
> > -----------------|
> > | | Read-Only
> > Cacheable | Code | Not-Executable
>
> Code is executable I think...
>
Good catch, copy/paste error from a rectangle above.
>
> If all DDR is " Not-Executable", excepted code of U-boot himself and EFI, I think that
> the standalone application can't be more execute except if the MMU configuration
> change before to execute it.
Yes, that's why I referred that any new use of memory, except already
remapped by this patch should take account of attributing properly
Instruction and Data regions. Maybe I should re-phrase that part.
The current situation in which the whole RAM is RW, XN=0 (Executable)
advocates the improper CPU uses. CPU (fetch logic) really behaves differently
whether interacting with a Code or Data region. Situation in which it is generic
(but wrong) and working for 99% cases doesn't justify.
I really think we should impose on users/developers the proper use of the CPU.
>
> See do_bootm_standalone().
>
> PS: it is done for Linux in do_bootm_linux/ do_bootm_linux/ announce_and_cleanup.....
> caches ans MMU are deactivated
>
Will check but after MMU is turned off the pages are no matter anymore so
should be good. With MMU off for armv8 all data accesses are Device_nGnRnE,
all instruction fetches are cacheable, all addresses are RW, XN=1 (Executable).
> For information I have the same issue on armV7 platform stm32mp1: speculative access
> on memory, used by OP-TEE, protected by firewall.
>
The fault/faults I'm observing on Cortex-A57 is the System Memory Controller gets
violated and raises an error interrupt. The error is out-of-order, an attempt to
read from an out-of-range address with sample details below:
out_of_range_addr 0x702200200 -> faulting address
out_of_range_length 0x40 -> burst size
out_of_range_type 0x5 -> stands for a wrapped read command
out_of_range_source_id 0x0 -> source indicates it is coming from CPU0
Burst size 64 bytes (a cache line size) and wrapped read (AXI4) may suggest
the data side memory system hit into the cache loaded by mispredited
instruction fetch, all due to improper MMU programming.
> I propose a other solution [1]: no more map the reserved memory region with the property
> "no-map", bt only for cache-cp15.
>
> It is based lmb library so it could done also in armv8 cache functions.
>
> [1] http://patchwork.ozlabs.org/project/uboot/list/?series=199486
>
>
If there is another (better) solution I'm in for it as we *should really* fix it
this or that way. The link copied doesn't work with me. Can you resend?
Marek
> > Signed-off-by: Marek Bykowski <marek.bykowski at gmail.com>
> > ---
> > Changes in PATCH v1:
> > - now it re-maps the whole RAM to the proper attributes,
> > - took account of other images, eg. PSCI, EFI that need a separate attention
> > - it has been tested on qemu arm 64 and two of my armv8 boards, one is Axxia
> > series of the processor, the other is so early that the name cannot be
> > revealed yet
> >
> > arch/arm/cpu/armv8/cache_v8.c | 103
> > +++++++++++++++++++++++++++++++
> > arch/arm/cpu/armv8/u-boot.lds | 39 ++++++++++--
> > arch/arm/include/asm/armv8/mmu.h | 6 ++
> > arch/arm/lib/sections.c | 19 ++++--
> > include/asm-generic/sections.h | 9 +++
> > 5 files changed, 164 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > index 7c31d98a6f..4d8843d05e 100644
> > --- a/arch/arm/cpu/armv8/cache_v8.c
> > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > @@ -14,6 +14,7 @@
> > #include <asm/cache.h>
> > #include <asm/system.h>
> > #include <asm/armv8/mmu.h>
> > +#include <asm/sections.h>
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > @@ -364,6 +365,100 @@ __weak u64 get_page_table_size(void)
> > return size;
> > }
> >
> > +__weak void force_remaping_ram(void)
> > +{
> > + int i = 0;
> > +
> > + if (!(gd->flags & GD_FLG_RELOC))
> > + return;
> > +
> > + struct mm_region mem_map_ram[] = {
> > + /*
> > + * Re-map the whole RAM to Read-Write, Non-Executable, and
> > + * then .text section/s to Read-Only, Executable.
> > + */
> > + {
> > + .virt = (u64)gd->ram_base,
> > + .phys = (u64)gd->ram_base,
> > + .size = (u64)gd->ram_size,
> > + .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> > + PTE_BLOCK_INNER_SHARE |
> > + PTE_BLOCK_UXN
> > + },
> > +#if IS_ENABLED(CONFIG_EFI_LOADER)
> > + {
> > + .virt = (u64)__efi_runtime_start_section,
> > + .phys = (u64)__efi_runtime_start_section,
> > + .size = (u64)(__efi_runtime_stop_section -
> > + __efi_runtime_start_section),
> > + .attrs = (PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> > + PTE_BLOCK_INNER_SHARE) &
> > ~PTE_BLOCK_UXN
> > + },
> > + {
> > + .virt = (u64)__efi_runtime_rel_start_section,
> > + .phys = (u64)__efi_runtime_rel_start_section,
> > + .size = (u64)(__efi_runtime_rel_stop_section -
> > + __efi_runtime_rel_start_section),
> > + .attrs = (PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> > + PTE_BLOCK_INNER_SHARE) &
> > ~PTE_BLOCK_UXN
> > + },
> > +#endif
> > + {
> > + .virt = (u64)__image_copy_start,
> > + .phys = (u64)__image_copy_start,
> > + .size = (u64)(__text_end - __image_copy_start),
> > + .attrs = (PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> > + PTE_BLOCK_INNER_SHARE |
> > PTE_BLOCK_AP_RO) &
> > + ~PTE_BLOCK_UXN
> > + },
> > + {
> > + .virt = (u64)__text_rest_start,
> > + .phys = (u64)__text_rest_start,
> > + .size = (u64)(__text_rest_end - __text_rest_start),
> > + .attrs = (PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> > + PTE_BLOCK_INNER_SHARE |
> > PTE_BLOCK_AP_RO) &
> > + ~PTE_BLOCK_UXN
> > + },
> > +#if IS_ENABLED(CONFIG_ARMV8_SECURE_BASE)
> > + {
> > + .virt = (u64)__secure_text_start,
> > + .phys = (u64)__secure_text_start,
> > + .size = (u64)(__secure_text_end - __secure_text_start),
> > + .attrs = (PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> > + PTE_BLOCK_INNER_SHARE |
> > PTE_BLOCK_AP_RO) &
> > + ~PTE_BLOCK_UXN
> > + },
> > +#endif
> > + { 0 }
> > + };
> > +
> > + debug("Re-mapping RAM: Code to RO,XN=0, Data - RW,XN=1");
> > + if (IS_ENABLED(CONFIG_EFI_LOADER) ||
> > IS_ENABLED(CONFIG_ARMV8_SECURE_BASE))
> > + debug(", Non-U-Boot images (eg. efi, psci) - RW,XN=0
> > (unchanged)");
> > + debug("\n");
> > +
> > + for (; mem_map_ram[i].size || mem_map_ram[i].attrs; i++) {
> > + /*
> > + * MT_NORMAL - Normal Memory
> > + * MT_DEVICE_NGNRNE - Device Memory (we don't expect that
> > + * really for the RAM to happen...)
> > + * RO - read-only
> > + * RW - read-write
> > + * XN=0 - Executable
> > + * XN=1 - Non-executable
> > + */
> > + debug("[%d]: 0x%llx-0x%llx %s%s%s\n",
> > + i, mem_map_ram[i].phys, mem_map_ram[i].phys +
> > + mem_map_ram[i].size,
> > + mem_map_ram[i].attrs &
> > PTE_BLOCK_MEMTYPE(MT_NORMAL) ?
> > + "MT_NORMAL" : "MT_DEVICE",
> > + mem_map_ram[i].attrs & PTE_BLOCK_AP_RO ? "-RO" : "-
> > RW",
> > + mem_map_ram[i].attrs & PTE_BLOCK_UXN ?
> > + "-XN=1" : "-XN=0");
> > + add_map(&mem_map_ram[i]);
> > + }
> > +}
> > +
> > void setup_pgtables(void)
> > {
> > int i;
> > @@ -381,6 +476,14 @@ void setup_pgtables(void)
> > /* Now add all MMU table entries one after another to the table */
> > for (i = 0; mem_map[i].size || mem_map[i].attrs; i++)
> > add_map(&mem_map[i]);
> > +
> > + /*
> > + * Force re-mapping RAM only if the generic linker script in use.
> > + * The boundaries of the regions for re-mapping are defined in
> > + * the generic ARM64 ld script and won't work for the custom ones.
> > + */
> > + if (!IS_ENABLED(CONFIG_SYS_CUSTOM_LDSCRIPT))
> > + force_remaping_ram();
> > }
> >
> > static void setup_all_pgtables(void)
> > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds index
> > 2554980595..8e98b143d5 100644
> > --- a/arch/arm/cpu/armv8/u-boot.lds
> > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > @@ -9,6 +9,7 @@
> >
> > #include <config.h>
> > #include <asm/psci.h>
> > +#include <asm/armv8/mmu.h>
> >
> > OUTPUT_FORMAT("elf64-littleaarch64", "elf64-littleaarch64", "elf64-
> > littleaarch64")
> > OUTPUT_ARCH(aarch64)
> > @@ -20,25 +21,36 @@ SECTIONS
> > #endif
> > . = 0x00000000;
> >
> > - . = ALIGN(8);
> > + /* Align .text to the page size */
> > + . = ALIGN(PAGE_SIZE);
> > .text :
> > {
> > *(.__image_copy_start)
> > CPUDIR/start.o (.text*)
> > + . = ALIGN(PAGE_SIZE);
> > + KEEP(*(.__text_end))
> > }
> >
> > /* This needs to come before *(.text*) */
> > - .efi_runtime : {
> > - __efi_runtime_start = .;
> > + .efi_runtime ALIGN(CONSTANT(COMMONPAGESIZE)):
> > + {
> > + KEEP(*(.__efi_runtime_start))
> > + __efi_runtime_start = .;
> > *(.text.efi_runtime*)
> > *(.rodata.efi_runtime*)
> > *(.data.efi_runtime*)
> > - __efi_runtime_stop = .;
> > + __efi_runtime_stop = .;
> > + . = ALIGN(PAGE_SIZE);
> > + KEEP(*(.__efi_runtime_stop))
> > }
> >
> > + . = ALIGN(PAGE_SIZE);
> > .text_rest :
> > {
> > + KEEP(*(.__text_rest_start))
> > *(.text*)
> > + . = ALIGN(PAGE_SIZE);
> > + KEEP(*(.__text_rest_end))
> > }
> >
> > #ifdef CONFIG_ARMV8_PSCI
> > @@ -54,14 +66,18 @@ SECTIONS
> > #define CONFIG_ARMV8_SECURE_BASE
> > #define __ARMV8_PSCI_STACK_IN_RAM
> > #endif
> > + . = ALIGN(PAGE_SIZE);
> > .secure_text CONFIG_ARMV8_SECURE_BASE :
> > AT(ADDR(.__secure_start) + SIZEOF(.__secure_start))
> > {
> > + KEEP(*(.__secure_text_start))
> > *(._secure.text)
> > . = ALIGN(8);
> > __secure_svc_tbl_start = .;
> > KEEP(*(._secure_svc_tbl_entries))
> > __secure_svc_tbl_end = .;
> > + . = ALIGN(PAGE_SIZE);
> > + KEEP(*(.__secure_text_end))
> > }
> >
> > .secure_data : AT(LOADADDR(.secure_text) + SIZEOF(.secure_text))
> > @@ -113,15 +129,26 @@ SECTIONS
> > KEEP(*(SORT(.u_boot_list*)));
> > }
> >
> > - . = ALIGN(8);
> > + . = ALIGN(PAGE_SIZE);
> > + .efi_runtime_rel_start :
> > + {
> > + KEEP(*(.__efi_runtime_rel_start))
> > + }
> >
> > - .efi_runtime_rel : {
> > + .efi_runtime_rel :
> > + {
> > __efi_runtime_rel_start = .;
> > *(.rel*.efi_runtime)
> > *(.rel*.efi_runtime.*)
> > __efi_runtime_rel_stop = .;
> > }
> >
> > + . = ALIGN(PAGE_SIZE);
> > + .efi_runtime_rel_stop :
> > + {
> > + KEEP(*(.__efi_runtime_rel_stop))
> > + }
> > +
> > . = ALIGN(8);
> >
> > .image_copy_end :
> > diff --git a/arch/arm/include/asm/armv8/mmu.h
> > b/arch/arm/include/asm/armv8/mmu.h
> > index fc97c55114..571cc283eb 100644
> > --- a/arch/arm/include/asm/armv8/mmu.h
> > +++ b/arch/arm/include/asm/armv8/mmu.h
> > @@ -59,6 +59,12 @@
> > */
> > #define PTE_BLOCK_MEMTYPE(x) ((x) << 2)
> > #define PTE_BLOCK_NS (1 << 5)
> > +/*
> > + * AP[1] bit is ignored by hardware and is
> > + * treated as if it was One in EL2/EL3
> > + */
> > +#define PTE_BLOCK_AP_RO (1 << 7)
> > +#define PTE_BLOCK_AP_RW (0 << 7)
> > #define PTE_BLOCK_NON_SHARE (0 << 8)
> > #define PTE_BLOCK_OUTER_SHARE (2 << 8)
> > #define PTE_BLOCK_INNER_SHARE (3 << 8)
> > diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c index
> > 3bb2d8468c..b00d24843d 100644
> > --- a/arch/arm/lib/sections.c
> > +++ b/arch/arm/lib/sections.c
> > @@ -3,6 +3,8 @@
> > * Copyright 2013 Albert ARIBAUD <albert.u.boot at aribaud.net>
> > */
> >
> > +#include <linux/compiler_types.h>
> > +
> > /**
> > * These two symbols are declared in a C file so that the linker
> > * uses R_ARM_RELATIVE relocation, rather than the R_ARM_ABS32 one @@ -
> > 18,6 +20,11 @@
> > * aliasing warnings.
> > */
> >
> > +char __text_end[0] __section(".__text_end"); char __text_rest_start[0]
> > +__section(".__text_rest_start"); char __text_rest_end[0]
> > +__section(".__text_rest_end"); char __secure_text_start[0]
> > +__section(".__secure_text_start");
> > +char __secure_text_end[0] __section(".__secure_text_end");
> > char __bss_start[0] __attribute__((section(".__bss_start")));
> > char __bss_end[0] __attribute__((section(".__bss_end")));
> > char __image_copy_start[0] __attribute__((section(".__image_copy_start")));
> > @@ -26,10 +33,10 @@ char __rel_dyn_start[0]
> > __attribute__((section(".__rel_dyn_start")));
> > char __rel_dyn_end[0] __attribute__((section(".__rel_dyn_end")));
> > char __secure_start[0] __attribute__((section(".__secure_start")));
> > char __secure_end[0] __attribute__((section(".__secure_end")));
> > -char __secure_stack_start[0] __attribute__((section(".__secure_stack_start")));
> > -char __secure_stack_end[0] __attribute__((section(".__secure_stack_end")));
> > -char __efi_runtime_start[0] __attribute__((section(".__efi_runtime_start")));
> > -char __efi_runtime_stop[0] __attribute__((section(".__efi_runtime_stop")));
> > -char __efi_runtime_rel_start[0]
> > __attribute__((section(".__efi_runtime_rel_start")));
> > -char __efi_runtime_rel_stop[0] __attribute__((section(".__efi_runtime_rel_stop")));
> > +char __secure_stack_start[0] __section(".__secure_stack_start");
> > +char __secure_stack_end[0] __section(".__secure_stack_end"); char
> > +__efi_runtime_start_section[0] __section(".__efi_runtime_start");
> > +char __efi_runtime_stop_section[0] __section(".__efi_runtime_stop");
> > +char __efi_runtime_rel_start_section[0]
> > +__section(".__efi_runtime_rel_start");
> > +char __efi_runtime_rel_stop_section[0]
> > +__section(".__efi_runtime_rel_stop");
> > char _end[0] __attribute__((section(".__end")));
> > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index
> > 0577238d60..c3dc0522ee 100644
> > --- a/include/asm-generic/sections.h
> > +++ b/include/asm-generic/sections.h
> > @@ -72,6 +72,11 @@ extern void _start(void);
> > */
> > #ifdef CONFIG_ARM
> >
> > +extern char __text_end[];
> > +extern char __text_rest_start[];
> > +extern char __text_rest_end[];
> > +extern char __secure_text_start[];
> > +extern char __secure_text_end[];
> > extern char __bss_start[];
> > extern char __bss_end[];
> > extern char __image_copy_start[];
> > @@ -79,6 +84,10 @@ extern char __image_copy_end[]; extern char
> > _image_binary_end[]; extern char __rel_dyn_start[]; extern char __rel_dyn_end[];
> > +extern char __efi_runtime_start_section[]; extern char
> > +__efi_runtime_stop_section[]; extern char
> > +__efi_runtime_rel_start_section[];
> > +extern char __efi_runtime_rel_stop_section[];
> >
> > #else /* don't use offsets: */
> >
> > --
> > 2.21.0.896.g6a6c0f1
>
More information about the U-Boot
mailing list