On Fri, May 07, 2021 at 02:50:28PM +0200, Patrick Delaunay wrote:

> Hi,
> 
> It it the v4 serie of [1].
> 
> This v4 serie is rebased on top of master branch with update after Simon
> Glass review and added tags.
> 
> On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
> protected by a firewall. This region is reserved in the device with
> the "no-map" property as defined in the binding file
> doc/device-tree-bindings/reserved-memory/reserved-memory.txt.
> 
> Sometime the platform boot failed in U-Boot on a Cortex A7 access to
> this region (depending of the binary and the issue can change with compiler
> version or with code alignment), then the firewall raise an error,
> for example:
> 
> E/TC:0   tzc_it_handler:19 TZC permission failure
> E/TC:0   dump_fail_filter:420 Permission violation on filter 0
> E/TC:0   dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged read,
>          AXI ID 5c0
> E/TC:0   Panic
> 
> After investigation, the forbidden access is a speculative request performed
> by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE
> property.
> 
> The issue is solved only when the region reserved by OP-TEE is no more
> mapped in U-Boot as it is already done in Linux kernel.
> 
> Tested on DK2 board with OP-TEE 3.12 / TF-A 2.4:
> 
> With hard-coded address for OP-TEE reserved memory,
> the error doesn't occur.
> 
>  void dram_bank_mmu_setup(int bank)
>  {
>  ....
> 
>     	for (i = start >> MMU_SECTION_SHIFT;
>  	     i < (start >> MMU_SECTION_SHIFT) + (size >> MMU_SECTION_SHIFT);
>  	     i++) {
>  		option = DCACHE_DEFAULT_OPTION;
>  		if (i >= 0xde0)
>  			option = INVALID_ENTRY;
>  		set_section_dcache(i, option);
>  	}
>  }
> 
> Just by modifying the test on 0xde0 to 0xdf0, the OP-TEE memory protected
> by firewall is mapped cacheable and the error occurs.
> 
> I think that it can be a general issue for ARM architecture: the "no-map" tag
> of reserved memory in device should be respected by U-Boot if firewall
> is configured before U-Boot execution.
> 
> But I don't propose a generic solution in
> arm/lib/cache-cp15.c:dram_bank_mmu_setup()
> because the device tree parsing done in lmb_init_and_reserve() takes a
> long time when it is executed without data cache.
> 
> => the previous path 7/7 of v2 series is dropped to avoid
>   performance issue on other ARM target.
> 
> To avoid this performance issue on stm32mp32mp platform, the lmb
> initialization is done in enable_caches() when dcache is still enable.
> 
> This v3 series is composed by 7 patches
> - 1..3/7: preliminary steps to support flags in library in lmb
>   (as it is done in memblock.c in Linux)
> - 4/7: unitary test on the added feature in lmb lib
> - 5/7: save the no-map flags in lmb when the device tree is parsed
> - 6/7: solve issue for the size of cacheable area in pre-reloc case
> - 7/7: update the stm32mp mmu support
> 
> See also [2] which handle same speculative access on armv8 for area
> with Executable attribute.
> 
> [1] http://patchwork.ozlabs.org/project/uboot/list/?series=241122&state=*
> [2] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek.bykowski@gmail.com/

For the series, applied to u-boot/next, and I've taken the above and
reworked it slightly to use as the merge commit message.  Thanks!

-- 
Tom