On Tue, 30 Jan 2024 at 19:35, Caleb Connolly <caleb.conno...@linaro.org> wrote:
>
> On Qualcomm platforms, the TZ may already have certain memory regions
> under protection by the time U-Boot starts. There is a rare case on some
> platforms where the prefetcher might speculatively access one of these
> regions resulting in a board crash (TZ traps and then resets the board).
>
> We shouldn't be accessing these regions from within U-Boot anyway, so
> let's mark them all with PTE_TYPE_FAULT to prevent any speculative
> access and correctly trap in EL1 rather than EL3.
>
> This is quite costly with caches off (takes ~2 seconds on SDM845 vs 35ms
> with caches on). So to minimise the impact this is only enabled on
> QCS404 for now (where the issue is known to occur).
It only took 65ms with caches off on QCS404 as per below print:
carveout time: 65ms
It's probably due to some missing bits as pointed below to get it
working fast on SDM845 too..
>
> In the future, we should try to find a more efficient way to handle
> this, perhaps by turning on the MMU in stages.
>
> Signed-off-by: Caleb Connolly <caleb.conno...@linaro.org>
> ---
> abort | 20 ++++++
> arch/arm/mach-snapdragon/board.c | 151
> ++++++++++++++++++++++++++++++++-------
> 2 files changed, 147 insertions(+), 24 deletions(-)
>
> diff --git a/abort b/abort
> new file mode 100644
> index 000000000000..34c3e2f0596a
> --- /dev/null
> +++ b/abort
> @@ -0,0 +1,20 @@
> +"Synchronous Abort" handler, esr 0x96000007, far 0x0
> +elr: 0000000000005070 lr : 00000000000039a8 (reloc)
> +elr: 000000017ff27070 lr : 000000017ff259a8
> +x0 : 0000000000000000 x1 : 0000000000000000
> +x2 : 000000017faeb328 x3 : 000000017faeb320
> +x4 : 00000000000feae8 x5 : 00000000feae8000
> +x6 : 0000000000000007 x7 : 0000000000000004
> +x8 : 0000000000000000 x9 : 000000017eae7000
> +x10: 0000000000000000 x11: 000000027c89ffff
> +x12: 000000017fffffff x13: 0000000180000000
> +x14: 0000000000518db0 x15: 000000017faeb0cc
> +x16: 000000017ff2edac x17: 0000000000000000
> +x18: 000000017fb02d50 x19: 000000017ffbd600
> +x20: 000000017ffbd600 x21: 0000000000000000
> +x22: 0000000000001f1f x23: 000000017faeb370
> +x24: 000000017ffbd000 x25: 000000017ff94993
> +x26: 0000000000000030 x27: 000000017ffbecf0
> +x28: 0000000000000000 x29: 000000017faeb2a0
> +
> +Code: a94153f3 f94013f5 a8c47bfd d65f03c0 (b9400002)
> diff --git a/arch/arm/mach-snapdragon/board.c
> b/arch/arm/mach-snapdragon/board.c
> index 9d8684a21fa1..ca300dc843a9 100644
> --- a/arch/arm/mach-snapdragon/board.c
> +++ b/arch/arm/mach-snapdragon/board.c
> @@ -6,8 +6,9 @@
> * Author: Caleb Connolly <caleb.conno...@linaro.org>
> */
>
> -#define LOG_DEBUG
> +//#define LOG_DEBUG
>
> +#include "time.h"
> #include <asm/armv8/mmu.h>
> #include <asm/gpio.h>
> #include <asm/io.h>
> @@ -26,6 +27,7 @@
> #include <lmb.h>
> #include <malloc.h>
> #include <usb.h>
> +#include <sort.h>
>
> DECLARE_GLOBAL_DATA_PTR;
>
> @@ -297,7 +299,7 @@ int board_late_init(void)
>
> static void build_mem_map(void)
> {
> - int i;
> + int i, j;
>
> /*
> * Ensure the peripheral block is sized to correctly cover the
> address range
> @@ -313,39 +315,140 @@ static void build_mem_map(void)
> PTE_BLOCK_NON_SHARE |
> PTE_BLOCK_PXN | PTE_BLOCK_UXN;
>
> - debug("Configured memory map:\n");
> - debug(" 0x%016llx - 0x%016llx: Peripheral block\n",
> - mem_map[0].phys, mem_map[0].phys + mem_map[0].size);
> -
> - /*
> - * Now add memory map entries for each DRAM bank, ensuring we don't
> - * overwrite the list terminator
> - */
> - for (i = 0; i < ARRAY_SIZE(rbx_mem_map) - 2 &&
> gd->bd->bi_dram[i].size; i++) {
> - if (i == ARRAY_SIZE(rbx_mem_map) - 1) {
> - log_warning("Too many DRAM banks!\n");
> - break;
> - }
> - mem_map[i + 1].phys = gd->bd->bi_dram[i].start;
> - mem_map[i + 1].virt = mem_map[i + 1].phys;
> - mem_map[i + 1].size = gd->bd->bi_dram[i].size;
> - mem_map[i + 1].attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |
> - PTE_BLOCK_INNER_SHARE;
> -
> - debug(" 0x%016llx - 0x%016llx: DDR bank %d\n",
> - mem_map[i + 1].phys, mem_map[i + 1].phys + mem_map[i +
> 1].size, i);
> + for (i = 1, j = 0; i < ARRAY_SIZE(rbx_mem_map) - 1 &&
> gd->bd->bi_dram[j].size; i++, j++) {
> + mem_map[i].phys = gd->bd->bi_dram[j].start;
> + mem_map[i].virt = mem_map[i].phys;
> + mem_map[i].size = gd->bd->bi_dram[j].size;
> + mem_map[i].attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | \
> + PTE_BLOCK_INNER_SHARE;
> }
> +
> + mem_map[i].phys = UINT64_MAX;
> + mem_map[i].size = 0;
> +
> + debug("Configured memory map:\n");
> + for (i = 0; mem_map[i].size; i++)
> + debug(" 0x%016llx - 0x%016llx: entry %d\n",
> + mem_map[i].phys, mem_map[i].phys + mem_map[i].size, i);
> }
>
> u64 get_page_table_size(void)
> {
> - return SZ_64K;
> + return SZ_256K;
> }
>
> +static int fdt_cmp_res(const void *v1, const void *v2)
> +{
> + const struct fdt_resource *res1 = v1, *res2 = v2;
> +
> + return res1->start - res2->start;
> +}
> +
> +#define N_RESERVED_REGIONS 32
> +
> +/* Mark all no-map regions as PTE_TYPE_FAULT to prevent speculative access.
> + * On some platforms this is enough to trigger a security violation and trap
> + * to EL3.
> + */
> +static void carve_out_reserved_memory(void)
> +{
> + static struct fdt_resource res[N_RESERVED_REGIONS] = { 0 };
> + int parent, rmem, count, i = 0;
> + phys_addr_t start;
> + size_t size;
> +
> + /* Some reserved nodes must be carved out, as the cache-prefetcher
> may otherwise
> + * attempt to access them, causing a security exception.
> + */
> + parent = fdt_path_offset(gd->fdt_blob, "/reserved-memory");
> + if (parent <= 0) {
> + log_err("No reserved memory regions found\n");
> + return;
> + }
> + count = fdtdec_get_child_count(gd->fdt_blob, parent);
> + if (count > N_RESERVED_REGIONS) {
> + log_err("Too many reserved memory regions!\n");
> + count = N_RESERVED_REGIONS;
> + }
> +
> + /* Collect the reserved memory regions */
> + fdt_for_each_subnode(rmem, gd->fdt_blob, parent) {
> + if (!fdt_getprop(gd->fdt_blob, rmem, "no-map", NULL))
> + continue;
> + if (fdt_getprop(gd->fdt_blob, rmem, "compatible", NULL))
> + continue;
> + /* Skip regions that don't have a fixed address/size */
> + if (fdt_get_resource(gd->fdt_blob, rmem, "reg", 0, &res[i++]))
> + i--;
> + }
> +
> + /* Sort the reserved memory regions by address */
> + count = i;
> + qsort(res, count, sizeof(struct fdt_resource), fdt_cmp_res);
> +
> + /* Now set the right attributes for them. Often a lot of the regions
> are tightly packed together
> + * so we can optimise the number of calls to mmu_change_region_attr()
> by combining adjacent
> + * regions.
> + */
> + start = ALIGN_DOWN(res[0].start, SZ_2M);
> + size = ALIGN(res[0].end, SZ_4K) - start;
> + for (i = 1; i < count; i++) {
I suppose this loop fails to configure attributes for the last no-map
region like uefi_mem region on QCS404.
> + /* We ideally want to 2M align everything for more efficient
> pagetables, but we must avoid
> + * overwriting reserved memory regions which shouldn't be
> mapped as FAULT (like those with
> + * compatible properties).
> + * If within 2M of the previous region, bump the size to
> include this region. Otherwise
> + * start a new region.
> + */
> + if (start + size < res[i].start - SZ_2M) {
> + debug(" 0x%016llx - 0x%016llx FAULT\n",
> + start, start + size);
> + mmu_change_region_attr(start, size, PTE_TYPE_FAULT);
Here the size won't always be 2M aligned which is the case for SDM845.
I would suggest you do:
mmu_change_region_attr(start, ALIGN(size,
SZ_2M), PTE_TYPE_FAULT);
-Sumit
> + start = ALIGN_DOWN(res[i].start, SZ_2M);
> + size = ALIGN(res[i].end, SZ_4K) - start;
> + } else {
> + /* Bump size if this region is immediately after the
> previous one */
> + size = ALIGN(res[i].end, SZ_4K) - start;
> + }
> + }
> +}
> +
> +/* This function open-codes setup_all_pgtables() so that we can
> + * insert additional mappings *before* turning on the MMU.
> + */
> void enable_caches(void)
> {
> + u64 tlb_addr = gd->arch.tlb_addr;
> + u64 tlb_size = gd->arch.tlb_size;
> + ulong carveout_start;
> +
> + gd->arch.tlb_fillptr = tlb_addr;
> +
> build_mem_map();
>
> icache_enable();
> +
> + /* Create normal system page tables */
> + setup_pgtables();
> +
> + /* Create emergency page tables */
> + gd->arch.tlb_size -= (uintptr_t)gd->arch.tlb_fillptr -
> + (uintptr_t)gd->arch.tlb_addr;
> + gd->arch.tlb_addr = gd->arch.tlb_fillptr;
> + setup_pgtables();
> + gd->arch.tlb_emerg = gd->arch.tlb_addr;
> + gd->arch.tlb_addr = tlb_addr;
> + gd->arch.tlb_size = tlb_size;
> +
> + /* Doing this has a noticeable impact on boot time, so until we can
> + * find a more efficient solution, just enable the workaround for
> + * the one board where it's necessary. Without this there seems to
> + * be a speculative access to a region which is protected by the TZ.
> + */
> + if (of_machine_is_compatible("qcom,qcs404")) {
> + carveout_start = get_timer(0);
> + carve_out_reserved_memory();
> + debug("carveout time: %lums\n", get_timer(carveout_start));
> + }
> +
> dcache_enable();
> }
>
> --
> 2.43.0
>
