Re: [PATCH v5 23/39] mach-snapdragon: carve out no-map regions
On Mon, 26 Feb 2024 at 22:56, 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. > > Reviewed-by: Sumit Garg > Tested-by: Sumit Garg #qcs404 > Signed-off-by: Caleb Connolly > --- > arch/arm/mach-snapdragon/board.c | 162 > +-- > 1 file changed, 140 insertions(+), 22 deletions(-) > I still can't see commit message updates as per [1], probably you can take care of them while applying this patch-set. [1] https://lore.kernel.org/all/CAFA6WYPg=mbrvlqtdaq1omzeku6wcbnt_dwbqv4h36swb3-...@mail.gmail.com/#t -Sumit > diff --git a/arch/arm/mach-snapdragon/board.c > b/arch/arm/mach-snapdragon/board.c > index 5a859aabd5c4..f12f5791a136 100644 > --- a/arch/arm/mach-snapdragon/board.c > +++ b/arch/arm/mach-snapdragon/board.c > @@ -24,8 +24,9 @@ > #include > #include > #include > #include > +#include > > DECLARE_GLOBAL_DATA_PTR; > > static struct mm_region rbx_mem_map[CONFIG_NR_DRAM_BANKS + 2] = { { 0 } }; > @@ -295,9 +296,9 @@ 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 > * up to the first memory bank. > @@ -311,40 +312,157 @@ static void build_mem_map(void) > mem_map[0].attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) | > 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; > + > +#ifdef DEBUG > + 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); > +#endif > } > > u64 get_page_table_size(void) > { > return SZ_64K; > } > > +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 =
[PATCH v5 23/39] mach-snapdragon: carve out no-map regions
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. Reviewed-by: Sumit Garg Tested-by: Sumit Garg #qcs404 Signed-off-by: Caleb Connolly --- arch/arm/mach-snapdragon/board.c | 162 +-- 1 file changed, 140 insertions(+), 22 deletions(-) diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c index 5a859aabd5c4..f12f5791a136 100644 --- a/arch/arm/mach-snapdragon/board.c +++ b/arch/arm/mach-snapdragon/board.c @@ -24,8 +24,9 @@ #include #include #include #include +#include DECLARE_GLOBAL_DATA_PTR; static struct mm_region rbx_mem_map[CONFIG_NR_DRAM_BANKS + 2] = { { 0 } }; @@ -295,9 +296,9 @@ 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 * up to the first memory bank. @@ -311,40 +312,157 @@ static void build_mem_map(void) mem_map[0].attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) | 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; + +#ifdef DEBUG + 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); +#endif } u64 get_page_table_size(void) { return SZ_64K; } +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; + } + + /* Collect the reserved memory regions */ + fdt_for_each_subnode(rmem, gd->fdt_blob, parent) { + const fdt32_t *ptr; + int len; + if (!fdt_getprop(gd->fdt_blob, rmem, "no-map", NULL)) + continue; + + if (i == N_RESERVED_REGIONS) { +