On 30/01/2024 14:05, Caleb Connolly 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).
> 
> 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

Looks like I got a bit excited with the `git add .` 😅 I'll fix this on
the next spin or while merging if a respin isn't needed.
> --- /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++) {
> +             /* 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);
> +                     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();
>  }
> 

-- 
// Caleb (they/them)

Reply via email to