[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Ard Biesheuvel]

On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay <patrick.delaunay at st.com> wrote:
>
>
> 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.
>

Spurious peculative accesses to device regions would be a severe
silicon bug, so I wonder what is going on here.

(Apologies if we are rehashing stuff here that has already been
discussed - I don't remember the details)

Are you sure that the speculative accesses were not caused by
misconfigured CPU or page tables, missing TLB maintenance, etc etc?
Because it really does smell like a software issue not a hardware
issue.

> 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 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()
>
> It is a rebase on master branch of previous RFC [2].
>
> I can change this last patch if this feature is note 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/
> [2] http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=both&state=*
>
> 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     |  19 ++++++-
>  common/image-fdt.c            |  23 +++++---
>  include/lmb.h                 |  22 +++++++-
>  lib/lmb.c                     | 100 +++++++++++++++++++++++-----------
>  test/lib/lmb.c                |  89 ++++++++++++++++++++++++++++++
>  6 files changed, 212 insertions(+), 44 deletions(-)
>
> --
> 2.17.1
>