[RFC PATCH 0/7]

[Ard Biesheuvel]

On Fri, 4 Sep 2020 at 13:51, Patrick Delaunay <patrick.delaunay at 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 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.bykowski@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
>