[RFC PATCH 0/7]

[Etienne Carriere]

On Fri, 4 Sep 2020 at 14:25, Ard Biesheuvel <ardb at kernel.org> wrote:
>
> 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 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.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
> >