On Fri, 4 Sep 2020 at 14:25, Ard Biesheuvel <a...@kernel.org> wrote: > > On Fri, 4 Sep 2020 at 13:51, Patrick Delaunay <patrick.delau...@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.bykow...@gmail.com/ > > > > 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 > >
