On Fri, 4 Sep 2020 at 14:25, Ard Biesheuvel ardb@kernel.org wrote:
On Fri, 4 Sep 2020 at 13:51, Patrick Delaunay patrick.delaunay@st.com wrote:
arm: cache: cp15: don't map reserved region with no-map property
Hi,
On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region protected by a firewall. This region is reserved in device with "no-map" property.
But 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 a 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 (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is already done in Linux kernel.
The only speculative accesses to such regions permitted by the architecture are instruction fetches, which is why setting the nx attribute is required on v7 for device mappings.
Does uboot currently honour this requirement?
I think current U-Boot honours the speculative side by using a IO memory default mapping strategy. If the reserved memory is used by a driver with a specific mapping, u-boot cannot ensure the mapping won't conflict with default mapping. For example OP-TEE defines a portion of this memory as non-secure cached shared memory. With Arm, we cannot map an area both as cached memory and as IO memory (Device/StronglyOrdered). So here, using a not-mapped strategy for "no-map" property looks better.
Regards, Etienne
I think that can be a general issue for ARM architecture: the no-map tag in device should be respected by U-boot, so I propose a generic solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
This RFC serie is composed by 7 patches
- 1..4/7: preliminary steps to support flags in library in lmb (as it is done in memblock.c in Linux)
- 5/7: unitary test on the added feature in lmb lib
- 6/7: save the no-map flags in lmb when the device tree is parsed
- 7/7: update the generic behavior for "no-map" region in arm/lib/cache-cp15.c::dram_bank_mmu_setup()
I can change this last patch if it is required by other ARM architecture; it is a weak function so I can avoid to map the region with "no-map" property in device tree only for STM32MP architecture (in arch/arm/mach-stm32mp/cpu.c).
See also [1] which handle same speculative access on armv8 for area with Executable attribute.
[1] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek....
Regards Patrick
Patrick Delaunay (7): lmb: Add support of flags for no-map properties lmb: add lmb_is_reserved_flags lmb: remove lmb_region.size lmb: add lmb_dump_region() function test: lmb: add test for lmb_reserve_flags image-fdt: save no-map parameter of reserve-memory arm: cache: cp15: don't map the reserved region with no-map property
arch/arm/include/asm/system.h | 3 + arch/arm/lib/cache-cp15.c | 17 +++++- common/image-fdt.c | 23 +++++--- include/lmb.h | 22 +++++++- lib/lmb.c | 100 +++++++++++++++++++++++----------- test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++ 6 files changed, 210 insertions(+), 44 deletions(-)
-- 2.17.1