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

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

[Patrick Delaunay]

Hi,

It it the v3 serie of [1].

This v3 serie is rebased on top of v2021.07-rc1 with integrated previous series:
- [2] for stm32mp parts and added dram_bank_mmu_setup
- [3] for LMB impacts

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.

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 an 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 as it is already done in Linux kernel.

Tested on DK2 board with OP-TEE 3.12 / TF-A 2.4:

With hard-coded address for OP-TEE reserved memory,
the error doesn't occur.

 void dram_bank_mmu_setup(int bank)
 {
 ....

    	for (i = start >> MMU_SECTION_SHIFT;
 	     i < (start >> MMU_SECTION_SHIFT) + (size >> MMU_SECTION_SHIFT);
 	     i++) {
 		option = DCACHE_DEFAULT_OPTION;
 		if (i >= 0xde0)
 			option = INVALID_ENTRY;
 		set_section_dcache(i, option);
 	}
 }

Just by modifying the test on 0xde0 to 0xdf0, the OP-TEE memory protected
by firewall is mapped cacheable and the error occurs.

I think that can be a general issue for ARM architecture: the no-map tag
in device should be respected by U-Boot.

But I don't propose a generic
solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup()
because the device tree parsing done in lmb_init_and_reserve() take a
long time when it is executed without data cache.

=> the previous path 7/7 of v2 series is dropped to avoid
   performance issue on other ARM target.

To avoid this issue on stm32mp32mp platform, this V3 series moves
the lmb initialization in enable_caches() and the lmb variable becomes a
static struct.

This v3 series is composed by 7 patches
- 1..3/7: preliminary steps to support flags in library in lmb
  (as it is done in memblock.c in Linux)
- 4/7: unitary test on the added feature in lmb lib
- 5/7: save the no-map flags in lmb when the device tree is parsed
- 6/7: solve issue for the size of cacheable area in pre-reloc case
- 7/7: update the stm32mp mmu support

See also [4] which handle same speculative access on armv8 for area
with Executable attribute.

[1] http://patchwork.ozlabs.org/project/uboot/list/?series=228543&state=*
[2] http://patchwork.ozlabs.org/project/uboot/list/?series=228202&state=*
[3] http://patchwork.ozlabs.org/project/uboot/list/?series=227570&state=*
[4] http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek.bykowski at gmail.com/

Regards
Patrick

Changes in v3:
- NEW: solve performance issue as relocated DT is not marked cacheable
- call lmb_init_and_reserve when data cache is activated in enable_caches()
- drop v2 patch "arm: cache: cp15: don't map the reserved region
  with no-map property"

Changes in v2:
- remove unnecessary comments in lmb.h
- rebase on latest lmb patches
- NEW: update in stm32mp specific MMU setup functions

Patrick Delaunay (7):
  lmb: Add support of flags for no-map properties
  lmb: add lmb_is_reserved_flags
  lmb: add lmb_dump_region() function
  test: lmb: add test for lmb_reserve_flags
  image-fdt: save no-map parameter of reserve-memory
  stm32mp: Increase the reserved memory in board_get_usable_ram_top
  stm32mp: don't map the reserved region with no-map property

 arch/arm/mach-stm32mp/cpu.c       | 17 +++++-
 arch/arm/mach-stm32mp/dram_init.c |  3 +-
 common/image-fdt.c                | 23 +++++---
 include/lmb.h                     | 21 +++++++
 lib/lmb.c                         | 94 ++++++++++++++++++++++---------
 test/lib/lmb.c                    | 89 +++++++++++++++++++++++++++++
 6 files changed, 209 insertions(+), 38 deletions(-)

-- 
2.17.1

[PATCH v3 1/7] lmb: Add support of flags for no-map properties

[Patrick Delaunay]

Add "flags" in lmb_property to save the "no-map" property of
reserved region and a new function lmb_reserve_flags() to check
this flag.

The default allocation use flags = LMB_NONE.

The adjacent reserved memory region are merged only when they have
the same flags value.

This patch is partially based on flags support done in Linux kernel
mm/memblock .c (previously lmb.c); it is why LMB_NOMAP = 0x4, it is
aligned with MEMBLOCK_NOMAP value.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

(no changes since v2)

Changes in v2:
- remove unnecessary comments in lmb.h
- rebase on latest lmb patches

 include/lmb.h | 20 ++++++++++++++++++++
 lib/lmb.c     | 52 ++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index 541e17093c..aa196c63bf 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -12,6 +12,16 @@
  * Copyright (C) 2001 Peter Bergner, IBM Corp.
  */
 
+/**
+ * enum lmb_flags - definition of memory region attributes
+ * @LMB_NONE: no special request
+ * @LMB_NOMAP: don't add to mmu configuration
+ */
+enum lmb_flags {
+	LMB_NONE		= 0x0,
+	LMB_NOMAP		= 0x4,
+};
+
 /**
  * struct lmb_property - Description of one region.
  *
@@ -21,6 +31,7 @@
 struct lmb_property {
 	phys_addr_t base;
 	phys_size_t size;
+	enum lmb_flags flags;
 };
 
 /**
@@ -69,6 +80,8 @@ extern void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base,
 				       phys_size_t size, void *fdt_blob);
 extern long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size);
 extern long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size);
+extern long lmb_reserve_flags(struct lmb *lmb, phys_addr_t base,
+			      phys_size_t size, enum lmb_flags flags);
 extern phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align);
 extern phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align,
 			    phys_addr_t max_addr);
@@ -92,6 +105,13 @@ lmb_size_bytes(struct lmb_region *type, unsigned long region_nr)
 void board_lmb_reserve(struct lmb *lmb);
 void arch_lmb_reserve(struct lmb *lmb);
 
+/* Low level functions */
+
+static inline bool lmb_is_nomap(struct lmb_property *m)
+{
+	return !!(m->flags & LMB_NOMAP);
+}
+
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_LMB_H */
diff --git a/lib/lmb.c b/lib/lmb.c
index c08c4d942b..69700bf9ba 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -25,6 +25,8 @@ void lmb_dump_all_force(struct lmb *lmb)
 		       (unsigned long long)lmb->memory.region[i].base);
 		printf("		   .size   = 0x%llx\n",
 		       (unsigned long long)lmb->memory.region[i].size);
+		printf("		   .flags   = 0x%x\n",
+		       lmb->memory.region[i].flags);
 	}
 
 	printf("\n    reserved.cnt	   = 0x%lx\n", lmb->reserved.cnt);
@@ -33,6 +35,8 @@ void lmb_dump_all_force(struct lmb *lmb)
 		       (unsigned long long)lmb->reserved.region[i].base);
 		printf("		     .size = 0x%llx\n",
 		       (unsigned long long)lmb->reserved.region[i].size);
+		printf("		     .flags = 0x%x\n",
+		       lmb->reserved.region[i].flags);
 	}
 }
 
@@ -81,6 +85,7 @@ static void lmb_remove_region(struct lmb_region *rgn, unsigned long r)
 	for (i = r; i < rgn->cnt - 1; i++) {
 		rgn->region[i].base = rgn->region[i + 1].base;
 		rgn->region[i].size = rgn->region[i + 1].size;
+		rgn->region[i].flags = rgn->region[i + 1].flags;
 	}
 	rgn->cnt--;
 }
@@ -144,7 +149,8 @@ void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base,
 }
 
 /* This routine called with relocation disabled. */
-static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t size)
+static long lmb_add_region_flags(struct lmb_region *rgn, phys_addr_t base,
+				 phys_size_t size, enum lmb_flags flags)
 {
 	unsigned long coalesced = 0;
 	long adjacent, i;
@@ -152,6 +158,7 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	if (rgn->cnt == 0) {
 		rgn->region[0].base = base;
 		rgn->region[0].size = size;
+		rgn->region[0].flags = flags;
 		rgn->cnt = 1;
 		return 0;
 	}
@@ -160,18 +167,27 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	for (i = 0; i < rgn->cnt; i++) {
 		phys_addr_t rgnbase = rgn->region[i].base;
 		phys_size_t rgnsize = rgn->region[i].size;
+		phys_size_t rgnflags = rgn->region[i].flags;
 
-		if ((rgnbase == base) && (rgnsize == size))
-			/* Already have this region, so we're done */
-			return 0;
+		if (rgnbase == base && rgnsize == size) {
+			if (flags == rgnflags)
+				/* Already have this region, so we're done */
+				return 0;
+			else
+				return -1; /* regions with new flags */
+		}
 
 		adjacent = lmb_addrs_adjacent(base, size, rgnbase, rgnsize);
 		if (adjacent > 0) {
+			if (flags != rgnflags)
+				break;
 			rgn->region[i].base -= size;
 			rgn->region[i].size += size;
 			coalesced++;
 			break;
 		} else if (adjacent < 0) {
+			if (flags != rgnflags)
+				break;
 			rgn->region[i].size += size;
 			coalesced++;
 			break;
@@ -182,8 +198,10 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	}
 
 	if ((i < rgn->cnt - 1) && lmb_regions_adjacent(rgn, i, i + 1)) {
-		lmb_coalesce_regions(rgn, i, i + 1);
-		coalesced++;
+		if (rgn->region[i].flags == rgn->region[i + 1].flags) {
+			lmb_coalesce_regions(rgn, i, i + 1);
+			coalesced++;
+		}
 	}
 
 	if (coalesced)
@@ -196,9 +214,11 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 		if (base < rgn->region[i].base) {
 			rgn->region[i + 1].base = rgn->region[i].base;
 			rgn->region[i + 1].size = rgn->region[i].size;
+			rgn->region[i + 1].flags = rgn->region[i].flags;
 		} else {
 			rgn->region[i + 1].base = base;
 			rgn->region[i + 1].size = size;
+			rgn->region[i + 1].flags = flags;
 			break;
 		}
 	}
@@ -206,6 +226,7 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	if (base < rgn->region[0].base) {
 		rgn->region[0].base = base;
 		rgn->region[0].size = size;
+		rgn->region[0].flags = flags;
 	}
 
 	rgn->cnt++;
@@ -213,6 +234,12 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	return 0;
 }
 
+static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base,
+			   phys_size_t size)
+{
+	return lmb_add_region_flags(rgn, base, size, LMB_NONE);
+}
+
 /* This routine may be called with relocation disabled. */
 long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 {
@@ -267,14 +294,21 @@ long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size)
 	 * beginging of the hole and add the region after hole.
 	 */
 	rgn->region[i].size = base - rgn->region[i].base;
-	return lmb_add_region(rgn, end + 1, rgnend - end);
+	return lmb_add_region_flags(rgn, end + 1, rgnend - end,
+				    rgn->region[i].flags);
 }
 
-long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size)
+long lmb_reserve_flags(struct lmb *lmb, phys_addr_t base, phys_size_t size,
+		       enum lmb_flags flags)
 {
 	struct lmb_region *_rgn = &(lmb->reserved);
 
-	return lmb_add_region(_rgn, base, size);
+	return lmb_add_region_flags(_rgn, base, size, flags);
+}
+
+long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size)
+{
+	return lmb_reserve_flags(lmb, base, size, LMB_NONE);
 }
 
 static long lmb_overlaps_region(struct lmb_region *rgn, phys_addr_t base,
-- 
2.17.1

[PATCH v3 1/7] lmb: Add support of flags for no-map properties

[Simon Glass]

Hi Patrick,

On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
<patrick.delaunay@foss.st.com> wrote:
>
> Add "flags" in lmb_property to save the "no-map" property of
> reserved region and a new function lmb_reserve_flags() to check
> this flag.
>
> The default allocation use flags = LMB_NONE.
>
> The adjacent reserved memory region are merged only when they have
> the same flags value.
>
> This patch is partially based on flags support done in Linux kernel
> mm/memblock .c (previously lmb.c); it is why LMB_NOMAP = 0x4, it is
> aligned with MEMBLOCK_NOMAP value.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - remove unnecessary comments in lmb.h
> - rebase on latest lmb patches
>
>  include/lmb.h | 20 ++++++++++++++++++++
>  lib/lmb.c     | 52 ++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 63 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/include/lmb.h b/include/lmb.h
> index 541e17093c..aa196c63bf 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -12,6 +12,16 @@
>   * Copyright (C) 2001 Peter Bergner, IBM Corp.
>   */
>
> +/**
> + * enum lmb_flags - definition of memory region attributes
> + * @LMB_NONE: no special request
> + * @LMB_NOMAP: don't add to mmu configuration
> + */
> +enum lmb_flags {
> +       LMB_NONE                = 0x0,
> +       LMB_NOMAP               = 0x4,
> +};
> +
>  /**
>   * struct lmb_property - Description of one region.
>   *
> @@ -21,6 +31,7 @@
>  struct lmb_property {
>         phys_addr_t base;
>         phys_size_t size;
> +       enum lmb_flags flags;
>  };
>
>  /**
> @@ -69,6 +80,8 @@ extern void lmb_init_and_reserve_range(struct lmb *lmb, phys_addr_t base,
>                                        phys_size_t size, void *fdt_blob);
>  extern long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size);
>  extern long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size);
> +extern long lmb_reserve_flags(struct lmb *lmb, phys_addr_t base,
> +                             phys_size_t size, enum lmb_flags flags);

Needs a comment

[..]

Regards,
Simon

[PATCH v3 2/7] lmb: add lmb_is_reserved_flags

[Patrick Delaunay]

Add a new function lmb_is_reserved_flags to check is a
address is reserved with a specific flags.

This function can be used to check if an address was
reserved with no-map flags with:

lmb_is_reserved_flags(lmb, addr, LMB_NOMAP);

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

(no changes since v1)

 include/lmb.h |  1 +
 lib/lmb.c     | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/lmb.h b/include/lmb.h
index aa196c63bf..6537d56e18 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -91,6 +91,7 @@ extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
 				  phys_size_t size);
 extern phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr);
 extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr);
+extern int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags);
 extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size);
 
 extern void lmb_dump_all(struct lmb *lmb);
diff --git a/lib/lmb.c b/lib/lmb.c
index 69700bf9ba..e270e86186 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -443,7 +443,7 @@ phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr)
 	return 0;
 }
 
-int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
+int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags)
 {
 	int i;
 
@@ -451,11 +451,17 @@ int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
 		phys_addr_t upper = lmb->reserved.region[i].base +
 			lmb->reserved.region[i].size - 1;
 		if ((addr >= lmb->reserved.region[i].base) && (addr <= upper))
-			return 1;
+			return !!((lmb->reserved.region[i].flags & flags)
+				   == flags);
 	}
 	return 0;
 }
 
+int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
+{
+	return lmb_is_reserved_flags(lmb, addr, LMB_NONE);
+}
+
 __weak void board_lmb_reserve(struct lmb *lmb)
 {
 	/* please define platform specific board_lmb_reserve() */
-- 
2.17.1

[PATCH v3 2/7] lmb: add lmb_is_reserved_flags

[Simon Glass]

Hi Patrick,

On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
<patrick.delaunay@foss.st.com> wrote:
>
> Add a new function lmb_is_reserved_flags to check is a
> address is reserved with a specific flags.
>
> This function can be used to check if an address was
> reserved with no-map flags with:
>
> lmb_is_reserved_flags(lmb, addr, LMB_NOMAP);
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
> (no changes since v1)
>
>  include/lmb.h |  1 +
>  lib/lmb.c     | 10 ++++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/lmb.h b/include/lmb.h
> index aa196c63bf..6537d56e18 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -91,6 +91,7 @@ extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
>                                   phys_size_t size);
>  extern phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr);
>  extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr);
> +extern int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags);

drop extern and please add a function comment

>  extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size);
>
>  extern void lmb_dump_all(struct lmb *lmb);
> diff --git a/lib/lmb.c b/lib/lmb.c
> index 69700bf9ba..e270e86186 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -443,7 +443,7 @@ phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr)
>         return 0;
>  }
>
> -int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
> +int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags)
>  {
>         int i;
>
> @@ -451,11 +451,17 @@ int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
>                 phys_addr_t upper = lmb->reserved.region[i].base +
>                         lmb->reserved.region[i].size - 1;
>                 if ((addr >= lmb->reserved.region[i].base) && (addr <= upper))
> -                       return 1;
> +                       return !!((lmb->reserved.region[i].flags & flags)
> +                                  == flags);

I don't know what flags is since there is no comment, but it seems
that you should drop the !!()

>         }
>         return 0;
>  }
>
> +int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
> +{
> +       return lmb_is_reserved_flags(lmb, addr, LMB_NONE);
> +}
> +
>  __weak void board_lmb_reserve(struct lmb *lmb)
>  {
>         /* please define platform specific board_lmb_reserve() */
> --
> 2.17.1
>

Regards,
Simon

[PATCH v3 2/7] lmb: add lmb_is_reserved_flags

[Patrick DELAUNAY]

On 4/29/21 6:10 PM, Simon Glass wrote:
> Hi Patrick,
>
> On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
> <patrick.delaunay@foss.st.com> wrote:
>> Add a new function lmb_is_reserved_flags to check is a
>> address is reserved with a specific flags.
>>
>> This function can be used to check if an address was
>> reserved with no-map flags with:
>>
>> lmb_is_reserved_flags(lmb, addr, LMB_NOMAP);
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>>
>> (no changes since v1)
>>
>>   include/lmb.h |  1 +
>>   lib/lmb.c     | 10 ++++++++--
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/lmb.h b/include/lmb.h
>> index aa196c63bf..6537d56e18 100644
>> --- a/include/lmb.h
>> +++ b/include/lmb.h
>> @@ -91,6 +91,7 @@ extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
>>                                    phys_size_t size);
>>   extern phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr);
>>   extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr);
>> +extern int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags);
> drop extern and please add a function comment


I chosed to use extern to by aligned with other function and the linux 
memblock library.

But I will drop them in v4



>>   extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size);
>>
>>   extern void lmb_dump_all(struct lmb *lmb);
>> diff --git a/lib/lmb.c b/lib/lmb.c
>> index 69700bf9ba..e270e86186 100644
>> --- a/lib/lmb.c
>> +++ b/lib/lmb.c
>> @@ -443,7 +443,7 @@ phys_size_t lmb_get_free_size(struct lmb *lmb, phys_addr_t addr)
>>          return 0;
>>   }
>>
>> -int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
>> +int lmb_is_reserved_flags(struct lmb *lmb, phys_addr_t addr, int flags)
>>   {
>>          int i;
>>
>> @@ -451,11 +451,17 @@ int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
>>                  phys_addr_t upper = lmb->reserved.region[i].base +
>>                          lmb->reserved.region[i].size - 1;
>>                  if ((addr >= lmb->reserved.region[i].base) && (addr <= upper))
>> -                       return 1;
>> +                       return !!((lmb->reserved.region[i].flags & flags)
>> +                                  == flags);
> I don't know what flags is since there is no comment, but it seems
> that you should drop the !!()


Yes, it is a old C-coding habit.


>>          }
>>          return 0;
>>   }
>>
>> +int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
>> +{
>> +       return lmb_is_reserved_flags(lmb, addr, LMB_NONE);
>> +}
>> +
>>   __weak void board_lmb_reserve(struct lmb *lmb)
>>   {
>>          /* please define platform specific board_lmb_reserve() */
>> --
>> 2.17.1
>>
> Regards,
> Simon

[PATCH v3 3/7] lmb: add lmb_dump_region() function

[Patrick Delaunay]

Add lmb_dump_region() function, to simplify lmb_dump_all_force().
This patch is based on Linux memblock dump function.

An example of bdinfo output is:

.....
fdt_size    = 0x000146a0
FB base     = 0xfdd00000
lmb_dump_all:
 memory.cnt  = 0x1
 memory[0]	[0xc0000000-0xffffffff], 0x40000000 bytes flags: 0
 reserved.cnt  = 0x6
 reserved[0]	[0x10000000-0x10045fff], 0x00046000 bytes flags: 4
 reserved[1]	[0x30000000-0x3003ffff], 0x00040000 bytes flags: 4
 reserved[2]	[0x38000000-0x3800ffff], 0x00010000 bytes flags: 4
 reserved[3]	[0xe8000000-0xefffffff], 0x08000000 bytes flags: 4
 reserved[4]	[0xfbaea344-0xfdffffff], 0x02515cbc bytes flags: 0
 reserved[5]	[0xfe000000-0xffffffff], 0x02000000 bytes flags: 4
arch_number = 0x00000000
TLB addr    = 0xfdff0000
....

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

(no changes since v1)

 lib/lmb.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index e270e86186..3b1878fd58 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -14,32 +14,32 @@
 
 #define LMB_ALLOC_ANYWHERE	0
 
-void lmb_dump_all_force(struct lmb *lmb)
+static void lmb_dump_region(struct lmb_region *rgn, char *name)
 {
-	unsigned long i;
+	unsigned long long base, size, end;
+	enum lmb_flags flags;
+	int i;
 
-	printf("lmb_dump_all:\n");
-	printf("    memory.cnt		   = 0x%lx\n", lmb->memory.cnt);
-	for (i = 0; i < lmb->memory.cnt; i++) {
-		printf("    memory.reg[0x%lx].base   = 0x%llx\n", i,
-		       (unsigned long long)lmb->memory.region[i].base);
-		printf("		   .size   = 0x%llx\n",
-		       (unsigned long long)lmb->memory.region[i].size);
-		printf("		   .flags   = 0x%x\n",
-		       lmb->memory.region[i].flags);
-	}
+	printf(" %s.cnt  = 0x%lx\n", name, rgn->cnt);
 
-	printf("\n    reserved.cnt	   = 0x%lx\n", lmb->reserved.cnt);
-	for (i = 0; i < lmb->reserved.cnt; i++) {
-		printf("    reserved.reg[0x%lx].base = 0x%llx\n", i,
-		       (unsigned long long)lmb->reserved.region[i].base);
-		printf("		     .size = 0x%llx\n",
-		       (unsigned long long)lmb->reserved.region[i].size);
-		printf("		     .flags = 0x%x\n",
-		       lmb->reserved.region[i].flags);
+	for (i = 0; i < rgn->cnt; i++) {
+		base = rgn->region[i].base;
+		size = rgn->region[i].size;
+		end = base + size - 1;
+		flags = rgn->region[i].flags;
+
+		printf(" %s[%d]\t[0x%llx-0x%llx], 0x%08llx bytes flags: %x\n",
+		       name, i, base, end, size, flags);
 	}
 }
 
+void lmb_dump_all_force(struct lmb *lmb)
+{
+	printf("lmb_dump_all:\n");
+	lmb_dump_region(&lmb->memory, "memory");
+	lmb_dump_region(&lmb->reserved, "reserved");
+}
+
 void lmb_dump_all(struct lmb *lmb)
 {
 #ifdef DEBUG
-- 
2.17.1

[PATCH v3 3/7] lmb: add lmb_dump_region() function

[Simon Glass]

On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
<patrick.delaunay@foss.st.com> wrote:
>
> Add lmb_dump_region() function, to simplify lmb_dump_all_force().
> This patch is based on Linux memblock dump function.
>
> An example of bdinfo output is:
>
> .....
> fdt_size    = 0x000146a0
> FB base     = 0xfdd00000
> lmb_dump_all:
>  memory.cnt  = 0x1
>  memory[0]      [0xc0000000-0xffffffff], 0x40000000 bytes flags: 0
>  reserved.cnt  = 0x6
>  reserved[0]    [0x10000000-0x10045fff], 0x00046000 bytes flags: 4
>  reserved[1]    [0x30000000-0x3003ffff], 0x00040000 bytes flags: 4
>  reserved[2]    [0x38000000-0x3800ffff], 0x00010000 bytes flags: 4
>  reserved[3]    [0xe8000000-0xefffffff], 0x08000000 bytes flags: 4
>  reserved[4]    [0xfbaea344-0xfdffffff], 0x02515cbc bytes flags: 0
>  reserved[5]    [0xfe000000-0xffffffff], 0x02000000 bytes flags: 4
> arch_number = 0x00000000
> TLB addr    = 0xfdff0000
> ....
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
> (no changes since v1)
>
>  lib/lmb.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v3 4/7] test: lmb: add test for lmb_reserve_flags

[Patrick Delaunay]

Add a test to check the management of reserved region with flags.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

(no changes since v1)

 test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/test/lib/lmb.c b/test/lib/lmb.c
index 0d8963fcbf..b2c2b99ef1 100644
--- a/test/lib/lmb.c
+++ b/test/lib/lmb.c
@@ -723,3 +723,92 @@ static int lib_test_lmb_max_regions(struct unit_test_state *uts)
 
 DM_TEST(lib_test_lmb_max_regions,
 	UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+static int lib_test_lmb_flags(struct unit_test_state *uts)
+{
+	const phys_addr_t ram = 0x40000000;
+	const phys_size_t ram_size = 0x20000000;
+	struct lmb lmb;
+	long ret;
+
+	lmb_init(&lmb);
+
+	ret = lmb_add(&lmb, ram, ram_size);
+	ut_asserteq(ret, 0);
+
+	/* reserve, same flag */
+	ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+
+	/* reserve again, same flag */
+	ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+
+	/* reserve again, new flag */
+	ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NONE);
+	ut_asserteq(ret, -1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
+		   0, 0, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+
+	/* merge after */
+	ret = lmb_reserve_flags(&lmb, 0x40020000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x20000,
+		   0, 0, 0, 0);
+
+	/* merge before */
+	ret = lmb_reserve_flags(&lmb, 0x40000000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 1);
+	ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40000000, 0x30000,
+		   0, 0, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+
+	ret = lmb_reserve_flags(&lmb, 0x40030000, 0x10000, LMB_NONE);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000,
+		   0x40030000, 0x10000, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
+
+	/* test that old API use LMB_NONE */
+	ret = lmb_reserve(&lmb, 0x40040000, 0x10000);
+	ut_asserteq(ret, 1);
+	ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0, 0);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
+
+	ret = lmb_reserve_flags(&lmb, 0x40070000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0x40070000, 0x10000);
+
+	ret = lmb_reserve_flags(&lmb, 0x40050000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 0);
+	ASSERT_LMB(&lmb, ram, ram_size, 4, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0x40050000, 0x10000);
+
+	/* merge with 2 adjacent regions */
+	ret = lmb_reserve_flags(&lmb, 0x40060000, 0x10000, LMB_NOMAP);
+	ut_asserteq(ret, 2);
+	ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000,
+		   0x40030000, 0x20000, 0x40050000, 0x30000);
+
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
+	ut_asserteq(lmb_is_nomap(&lmb.reserved.region[2]), 1);
+
+	return 0;
+}
+
+DM_TEST(lib_test_lmb_flags,
+	UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
-- 
2.17.1

[PATCH v3 4/7] test: lmb: add test for lmb_reserve_flags

[Simon Glass]

On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
<patrick.delaunay@foss.st.com> wrote:
>
> Add a test to check the management of reserved region with flags.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
> (no changes since v1)
>
>  test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/test/lib/lmb.c b/test/lib/lmb.c
> index 0d8963fcbf..b2c2b99ef1 100644
> --- a/test/lib/lmb.c
> +++ b/test/lib/lmb.c
> @@ -723,3 +723,92 @@ static int lib_test_lmb_max_regions(struct unit_test_state *uts)
>
>  DM_TEST(lib_test_lmb_max_regions,
>         UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> +
> +static int lib_test_lmb_flags(struct unit_test_state *uts)
> +{
> +       const phys_addr_t ram = 0x40000000;
> +       const phys_size_t ram_size = 0x20000000;
> +       struct lmb lmb;
> +       long ret;
> +
> +       lmb_init(&lmb);
> +
> +       ret = lmb_add(&lmb, ram, ram_size);
> +       ut_asserteq(ret, 0);
> +
> +       /* reserve, same flag */
> +       ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP);
> +       ut_asserteq(ret, 0);
> +       ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
> +                  0, 0, 0, 0);
> +
> +       /* reserve again, same flag */
> +       ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NOMAP);
> +       ut_asserteq(ret, 0);
> +       ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
> +                  0, 0, 0, 0);
> +
> +       /* reserve again, new flag */
> +       ret = lmb_reserve_flags(&lmb, 0x40010000, 0x10000, LMB_NONE);
> +       ut_asserteq(ret, -1);
> +       ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x10000,
> +                  0, 0, 0, 0);
> +
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
> +
> +       /* merge after */
> +       ret = lmb_reserve_flags(&lmb, 0x40020000, 0x10000, LMB_NOMAP);
> +       ut_asserteq(ret, 1);
> +       ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40010000, 0x20000,
> +                  0, 0, 0, 0);
> +
> +       /* merge before */
> +       ret = lmb_reserve_flags(&lmb, 0x40000000, 0x10000, LMB_NOMAP);
> +       ut_asserteq(ret, 1);
> +       ASSERT_LMB(&lmb, ram, ram_size, 1, 0x40000000, 0x30000,
> +                  0, 0, 0, 0);
> +
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
> +
> +       ret = lmb_reserve_flags(&lmb, 0x40030000, 0x10000, LMB_NONE);
> +       ut_asserteq(ret, 0);
> +       ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000,
> +                  0x40030000, 0x10000, 0, 0);
> +
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
> +
> +       /* test that old API use LMB_NONE */
> +       ret = lmb_reserve(&lmb, 0x40040000, 0x10000);
> +       ut_asserteq(ret, 1);
> +       ASSERT_LMB(&lmb, ram, ram_size, 2, 0x40000000, 0x30000,
> +                  0x40030000, 0x20000, 0, 0);
> +
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
> +
> +       ret = lmb_reserve_flags(&lmb, 0x40070000, 0x10000, LMB_NOMAP);
> +       ut_asserteq(ret, 0);
> +       ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000,
> +                  0x40030000, 0x20000, 0x40070000, 0x10000);
> +
> +       ret = lmb_reserve_flags(&lmb, 0x40050000, 0x10000, LMB_NOMAP);
> +       ut_asserteq(ret, 0);
> +       ASSERT_LMB(&lmb, ram, ram_size, 4, 0x40000000, 0x30000,
> +                  0x40030000, 0x20000, 0x40050000, 0x10000);
> +
> +       /* merge with 2 adjacent regions */
> +       ret = lmb_reserve_flags(&lmb, 0x40060000, 0x10000, LMB_NOMAP);
> +       ut_asserteq(ret, 2);
> +       ASSERT_LMB(&lmb, ram, ram_size, 3, 0x40000000, 0x30000,
> +                  0x40030000, 0x20000, 0x40050000, 0x30000);
> +
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[0]), 1);
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[1]), 0);
> +       ut_asserteq(lmb_is_nomap(&lmb.reserved.region[2]), 1);
> +
> +       return 0;
> +}
> +
> +DM_TEST(lib_test_lmb_flags,
> +       UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> --
> 2.17.1
>

Regards,
Simon

[PATCH v3 5/7] image-fdt: save no-map parameter of reserve-memory

[Patrick Delaunay]

Save the no-map information present in 'reserved-memory' node to allow
correct handling when the MMU is configured in board to avoid
speculative access.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

(no changes since v1)

 common/image-fdt.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/common/image-fdt.c b/common/image-fdt.c
index d50e1ba3fe..06dce92a28 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -75,18 +75,20 @@ static const image_header_t *image_get_fdt(ulong fdt_addr)
 #endif
 
 static void boot_fdt_reserve_region(struct lmb *lmb, uint64_t addr,
-				    uint64_t size)
+				    uint64_t size, enum lmb_flags flags)
 {
 	long ret;
 
-	ret = lmb_reserve(lmb, addr, size);
+	ret = lmb_reserve_flags(lmb, addr, size, flags);
 	if (ret >= 0) {
-		debug("   reserving fdt memory region: addr=%llx size=%llx\n",
-		      (unsigned long long)addr, (unsigned long long)size);
+		debug("   reserving fdt memory region: addr=%llx size=%llx flags=%x\n",
+		      (unsigned long long)addr,
+		      (unsigned long long)size, flags);
 	} else {
 		puts("ERROR: reserving fdt memory region failed ");
-		printf("(addr=%llx size=%llx)\n",
-		       (unsigned long long)addr, (unsigned long long)size);
+		printf("(addr=%llx size=%llx flags=%x)\n",
+		       (unsigned long long)addr,
+		       (unsigned long long)size, flags);
 	}
 }
 
@@ -106,6 +108,7 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob)
 	int i, total, ret;
 	int nodeoffset, subnode;
 	struct fdt_resource res;
+	enum lmb_flags flags;
 
 	if (fdt_check_header(fdt_blob) != 0)
 		return;
@@ -115,7 +118,7 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob)
 	for (i = 0; i < total; i++) {
 		if (fdt_get_mem_rsv(fdt_blob, i, &addr, &size) != 0)
 			continue;
-		boot_fdt_reserve_region(lmb, addr, size);
+		boot_fdt_reserve_region(lmb, addr, size, LMB_NONE);
 	}
 
 	/* process reserved-memory */
@@ -127,9 +130,13 @@ void boot_fdt_add_mem_rsv_regions(struct lmb *lmb, void *fdt_blob)
 			ret = fdt_get_resource(fdt_blob, subnode, "reg", 0,
 					       &res);
 			if (!ret && fdtdec_get_is_enabled(fdt_blob, subnode)) {
+				flags = LMB_NONE;
+				if (fdtdec_get_bool(fdt_blob, subnode,
+						    "no-map"))
+					flags = LMB_NOMAP;
 				addr = res.start;
 				size = res.end - res.start + 1;
-				boot_fdt_reserve_region(lmb, addr, size);
+				boot_fdt_reserve_region(lmb, addr, size, flags);
 			}
 
 			subnode = fdt_next_subnode(fdt_blob, subnode);
-- 
2.17.1

[PATCH v3 5/7] image-fdt: save no-map parameter of reserve-memory

[Simon Glass]

Hi Patrick,

On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
<patrick.delaunay@foss.st.com> wrote:
>
> Save the no-map information present in 'reserved-memory' node to allow
> correct handling when the MMU is configured in board to avoid
> speculative access.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> ---
>
> (no changes since v1)
>
>  common/image-fdt.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

Where is no-map documented?

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v3 5/7] image-fdt: save no-map parameter of reserve-memory

[Bin Meng]

Hi Simon,

On Fri, Apr 30, 2021 at 12:13 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Patrick,
>
> On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
> <patrick.delaunay@foss.st.com> wrote:
> >
> > Save the no-map information present in 'reserved-memory' node to allow
> > correct handling when the MMU is configured in board to avoid
> > speculative access.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > ---
> >
> > (no changes since v1)
> >
> >  common/image-fdt.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
>
> Where is no-map documented?

See Linux kernel
Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt

>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Bin

[PATCH v3 5/7] image-fdt: save no-map parameter of reserve-memory

[Ard Biesheuvel]

On Thu, 29 Apr 2021 at 18:11, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Patrick,
>
> On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
> <patrick.delaunay@foss.st.com> wrote:
> >
> > Save the no-map information present in 'reserved-memory' node to allow
> > correct handling when the MMU is configured in board to avoid
> > speculative access.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > ---
> >
> > (no changes since v1)
> >
> >  common/image-fdt.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
>
> Where is no-map documented?
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

I don't remember exactly where the discussion ended up the last time,
so please disregard this if we settled it, but I still don't think
that secure-only memory should be described in the DT at all.

The v7 and v8 documentation are not 100% aligned on this, but the S
bit is now considered a true address bit, and so secure address 0x0
and non-secure address zero could be decoded by different peripherals
at the same time, even if some TZ firewall implementations don't
implement it that way.

Since DT addresses don't carry the S bit at all, any address described
in DT must be assumed to be a non-secure address. This means that the
no-map region essentially describes a non-secure region that does not
exist, in order to prevent a secure region at the same offset (which
DT cannot describe in the first place) from being covered by the
linear mapping.

So, apologies if we are going around in circles here, but could you
please explain again why the DT is describing secure memory as
non-secure memory, and then reserving it again using no-map?

[PATCH v3 5/7] image-fdt: save no-map parameter of reserve-memory

[Patrick DELAUNAY]

Hi Simon,

On 4/29/21 6:10 PM, Simon Glass wrote:
> Hi Patrick,
>
> On Wed, 28 Apr 2021 at 03:23, Patrick Delaunay
> <patrick.delaunay@foss.st.com> wrote:
>> Save the no-map information present in 'reserved-memory' node to allow
>> correct handling when the MMU is configured in board to avoid
>> speculative access.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> ---
>>
>> (no changes since v1)
>>
>>   common/image-fdt.c | 23 +++++++++++++++--------
>>   1 file changed, 15 insertions(+), 8 deletions(-)
> Where is no-map documented?


In linux: 
Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt

in U-Boot: doc/device-tree-bindings/reserved-memory/reserved-memory.txt:55

no-map (optional) - empty property
 ??? - Indicates the operating system must not create a virtual mapping
 ????? of the region as part of its standard mapping of system memory,
 ????? nor permit speculative access to it under any circumstances other
 ????? than under the control of the device driver using the region.


But I will add this indication in the commit message.


>
> Reviewed-by: Simon Glass <sjg@chromium.org>


Thanks,

Patrick

[PATCH v3 6/7] stm32mp: Increase the reserved memory in board_get_usable_ram_top

[Patrick Delaunay]

Add 8M for the U-Boot reserved memory (display, fdt, gd, ...).

Without this patch the device tree, located before the MALLOC area
is not tagged cacheable just after relocation, before mmu reconfiguration.

This patch reduces the duration for device tree parsing in
lmb_init_and_reserve.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

Changes in v3:
- NEW: solve performance issue as relocated DT is not marked cacheable

 arch/arm/mach-stm32mp/dram_init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-stm32mp/dram_init.c b/arch/arm/mach-stm32mp/dram_init.c
index 66e81bacca..9fb3ade82e 100644
--- a/arch/arm/mach-stm32mp/dram_init.c
+++ b/arch/arm/mach-stm32mp/dram_init.c
@@ -50,7 +50,8 @@ ulong board_get_usable_ram_top(ulong total_size)
 	lmb_init(&lmb);
 	lmb_add(&lmb, gd->ram_base, gd->ram_size);
 	boot_fdt_add_mem_rsv_regions(&lmb, (void *)gd->fdt_blob);
-	size = ALIGN(CONFIG_SYS_MALLOC_LEN + total_size, MMU_SECTION_SIZE),
+	/* add 8M for reserved memory for display, fdt, gd,... */
+	size = ALIGN(SZ_8M + CONFIG_SYS_MALLOC_LEN + total_size, MMU_SECTION_SIZE),
 	reg = lmb_alloc(&lmb, size, MMU_SECTION_SIZE);
 
 	if (!reg)
-- 
2.17.1

[PATCH v3 7/7] stm32mp: don't map the reserved region with no-map property

[Patrick Delaunay]

No more map the reserved region with "no-map" property by marking
the corresponding TLB entries with invalid entry (=0) to avoid
speculative access.

The device tree parsing done in lmb_init_and_reserve() takes a
long time when it is executed without data cache, so it is called in
enable_caches() before to disable it.

This patch fixes an issue where predictive read access on secure DDR
OP-TEE reserved area are caught by firewall.

Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
---

Changes in v3:
- call lmb_init_and_reserve when data cache is activated in enable_caches()
- drop v2 patch "arm: cache: cp15: don't map the reserved region
  with no-map property"

Changes in v2:
- NEW: update in stm32mp specific MMU setup functions

 arch/arm/mach-stm32mp/cpu.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index 8115d58b19..592bfd413d 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -12,6 +12,7 @@
 #include <env.h>
 #include <init.h>
 #include <log.h>
+#include <lmb.h>
 #include <misc.h>
 #include <net.h>
 #include <asm/io.h>
@@ -90,6 +91,8 @@
  */
 u8 early_tlb[PGTABLE_SIZE] __section(".data") __aligned(0x4000);
 
+struct lmb lmb;
+
 #if !defined(CONFIG_SPL) || defined(CONFIG_SPL_BUILD)
 #ifndef CONFIG_TFABOOT
 static void security_init(void)
@@ -221,6 +224,8 @@ void dram_bank_mmu_setup(int bank)
 	int	i;
 	phys_addr_t start;
 	phys_size_t size;
+	bool use_lmb = false;
+	enum dcache_option option;
 
 	if (IS_ENABLED(CONFIG_SPL_BUILD)) {
 		start = ALIGN_DOWN(STM32_SYSRAM_BASE, MMU_SECTION_SIZE);
@@ -229,6 +234,7 @@ void dram_bank_mmu_setup(int bank)
 		/* bd->bi_dram is available only after relocation */
 		start = bd->bi_dram[bank].start;
 		size =  bd->bi_dram[bank].size;
+		use_lmb = true;
 	} else {
 		/* mark cacheable and executable the beggining of the DDR */
 		start = STM32_DDR_BASE;
@@ -237,8 +243,12 @@ void dram_bank_mmu_setup(int bank)
 
 	for (i = start >> MMU_SECTION_SHIFT;
 	     i < (start >> MMU_SECTION_SHIFT) + (size >> MMU_SECTION_SHIFT);
-	     i++)
-		set_section_dcache(i, DCACHE_DEFAULT_OPTION);
+	     i++) {
+		option = DCACHE_DEFAULT_OPTION;
+		if (use_lmb && lmb_is_reserved_flags(&lmb, i << MMU_SECTION_SHIFT, LMB_NOMAP))
+			option = 0; /* INVALID ENTRY in TLB */
+		set_section_dcache(i, option);
+	}
 }
 /*
  * initialize the MMU and activate cache in SPL or in U-Boot pre-reloc stage
@@ -302,6 +312,9 @@ int arch_cpu_init(void)
 
 void enable_caches(void)
 {
+	/* parse device tree when data cache is still activated */
+	lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
+
 	/* I-cache is already enabled in start.S: icache_enable() not needed */
 
 	/* deactivate the data cache, early enabled in arch_cpu_init() */
-- 
2.17.1