[PATCH v1 0/6] Fix page permission on arm64 architectures

[PATCH v1 0/6] Fix page permission on arm64 architectures

[Ilias Apalodimas]

Hi!
This is v1 of [0].

This is an attempt to map the U-Boot binary properly, but leave the
area we load binaries unaffected and RWX.

There's a few changes since the RFC
- Fixed the alignment of meminfo command when printing regions
- 'meminfo' now prints arch specific attributes e.g PXN, UXN etc for arm
  instead of RW, RO, RX
- Since we don't set the permissions of EFI runtime services yet and keep
  them as RWX, I removed the linker alignment changes which makes patch #3
  easier to review. It's worth noting that qemu-arm sbsa was crashing with
  the efi services page aligned. This is probably due to a mismatch of
  memory, since the crash is only reproducible with QEMU instances that
  have < 2 GB of RAM. I'll fix that along with the efi runtime services
- Defined memory attribute changes properly with an enum for RW, RO, RX
  instead of the hardcoded '1,2,3' I had on the RFC
- Enabling mappings is now under a Kconfig (CONFIG_MMU_PGPROT), since
  peope reported crashes when testing this, which are orthogonal to this
  patch. We still have places in U-Boot where we define and later write
  const variables. This will lead to a crash now as const variables are
  properly managed and places in RO memory
- Split patches to be easier to review
- Added a patch updating 'meminfo'
- Picked up acked-by tags from Jerome

patch #1 adds printing capabilities for page mappings in 'meminfo'
patch #2 adds documention in 'meminfo' command
patch #3 prepares linker scripts, aligns sections in page boundaries etc
patch #4 prepares an internal function to change the PTEs
patch #5 adds function definitions & stubs for all archs
patch #6 wires up the changes in U-Boot after it relocates

[0] https://lore.kernel.org/u-boot/20250130072100.27297-1-ilias.apalodimas@linaro.org/

Ilias Apalodimas (6):
  meminfo: add memory details for armv8
  doc: update meminfo with arch specific information
  arm: Prepare linker scripts for memory permissions
  arm64: mmu_change_region_attr() add an option not to break PTEs
  treewide: Add a function to change page permissions
  arm64: Enable RW, RX and RO mappings for the relocated binary

 arch/arc/lib/cache.c                    |  2 +
 arch/arm/cpu/arm926ejs/cache.c          |  2 +
 arch/arm/cpu/armv7/cache_v7.c           |  1 +
 arch/arm/cpu/armv7m/cache.c             |  2 +
 arch/arm/cpu/armv8/cache_v8.c           | 54 +++++++++++++++++--
 arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 10 ++--
 arch/arm/cpu/armv8/u-boot.lds           | 29 +++++-----
 arch/arm/include/asm/armv8/mmu.h        |  2 +
 arch/arm/include/asm/system.h           | 11 +++-
 arch/arm/lib/cache.c                    |  2 +
 arch/arm/mach-snapdragon/board.c        |  2 +-
 arch/m68k/lib/cache.c                   |  2 +
 arch/nios2/lib/cache.c                  |  2 +
 arch/powerpc/lib/cache.c                |  2 +
 arch/riscv/lib/cache.c                  |  2 +
 arch/sh/cpu/sh4/cache.c                 |  2 +
 arch/xtensa/lib/cache.c                 |  2 +
 cmd/meminfo.c                           |  5 ++
 common/Kconfig                          | 13 +++++
 common/board_r.c                        | 20 +++++++
 doc/usage/cmd/meminfo.rst               | 71 ++++++++++++++++++-------
 include/asm-generic/sections.h          |  2 +
 include/cpu_func.h                      | 16 ++++++
 23 files changed, 215 insertions(+), 41 deletions(-)

--
2.47.2

[PATCH v1 1/6] meminfo: add memory details for armv8

[Ilias Apalodimas]

Upcoming patches are mapping memory with RO, RW^X etc permsissions.
Fix the meminfo command to display them properly

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 arch/arm/cpu/armv8/cache_v8.c    | 26 +++++++++++++++++++++++---
 arch/arm/include/asm/armv8/mmu.h |  2 ++
 cmd/meminfo.c                    |  5 +++++
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
index 5d6953ffedd1..c4b3da4a8da7 100644
--- a/arch/arm/cpu/armv8/cache_v8.c
+++ b/arch/arm/cpu/armv8/cache_v8.c
@@ -421,7 +421,7 @@ static int count_ranges(void)
 	return count;
 }
 
-#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK)
+#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK)
 #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3)
 
 enum walker_state {
@@ -568,6 +568,20 @@ static void pretty_print_table_attrs(u64 pte)
 static void pretty_print_block_attrs(u64 pte)
 {
 	u64 attrs = pte & PMD_ATTRINDX_MASK;
+	u64 perm_attrs = pte & PMD_ATTRMASK;
+	char mem_attrs[16] = { 0 };
+	int cnt = 0;
+
+	if (perm_attrs & PTE_BLOCK_PXN)
+		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "PXN ");
+	if (perm_attrs & PTE_BLOCK_UXN)
+		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "UXN ");
+	if (perm_attrs & PTE_BLOCK_RO)
+		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "RO");
+	if (!mem_attrs[0])
+		snprintf(mem_attrs, sizeof(mem_attrs), "RWX ");
+
+	printf(" | %-10s", mem_attrs);
 
 	switch (attrs) {
 	case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE):
@@ -613,6 +627,7 @@ static void print_pte(u64 pte, int level)
 {
 	if (PTE_IS_TABLE(pte, level)) {
 		printf(" %-5s", "Table");
+		printf(" %-12s", "|");
 		pretty_print_table_attrs(pte);
 	} else {
 		pretty_print_pte_type(pte);
@@ -642,9 +657,9 @@ static bool pagetable_print_entry(u64 start_attrs, u64 end, int va_bits, int lev
 
 	printf("%*s", indent * 2, "");
 	if (PTE_IS_TABLE(start_attrs, level))
-		printf("[%#011llx]%14s", _addr, "");
+		printf("[%#016llx]%19s", _addr, "");
 	else
-		printf("[%#011llx - %#011llx]", _addr, end);
+		printf("[%#016llx - %#016llx]", _addr, end);
 
 	printf("%*s | ", (3 - level) * 2, "");
 	print_pte(start_attrs, level);
@@ -1112,3 +1127,8 @@ void __weak enable_caches(void)
 	icache_enable();
 	dcache_enable();
 }
+
+void arch_dump_mem_attrs(void)
+{
+	dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL));
+}
diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h
index 0ab681c893d3..6af8cd111a44 100644
--- a/arch/arm/include/asm/armv8/mmu.h
+++ b/arch/arm/include/asm/armv8/mmu.h
@@ -66,6 +66,7 @@
 #define PTE_BLOCK_NG		(1 << 11)
 #define PTE_BLOCK_PXN		(UL(1) << 53)
 #define PTE_BLOCK_UXN		(UL(1) << 54)
+#define PTE_BLOCK_RO            (UL(1) << 7)
 
 /*
  * AttrIndx[2:0]
@@ -75,6 +76,7 @@
 #define PMD_ATTRMASK		(PTE_BLOCK_PXN		| \
 				 PTE_BLOCK_UXN		| \
 				 PMD_ATTRINDX_MASK	| \
+				 PTE_BLOCK_RO		| \
 				 PTE_TYPE_VALID)
 
 /*
diff --git a/cmd/meminfo.c b/cmd/meminfo.c
index 5e83d61c2dd3..3915e2bbb268 100644
--- a/cmd/meminfo.c
+++ b/cmd/meminfo.c
@@ -15,6 +15,10 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+void __weak arch_dump_mem_attrs(void)
+{
+}
+
 static void print_region(const char *name, ulong base, ulong size, ulong *uptop)
 {
 	ulong end = base + size;
@@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	puts("DRAM:  ");
 	print_size(gd->ram_size, "\n");
+	arch_dump_mem_attrs();
 
 	if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP))
 		return 0;
-- 
2.47.2

[PATCH v1 2/6] doc: update meminfo with arch specific information

[Ilias Apalodimas]

Since we added support in meminfo to dump live page tables, describe
the only working architecture for now (aarch64) and add links to public
documentation for further reading.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 doc/usage/cmd/meminfo.rst | 71 +++++++++++++++++++++++++++++----------
 1 file changed, 53 insertions(+), 18 deletions(-)

diff --git a/doc/usage/cmd/meminfo.rst b/doc/usage/cmd/meminfo.rst
index 6c94493cccc6..cf123f653059 100644
--- a/doc/usage/cmd/meminfo.rst
+++ b/doc/usage/cmd/meminfo.rst
@@ -18,7 +18,8 @@ Description
 
 The meminfo command shows the amount of memory. If ``CONFIG_CMD_MEMINFO_MAP`` is
 enabled, then it also shows the layout of memory used by U-Boot and the region
-which is free for use by images.
+which is free for use by images. In architectures that support it, it also prints
+the mapped pages and their permissions. The latter is architecture specific.
 
 The layout of memory is set up before relocation, within the init sequence in
 ``board_init_f()``, specifically the various ``reserve_...()`` functions. This
@@ -26,8 +27,9 @@ The layout of memory is set up before relocation, within the init sequence in
 ending with the stack. This results in the maximum possible amount of memory
 being left free for image-loading.
 
-The meminfo command writes the DRAM size, then the rest of its outputs in 5
-columns:
+The meminfo command writes the DRAM size. If the architecture supports it
+(currently only aarch64) dumps the page table entries and then the rest of
+its outputs in 5 columns:
 
 Region
    Name of the region
@@ -99,28 +101,61 @@ free
     Free memory, which is available for loading images. The base address of
     this is ``gd->ram_base`` which is generally set by ``CFG_SYS_SDRAM_BASE``.
 
+Aarch64 specific flags
+----------------------
+
+More information on the output can be found
+Chapter D8 - The AArch64 Virtual Memory System Architecture at
+https://developer.arm.com/documentation/ddi0487/latest/
+
+In short, for a stage 1 translation regime the following apply:
+
+* RWX: Pages mapped with Read, Write and Execute permissions
+* RO:  Pages mapped with Read-Only permissions
+* PXN: PXN (Privileged Execute Never) applies to execution at EL1 and above
+* UXN: UXN (Unprivileged Execute Never) applies to EL0
+
 Example
 -------
 
 This example shows output with both ``CONFIG_CMD_MEMINFO`` and
-``CONFIG_CMD_MEMINFO_MAP`` enabled::
-
-    => meminfo
-    DRAM:  256 MiB
+``CONFIG_CMD_MEMINFO_MAP`` enabled for aarch64 qemu::
+
+    DRAM:  8 GiB
+    Walking pagetable at 000000023ffe0000, va_bits: 40. Using 4 levels
+    [0x0000023ffe1000]                          |  Table |            |               |
+      [0x0000023ffe2000]                        |  Table |            |               |
+        [0x00000000000000 - 0x00000008000000]   |  Block | RWX        | Normal        | Inner-shareable
+        [0x00000008000000 - 0x00000040000000]   |  Block | PXN UXN    | Device-nGnRnE | Non-shareable
+      [0x00000040000000 - 0x00000200000000]     |  Block | RWX        | Normal        | Inner-shareable
+      [0x0000023ffea000]                        |  Table |            |               |
+        [0x00000200000000 - 0x0000023f600000]   |  Block | RWX        | Normal        | Inner-shareable
+        [0x0000023ffeb000]                      |  Table |            |               |
+          [0x0000023f600000 - 0x0000023f68c000] |  Pages | RWX        | Normal        | Inner-shareable
+          [0x0000023f68c000 - 0x0000023f74f000] |  Pages | RO         | Normal        | Inner-shareable
+          [0x0000023f74f000 - 0x0000023f794000] |  Pages | PXN UXN RO | Normal        | Inner-shareable
+          [0x0000023f794000 - 0x0000023f79d000] |  Pages | PXN UXN    | Normal        | Inner-shareable
+          [0x0000023f79d000 - 0x0000023f800000] |  Pages | RWX        | Normal        | Inner-shareable
+        [0x0000023f800000 - 0x00000240000000]   |  Block | RWX        | Normal        | Inner-shareable
+      [0x00000240000000 - 0x00004000000000]     |  Block | RWX        | Normal        | Inner-shareable
+      [0x0000023ffe3000]                        |  Table |            |               |
+        [0x00004010000000 - 0x00004020000000]   |  Block | PXN UXN    | Device-nGnRnE | Non-shareable
+    [0x0000023ffe4000]                          |  Table |            |               |
+      [0x00008000000000 - 0x00010000000000]     |  Block | PXN UXN    | Device-nGnRnE | Non-shareable
 
     Region           Base     Size      End      Gap
     ------------------------------------------------
-    video         f000000  1000000 10000000
-    code          ec3a000   3c5d28  efffd28      2d8
-    malloc        8c38000  6002000  ec3a000        0
-    board_info    8c37f90       68  8c37ff8        8
-    global_data   8c37d80      208  8c37f88        8
-    devicetree    8c33000     4d7d  8c37d7d        3
-    bootstage     8c32c20      3c8  8c32fe8       18
-    bloblist      8c32000      400  8c32400      820
-    stack         7c31ff0  1000000  8c31ff0       10
-    free                0  7c31ff0  7c31ff0        0
-
+    video        23f7e0000   800000 23ffe0000
+    code         23f68a000   156000 23f7e0000        0
+    malloc       23e64a000  1040000 23f68a000        0
+    board_info   23e649f80       78 23e649ff8        8
+    global_data  23e649df0      188 23e649f78        8
+    devicetree   23e549df0   100000 23e649df0        0
+    bloblist     23e547000     2000 23e549000      df0
+    stack        23d546ff0  1000000 23e546ff0       10
+    lmb          23d546ff0        0 23d546ff0        0
+    lmb          23d543000     3ff0 23d546ff0        0
+    free         40000000 23d543000 27d543000 ffffffffc0000000
 
 Return value
 ------------
-- 
2.47.2

[PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Ilias Apalodimas]

Upcoming patches are switching the memory mappings to RW, RO, RX
after the U-Boot binary and its data are relocated. Add
annotations in the linker scripts to and mark text, data, rodata
sections and align them to a page boundary.

It's worth noting that efi_runtime relocations are left untouched for
now. The efi runtime regions can be relocated by the OS when the latter
is calling SetVirtualAddressMap. Which means we have to configure the
pages as RX for U-Boot but convert them to RWX just before
ExitBootServices. It also needs extra code in efi_tuntime relocation
code since R_AARCH64_NONE are emitted as well if we page align the
section. Keep it out for now and we can fix it in future patches.

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 arch/arm/cpu/armv8/u-boot.lds  | 29 +++++++++++++++++------------
 include/asm-generic/sections.h |  2 ++
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
index 857f44412e07..35afc3cbe7ec 100644
--- a/arch/arm/cpu/armv8/u-boot.lds
+++ b/arch/arm/cpu/armv8/u-boot.lds
@@ -22,7 +22,7 @@ SECTIONS
 
 	. = ALIGN(8);
 	__image_copy_start = ADDR(.text);
-	.text :
+	.text ALIGN(4096):
 	{
 		CPUDIR/start.o (.text*)
 	}
@@ -36,9 +36,12 @@ SECTIONS
                 __efi_runtime_stop = .;
 	}
 
-	.text_rest :
+	.text_rest ALIGN(4096) :
 	{
+		__text_start = .;
 		*(.text*)
+		. = ALIGN(4096);
+		__text_end = .;
 	}
 
 #ifdef CONFIG_ARMV8_PSCI
@@ -98,18 +101,20 @@ SECTIONS
 	}
 #endif
 
-	. = ALIGN(8);
-	.rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }
+	.rodata ALIGN(4096): {
+	    __start_rodata = .;
+	    *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*)))
+	    . = ALIGN(4096);
+	    __end_rodata = .;
+	}
 
-	. = ALIGN(8);
-	.data : {
+	.data ALIGN(4096) : {
+	    __start_data = .;
 		*(.data*)
+	    . = ALIGN(4096);
+	    __end_data = .;
 	}
 
-	. = ALIGN(8);
-
-	. = .;
-
 	. = ALIGN(8);
 	__u_boot_list : {
 		KEEP(*(SORT(__u_boot_list*)));
@@ -136,10 +141,10 @@ SECTIONS
 	/*
 	 * arch/arm/lib/crt0_64.S assumes __bss_start - __bss_end % 8 == 0
 	 */
-	.bss ALIGN(8) : {
+	.bss ALIGN(4096) : {
 		__bss_start = .;
 		*(.bss*)
-		. = ALIGN(8);
+		. = ALIGN(4096);
 		__bss_end = .;
 	}
 
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 3fd5c772a1af..024b1adde270 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -23,6 +23,7 @@ extern char __kprobes_text_start[], __kprobes_text_end[];
 extern char __entry_text_start[], __entry_text_end[];
 extern char __initdata_begin[], __initdata_end[];
 extern char __start_rodata[], __end_rodata[];
+extern char __start_data[], __end_data[];
 extern char __efi_helloworld_begin[];
 extern char __efi_helloworld_end[];
 extern char __efi_var_file_begin[];
@@ -63,6 +64,7 @@ static inline int arch_is_kernel_data(unsigned long addr)
 
 /* Start of U-Boot text region */
 extern char __text_start[];
+extern char __text_end[];
 
 /* This marks the text region which must be relocated */
 extern char __image_copy_start[], __image_copy_end[];
-- 
2.47.2

[PATCH v1 4/6] arm64: mmu_change_region_attr() add an option not to break PTEs

[Ilias Apalodimas]

The ARM ARM on section 8.17.1 describes the cases where
break-before-make is required when changing live page tables.
Since we can use this function to tweak block and page permssions,
where BBM is not required add an extra argument to the function.

While at it add a function description.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 arch/arm/cpu/armv8/cache_v8.c           |  6 +++++-
 arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 10 +++++-----
 arch/arm/include/asm/system.h           | 11 ++++++++++-
 arch/arm/mach-snapdragon/board.c        |  2 +-
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
index c4b3da4a8da7..670379e17b7a 100644
--- a/arch/arm/cpu/armv8/cache_v8.c
+++ b/arch/arm/cpu/armv8/cache_v8.c
@@ -972,11 +972,14 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size,
  * The procecess is break-before-make. The target region will be marked as
  * invalid during the process of changing.
  */
-void mmu_change_region_attr(phys_addr_t addr, size_t siz, u64 attrs)
+void mmu_change_region_attr(phys_addr_t addr, size_t siz, u64 attrs, bool bbm)
 {
 	int level;
 	u64 r, size, start;
 
+	if (!bbm)
+		goto skip_break;
+
 	start = addr;
 	size = siz;
 	/*
@@ -1001,6 +1004,7 @@ void mmu_change_region_attr(phys_addr_t addr, size_t siz, u64 attrs)
 			   gd->arch.tlb_addr + gd->arch.tlb_size);
 	__asm_invalidate_tlb_all();
 
+skip_break:
 	/*
 	 * Loop through the address range until we find a page granule that fits
 	 * our alignment constraints, then set it to the new cache attributes
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
index d2d3e346a36f..caf1dab05936 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
@@ -1573,7 +1573,7 @@ void update_early_mmu_table(void)
 					PTE_BLOCK_MEMTYPE(MT_NORMAL)	|
 					PTE_BLOCK_OUTER_SHARE		|
 					PTE_BLOCK_NS			|
-					PTE_TYPE_VALID);
+					PTE_TYPE_VALID, true);
 	} else {
 		mmu_change_region_attr(
 					CFG_SYS_SDRAM_BASE,
@@ -1581,7 +1581,7 @@ void update_early_mmu_table(void)
 					PTE_BLOCK_MEMTYPE(MT_NORMAL)	|
 					PTE_BLOCK_OUTER_SHARE		|
 					PTE_BLOCK_NS			|
-					PTE_TYPE_VALID);
+					PTE_TYPE_VALID, true);
 #ifdef CONFIG_SYS_DDR_BLOCK3_BASE
 #ifndef CONFIG_SYS_DDR_BLOCK2_SIZE
 #error "Missing CONFIG_SYS_DDR_BLOCK2_SIZE"
@@ -1594,7 +1594,7 @@ void update_early_mmu_table(void)
 					PTE_BLOCK_MEMTYPE(MT_NORMAL)	|
 					PTE_BLOCK_OUTER_SHARE		|
 					PTE_BLOCK_NS			|
-					PTE_TYPE_VALID);
+					PTE_TYPE_VALID, true);
 			mmu_change_region_attr(
 					CONFIG_SYS_DDR_BLOCK3_BASE,
 					gd->ram_size -
@@ -1603,7 +1603,7 @@ void update_early_mmu_table(void)
 					PTE_BLOCK_MEMTYPE(MT_NORMAL)	|
 					PTE_BLOCK_OUTER_SHARE		|
 					PTE_BLOCK_NS			|
-					PTE_TYPE_VALID);
+					PTE_TYPE_VALID, true);
 		} else
 #endif
 		{
@@ -1614,7 +1614,7 @@ void update_early_mmu_table(void)
 					PTE_BLOCK_MEMTYPE(MT_NORMAL)	|
 					PTE_BLOCK_OUTER_SHARE		|
 					PTE_BLOCK_NS			|
-					PTE_TYPE_VALID);
+					PTE_TYPE_VALID, true);
 		}
 	}
 }
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index dbf9ab43e280..f84bffe9d4fb 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -287,7 +287,16 @@ void flush_l3_cache(void);
  * @emerg: Also map the region in the emergency table
  */
 void mmu_map_region(phys_addr_t start, u64 size, bool emerg);
-void mmu_change_region_attr(phys_addr_t start, size_t size, u64 attrs);
+
+/**
+ * mmu_change_region_attr() - change a mapped region attributes
+ *
+ * @start: Start address of the region
+ * @size:  Size of the region
+ * @aatrs: New attributes
+ * @bbm:   Perform a break-before-make on the page tables entries
+ */
+void mmu_change_region_attr(phys_addr_t start, size_t size, u64 attrs, bool bbm);
 
 /*
  * smc_call() - issue a secure monitor call
diff --git a/arch/arm/mach-snapdragon/board.c b/arch/arm/mach-snapdragon/board.c
index 2ef936aab757..13f4e8e640ef 100644
--- a/arch/arm/mach-snapdragon/board.c
+++ b/arch/arm/mach-snapdragon/board.c
@@ -577,7 +577,7 @@ static void carve_out_reserved_memory(void)
 		if (i == count || start + size < res[i].start - SZ_2M) {
 			debug("  0x%016llx - 0x%016llx: reserved\n",
 			      start, start + size);
-			mmu_change_region_attr(start, size, PTE_TYPE_FAULT);
+			mmu_change_region_attr(start, size, PTE_TYPE_FAULT, true);
 			/* If this is the final region then quit here before we index
 			 * out of bounds...
 			 */
-- 
2.47.2

[PATCH v1 5/6] treewide: Add a function to change page permissions

[Ilias Apalodimas]

For armv8 we are adding proper page permissions for the relocated U-Boot
binary. Add a weak function that can be used across architectures to change
the page permissions

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 arch/arc/lib/cache.c           |  2 ++
 arch/arm/cpu/arm926ejs/cache.c |  2 ++
 arch/arm/cpu/armv7/cache_v7.c  |  1 +
 arch/arm/cpu/armv7m/cache.c    |  2 ++
 arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
 arch/arm/lib/cache.c           |  2 ++
 arch/m68k/lib/cache.c          |  2 ++
 arch/nios2/lib/cache.c         |  2 ++
 arch/powerpc/lib/cache.c       |  2 ++
 arch/riscv/lib/cache.c         |  2 ++
 arch/sh/cpu/sh4/cache.c        |  2 ++
 arch/xtensa/lib/cache.c        |  2 ++
 include/cpu_func.h             | 16 ++++++++++++++++
 13 files changed, 59 insertions(+)

diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
index 5169fc627fa5..5c79243d7223 100644
--- a/arch/arc/lib/cache.c
+++ b/arch/arc/lib/cache.c
@@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
 
 	__ic_entire_invalidate();
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
index 5b87a3af91b2..857311b3dfad 100644
--- a/arch/arm/cpu/arm926ejs/cache.c
+++ b/arch/arm/cpu/arm926ejs/cache.c
@@ -88,3 +88,5 @@ void enable_caches(void)
 	dcache_enable();
 #endif
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
index d11420d2fdd0..14c9be77db8d 100644
--- a/arch/arm/cpu/armv7/cache_v7.c
+++ b/arch/arm/cpu/armv7/cache_v7.c
@@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
 __weak void v7_outer_cache_inval_all(void) {}
 __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
 __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
+__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
index b6d08b7aad73..458a214e9577 100644
--- a/arch/arm/cpu/armv7m/cache.c
+++ b/arch/arm/cpu/armv7m/cache.c
@@ -370,3 +370,5 @@ void enable_caches(void)
 	dcache_enable();
 #endif
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
index 670379e17b7a..1cf3870177ee 100644
--- a/arch/arm/cpu/armv8/cache_v8.c
+++ b/arch/arm/cpu/armv8/cache_v8.c
@@ -1028,6 +1028,28 @@ skip_break:
 	__asm_invalidate_tlb_all();
 }
 
+void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
+{
+	u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
+
+	switch (perm) {
+	case MMU_ATTR_RO:
+		attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
+		break;
+	case MMU_ATTR_RX:
+		attrs |= PTE_BLOCK_RO;
+		break;
+	case MMU_ATTR_RW:
+		attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
+		break;
+	default:
+		log_err("Unknown attribute %llx\n", perm);
+		return;
+	}
+
+	mmu_change_region_attr(addr, size, attrs, false);
+}
+
 #else	/* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
 
 /*
diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
index 516754caeaf9..c7704d8ee354 100644
--- a/arch/arm/lib/cache.c
+++ b/arch/arm/lib/cache.c
@@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
 
 	return 0;
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/m68k/lib/cache.c b/arch/m68k/lib/cache.c
index 370ad40f1423..b275646384a5 100644
--- a/arch/m68k/lib/cache.c
+++ b/arch/m68k/lib/cache.c
@@ -151,3 +151,5 @@ __weak void flush_dcache_range(unsigned long start, unsigned long stop)
 {
 	/* An empty stub, real implementation should be in platform code */
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/nios2/lib/cache.c b/arch/nios2/lib/cache.c
index 8f543f2a2f26..7a93a8fcc6a7 100644
--- a/arch/nios2/lib/cache.c
+++ b/arch/nios2/lib/cache.c
@@ -127,3 +127,5 @@ void dcache_disable(void)
 {
 	flush_dcache_all();
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/powerpc/lib/cache.c b/arch/powerpc/lib/cache.c
index a9cd7b8d30ac..3d0536caccde 100644
--- a/arch/powerpc/lib/cache.c
+++ b/arch/powerpc/lib/cache.c
@@ -58,3 +58,5 @@ void invalidate_icache_all(void)
 {
 	puts("No arch specific invalidate_icache_all available!\n");
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
index 71e4937ab542..1c751e562157 100644
--- a/arch/riscv/lib/cache.c
+++ b/arch/riscv/lib/cache.c
@@ -151,3 +151,5 @@ __weak void enable_caches(void)
 	if (!zicbom_block_size)
 		log_debug("Zicbom not initialized.\n");
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/sh/cpu/sh4/cache.c b/arch/sh/cpu/sh4/cache.c
index 99acc5999652..22e0f1484a33 100644
--- a/arch/sh/cpu/sh4/cache.c
+++ b/arch/sh/cpu/sh4/cache.c
@@ -126,3 +126,5 @@ int dcache_status(void)
 {
 	return 0;
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/arch/xtensa/lib/cache.c b/arch/xtensa/lib/cache.c
index e6a7f6827fc2..aacc2d2627d6 100644
--- a/arch/xtensa/lib/cache.c
+++ b/arch/xtensa/lib/cache.c
@@ -57,3 +57,5 @@ void invalidate_icache_all(void)
 {
 	__invalidate_icache_all();
 }
+
+void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
diff --git a/include/cpu_func.h b/include/cpu_func.h
index 7e81c4364a73..17b6d199302a 100644
--- a/include/cpu_func.h
+++ b/include/cpu_func.h
@@ -69,6 +69,22 @@ void flush_dcache_range(unsigned long start, unsigned long stop);
 void invalidate_dcache_range(unsigned long start, unsigned long stop);
 void invalidate_dcache_all(void);
 void invalidate_icache_all(void);
+
+enum pgprot_attrs {
+	MMU_ATTR_RO,
+	MMU_ATTR_RX,
+	MMU_ATTR_RW,
+};
+
+/** pgprot_set_attrs() - Set page table permissions
+ *
+ * @addr: Physical address start
+ * @size: size of memory to change
+ * @perm: New permissions
+ *
+ **/
+void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm);
+
 /**
  * noncached_init() - Initialize non-cached memory region
  *
-- 
2.47.2

[PATCH v1 6/6] arm64: Enable RW, RX and RO mappings for the relocated binary

[Ilias Apalodimas]

Now that we have everything in place switch the page permissions for
.rodata, .text and .data just after we relocate everything in top of the
RAM.

Unfortunately we can't enable this by default, since we have examples of
U-Boot crashing due to invalid access. This usually happens because code
defines const variables that it later writes. So hide it behind a Kconfig
option until we sort it out.

It's worth noting that EFI runtime services are not covered by this
patch on purpose. Since the OS can call SetVirtualAddressMap which can
relocate runtime services, we need to set them to RX initially but remap
them as RWX right before ExitBootServices.

Link: https://lore.kernel.org/u-boot/20250129-rockchip-pinctrl-const-v1-0-450ccdadfa7e@cherry.de/
Link: https://lore.kernel.org/u-boot/20250130133646.2177194-1-andre.przywara@arm.com/
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 common/Kconfig   | 13 +++++++++++++
 common/board_r.c | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/common/Kconfig b/common/Kconfig
index 7685914fa6fd..dbae7e062b0a 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -914,6 +914,19 @@ config STACKPROTECTOR
 	  Enable stack smash detection through compiler's stack-protector
 	  canary logic
 
+config MMU_PGPROT
+	bool "Enable RO, RW and RX mappings"
+	help
+	  U-Boot maps all pages as RWX. If selected pages will
+	  be marked as RO(.rodata), RX(.text), RW(.data) right after
+	  we relocate. Since code sections needs to be page aligned
+	  the final binary size will increase.
+	  The mapping can be dumped using the 'meminfo' command.
+	  We should make this default 'y' in the future, but currently
+	  we have code defining const variables that are later written.
+	  Enabling this will crash U-Boot if that code path runs, so keep
+	  it off by default until we fix invalid accesses.
+
 config SPL_STACKPROTECTOR
 	bool "Stack Protector buffer overflow detection for SPL"
 	depends on STACKPROTECTOR && SPL
diff --git a/common/board_r.c b/common/board_r.c
index 179259b00de8..c1e9aa46e3fa 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -170,7 +170,27 @@ static int initr_reloc_global_data(void)
 	efi_save_gd();
 
 	efi_runtime_relocate(gd->relocaddr, NULL);
+
 #endif
+	/*
+	 * We are done with all relocations change the permissions of the binary
+	 * NOTE: __start_rodata etc are defined in arm64 linker scripts and
+	 * sections.h. If you want to add support for your platform you need to
+	 * add the symbols on your linker script, otherwise they will point to
+	 * random addresses.
+	 *
+	 */
+	if (IS_ENABLED(CONFIG_MMU_PGPROT)) {
+		pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_rodata),
+				 (phys_addr_t)(uintptr_t)(__end_rodata - __start_rodata),
+				 MMU_ATTR_RO);
+		pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_data),
+				 (phys_addr_t)(uintptr_t)(__end_data - __start_data),
+				 MMU_ATTR_RW);
+		pgprot_set_attrs((phys_addr_t)(uintptr_t)(__text_start),
+				 (phys_addr_t)(uintptr_t)(__text_end - __text_start),
+				 MMU_ATTR_RX);
+	}
 
 	return 0;
 }
-- 
2.47.2

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Ilias Apalodimas]

I'm just replying to myself here but I'll send a v2 when the patches
are reviewed.

I can add the linker under an ifdef, so u-boot size won't change
unless a Kconfig options is selected

/Ilias

On Wed, 5 Feb 2025 at 09:17, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Upcoming patches are switching the memory mappings to RW, RO, RX
> after the U-Boot binary and its data are relocated. Add
> annotations in the linker scripts to and mark text, data, rodata
> sections and align them to a page boundary.
>
> It's worth noting that efi_runtime relocations are left untouched for
> now. The efi runtime regions can be relocated by the OS when the latter
> is calling SetVirtualAddressMap. Which means we have to configure the
> pages as RX for U-Boot but convert them to RWX just before
> ExitBootServices. It also needs extra code in efi_tuntime relocation
> code since R_AARCH64_NONE are emitted as well if we page align the
> section. Keep it out for now and we can fix it in future patches.
>
> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  arch/arm/cpu/armv8/u-boot.lds  | 29 +++++++++++++++++------------
>  include/asm-generic/sections.h |  2 ++
>  2 files changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index 857f44412e07..35afc3cbe7ec 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -22,7 +22,7 @@ SECTIONS
>
>         . = ALIGN(8);
>         __image_copy_start = ADDR(.text);
> -       .text :
> +       .text ALIGN(4096):
>         {
>                 CPUDIR/start.o (.text*)
>         }
> @@ -36,9 +36,12 @@ SECTIONS
>                  __efi_runtime_stop = .;
>         }
>
> -       .text_rest :
> +       .text_rest ALIGN(4096) :
>         {
> +               __text_start = .;
>                 *(.text*)
> +               . = ALIGN(4096);
> +               __text_end = .;
>         }
>
>  #ifdef CONFIG_ARMV8_PSCI
> @@ -98,18 +101,20 @@ SECTIONS
>         }
>  #endif
>
> -       . = ALIGN(8);
> -       .rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }
> +       .rodata ALIGN(4096): {
> +           __start_rodata = .;
> +           *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*)))
> +           . = ALIGN(4096);
> +           __end_rodata = .;
> +       }
>
> -       . = ALIGN(8);
> -       .data : {
> +       .data ALIGN(4096) : {
> +           __start_data = .;
>                 *(.data*)
> +           . = ALIGN(4096);
> +           __end_data = .;
>         }
>
> -       . = ALIGN(8);
> -
> -       . = .;
> -
>         . = ALIGN(8);
>         __u_boot_list : {
>                 KEEP(*(SORT(__u_boot_list*)));
> @@ -136,10 +141,10 @@ SECTIONS
>         /*
>          * arch/arm/lib/crt0_64.S assumes __bss_start - __bss_end % 8 == 0
>          */
> -       .bss ALIGN(8) : {
> +       .bss ALIGN(4096) : {
>                 __bss_start = .;
>                 *(.bss*)
> -               . = ALIGN(8);
> +               . = ALIGN(4096);
>                 __bss_end = .;
>         }
>
> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
> index 3fd5c772a1af..024b1adde270 100644
> --- a/include/asm-generic/sections.h
> +++ b/include/asm-generic/sections.h
> @@ -23,6 +23,7 @@ extern char __kprobes_text_start[], __kprobes_text_end[];
>  extern char __entry_text_start[], __entry_text_end[];
>  extern char __initdata_begin[], __initdata_end[];
>  extern char __start_rodata[], __end_rodata[];
> +extern char __start_data[], __end_data[];
>  extern char __efi_helloworld_begin[];
>  extern char __efi_helloworld_end[];
>  extern char __efi_var_file_begin[];
> @@ -63,6 +64,7 @@ static inline int arch_is_kernel_data(unsigned long addr)
>
>  /* Start of U-Boot text region */
>  extern char __text_start[];
> +extern char __text_end[];
>
>  /* This marks the text region which must be relocated */
>  extern char __image_copy_start[], __image_copy_end[];
> --
> 2.47.2
>

Re: [PATCH v1 1/6] meminfo: add memory details for armv8

[Jerome Forissier]

On 2/5/25 08:16, Ilias Apalodimas wrote:
> Upcoming patches are mapping memory with RO, RW^X etc permsissions.
> Fix the meminfo command to display them properly
> 
> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  arch/arm/cpu/armv8/cache_v8.c    | 26 +++++++++++++++++++++++---
>  arch/arm/include/asm/armv8/mmu.h |  2 ++
>  cmd/meminfo.c                    |  5 +++++
>  3 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> index 5d6953ffedd1..c4b3da4a8da7 100644
> --- a/arch/arm/cpu/armv8/cache_v8.c
> +++ b/arch/arm/cpu/armv8/cache_v8.c
> @@ -421,7 +421,7 @@ static int count_ranges(void)
>  	return count;
>  }
>  
> -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK)
> +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK)
>  #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3)
>  
>  enum walker_state {
> @@ -568,6 +568,20 @@ static void pretty_print_table_attrs(u64 pte)
>  static void pretty_print_block_attrs(u64 pte)
>  {
>  	u64 attrs = pte & PMD_ATTRINDX_MASK;
> +	u64 perm_attrs = pte & PMD_ATTRMASK;
> +	char mem_attrs[16] = { 0 };

11 is enough is seems?

> +	int cnt = 0;
> +
> +	if (perm_attrs & PTE_BLOCK_PXN)
> +		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "PXN ");
> +	if (perm_attrs & PTE_BLOCK_UXN)
> +		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "UXN ");
> +	if (perm_attrs & PTE_BLOCK_RO)
> +		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "RO");
> +	if (!mem_attrs[0])
> +		snprintf(mem_attrs, sizeof(mem_attrs), "RWX ");
> +
> +	printf(" | %-10s", mem_attrs);
>  
>  	switch (attrs) {
>  	case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE):
> @@ -613,6 +627,7 @@ static void print_pte(u64 pte, int level)
>  {
>  	if (PTE_IS_TABLE(pte, level)) {
>  		printf(" %-5s", "Table");
> +		printf(" %-12s", "|");
>  		pretty_print_table_attrs(pte);
>  	} else {
>  		pretty_print_pte_type(pte);
> @@ -642,9 +657,9 @@ static bool pagetable_print_entry(u64 start_attrs, u64 end, int va_bits, int lev
>  
>  	printf("%*s", indent * 2, "");
>  	if (PTE_IS_TABLE(start_attrs, level))
> -		printf("[%#011llx]%14s", _addr, "");
> +		printf("[%#016llx]%19s", _addr, "");
>  	else
> -		printf("[%#011llx - %#011llx]", _addr, end);
> +		printf("[%#016llx - %#016llx]", _addr, end);
>  
>  	printf("%*s | ", (3 - level) * 2, "");
>  	print_pte(start_attrs, level);
> @@ -1112,3 +1127,8 @@ void __weak enable_caches(void)
>  	icache_enable();
>  	dcache_enable();
>  }
> +
> +void arch_dump_mem_attrs(void)
> +{
> +	dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL));
> +}
> diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h
> index 0ab681c893d3..6af8cd111a44 100644
> --- a/arch/arm/include/asm/armv8/mmu.h
> +++ b/arch/arm/include/asm/armv8/mmu.h
> @@ -66,6 +66,7 @@
>  #define PTE_BLOCK_NG		(1 << 11)
>  #define PTE_BLOCK_PXN		(UL(1) << 53)
>  #define PTE_BLOCK_UXN		(UL(1) << 54)
> +#define PTE_BLOCK_RO            (UL(1) << 7)
>  
>  /*
>   * AttrIndx[2:0]
> @@ -75,6 +76,7 @@
>  #define PMD_ATTRMASK		(PTE_BLOCK_PXN		| \
>  				 PTE_BLOCK_UXN		| \
>  				 PMD_ATTRINDX_MASK	| \
> +				 PTE_BLOCK_RO		| \
>  				 PTE_TYPE_VALID)
>  
>  /*
> diff --git a/cmd/meminfo.c b/cmd/meminfo.c
> index 5e83d61c2dd3..3915e2bbb268 100644
> --- a/cmd/meminfo.c
> +++ b/cmd/meminfo.c
> @@ -15,6 +15,10 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> +void __weak arch_dump_mem_attrs(void)
> +{
> +}
> +
>  static void print_region(const char *name, ulong base, ulong size, ulong *uptop)
>  {
>  	ulong end = base + size;
> @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc,
>  
>  	puts("DRAM:  ");
>  	print_size(gd->ram_size, "\n");
> +	arch_dump_mem_attrs();
>  
>  	if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP))
>  		return 0;

Re: [PATCH v1 6/6] arm64: Enable RW, RX and RO mappings for the relocated binary

[Jerome Forissier]

On 2/5/25 08:16, Ilias Apalodimas wrote:
> Now that we have everything in place switch the page permissions for
> .rodata, .text and .data just after we relocate everything in top of the
> RAM.
> 
> Unfortunately we can't enable this by default, since we have examples of
> U-Boot crashing due to invalid access. This usually happens because code
> defines const variables that it later writes. So hide it behind a Kconfig
> option until we sort it out.
> 
> It's worth noting that EFI runtime services are not covered by this
> patch on purpose. Since the OS can call SetVirtualAddressMap which can
> relocate runtime services, we need to set them to RX initially but remap
> them as RWX right before ExitBootServices.
> 
> Link: https://lore.kernel.org/u-boot/20250129-rockchip-pinctrl-const-v1-0-450ccdadfa7e@cherry.de/
> Link: https://lore.kernel.org/u-boot/20250130133646.2177194-1-andre.przywara@arm.com/
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  common/Kconfig   | 13 +++++++++++++
>  common/board_r.c | 20 ++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index 7685914fa6fd..dbae7e062b0a 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -914,6 +914,19 @@ config STACKPROTECTOR
>  	  Enable stack smash detection through compiler's stack-protector
>  	  canary logic
>  
> +config MMU_PGPROT
> +	bool "Enable RO, RW and RX mappings"
> +	help
> +	  U-Boot maps all pages as RWX. If selected pages will
> +	  be marked as RO(.rodata), RX(.text), RW(.data) right after

Space before (

> +	  we relocate. Since code sections needs to be page aligned
> +	  the final binary size will increase.
> +	  The mapping can be dumped using the 'meminfo' command.

OK

> +	  We should make this default 'y' in the future, but currently
> +	  we have code defining const variables that are later written.
> +	  Enabling this will crash U-Boot if that code path runs, so keep
> +	  it off by default until we fix invalid accesses.

Not sure this needs to be documented in the Kconfig help. Perhaps just
keep a patch ready and send it early in the next release cycle for people
to test and debug?

> +
>  config SPL_STACKPROTECTOR
>  	bool "Stack Protector buffer overflow detection for SPL"
>  	depends on STACKPROTECTOR && SPL
> diff --git a/common/board_r.c b/common/board_r.c
> index 179259b00de8..c1e9aa46e3fa 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -170,7 +170,27 @@ static int initr_reloc_global_data(void)
>  	efi_save_gd();
>  
>  	efi_runtime_relocate(gd->relocaddr, NULL);
> +
>  #endif
> +	/*
> +	 * We are done with all relocations change the permissions of the binary
> +	 * NOTE: __start_rodata etc are defined in arm64 linker scripts and
> +	 * sections.h. If you want to add support for your platform you need to
> +	 * add the symbols on your linker script, otherwise they will point to
> +	 * random addresses.
> +	 *
> +	 */
> +	if (IS_ENABLED(CONFIG_MMU_PGPROT)) {
> +		pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_rodata),
> +				 (phys_addr_t)(uintptr_t)(__end_rodata - __start_rodata),
> +				 MMU_ATTR_RO);
> +		pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_data),
> +				 (phys_addr_t)(uintptr_t)(__end_data - __start_data),
> +				 MMU_ATTR_RW);
> +		pgprot_set_attrs((phys_addr_t)(uintptr_t)(__text_start),
> +				 (phys_addr_t)(uintptr_t)(__text_end - __text_start),
> +				 MMU_ATTR_RX);
> +	}
>  
>  	return 0;
>  }

Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Thanks,
-- 
Jerome

Re: [PATCH v1 6/6] arm64: Enable RW, RX and RO mappings for the relocated binary

[Ilias Apalodimas]

Hi Jerome,

On Wed, 5 Feb 2025 at 11:57, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
>
>
> On 2/5/25 08:16, Ilias Apalodimas wrote:
> > Now that we have everything in place switch the page permissions for
> > .rodata, .text and .data just after we relocate everything in top of the
> > RAM.
> >
> > Unfortunately we can't enable this by default, since we have examples of
> > U-Boot crashing due to invalid access. This usually happens because code
> > defines const variables that it later writes. So hide it behind a Kconfig
> > option until we sort it out.
> >
> > It's worth noting that EFI runtime services are not covered by this
> > patch on purpose. Since the OS can call SetVirtualAddressMap which can
> > relocate runtime services, we need to set them to RX initially but remap
> > them as RWX right before ExitBootServices.
> >
> > Link: https://lore.kernel.org/u-boot/20250129-rockchip-pinctrl-const-v1-0-450ccdadfa7e@cherry.de/
> > Link: https://lore.kernel.org/u-boot/20250130133646.2177194-1-andre.przywara@arm.com/
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  common/Kconfig   | 13 +++++++++++++
> >  common/board_r.c | 20 ++++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> >
> > diff --git a/common/Kconfig b/common/Kconfig
> > index 7685914fa6fd..dbae7e062b0a 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -914,6 +914,19 @@ config STACKPROTECTOR
> >         Enable stack smash detection through compiler's stack-protector
> >         canary logic
> >
> > +config MMU_PGPROT
> > +     bool "Enable RO, RW and RX mappings"
> > +     help
> > +       U-Boot maps all pages as RWX. If selected pages will
> > +       be marked as RO(.rodata), RX(.text), RW(.data) right after
>
> Space before (

Sure

>
> > +       we relocate. Since code sections needs to be page aligned
> > +       the final binary size will increase.
> > +       The mapping can be dumped using the 'meminfo' command.
>
> OK
>
> > +       We should make this default 'y' in the future, but currently
> > +       we have code defining const variables that are later written.
> > +       Enabling this will crash U-Boot if that code path runs, so keep
> > +       it off by default until we fix invalid accesses.
>
> Not sure this needs to be documented in the Kconfig help. Perhaps just
> keep a patch ready and send it early in the next release cycle for people
> to test and debug?

I'd like people to see it and try to debug when they see random
crashes. I think it's easier if we document that here until things are
fixed.
OTOH i don't really mind whatever people think it's best

>
> > +
> >  config SPL_STACKPROTECTOR
> >       bool "Stack Protector buffer overflow detection for SPL"
> >       depends on STACKPROTECTOR && SPL
> > diff --git a/common/board_r.c b/common/board_r.c
> > index 179259b00de8..c1e9aa46e3fa 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -170,7 +170,27 @@ static int initr_reloc_global_data(void)
> >       efi_save_gd();
> >
> >       efi_runtime_relocate(gd->relocaddr, NULL);
> > +
> >  #endif
> > +     /*
> > +      * We are done with all relocations change the permissions of the binary
> > +      * NOTE: __start_rodata etc are defined in arm64 linker scripts and
> > +      * sections.h. If you want to add support for your platform you need to
> > +      * add the symbols on your linker script, otherwise they will point to
> > +      * random addresses.
> > +      *
> > +      */
> > +     if (IS_ENABLED(CONFIG_MMU_PGPROT)) {
> > +             pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_rodata),
> > +                              (phys_addr_t)(uintptr_t)(__end_rodata - __start_rodata),
> > +                              MMU_ATTR_RO);
> > +             pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_data),
> > +                              (phys_addr_t)(uintptr_t)(__end_data - __start_data),
> > +                              MMU_ATTR_RW);
> > +             pgprot_set_attrs((phys_addr_t)(uintptr_t)(__text_start),
> > +                              (phys_addr_t)(uintptr_t)(__text_end - __text_start),
> > +                              MMU_ATTR_RX);
> > +     }
> >
> >       return 0;
> >  }
>
> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Thanks!
/Ilias
>
> Thanks,
> --
> Jerome

Re: [PATCH v1 1/6] meminfo: add memory details for armv8

[Ilias Apalodimas]

On Wed, 5 Feb 2025 at 11:27, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
>
>
> On 2/5/25 08:16, Ilias Apalodimas wrote:
> > Upcoming patches are mapping memory with RO, RW^X etc permsissions.
> > Fix the meminfo command to display them properly
> >
> > Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  arch/arm/cpu/armv8/cache_v8.c    | 26 +++++++++++++++++++++++---
> >  arch/arm/include/asm/armv8/mmu.h |  2 ++
> >  cmd/meminfo.c                    |  5 +++++
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > index 5d6953ffedd1..c4b3da4a8da7 100644
> > --- a/arch/arm/cpu/armv8/cache_v8.c
> > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > @@ -421,7 +421,7 @@ static int count_ranges(void)
> >       return count;
> >  }
> >
> > -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK)
> > +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK)
> >  #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3)
> >
> >  enum walker_state {
> > @@ -568,6 +568,20 @@ static void pretty_print_table_attrs(u64 pte)
> >  static void pretty_print_block_attrs(u64 pte)
> >  {
> >       u64 attrs = pte & PMD_ATTRINDX_MASK;
> > +     u64 perm_attrs = pte & PMD_ATTRMASK;
> > +     char mem_attrs[16] = { 0 };
>
> 11 is enough is seems?

I can but OTOH, I prefer defining the arrays aligned. I dont think
anyone will miss 5 bytes!

Cheers
/Ilias
>
> > +     int cnt = 0;
> > +
> > +     if (perm_attrs & PTE_BLOCK_PXN)
> > +             cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "PXN ");
> > +     if (perm_attrs & PTE_BLOCK_UXN)
> > +             cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "UXN ");
> > +     if (perm_attrs & PTE_BLOCK_RO)
> > +             cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "RO");
> > +     if (!mem_attrs[0])
> > +             snprintf(mem_attrs, sizeof(mem_attrs), "RWX ");
> > +
> > +     printf(" | %-10s", mem_attrs);
> >
> >       switch (attrs) {
> >       case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE):
> > @@ -613,6 +627,7 @@ static void print_pte(u64 pte, int level)
> >  {
> >       if (PTE_IS_TABLE(pte, level)) {
> >               printf(" %-5s", "Table");
> > +             printf(" %-12s", "|");
> >               pretty_print_table_attrs(pte);
> >       } else {
> >               pretty_print_pte_type(pte);
> > @@ -642,9 +657,9 @@ static bool pagetable_print_entry(u64 start_attrs, u64 end, int va_bits, int lev
> >
> >       printf("%*s", indent * 2, "");
> >       if (PTE_IS_TABLE(start_attrs, level))
> > -             printf("[%#011llx]%14s", _addr, "");
> > +             printf("[%#016llx]%19s", _addr, "");
> >       else
> > -             printf("[%#011llx - %#011llx]", _addr, end);
> > +             printf("[%#016llx - %#016llx]", _addr, end);
> >
> >       printf("%*s | ", (3 - level) * 2, "");
> >       print_pte(start_attrs, level);
> > @@ -1112,3 +1127,8 @@ void __weak enable_caches(void)
> >       icache_enable();
> >       dcache_enable();
> >  }
> > +
> > +void arch_dump_mem_attrs(void)
> > +{
> > +     dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL));
> > +}
> > diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h
> > index 0ab681c893d3..6af8cd111a44 100644
> > --- a/arch/arm/include/asm/armv8/mmu.h
> > +++ b/arch/arm/include/asm/armv8/mmu.h
> > @@ -66,6 +66,7 @@
> >  #define PTE_BLOCK_NG         (1 << 11)
> >  #define PTE_BLOCK_PXN                (UL(1) << 53)
> >  #define PTE_BLOCK_UXN                (UL(1) << 54)
> > +#define PTE_BLOCK_RO            (UL(1) << 7)
> >
> >  /*
> >   * AttrIndx[2:0]
> > @@ -75,6 +76,7 @@
> >  #define PMD_ATTRMASK         (PTE_BLOCK_PXN          | \
> >                                PTE_BLOCK_UXN          | \
> >                                PMD_ATTRINDX_MASK      | \
> > +                              PTE_BLOCK_RO           | \
> >                                PTE_TYPE_VALID)
> >
> >  /*
> > diff --git a/cmd/meminfo.c b/cmd/meminfo.c
> > index 5e83d61c2dd3..3915e2bbb268 100644
> > --- a/cmd/meminfo.c
> > +++ b/cmd/meminfo.c
> > @@ -15,6 +15,10 @@
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> >
> > +void __weak arch_dump_mem_attrs(void)
> > +{
> > +}
> > +
> >  static void print_region(const char *name, ulong base, ulong size, ulong *uptop)
> >  {
> >       ulong end = base + size;
> > @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc,
> >
> >       puts("DRAM:  ");
> >       print_size(gd->ram_size, "\n");
> > +     arch_dump_mem_attrs();
> >
> >       if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP))
> >               return 0;

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Michal Simek]

Hi,

On 2/5/25 09:22, Ilias Apalodimas wrote:
> I'm just replying to myself here but I'll send a v2 when the patches
> are reviewed.
> 
> I can add the linker under an ifdef, so u-boot size won't change
> unless a Kconfig options is selected

I think you can just do it for alignment not really big ifdef in the file.
Definitely this will be needed for our mini configurations which are running out 
of OCM.
Feel free to look at xilinx_*_mini* configurations.

Thanks,
Michal

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Ilias Apalodimas]

On Wed, 5 Feb 2025 at 14:42, Michal Simek <michal.simek@amd.com> wrote:
>
> Hi,
>
> On 2/5/25 09:22, Ilias Apalodimas wrote:
> > I'm just replying to myself here but I'll send a v2 when the patches
> > are reviewed.
> >
> > I can add the linker under an ifdef, so u-boot size won't change
> > unless a Kconfig options is selected
>
> I think you can just do it for alignment not really big ifdef in the file.

Yes that's what I meant, I can have all the ALIGN(4096) stuff in an ifdef.

Cheers
/Ilias
> Definitely this will be needed for our mini configurations which are running out
> of OCM.
> Feel free to look at xilinx_*_mini* configurations.
>
> Thanks,
> Michal

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Heinrich Schuchardt]

On 2/5/25 08:16, Ilias Apalodimas wrote:
> For armv8 we are adding proper page permissions for the relocated U-Boot
> binary. Add a weak function that can be used across architectures to change
> the page permissions
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   arch/arc/lib/cache.c           |  2 ++
>   arch/arm/cpu/arm926ejs/cache.c |  2 ++
>   arch/arm/cpu/armv7/cache_v7.c  |  1 +
>   arch/arm/cpu/armv7m/cache.c    |  2 ++
>   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
>   arch/arm/lib/cache.c           |  2 ++
>   arch/m68k/lib/cache.c          |  2 ++
>   arch/nios2/lib/cache.c         |  2 ++
>   arch/powerpc/lib/cache.c       |  2 ++
>   arch/riscv/lib/cache.c         |  2 ++
>   arch/sh/cpu/sh4/cache.c        |  2 ++
>   arch/xtensa/lib/cache.c        |  2 ++
>   include/cpu_func.h             | 16 ++++++++++++++++
>   13 files changed, 59 insertions(+)
>
> diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> index 5169fc627fa5..5c79243d7223 100644
> --- a/arch/arc/lib/cache.c
> +++ b/arch/arc/lib/cache.c
> @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
>
>   	__ic_entire_invalidate();
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> index 5b87a3af91b2..857311b3dfad 100644
> --- a/arch/arm/cpu/arm926ejs/cache.c
> +++ b/arch/arm/cpu/arm926ejs/cache.c
> @@ -88,3 +88,5 @@ void enable_caches(void)
>   	dcache_enable();
>   #endif
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> index d11420d2fdd0..14c9be77db8d 100644
> --- a/arch/arm/cpu/armv7/cache_v7.c
> +++ b/arch/arm/cpu/armv7/cache_v7.c
> @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
>   __weak void v7_outer_cache_inval_all(void) {}
>   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
>   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> index b6d08b7aad73..458a214e9577 100644
> --- a/arch/arm/cpu/armv7m/cache.c
> +++ b/arch/arm/cpu/armv7m/cache.c
> @@ -370,3 +370,5 @@ void enable_caches(void)
>   	dcache_enable();
>   #endif
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> index 670379e17b7a..1cf3870177ee 100644
> --- a/arch/arm/cpu/armv8/cache_v8.c
> +++ b/arch/arm/cpu/armv8/cache_v8.c
> @@ -1028,6 +1028,28 @@ skip_break:
>   	__asm_invalidate_tlb_all();
>   }
>
> +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> +{
> +	u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> +
> +	switch (perm) {
> +	case MMU_ATTR_RO:
> +		attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> +		break;
> +	case MMU_ATTR_RX:
> +		attrs |= PTE_BLOCK_RO;
> +		break;
> +	case MMU_ATTR_RW:
> +		attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> +		break;
> +	default:
> +		log_err("Unknown attribute %llx\n", perm);
> +		return;
> +	}
> +
> +	mmu_change_region_attr(addr, size, attrs, false);
> +}
> +
>   #else	/* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
>
>   /*
> diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> index 516754caeaf9..c7704d8ee354 100644
> --- a/arch/arm/lib/cache.c
> +++ b/arch/arm/lib/cache.c
> @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
>
>   	return 0;
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}

I would prefer if the weak function would return -ENOSYS indicating the
missing implementation and the real function would return 0 in case of
success or an error code on failure. This way the EFI protocol could set
the return code if the architecture does not provide support for setting
the attributes the passed addresses are invalid.

Best regards

Heinrich

> diff --git a/arch/m68k/lib/cache.c b/arch/m68k/lib/cache.c
> index 370ad40f1423..b275646384a5 100644
> --- a/arch/m68k/lib/cache.c
> +++ b/arch/m68k/lib/cache.c
> @@ -151,3 +151,5 @@ __weak void flush_dcache_range(unsigned long start, unsigned long stop)
>   {
>   	/* An empty stub, real implementation should be in platform code */
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/nios2/lib/cache.c b/arch/nios2/lib/cache.c
> index 8f543f2a2f26..7a93a8fcc6a7 100644
> --- a/arch/nios2/lib/cache.c
> +++ b/arch/nios2/lib/cache.c
> @@ -127,3 +127,5 @@ void dcache_disable(void)
>   {
>   	flush_dcache_all();
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/powerpc/lib/cache.c b/arch/powerpc/lib/cache.c
> index a9cd7b8d30ac..3d0536caccde 100644
> --- a/arch/powerpc/lib/cache.c
> +++ b/arch/powerpc/lib/cache.c
> @@ -58,3 +58,5 @@ void invalidate_icache_all(void)
>   {
>   	puts("No arch specific invalidate_icache_all available!\n");
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
> index 71e4937ab542..1c751e562157 100644
> --- a/arch/riscv/lib/cache.c
> +++ b/arch/riscv/lib/cache.c
> @@ -151,3 +151,5 @@ __weak void enable_caches(void)
>   	if (!zicbom_block_size)
>   		log_debug("Zicbom not initialized.\n");
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/sh/cpu/sh4/cache.c b/arch/sh/cpu/sh4/cache.c
> index 99acc5999652..22e0f1484a33 100644
> --- a/arch/sh/cpu/sh4/cache.c
> +++ b/arch/sh/cpu/sh4/cache.c
> @@ -126,3 +126,5 @@ int dcache_status(void)
>   {
>   	return 0;
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/arch/xtensa/lib/cache.c b/arch/xtensa/lib/cache.c
> index e6a7f6827fc2..aacc2d2627d6 100644
> --- a/arch/xtensa/lib/cache.c
> +++ b/arch/xtensa/lib/cache.c
> @@ -57,3 +57,5 @@ void invalidate_icache_all(void)
>   {
>   	__invalidate_icache_all();
>   }
> +
> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> diff --git a/include/cpu_func.h b/include/cpu_func.h
> index 7e81c4364a73..17b6d199302a 100644
> --- a/include/cpu_func.h
> +++ b/include/cpu_func.h
> @@ -69,6 +69,22 @@ void flush_dcache_range(unsigned long start, unsigned long stop);
>   void invalidate_dcache_range(unsigned long start, unsigned long stop);
>   void invalidate_dcache_all(void);
>   void invalidate_icache_all(void);
> +
> +enum pgprot_attrs {
> +	MMU_ATTR_RO,
> +	MMU_ATTR_RX,
> +	MMU_ATTR_RW,
> +};
> +
> +/** pgprot_set_attrs() - Set page table permissions
> + *
> + * @addr: Physical address start
> + * @size: size of memory to change
> + * @perm: New permissions
> + *
> + **/
> +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm);
> +
>   /**
>    * noncached_init() - Initialize non-cached memory region
>    *

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Ilias Apalodimas]

Hi Heinrich,

On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/5/25 08:16, Ilias Apalodimas wrote:
> > For armv8 we are adding proper page permissions for the relocated U-Boot
> > binary. Add a weak function that can be used across architectures to change
> > the page permissions
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   arch/arc/lib/cache.c           |  2 ++
> >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> >   arch/arm/lib/cache.c           |  2 ++
> >   arch/m68k/lib/cache.c          |  2 ++
> >   arch/nios2/lib/cache.c         |  2 ++
> >   arch/powerpc/lib/cache.c       |  2 ++
> >   arch/riscv/lib/cache.c         |  2 ++
> >   arch/sh/cpu/sh4/cache.c        |  2 ++
> >   arch/xtensa/lib/cache.c        |  2 ++
> >   include/cpu_func.h             | 16 ++++++++++++++++
> >   13 files changed, 59 insertions(+)
> >
> > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > index 5169fc627fa5..5c79243d7223 100644
> > --- a/arch/arc/lib/cache.c
> > +++ b/arch/arc/lib/cache.c
> > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> >
> >       __ic_entire_invalidate();
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > index 5b87a3af91b2..857311b3dfad 100644
> > --- a/arch/arm/cpu/arm926ejs/cache.c
> > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > @@ -88,3 +88,5 @@ void enable_caches(void)
> >       dcache_enable();
> >   #endif
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > index d11420d2fdd0..14c9be77db8d 100644
> > --- a/arch/arm/cpu/armv7/cache_v7.c
> > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> >   __weak void v7_outer_cache_inval_all(void) {}
> >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > index b6d08b7aad73..458a214e9577 100644
> > --- a/arch/arm/cpu/armv7m/cache.c
> > +++ b/arch/arm/cpu/armv7m/cache.c
> > @@ -370,3 +370,5 @@ void enable_caches(void)
> >       dcache_enable();
> >   #endif
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > index 670379e17b7a..1cf3870177ee 100644
> > --- a/arch/arm/cpu/armv8/cache_v8.c
> > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > @@ -1028,6 +1028,28 @@ skip_break:
> >       __asm_invalidate_tlb_all();
> >   }
> >
> > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > +{
> > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > +
> > +     switch (perm) {
> > +     case MMU_ATTR_RO:
> > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > +             break;
> > +     case MMU_ATTR_RX:
> > +             attrs |= PTE_BLOCK_RO;
> > +             break;
> > +     case MMU_ATTR_RW:
> > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > +             break;
> > +     default:
> > +             log_err("Unknown attribute %llx\n", perm);
> > +             return;
> > +     }
> > +
> > +     mmu_change_region_attr(addr, size, attrs, false);
> > +}
> > +
> >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> >
> >   /*
> > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > index 516754caeaf9..c7704d8ee354 100644
> > --- a/arch/arm/lib/cache.c
> > +++ b/arch/arm/lib/cache.c
> > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> >
> >       return 0;
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>
> I would prefer if the weak function would return -ENOSYS indicating the
> missing implementation and the real function would return 0 in case of
> success or an error code on failure. This way the EFI protocol could set
> the return code if the architecture does not provide support for setting
> the attributes the passed addresses are invalid.

Sure I'll change that in v2

Cheers
/Ilias
>
> Best regards
>
> Heinrich
>
> > diff --git a/arch/m68k/lib/cache.c b/arch/m68k/lib/cache.c
> > index 370ad40f1423..b275646384a5 100644
> > --- a/arch/m68k/lib/cache.c
> > +++ b/arch/m68k/lib/cache.c
> > @@ -151,3 +151,5 @@ __weak void flush_dcache_range(unsigned long start, unsigned long stop)
> >   {
> >       /* An empty stub, real implementation should be in platform code */
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/nios2/lib/cache.c b/arch/nios2/lib/cache.c
> > index 8f543f2a2f26..7a93a8fcc6a7 100644
> > --- a/arch/nios2/lib/cache.c
> > +++ b/arch/nios2/lib/cache.c
> > @@ -127,3 +127,5 @@ void dcache_disable(void)
> >   {
> >       flush_dcache_all();
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/powerpc/lib/cache.c b/arch/powerpc/lib/cache.c
> > index a9cd7b8d30ac..3d0536caccde 100644
> > --- a/arch/powerpc/lib/cache.c
> > +++ b/arch/powerpc/lib/cache.c
> > @@ -58,3 +58,5 @@ void invalidate_icache_all(void)
> >   {
> >       puts("No arch specific invalidate_icache_all available!\n");
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
> > index 71e4937ab542..1c751e562157 100644
> > --- a/arch/riscv/lib/cache.c
> > +++ b/arch/riscv/lib/cache.c
> > @@ -151,3 +151,5 @@ __weak void enable_caches(void)
> >       if (!zicbom_block_size)
> >               log_debug("Zicbom not initialized.\n");
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/sh/cpu/sh4/cache.c b/arch/sh/cpu/sh4/cache.c
> > index 99acc5999652..22e0f1484a33 100644
> > --- a/arch/sh/cpu/sh4/cache.c
> > +++ b/arch/sh/cpu/sh4/cache.c
> > @@ -126,3 +126,5 @@ int dcache_status(void)
> >   {
> >       return 0;
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/arch/xtensa/lib/cache.c b/arch/xtensa/lib/cache.c
> > index e6a7f6827fc2..aacc2d2627d6 100644
> > --- a/arch/xtensa/lib/cache.c
> > +++ b/arch/xtensa/lib/cache.c
> > @@ -57,3 +57,5 @@ void invalidate_icache_all(void)
> >   {
> >       __invalidate_icache_all();
> >   }
> > +
> > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > diff --git a/include/cpu_func.h b/include/cpu_func.h
> > index 7e81c4364a73..17b6d199302a 100644
> > --- a/include/cpu_func.h
> > +++ b/include/cpu_func.h
> > @@ -69,6 +69,22 @@ void flush_dcache_range(unsigned long start, unsigned long stop);
> >   void invalidate_dcache_range(unsigned long start, unsigned long stop);
> >   void invalidate_dcache_all(void);
> >   void invalidate_icache_all(void);
> > +
> > +enum pgprot_attrs {
> > +     MMU_ATTR_RO,
> > +     MMU_ATTR_RX,
> > +     MMU_ATTR_RW,
> > +};
> > +
> > +/** pgprot_set_attrs() - Set page table permissions
> > + *
> > + * @addr: Physical address start
> > + * @size: size of memory to change
> > + * @perm: New permissions
> > + *
> > + **/
> > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm);
> > +
> >   /**
> >    * noncached_init() - Initialize non-cached memory region
> >    *
>

Re: [PATCH v1 2/6] doc: update meminfo with arch specific information

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Wed, Feb 05, 2025 at 09:16:46AM +0200, Ilias Apalodimas wrote:

> Since we added support in meminfo to dump live page tables, describe
> the only working architecture for now (aarch64) and add links to public
> documentation for further reading.
> 
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Generally looks good, a small change however please:

[snip]
> @@ -26,8 +27,9 @@ The layout of memory is set up before relocation, within the init sequence in
>  ending with the stack. This results in the maximum possible amount of memory
>  being left free for image-loading.
>  
> -The meminfo command writes the DRAM size, then the rest of its outputs in 5
> -columns:
> +The meminfo command writes the DRAM size. If the architecture supports it
> +(currently only aarch64) dumps the page table entries and then the rest of
> +its outputs in 5 columns:

How about:
If the architecture also supports it, page table entries will be shown
next. Finally the rest of the outputs are printed in 5 columns:

This will make sure that when the next architecture adds support the
documentation doesn't need to be updated here (and the temptation to
list every arch that does this will be resisted). Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Richard Henderson]

On 2/4/25 23:16, Ilias Apalodimas wrote:
> @@ -98,18 +101,20 @@ SECTIONS
>   	}
>   #endif
>   
> -	. = ALIGN(8);
> -	.rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }
> +	.rodata ALIGN(4096): {
> +	    __start_rodata = .;
> +	    *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*)))
> +	    . = ALIGN(4096);
> +	    __end_rodata = .;
> +	}
>   
> -	. = ALIGN(8);
> -	.data : {
> +	.data ALIGN(4096) : {
> +	    __start_data = .;
>   		*(.data*)
> +	    . = ALIGN(4096);
> +	    __end_data = .;
>   	}
>   
> -	. = ALIGN(8);
> -
> -	. = .;
> -
>   	. = ALIGN(8);
>   	__u_boot_list : {
>   		KEEP(*(SORT(__u_boot_list*)));
> @@ -136,10 +141,10 @@ SECTIONS
>   	/*
>   	 * arch/arm/lib/crt0_64.S assumes __bss_start - __bss_end % 8 == 0
>   	 */
> -	.bss ALIGN(8) : {
> +	.bss ALIGN(4096) : {
>   		__bss_start = .;
>   		*(.bss*)
> -		. = ALIGN(8);
> +		. = ALIGN(4096);
>   		__bss_end = .;
>   	}

You don't need to align .bss because it normally immediately follows .data, and they have 
the same page permissions.

You've got __u_boot_list, .efi_runtime_rel and .rela.dyn in between.  Consider if any of 
that ought to be moved around to become read-only.


r~

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]

On Wed, Feb 05, 2025 at 09:16:47AM +0200, Ilias Apalodimas wrote:

> Upcoming patches are switching the memory mappings to RW, RO, RX
> after the U-Boot binary and its data are relocated. Add
> annotations in the linker scripts to and mark text, data, rodata
> sections and align them to a page boundary.
> 
> It's worth noting that efi_runtime relocations are left untouched for
> now. The efi runtime regions can be relocated by the OS when the latter
> is calling SetVirtualAddressMap. Which means we have to configure the
> pages as RX for U-Boot but convert them to RWX just before
> ExitBootServices. It also needs extra code in efi_tuntime relocation
> code since R_AARCH64_NONE are emitted as well if we page align the
> section. Keep it out for now and we can fix it in future patches.
> 
> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  arch/arm/cpu/armv8/u-boot.lds  | 29 +++++++++++++++++------------
>  include/asm-generic/sections.h |  2 ++
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> index 857f44412e07..35afc3cbe7ec 100644
> --- a/arch/arm/cpu/armv8/u-boot.lds
> +++ b/arch/arm/cpu/armv8/u-boot.lds
> @@ -22,7 +22,7 @@ SECTIONS
>  
>  	. = ALIGN(8);
>  	__image_copy_start = ADDR(.text);
> -	.text :
> +	.text ALIGN(4096):
>  	{
>  		CPUDIR/start.o (.text*)
>  	}

Shouldn't this be:
-	. = ALIGN(8);
-	__image_copy_start = ADDR(.text);
-	.text :
+	.text ALIGN(4096):
 	{
+		__image_copy_start = ADDR(.text); // Or even just '= .;' ?
		CPUDIR/start.o (.text*)
 	}
Or so? Something like that would also make it easier in v2 to have a
Kconfig about page align / min align for sections in the linker script
and then ALIGN(CONFIG_FOO) or #ifdef CONFIG_FOO #define MINALIGN 8 #else
4096 #endif, or so. Also I guess today we're not going to worry about
bigger than 4KiB pages?

> @@ -36,9 +36,12 @@ SECTIONS
>                  __efi_runtime_stop = .;
>  	}
>  
> -	.text_rest :
> +	.text_rest ALIGN(4096) :
>  	{
> +		__text_start = .;
>  		*(.text*)
> +		. = ALIGN(4096);
> +		__text_end = .;
>  	}

Here and elsewhere in the patch, does that really need two ALIGN(4096)
lines? Or am I totally misremembering how these symbols work in a linker
script?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Ilias Apalodimas]

Hi Richard,

On Wed, 5 Feb 2025 at 19:23, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/4/25 23:16, Ilias Apalodimas wrote:
> > @@ -98,18 +101,20 @@ SECTIONS
> >       }
> >   #endif
> >
> > -     . = ALIGN(8);
> > -     .rodata : { *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) }
> > +     .rodata ALIGN(4096): {
> > +         __start_rodata = .;
> > +         *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*)))
> > +         . = ALIGN(4096);
> > +         __end_rodata = .;
> > +     }
> >
> > -     . = ALIGN(8);
> > -     .data : {
> > +     .data ALIGN(4096) : {
> > +         __start_data = .;
> >               *(.data*)
> > +         . = ALIGN(4096);
> > +         __end_data = .;
> >       }
> >
> > -     . = ALIGN(8);
> > -
> > -     . = .;
> > -
> >       . = ALIGN(8);
> >       __u_boot_list : {
> >               KEEP(*(SORT(__u_boot_list*)));
> > @@ -136,10 +141,10 @@ SECTIONS
> >       /*
> >        * arch/arm/lib/crt0_64.S assumes __bss_start - __bss_end % 8 == 0
> >        */
> > -     .bss ALIGN(8) : {
> > +     .bss ALIGN(4096) : {
> >               __bss_start = .;
> >               *(.bss*)
> > -             . = ALIGN(8);
> > +             . = ALIGN(4096);
> >               __bss_end = .;
> >       }
>
> You don't need to align .bss because it normally immediately follows .data, and they have
> the same page permissions.
>
> You've got __u_boot_list, .efi_runtime_rel and .rela.dyn in between.  Consider if any of
> that ought to be moved around to become read-only.

That's a good idea and I just realized I am not setting the .bss to RW
in later patches.
I can move .bss right after data and rela.dyn after .rodata. That will
probably same me ~2 pages due to alignment. I am not sure about
__u_boot_list, but if that's read-only I can group it as well.

The .efi_runtime_rel is a bit weird. This is where the UEFI runtime
services live and it contains executable code. However, when an OS
boots up it can call SetVirtualAddressMap. That call relocates the
runtime code to the new VA mapping the OS choose. This means I have to
initially set it to RX and later when we leave the firmware land
switch it RWX so the OS can relocate it... I've left that part out on
purpose but I can move the runtime services as the last segment and
fix up weird logic on relocation in later patches

Thanks!
/Ilias
>
>
> r~

Re: [PATCH v1 2/6] doc: update meminfo with arch specific information

[Ilias Apalodimas]

Hi Tom,

On Wed, 5 Feb 2025 at 19:22, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Feb 05, 2025 at 09:16:46AM +0200, Ilias Apalodimas wrote:
>
> > Since we added support in meminfo to dump live page tables, describe
> > the only working architecture for now (aarch64) and add links to public
> > documentation for further reading.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> Generally looks good, a small change however please:
>
> [snip]
> > @@ -26,8 +27,9 @@ The layout of memory is set up before relocation, within the init sequence in
> >  ending with the stack. This results in the maximum possible amount of memory
> >  being left free for image-loading.
> >
> > -The meminfo command writes the DRAM size, then the rest of its outputs in 5
> > -columns:
> > +The meminfo command writes the DRAM size. If the architecture supports it
> > +(currently only aarch64) dumps the page table entries and then the rest of
> > +its outputs in 5 columns:
>
> How about:
> If the architecture also supports it, page table entries will be shown
> next. Finally the rest of the outputs are printed in 5 columns:
>
> This will make sure that when the next architecture adds support the
> documentation doesn't need to be updated here (and the temptation to
> list every arch that does this will be resisted). Thanks!

Sure, this makes sense.

Thanks
/Ilias
>
> --
> Tom

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

On Wed, Feb 05, 2025 at 01:41:46PM +0100, Michal Simek wrote:
> Hi,
> 
> On 2/5/25 09:22, Ilias Apalodimas wrote:
> > I'm just replying to myself here but I'll send a v2 when the patches
> > are reviewed.
> > 
> > I can add the linker under an ifdef, so u-boot size won't change
> > unless a Kconfig options is selected
> 
> I think you can just do it for alignment not really big ifdef in the file.
> Definitely this will be needed for our mini configurations which are running
> out of OCM.
> Feel free to look at xilinx_*_mini* configurations.

Does the existing SPL_SIZE_LIMIT on the mini configs catch problems?
This series doesn't trip it there, but maybe we're only close? Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 6/6] arm64: Enable RW, RX and RO mappings for the relocated binary

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]

On Wed, Feb 05, 2025 at 12:31:33PM +0200, Ilias Apalodimas wrote:
> Hi Jerome,
> 
> On Wed, 5 Feb 2025 at 11:57, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
> >
> >
> >
> > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > Now that we have everything in place switch the page permissions for
> > > .rodata, .text and .data just after we relocate everything in top of the
> > > RAM.
> > >
> > > Unfortunately we can't enable this by default, since we have examples of
> > > U-Boot crashing due to invalid access. This usually happens because code
> > > defines const variables that it later writes. So hide it behind a Kconfig
> > > option until we sort it out.
> > >
> > > It's worth noting that EFI runtime services are not covered by this
> > > patch on purpose. Since the OS can call SetVirtualAddressMap which can
> > > relocate runtime services, we need to set them to RX initially but remap
> > > them as RWX right before ExitBootServices.
> > >
> > > Link: https://lore.kernel.org/u-boot/20250129-rockchip-pinctrl-const-v1-0-450ccdadfa7e@cherry.de/
> > > Link: https://lore.kernel.org/u-boot/20250130133646.2177194-1-andre.przywara@arm.com/
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >  common/Kconfig   | 13 +++++++++++++
> > >  common/board_r.c | 20 ++++++++++++++++++++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/common/Kconfig b/common/Kconfig
> > > index 7685914fa6fd..dbae7e062b0a 100644
> > > --- a/common/Kconfig
> > > +++ b/common/Kconfig
> > > @@ -914,6 +914,19 @@ config STACKPROTECTOR
> > >         Enable stack smash detection through compiler's stack-protector
> > >         canary logic
> > >
> > > +config MMU_PGPROT
> > > +     bool "Enable RO, RW and RX mappings"
> > > +     help
> > > +       U-Boot maps all pages as RWX. If selected pages will
> > > +       be marked as RO(.rodata), RX(.text), RW(.data) right after
> >
> > Space before (
> 
> Sure
> 
> >
> > > +       we relocate. Since code sections needs to be page aligned
> > > +       the final binary size will increase.
> > > +       The mapping can be dumped using the 'meminfo' command.
> >
> > OK
> >
> > > +       We should make this default 'y' in the future, but currently
> > > +       we have code defining const variables that are later written.
> > > +       Enabling this will crash U-Boot if that code path runs, so keep
> > > +       it off by default until we fix invalid accesses.
> >
> > Not sure this needs to be documented in the Kconfig help. Perhaps just
> > keep a patch ready and send it early in the next release cycle for people
> > to test and debug?
> 
> I'd like people to see it and try to debug when they see random
> crashes. I think it's easier if we document that here until things are
> fixed.
> OTOH i don't really mind whatever people think it's best

We should change the tone / emphasis I think. Something like:

Enabling this feature can expose bugs in U-Boot where we have code that
violates read-only permissions for example. Use this feature with
caution.

And then we should encourage our more active SoC maintainers to default
this option to being enabled after testing out a few platforms as long
term it should be on by default but initial testing has shown that
assuming most platforms are OK isn't a good idea.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Ilias Apalodimas]

Hi Tom,

On Wed, 5 Feb 2025 at 19:33, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Feb 05, 2025 at 09:16:47AM +0200, Ilias Apalodimas wrote:
>
> > Upcoming patches are switching the memory mappings to RW, RO, RX
> > after the U-Boot binary and its data are relocated. Add
> > annotations in the linker scripts to and mark text, data, rodata
> > sections and align them to a page boundary.
> >
> > It's worth noting that efi_runtime relocations are left untouched for
> > now. The efi runtime regions can be relocated by the OS when the latter
> > is calling SetVirtualAddressMap. Which means we have to configure the
> > pages as RX for U-Boot but convert them to RWX just before
> > ExitBootServices. It also needs extra code in efi_tuntime relocation
> > code since R_AARCH64_NONE are emitted as well if we page align the
> > section. Keep it out for now and we can fix it in future patches.
> >
> > Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  arch/arm/cpu/armv8/u-boot.lds  | 29 +++++++++++++++++------------
> >  include/asm-generic/sections.h |  2 ++
> >  2 files changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > index 857f44412e07..35afc3cbe7ec 100644
> > --- a/arch/arm/cpu/armv8/u-boot.lds
> > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > @@ -22,7 +22,7 @@ SECTIONS
> >
> >       . = ALIGN(8);
> >       __image_copy_start = ADDR(.text);
> > -     .text :
> > +     .text ALIGN(4096):
> >       {
> >               CPUDIR/start.o (.text*)
> >       }
>
> Shouldn't this be:
> -       . = ALIGN(8);
> -       __image_copy_start = ADDR(.text);
> -       .text :
> +       .text ALIGN(4096):
>         {
> +               __image_copy_start = ADDR(.text); // Or even just '= .;' ?
>                 CPUDIR/start.o (.text*)

IIRC this will produce the exact same output.
Given Richards's idea, I'll move the sections around a bit and define
rw_start_end, ro_start/end and rx_start/end symbols.
It would make other architectures life easier if they ever want to
replicate this. So with those new symbols, adding ifdefs around the
alignment should be more readable.

>         }
> Or so? Something like that would also make it easier in v2 to have a
> Kconfig about page align / min align for sections in the linker script
> and then ALIGN(CONFIG_FOO) or #ifdef CONFIG_FOO #define MINALIGN 8 #else
> 4096 #endif, or so. Also I guess today we're not going to worry about
> bigger than 4KiB pages?
>
> > @@ -36,9 +36,12 @@ SECTIONS
> >                  __efi_runtime_stop = .;
> >       }
> >
> > -     .text_rest :
> > +     .text_rest ALIGN(4096) :
> >       {
> > +             __text_start = .;
> >               *(.text*)
> > +             . = ALIGN(4096);
> > +             __text_end = .;
> >       }
>
> Here and elsewhere in the patch, does that really need two ALIGN(4096)
> lines? Or am I totally misremembering how these symbols work in a linker
> script?

Yes it does, the first one guarantees the start of the section and the
latter the end. IOW if I commit the second align, there's no guarantee
we'll end up page aliged. Our mmu code assumes that it's operating in
page boundaries.

Thanks
/Ilias


>
> --
> Tom

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3922 bytes --]

On Wed, Feb 05, 2025 at 09:18:50PM +0200, Ilias Apalodimas wrote:
> Hi Tom,
> 
> On Wed, 5 Feb 2025 at 19:33, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Feb 05, 2025 at 09:16:47AM +0200, Ilias Apalodimas wrote:
> >
> > > Upcoming patches are switching the memory mappings to RW, RO, RX
> > > after the U-Boot binary and its data are relocated. Add
> > > annotations in the linker scripts to and mark text, data, rodata
> > > sections and align them to a page boundary.
> > >
> > > It's worth noting that efi_runtime relocations are left untouched for
> > > now. The efi runtime regions can be relocated by the OS when the latter
> > > is calling SetVirtualAddressMap. Which means we have to configure the
> > > pages as RX for U-Boot but convert them to RWX just before
> > > ExitBootServices. It also needs extra code in efi_tuntime relocation
> > > code since R_AARCH64_NONE are emitted as well if we page align the
> > > section. Keep it out for now and we can fix it in future patches.
> > >
> > > Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >  arch/arm/cpu/armv8/u-boot.lds  | 29 +++++++++++++++++------------
> > >  include/asm-generic/sections.h |  2 ++
> > >  2 files changed, 19 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > > index 857f44412e07..35afc3cbe7ec 100644
> > > --- a/arch/arm/cpu/armv8/u-boot.lds
> > > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > > @@ -22,7 +22,7 @@ SECTIONS
> > >
> > >       . = ALIGN(8);
> > >       __image_copy_start = ADDR(.text);
> > > -     .text :
> > > +     .text ALIGN(4096):
> > >       {
> > >               CPUDIR/start.o (.text*)
> > >       }
> >
> > Shouldn't this be:
> > -       . = ALIGN(8);
> > -       __image_copy_start = ADDR(.text);
> > -       .text :
> > +       .text ALIGN(4096):
> >         {
> > +               __image_copy_start = ADDR(.text); // Or even just '= .;' ?
> >                 CPUDIR/start.o (.text*)
> 
> IIRC this will produce the exact same output.

OK, so we don't end up with 8192 worth of alignment here.

> Given Richards's idea, I'll move the sections around a bit and define
> rw_start_end, ro_start/end and rx_start/end symbols.
> It would make other architectures life easier if they ever want to
> replicate this. So with those new symbols, adding ifdefs around the
> alignment should be more readable.

Yes, re-organizing things will help too, I agree.

> > Or so? Something like that would also make it easier in v2 to have a
> > Kconfig about page align / min align for sections in the linker script
> > and then ALIGN(CONFIG_FOO) or #ifdef CONFIG_FOO #define MINALIGN 8 #else
> > 4096 #endif, or so. Also I guess today we're not going to worry about
> > bigger than 4KiB pages?
> >
> > > @@ -36,9 +36,12 @@ SECTIONS
> > >                  __efi_runtime_stop = .;
> > >       }
> > >
> > > -     .text_rest :
> > > +     .text_rest ALIGN(4096) :
> > >       {
> > > +             __text_start = .;
> > >               *(.text*)
> > > +             . = ALIGN(4096);
> > > +             __text_end = .;
> > >       }
> >
> > Here and elsewhere in the patch, does that really need two ALIGN(4096)
> > lines? Or am I totally misremembering how these symbols work in a linker
> > script?
> 
> Yes it does, the first one guarantees the start of the section and the
> latter the end. IOW if I commit the second align, there's no guarantee
> we'll end up page aliged. Our mmu code assumes that it's operating in
> page boundaries.

And I had mis-read the context. Yes, we need to make sure the end is
aligned so that we don't have incorrect permissions for the next
section. And with re-organization of the areas we'll have less extra
padding too.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 3/6] arm: Prepare linker scripts for memory permissions

[Ilias Apalodimas]

On Wed, 5 Feb 2025 at 21:25, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Feb 05, 2025 at 09:18:50PM +0200, Ilias Apalodimas wrote:
> > Hi Tom,
> >
> > On Wed, 5 Feb 2025 at 19:33, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Feb 05, 2025 at 09:16:47AM +0200, Ilias Apalodimas wrote:
> > >
> > > > Upcoming patches are switching the memory mappings to RW, RO, RX
> > > > after the U-Boot binary and its data are relocated. Add
> > > > annotations in the linker scripts to and mark text, data, rodata
> > > > sections and align them to a page boundary.
> > > >
> > > > It's worth noting that efi_runtime relocations are left untouched for
> > > > now. The efi runtime regions can be relocated by the OS when the latter
> > > > is calling SetVirtualAddressMap. Which means we have to configure the
> > > > pages as RX for U-Boot but convert them to RWX just before
> > > > ExitBootServices. It also needs extra code in efi_tuntime relocation
> > > > code since R_AARCH64_NONE are emitted as well if we page align the
> > > > section. Keep it out for now and we can fix it in future patches.
> > > >
> > > > Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > >  arch/arm/cpu/armv8/u-boot.lds  | 29 +++++++++++++++++------------
> > > >  include/asm-generic/sections.h |  2 ++
> > > >  2 files changed, 19 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/arch/arm/cpu/armv8/u-boot.lds b/arch/arm/cpu/armv8/u-boot.lds
> > > > index 857f44412e07..35afc3cbe7ec 100644
> > > > --- a/arch/arm/cpu/armv8/u-boot.lds
> > > > +++ b/arch/arm/cpu/armv8/u-boot.lds
> > > > @@ -22,7 +22,7 @@ SECTIONS
> > > >
> > > >       . = ALIGN(8);
> > > >       __image_copy_start = ADDR(.text);
> > > > -     .text :
> > > > +     .text ALIGN(4096):
> > > >       {
> > > >               CPUDIR/start.o (.text*)
> > > >       }
> > >
> > > Shouldn't this be:
> > > -       . = ALIGN(8);
> > > -       __image_copy_start = ADDR(.text);
> > > -       .text :
> > > +       .text ALIGN(4096):
> > >         {
> > > +               __image_copy_start = ADDR(.text); // Or even just '= .;' ?
> > >                 CPUDIR/start.o (.text*)
> >
> > IIRC this will produce the exact same output.
>
> OK, so we don't end up with 8192 worth of alignment here.

We do, but that's because I added an ALIGN(4096) in .text. The change
above won't change that.

But looking at it again, I don't have to do that now. I can remove the
alignment from .text since some efi_runtime services are between that
and .text_start.
Since my current patch ignores efi_runtime, I can skip the .text
alignment, save ~4k and fix the rest when I deal with EFI.

>
> > Given Richards's idea, I'll move the sections around a bit and define
> > rw_start_end, ro_start/end and rx_start/end symbols.
> > It would make other architectures life easier if they ever want to
> > replicate this. So with those new symbols, adding ifdefs around the
> > alignment should be more readable.
>
> Yes, re-organizing things will help too, I agree.
>
[...]

Regards
/Ilias

Re: [PATCH v1 0/6] Fix page permission on arm64 architectures

[Neil Armstrong]

On 05/02/2025 08:16, Ilias Apalodimas wrote:
> Hi!
> This is v1 of [0].
> 
> This is an attempt to map the U-Boot binary properly, but leave the
> area we load binaries unaffected and RWX.
> 
> There's a few changes since the RFC
> - Fixed the alignment of meminfo command when printing regions
> - 'meminfo' now prints arch specific attributes e.g PXN, UXN etc for arm
>    instead of RW, RO, RX
> - Since we don't set the permissions of EFI runtime services yet and keep
>    them as RWX, I removed the linker alignment changes which makes patch #3
>    easier to review. It's worth noting that qemu-arm sbsa was crashing with
>    the efi services page aligned. This is probably due to a mismatch of
>    memory, since the crash is only reproducible with QEMU instances that
>    have < 2 GB of RAM. I'll fix that along with the efi runtime services
> - Defined memory attribute changes properly with an enum for RW, RO, RX
>    instead of the hardcoded '1,2,3' I had on the RFC
> - Enabling mappings is now under a Kconfig (CONFIG_MMU_PGPROT), since
>    peope reported crashes when testing this, which are orthogonal to this
>    patch. We still have places in U-Boot where we define and later write
>    const variables. This will lead to a crash now as const variables are
>    properly managed and places in RO memory
> - Split patches to be easier to review
> - Added a patch updating 'meminfo'
> - Picked up acked-by tags from Jerome
> 
> patch #1 adds printing capabilities for page mappings in 'meminfo'
> patch #2 adds documention in 'meminfo' command
> patch #3 prepares linker scripts, aligns sections in page boundaries etc
> patch #4 prepares an internal function to change the PTEs
> patch #5 adds function definitions & stubs for all archs
> patch #6 wires up the changes in U-Boot after it relocates
> 
> [0] https://lore.kernel.org/u-boot/20250130072100.27297-1-ilias.apalodimas@linaro.org/
> 
> Ilias Apalodimas (6):
>    meminfo: add memory details for armv8
>    doc: update meminfo with arch specific information
>    arm: Prepare linker scripts for memory permissions
>    arm64: mmu_change_region_attr() add an option not to break PTEs
>    treewide: Add a function to change page permissions
>    arm64: Enable RW, RX and RO mappings for the relocated binary
> 
>   arch/arc/lib/cache.c                    |  2 +
>   arch/arm/cpu/arm926ejs/cache.c          |  2 +
>   arch/arm/cpu/armv7/cache_v7.c           |  1 +
>   arch/arm/cpu/armv7m/cache.c             |  2 +
>   arch/arm/cpu/armv8/cache_v8.c           | 54 +++++++++++++++++--
>   arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 10 ++--
>   arch/arm/cpu/armv8/u-boot.lds           | 29 +++++-----
>   arch/arm/include/asm/armv8/mmu.h        |  2 +
>   arch/arm/include/asm/system.h           | 11 +++-
>   arch/arm/lib/cache.c                    |  2 +
>   arch/arm/mach-snapdragon/board.c        |  2 +-
>   arch/m68k/lib/cache.c                   |  2 +
>   arch/nios2/lib/cache.c                  |  2 +
>   arch/powerpc/lib/cache.c                |  2 +
>   arch/riscv/lib/cache.c                  |  2 +
>   arch/sh/cpu/sh4/cache.c                 |  2 +
>   arch/xtensa/lib/cache.c                 |  2 +
>   cmd/meminfo.c                           |  5 ++
>   common/Kconfig                          | 13 +++++
>   common/board_r.c                        | 20 +++++++
>   doc/usage/cmd/meminfo.rst               | 71 ++++++++++++++++++-------
>   include/asm-generic/sections.h          |  2 +
>   include/cpu_func.h                      | 16 ++++++
>   23 files changed, 215 insertions(+), 41 deletions(-)
> 
> --
> 2.47.2
> 

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on AML-S905X-CC
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on AML-S805X-AC
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on BananaPi-M5
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on BananaPi-M2S
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-HDK
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK

I ran this patchset on all of those board and ran meminfo to check the permissions,
no issues nor crash observed.

Thanks,
Neil

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Ilias Apalodimas]

Hi Heinrich,

[...]

> > > +++ b/arch/arm/lib/cache.c
> > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > >
> > >       return 0;
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> >
> > I would prefer if the weak function would return -ENOSYS indicating the
> > missing implementation and the real function would return 0 in case of
> > success or an error code on failure. This way the EFI protocol could set
> > the return code if the architecture does not provide support for setting
> > the attributes the passed addresses are invalid.
>
> Sure I'll change that in v2

Okay, I ad a look at EFI_MEMORY_ATTRIBUTE_PROTOCOL.SetMemoryAttributes.
This function does not support EFI_UNSUPPORTED if you don't have code
around to set the PTEs.

So, I think we should
- make the prototype an int and return an error
- add a new bool *supported argument? And if the caller calls the function
with base & size == 0, set that value?
That way we can choose not to install the protocol at all if the
functionality doesn't exist.

Thanks
/Ilias
>
> Cheers
> /Ilias
> >
> > Best regards
> >
> > Heinrich
> >
> > > diff --git a/arch/m68k/lib/cache.c b/arch/m68k/lib/cache.c
> > > index 370ad40f1423..b275646384a5 100644
> > > --- a/arch/m68k/lib/cache.c
> > > +++ b/arch/m68k/lib/cache.c
> > > @@ -151,3 +151,5 @@ __weak void flush_dcache_range(unsigned long start, unsigned long stop)
> > >   {
> > >       /* An empty stub, real implementation should be in platform code */
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/nios2/lib/cache.c b/arch/nios2/lib/cache.c
> > > index 8f543f2a2f26..7a93a8fcc6a7 100644
> > > --- a/arch/nios2/lib/cache.c
> > > +++ b/arch/nios2/lib/cache.c
> > > @@ -127,3 +127,5 @@ void dcache_disable(void)
> > >   {
> > >       flush_dcache_all();
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/powerpc/lib/cache.c b/arch/powerpc/lib/cache.c
> > > index a9cd7b8d30ac..3d0536caccde 100644
> > > --- a/arch/powerpc/lib/cache.c
> > > +++ b/arch/powerpc/lib/cache.c
> > > @@ -58,3 +58,5 @@ void invalidate_icache_all(void)
> > >   {
> > >       puts("No arch specific invalidate_icache_all available!\n");
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
> > > index 71e4937ab542..1c751e562157 100644
> > > --- a/arch/riscv/lib/cache.c
> > > +++ b/arch/riscv/lib/cache.c
> > > @@ -151,3 +151,5 @@ __weak void enable_caches(void)
> > >       if (!zicbom_block_size)
> > >               log_debug("Zicbom not initialized.\n");
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/sh/cpu/sh4/cache.c b/arch/sh/cpu/sh4/cache.c
> > > index 99acc5999652..22e0f1484a33 100644
> > > --- a/arch/sh/cpu/sh4/cache.c
> > > +++ b/arch/sh/cpu/sh4/cache.c
> > > @@ -126,3 +126,5 @@ int dcache_status(void)
> > >   {
> > >       return 0;
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/xtensa/lib/cache.c b/arch/xtensa/lib/cache.c
> > > index e6a7f6827fc2..aacc2d2627d6 100644
> > > --- a/arch/xtensa/lib/cache.c
> > > +++ b/arch/xtensa/lib/cache.c
> > > @@ -57,3 +57,5 @@ void invalidate_icache_all(void)
> > >   {
> > >       __invalidate_icache_all();
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/include/cpu_func.h b/include/cpu_func.h
> > > index 7e81c4364a73..17b6d199302a 100644
> > > --- a/include/cpu_func.h
> > > +++ b/include/cpu_func.h
> > > @@ -69,6 +69,22 @@ void flush_dcache_range(unsigned long start, unsigned long stop);
> > >   void invalidate_dcache_range(unsigned long start, unsigned long stop);
> > >   void invalidate_dcache_all(void);
> > >   void invalidate_icache_all(void);
> > > +
> > > +enum pgprot_attrs {
> > > +     MMU_ATTR_RO,
> > > +     MMU_ATTR_RX,
> > > +     MMU_ATTR_RW,
> > > +};
> > > +
> > > +/** pgprot_set_attrs() - Set page table permissions
> > > + *
> > > + * @addr: Physical address start
> > > + * @size: size of memory to change
> > > + * @perm: New permissions
> > > + *
> > > + **/
> > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm);
> > > +
> > >   /**
> > >    * noncached_init() - Initialize non-cached memory region
> > >    *
> >

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Heinrich Schuchardt]

On 2/6/25 10:42, Ilias Apalodimas wrote:
> Hi Heinrich,
>
> [...]
>
>>>> +++ b/arch/arm/lib/cache.c
>>>> @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
>>>>
>>>>        return 0;
>>>>    }
>>>> +
>>>> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>>>
>>> I would prefer if the weak function would return -ENOSYS indicating the
>>> missing implementation and the real function would return 0 in case of
>>> success or an error code on failure. This way the EFI protocol could set
>>> the return code if the architecture does not provide support for setting
>>> the attributes the passed addresses are invalid.
>>
>> Sure I'll change that in v2
>
> Okay, I had a look at EFI_MEMORY_ATTRIBUTE_PROTOCOL.SetMemoryAttributes.
> This function does not support EFI_UNSUPPORTED if you don't have code
> around to set the PTEs.

37.7.1.2 "EFI_MEMORY_ATTRIBUTE_PROTOCOL.SetMemoryAttributes" in UEFI
spec 2.11 lists EFI_UNSUPPORTED:

EFI_UNSUPPORTED
The processor does not support one or more bytes of the memory resource
range specified by BaseAddress and Length. The bit mask of attributes
is not supported for the memory resource range specified by BaseAddress
and Length

>
> So, I think we should
> - make the prototype an int and return an error
> - add a new bool *supported argument? And if the caller calls the function
> with base & size == 0, set that value?
> That way we can choose not to install the protocol at all if the
> functionality doesn't exist.

Yes, it would help to be able to detect if the architecture specific
implementation exists.

Best regards

Heinrich

>
> Thanks
> /Ilias
>>
>> Cheers
>> /Ilias
>>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>> diff --git a/arch/m68k/lib/cache.c b/arch/m68k/lib/cache.c
>>>> index 370ad40f1423..b275646384a5 100644
>>>> --- a/arch/m68k/lib/cache.c
>>>> +++ b/arch/m68k/lib/cache.c
>>>> @@ -151,3 +151,5 @@ __weak void flush_dcache_range(unsigned long start, unsigned long stop)
>>>>    {
>>>>        /* An empty stub, real implementation should be in platform code */
>>>>    }
>>>> +
>>>> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>>>> diff --git a/arch/nios2/lib/cache.c b/arch/nios2/lib/cache.c
>>>> index 8f543f2a2f26..7a93a8fcc6a7 100644
>>>> --- a/arch/nios2/lib/cache.c
>>>> +++ b/arch/nios2/lib/cache.c
>>>> @@ -127,3 +127,5 @@ void dcache_disable(void)
>>>>    {
>>>>        flush_dcache_all();
>>>>    }
>>>> +
>>>> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>>>> diff --git a/arch/powerpc/lib/cache.c b/arch/powerpc/lib/cache.c
>>>> index a9cd7b8d30ac..3d0536caccde 100644
>>>> --- a/arch/powerpc/lib/cache.c
>>>> +++ b/arch/powerpc/lib/cache.c
>>>> @@ -58,3 +58,5 @@ void invalidate_icache_all(void)
>>>>    {
>>>>        puts("No arch specific invalidate_icache_all available!\n");
>>>>    }
>>>> +
>>>> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>>>> diff --git a/arch/riscv/lib/cache.c b/arch/riscv/lib/cache.c
>>>> index 71e4937ab542..1c751e562157 100644
>>>> --- a/arch/riscv/lib/cache.c
>>>> +++ b/arch/riscv/lib/cache.c
>>>> @@ -151,3 +151,5 @@ __weak void enable_caches(void)
>>>>        if (!zicbom_block_size)
>>>>                log_debug("Zicbom not initialized.\n");
>>>>    }
>>>> +
>>>> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>>>> diff --git a/arch/sh/cpu/sh4/cache.c b/arch/sh/cpu/sh4/cache.c
>>>> index 99acc5999652..22e0f1484a33 100644
>>>> --- a/arch/sh/cpu/sh4/cache.c
>>>> +++ b/arch/sh/cpu/sh4/cache.c
>>>> @@ -126,3 +126,5 @@ int dcache_status(void)
>>>>    {
>>>>        return 0;
>>>>    }
>>>> +
>>>> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>>>> diff --git a/arch/xtensa/lib/cache.c b/arch/xtensa/lib/cache.c
>>>> index e6a7f6827fc2..aacc2d2627d6 100644
>>>> --- a/arch/xtensa/lib/cache.c
>>>> +++ b/arch/xtensa/lib/cache.c
>>>> @@ -57,3 +57,5 @@ void invalidate_icache_all(void)
>>>>    {
>>>>        __invalidate_icache_all();
>>>>    }
>>>> +
>>>> +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
>>>> diff --git a/include/cpu_func.h b/include/cpu_func.h
>>>> index 7e81c4364a73..17b6d199302a 100644
>>>> --- a/include/cpu_func.h
>>>> +++ b/include/cpu_func.h
>>>> @@ -69,6 +69,22 @@ void flush_dcache_range(unsigned long start, unsigned long stop);
>>>>    void invalidate_dcache_range(unsigned long start, unsigned long stop);
>>>>    void invalidate_dcache_all(void);
>>>>    void invalidate_icache_all(void);
>>>> +
>>>> +enum pgprot_attrs {
>>>> +     MMU_ATTR_RO,
>>>> +     MMU_ATTR_RX,
>>>> +     MMU_ATTR_RW,
>>>> +};
>>>> +
>>>> +/** pgprot_set_attrs() - Set page table permissions
>>>> + *
>>>> + * @addr: Physical address start
>>>> + * @size: size of memory to change
>>>> + * @perm: New permissions
>>>> + *
>>>> + **/
>>>> +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm);
>>>> +
>>>>    /**
>>>>     * noncached_init() - Initialize non-cached memory region
>>>>     *
>>>

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Simon Glass]

Hi Ilias,

On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Heinrich,
>
> On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > binary. Add a weak function that can be used across architectures to change
> > > the page permissions
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >   arch/arc/lib/cache.c           |  2 ++
> > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > >   arch/arm/lib/cache.c           |  2 ++
> > >   arch/m68k/lib/cache.c          |  2 ++
> > >   arch/nios2/lib/cache.c         |  2 ++
> > >   arch/powerpc/lib/cache.c       |  2 ++
> > >   arch/riscv/lib/cache.c         |  2 ++
> > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > >   arch/xtensa/lib/cache.c        |  2 ++
> > >   include/cpu_func.h             | 16 ++++++++++++++++
> > >   13 files changed, 59 insertions(+)
> > >
> > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > index 5169fc627fa5..5c79243d7223 100644
> > > --- a/arch/arc/lib/cache.c
> > > +++ b/arch/arc/lib/cache.c
> > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > >
> > >       __ic_entire_invalidate();
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > index 5b87a3af91b2..857311b3dfad 100644
> > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > >       dcache_enable();
> > >   #endif
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > index d11420d2fdd0..14c9be77db8d 100644
> > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > >   __weak void v7_outer_cache_inval_all(void) {}
> > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > index b6d08b7aad73..458a214e9577 100644
> > > --- a/arch/arm/cpu/armv7m/cache.c
> > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > >       dcache_enable();
> > >   #endif
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > index 670379e17b7a..1cf3870177ee 100644
> > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > @@ -1028,6 +1028,28 @@ skip_break:
> > >       __asm_invalidate_tlb_all();
> > >   }
> > >
> > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > +{
> > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > +
> > > +     switch (perm) {
> > > +     case MMU_ATTR_RO:
> > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > +             break;
> > > +     case MMU_ATTR_RX:
> > > +             attrs |= PTE_BLOCK_RO;
> > > +             break;
> > > +     case MMU_ATTR_RW:
> > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > +             break;
> > > +     default:
> > > +             log_err("Unknown attribute %llx\n", perm);
> > > +             return;
> > > +     }
> > > +
> > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > +}
> > > +
> > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > >
> > >   /*
> > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > index 516754caeaf9..c7704d8ee354 100644
> > > --- a/arch/arm/lib/cache.c
> > > +++ b/arch/arm/lib/cache.c
> > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > >
> > >       return 0;
> > >   }
> > > +
> > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> >
> > I would prefer if the weak function would return -ENOSYS indicating the
> > missing implementation and the real function would return 0 in case of
> > success or an error code on failure. This way the EFI protocol could set
> > the return code if the architecture does not provide support for setting
> > the attributes the passed addresses are invalid.
>
> Sure I'll change that in v2

Instead of a weak function could you define an API for it, including
structs for the information, a command to adjust it, tests, etc? This
needs a little more thought.

How about adding to cpu_ops and the information there? The cpu_func.h
which is supposed to be deprecated.

Regards,
SImon

Re: [PATCH v1 2/6] doc: update meminfo with arch specific information

[Simon Glass]

Hi Ilias,

On Wed, 5 Feb 2025 at 00:17, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Since we added support in meminfo to dump live page tables, describe
> the only working architecture for now (aarch64) and add links to public
> documentation for further reading.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  doc/usage/cmd/meminfo.rst | 71 +++++++++++++++++++++++++++++----------
>  1 file changed, 53 insertions(+), 18 deletions(-)

This is useless information for most people. I would rather have it
behind a flag.

Regards,
Simon

Re: [PATCH v1 2/6] doc: update meminfo with arch specific information

[Ilias Apalodimas]

Hi Simon

On Thu, 6 Feb 2025 at 14:31, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 5 Feb 2025 at 00:17, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Since we added support in meminfo to dump live page tables, describe
> > the only working architecture for now (aarch64) and add links to public
> > documentation for further reading.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  doc/usage/cmd/meminfo.rst | 71 +++++++++++++++++++++++++++++----------
> >  1 file changed, 53 insertions(+), 18 deletions(-)
>
> This is useless information for most people. I would rather have it
> behind a flag.
>

Ok I can move the call a few lines below CONFIG_CMD_MEMINFO_MAP

Thanks
/Ilias
> Regards,
> Simon

Re: [PATCH v1 0/6] Fix page permission on arm64 architectures

[Simon Glass]

Hi Ilias,

On Wed, 5 Feb 2025 at 00:17, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi!
> This is v1 of [0].
>
> This is an attempt to map the U-Boot binary properly, but leave the
> area we load binaries unaffected and RWX.
>
> There's a few changes since the RFC
> - Fixed the alignment of meminfo command when printing regions
> - 'meminfo' now prints arch specific attributes e.g PXN, UXN etc for arm
>   instead of RW, RO, RX
> - Since we don't set the permissions of EFI runtime services yet and keep
>   them as RWX, I removed the linker alignment changes which makes patch #3
>   easier to review. It's worth noting that qemu-arm sbsa was crashing with
>   the efi services page aligned. This is probably due to a mismatch of
>   memory, since the crash is only reproducible with QEMU instances that
>   have < 2 GB of RAM. I'll fix that along with the efi runtime services
> - Defined memory attribute changes properly with an enum for RW, RO, RX
>   instead of the hardcoded '1,2,3' I had on the RFC
> - Enabling mappings is now under a Kconfig (CONFIG_MMU_PGPROT), since
>   peope reported crashes when testing this, which are orthogonal to this
>   patch. We still have places in U-Boot where we define and later write
>   const variables. This will lead to a crash now as const variables are
>   properly managed and places in RO memory
> - Split patches to be easier to review
> - Added a patch updating 'meminfo'
> - Picked up acked-by tags from Jerome
>
> patch #1 adds printing capabilities for page mappings in 'meminfo'
> patch #2 adds documention in 'meminfo' command
> patch #3 prepares linker scripts, aligns sections in page boundaries etc
> patch #4 prepares an internal function to change the PTEs
> patch #5 adds function definitions & stubs for all archs
> patch #6 wires up the changes in U-Boot after it relocates
>
> [0] https://lore.kernel.org/u-boot/20250130072100.27297-1-ilias.apalodimas@linaro.org/
>
> Ilias Apalodimas (6):
>   meminfo: add memory details for armv8
>   doc: update meminfo with arch specific information
>   arm: Prepare linker scripts for memory permissions
>   arm64: mmu_change_region_attr() add an option not to break PTEs
>   treewide: Add a function to change page permissions
>   arm64: Enable RW, RX and RO mappings for the relocated binary
>
>  arch/arc/lib/cache.c                    |  2 +
>  arch/arm/cpu/arm926ejs/cache.c          |  2 +
>  arch/arm/cpu/armv7/cache_v7.c           |  1 +
>  arch/arm/cpu/armv7m/cache.c             |  2 +
>  arch/arm/cpu/armv8/cache_v8.c           | 54 +++++++++++++++++--
>  arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 10 ++--
>  arch/arm/cpu/armv8/u-boot.lds           | 29 +++++-----
>  arch/arm/include/asm/armv8/mmu.h        |  2 +
>  arch/arm/include/asm/system.h           | 11 +++-
>  arch/arm/lib/cache.c                    |  2 +
>  arch/arm/mach-snapdragon/board.c        |  2 +-
>  arch/m68k/lib/cache.c                   |  2 +
>  arch/nios2/lib/cache.c                  |  2 +
>  arch/powerpc/lib/cache.c                |  2 +
>  arch/riscv/lib/cache.c                  |  2 +
>  arch/sh/cpu/sh4/cache.c                 |  2 +
>  arch/xtensa/lib/cache.c                 |  2 +
>  cmd/meminfo.c                           |  5 ++
>  common/Kconfig                          | 13 +++++
>  common/board_r.c                        | 20 +++++++
>  doc/usage/cmd/meminfo.rst               | 71 ++++++++++++++++++-------
>  include/asm-generic/sections.h          |  2 +
>  include/cpu_func.h                      | 16 ++++++
>  23 files changed, 215 insertions(+), 41 deletions(-)
>
> --
> 2.47.2
>

Can you please explain what problem this fixes, in your cover letter,
to match your series subject? I don't doubt there is a big problem,
but I'm not sure what it is?

Regards,
Simon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Ilias Apalodimas]

Hi Simon,

On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Heinrich,
> >
> > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > binary. Add a weak function that can be used across architectures to change
> > > > the page permissions
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > >   arch/arc/lib/cache.c           |  2 ++
> > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > >   arch/arm/lib/cache.c           |  2 ++
> > > >   arch/m68k/lib/cache.c          |  2 ++
> > > >   arch/nios2/lib/cache.c         |  2 ++
> > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > >   arch/riscv/lib/cache.c         |  2 ++
> > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > >   13 files changed, 59 insertions(+)
> > > >
> > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > index 5169fc627fa5..5c79243d7223 100644
> > > > --- a/arch/arc/lib/cache.c
> > > > +++ b/arch/arc/lib/cache.c
> > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > >
> > > >       __ic_entire_invalidate();
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > >       dcache_enable();
> > > >   #endif
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > index b6d08b7aad73..458a214e9577 100644
> > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > >       dcache_enable();
> > > >   #endif
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > index 670379e17b7a..1cf3870177ee 100644
> > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > >       __asm_invalidate_tlb_all();
> > > >   }
> > > >
> > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > +{
> > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > +
> > > > +     switch (perm) {
> > > > +     case MMU_ATTR_RO:
> > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > +             break;
> > > > +     case MMU_ATTR_RX:
> > > > +             attrs |= PTE_BLOCK_RO;
> > > > +             break;
> > > > +     case MMU_ATTR_RW:
> > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > +             break;
> > > > +     default:
> > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > +}
> > > > +
> > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > >
> > > >   /*
> > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > index 516754caeaf9..c7704d8ee354 100644
> > > > --- a/arch/arm/lib/cache.c
> > > > +++ b/arch/arm/lib/cache.c
> > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > >
> > > >       return 0;
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > >
> > > I would prefer if the weak function would return -ENOSYS indicating the
> > > missing implementation and the real function would return 0 in case of
> > > success or an error code on failure. This way the EFI protocol could set
> > > the return code if the architecture does not provide support for setting
> > > the attributes the passed addresses are invalid.
> >
> > Sure I'll change that in v2
>
> Instead of a weak function could you define an API for it, including
> structs for the information, a command to adjust it, tests, etc? This
> needs a little more thought.
>
> How about adding to cpu_ops and the information there? The cpu_func.h
> which is supposed to be deprecated.

Looking at it, I think it's easier as well. I can add move that
function in cpu_ops, so it becomes NULL for other architectures. But
since this patchset is doing enough already, I'll just move that. In
the future, we can move all the cache* and mapping* functions there as
well

Thanks
/Ilias
>
> Regards,
> SImon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Simon Glass]

Hi Ilias,

On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Heinrich,
> > >
> > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > > binary. Add a weak function that can be used across architectures to change
> > > > > the page permissions
> > > > >
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > ---
> > > > >   arch/arc/lib/cache.c           |  2 ++
> > > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > > >   arch/arm/lib/cache.c           |  2 ++
> > > > >   arch/m68k/lib/cache.c          |  2 ++
> > > > >   arch/nios2/lib/cache.c         |  2 ++
> > > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > > >   arch/riscv/lib/cache.c         |  2 ++
> > > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > > >   13 files changed, 59 insertions(+)
> > > > >
> > > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > > index 5169fc627fa5..5c79243d7223 100644
> > > > > --- a/arch/arc/lib/cache.c
> > > > > +++ b/arch/arc/lib/cache.c
> > > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > > >
> > > > >       __ic_entire_invalidate();
> > > > >   }
> > > > > +
> > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > > >       dcache_enable();
> > > > >   #endif
> > > > >   }
> > > > > +
> > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > > index b6d08b7aad73..458a214e9577 100644
> > > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > > >       dcache_enable();
> > > > >   #endif
> > > > >   }
> > > > > +
> > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > > index 670379e17b7a..1cf3870177ee 100644
> > > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > > >       __asm_invalidate_tlb_all();
> > > > >   }
> > > > >
> > > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > > +{
> > > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > > +
> > > > > +     switch (perm) {
> > > > > +     case MMU_ATTR_RO:
> > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > > +             break;
> > > > > +     case MMU_ATTR_RX:
> > > > > +             attrs |= PTE_BLOCK_RO;
> > > > > +             break;
> > > > > +     case MMU_ATTR_RW:
> > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > > +             break;
> > > > > +     default:
> > > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > > +             return;
> > > > > +     }
> > > > > +
> > > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > > +}
> > > > > +
> > > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > > >
> > > > >   /*
> > > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > > index 516754caeaf9..c7704d8ee354 100644
> > > > > --- a/arch/arm/lib/cache.c
> > > > > +++ b/arch/arm/lib/cache.c
> > > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > > >
> > > > >       return 0;
> > > > >   }
> > > > > +
> > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > >
> > > > I would prefer if the weak function would return -ENOSYS indicating the
> > > > missing implementation and the real function would return 0 in case of
> > > > success or an error code on failure. This way the EFI protocol could set
> > > > the return code if the architecture does not provide support for setting
> > > > the attributes the passed addresses are invalid.
> > >
> > > Sure I'll change that in v2
> >
> > Instead of a weak function could you define an API for it, including
> > structs for the information, a command to adjust it, tests, etc? This
> > needs a little more thought.
> >
> > How about adding to cpu_ops and the information there? The cpu_func.h
> > which is supposed to be deprecated.
>
> Looking at it, I think it's easier as well. I can add move that
> function in cpu_ops, so it becomes NULL for other architectures. But
> since this patchset is doing enough already, I'll just move that. In
> the future, we can move all the cache* and mapping* functions there as
> well

That sounds great, thank you. I suspect that RISC-V will follow your lead here.

Regards,
Simon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Ilias Apalodimas]

On Thu, 6 Feb 2025 at 14:58, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Heinrich,
> > > >
> > > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > > > binary. Add a weak function that can be used across architectures to change
> > > > > > the page permissions
> > > > > >
> > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > ---
> > > > > >   arch/arc/lib/cache.c           |  2 ++
> > > > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > > > >   arch/arm/lib/cache.c           |  2 ++
> > > > > >   arch/m68k/lib/cache.c          |  2 ++
> > > > > >   arch/nios2/lib/cache.c         |  2 ++
> > > > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > > > >   arch/riscv/lib/cache.c         |  2 ++
> > > > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > > > >   13 files changed, 59 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > > > index 5169fc627fa5..5c79243d7223 100644
> > > > > > --- a/arch/arc/lib/cache.c
> > > > > > +++ b/arch/arc/lib/cache.c
> > > > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > > > >
> > > > > >       __ic_entire_invalidate();
> > > > > >   }
> > > > > > +
> > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > > > >       dcache_enable();
> > > > > >   #endif
> > > > > >   }
> > > > > > +
> > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > > > index b6d08b7aad73..458a214e9577 100644
> > > > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > > > >       dcache_enable();
> > > > > >   #endif
> > > > > >   }
> > > > > > +
> > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > index 670379e17b7a..1cf3870177ee 100644
> > > > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > > > >       __asm_invalidate_tlb_all();
> > > > > >   }
> > > > > >
> > > > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > > > +{
> > > > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > > > +
> > > > > > +     switch (perm) {
> > > > > > +     case MMU_ATTR_RO:
> > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > > > +             break;
> > > > > > +     case MMU_ATTR_RX:
> > > > > > +             attrs |= PTE_BLOCK_RO;
> > > > > > +             break;
> > > > > > +     case MMU_ATTR_RW:
> > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > > > +             break;
> > > > > > +     default:
> > > > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > > > +             return;
> > > > > > +     }
> > > > > > +
> > > > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > > > +}
> > > > > > +
> > > > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > > > >
> > > > > >   /*
> > > > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > > > index 516754caeaf9..c7704d8ee354 100644
> > > > > > --- a/arch/arm/lib/cache.c
> > > > > > +++ b/arch/arm/lib/cache.c
> > > > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > > > >
> > > > > >       return 0;
> > > > > >   }
> > > > > > +
> > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > >
> > > > > I would prefer if the weak function would return -ENOSYS indicating the
> > > > > missing implementation and the real function would return 0 in case of
> > > > > success or an error code on failure. This way the EFI protocol could set
> > > > > the return code if the architecture does not provide support for setting
> > > > > the attributes the passed addresses are invalid.
> > > >
> > > > Sure I'll change that in v2
> > >
> > > Instead of a weak function could you define an API for it, including
> > > structs for the information, a command to adjust it, tests, etc? This
> > > needs a little more thought.
> > >
> > > How about adding to cpu_ops and the information there? The cpu_func.h
> > > which is supposed to be deprecated.
> >
> > Looking at it, I think it's easier as well. I can add move that
> > function in cpu_ops, so it becomes NULL for other architectures. But
> > since this patchset is doing enough already, I'll just move that. In
> > the future, we can move all the cache* and mapping* functions there as
> > well
>
> That sounds great, thank you. I suspect that RISC-V will follow your lead here.

I wrote the code but cpu uclass isn't even close to doing what we want.
Grepping for defined boards with a cpu uclass driver for arm gives me
back < 20 boards. Given the fact we already found 3 bugs with this
patch applied, I prefer to have a wider coverage and fix U-Boot.
I've uploaded the uclass version here [0], once more boards decide to
use it I will be happy to switch to that, but unfortunately for now,
I'll am obliged to keep the weak function definition.

[0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/fix_memory_permissions_uclass?ref_type=heads

Thanks
/Ilias

>
> Regards,
> Simon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Simon Glass]

Hi Ilias,

On Thu, 6 Feb 2025 at 08:16, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Thu, 6 Feb 2025 at 14:58, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Heinrich,
> > > > >
> > > > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > > > > binary. Add a weak function that can be used across architectures to change
> > > > > > > the page permissions
> > > > > > >
> > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > ---
> > > > > > >   arch/arc/lib/cache.c           |  2 ++
> > > > > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > > > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > > > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > > > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > > > > >   arch/arm/lib/cache.c           |  2 ++
> > > > > > >   arch/m68k/lib/cache.c          |  2 ++
> > > > > > >   arch/nios2/lib/cache.c         |  2 ++
> > > > > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > > > > >   arch/riscv/lib/cache.c         |  2 ++
> > > > > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > > > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > > > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > > > > >   13 files changed, 59 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > > > > index 5169fc627fa5..5c79243d7223 100644
> > > > > > > --- a/arch/arc/lib/cache.c
> > > > > > > +++ b/arch/arc/lib/cache.c
> > > > > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > > > > >
> > > > > > >       __ic_entire_invalidate();
> > > > > > >   }
> > > > > > > +
> > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > > > > >       dcache_enable();
> > > > > > >   #endif
> > > > > > >   }
> > > > > > > +
> > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > > > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > > > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > > > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > > > > index b6d08b7aad73..458a214e9577 100644
> > > > > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > > > > >       dcache_enable();
> > > > > > >   #endif
> > > > > > >   }
> > > > > > > +
> > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > index 670379e17b7a..1cf3870177ee 100644
> > > > > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > > > > >       __asm_invalidate_tlb_all();
> > > > > > >   }
> > > > > > >
> > > > > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > > > > +{
> > > > > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > > > > +
> > > > > > > +     switch (perm) {
> > > > > > > +     case MMU_ATTR_RO:
> > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > > > > +             break;
> > > > > > > +     case MMU_ATTR_RX:
> > > > > > > +             attrs |= PTE_BLOCK_RO;
> > > > > > > +             break;
> > > > > > > +     case MMU_ATTR_RW:
> > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > > > > +             break;
> > > > > > > +     default:
> > > > > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > > > > +             return;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > > > > +}
> > > > > > > +
> > > > > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > > > > >
> > > > > > >   /*
> > > > > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > > > > index 516754caeaf9..c7704d8ee354 100644
> > > > > > > --- a/arch/arm/lib/cache.c
> > > > > > > +++ b/arch/arm/lib/cache.c
> > > > > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > > > > >
> > > > > > >       return 0;
> > > > > > >   }
> > > > > > > +
> > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > >
> > > > > > I would prefer if the weak function would return -ENOSYS indicating the
> > > > > > missing implementation and the real function would return 0 in case of
> > > > > > success or an error code on failure. This way the EFI protocol could set
> > > > > > the return code if the architecture does not provide support for setting
> > > > > > the attributes the passed addresses are invalid.
> > > > >
> > > > > Sure I'll change that in v2
> > > >
> > > > Instead of a weak function could you define an API for it, including
> > > > structs for the information, a command to adjust it, tests, etc? This
> > > > needs a little more thought.
> > > >
> > > > How about adding to cpu_ops and the information there? The cpu_func.h
> > > > which is supposed to be deprecated.
> > >
> > > Looking at it, I think it's easier as well. I can add move that
> > > function in cpu_ops, so it becomes NULL for other architectures. But
> > > since this patchset is doing enough already, I'll just move that. In
> > > the future, we can move all the cache* and mapping* functions there as
> > > well
> >
> > That sounds great, thank you. I suspect that RISC-V will follow your lead here.
>
> I wrote the code but cpu uclass isn't even close to doing what we want.
> Grepping for defined boards with a cpu uclass driver for arm gives me
> back < 20 boards. Given the fact we already found 3 bugs with this
> patch applied, I prefer to have a wider coverage and fix U-Boot.
> I've uploaded the uclass version here [0], once more boards decide to
> use it I will be happy to switch to that, but unfortunately for now,
> I'll am obliged to keep the weak function definition.

Please at least post the patches as an RFC.

We had the same issue with EFI_LOADER back in the day (not wanting to
depend on CONFIG_DM) and it has cost us a lot of time.

Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
way you want to go, I'd be happy to do a precursor series to deal with
the fallout.

>
> [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/fix_memory_permissions_uclass?ref_type=heads

Regards,
Simon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Ilias Apalodimas]

Hi Simon,

On Thu, 6 Feb 2025 at 17:48, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 6 Feb 2025 at 08:16, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Thu, 6 Feb 2025 at 14:58, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ilias,
> > > > >
> > > > > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > > > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > > > > > binary. Add a weak function that can be used across architectures to change
> > > > > > > > the page permissions
> > > > > > > >
> > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > > ---
> > > > > > > >   arch/arc/lib/cache.c           |  2 ++
> > > > > > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > > > > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > > > > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > > > > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > > > > > >   arch/arm/lib/cache.c           |  2 ++
> > > > > > > >   arch/m68k/lib/cache.c          |  2 ++
> > > > > > > >   arch/nios2/lib/cache.c         |  2 ++
> > > > > > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > > > > > >   arch/riscv/lib/cache.c         |  2 ++
> > > > > > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > > > > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > > > > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > > > > > >   13 files changed, 59 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > > > > > index 5169fc627fa5..5c79243d7223 100644
> > > > > > > > --- a/arch/arc/lib/cache.c
> > > > > > > > +++ b/arch/arc/lib/cache.c
> > > > > > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > > > > > >
> > > > > > > >       __ic_entire_invalidate();
> > > > > > > >   }
> > > > > > > > +
> > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > > > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > > > > > >       dcache_enable();
> > > > > > > >   #endif
> > > > > > > >   }
> > > > > > > > +
> > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > > > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > > > > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > > > > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > > > > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > > > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > > > > > index b6d08b7aad73..458a214e9577 100644
> > > > > > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > > > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > > > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > > > > > >       dcache_enable();
> > > > > > > >   #endif
> > > > > > > >   }
> > > > > > > > +
> > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > index 670379e17b7a..1cf3870177ee 100644
> > > > > > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > > > > > >       __asm_invalidate_tlb_all();
> > > > > > > >   }
> > > > > > > >
> > > > > > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > > > > > +{
> > > > > > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > > > > > +
> > > > > > > > +     switch (perm) {
> > > > > > > > +     case MMU_ATTR_RO:
> > > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > > > > > +             break;
> > > > > > > > +     case MMU_ATTR_RX:
> > > > > > > > +             attrs |= PTE_BLOCK_RO;
> > > > > > > > +             break;
> > > > > > > > +     case MMU_ATTR_RW:
> > > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > > > > > +             break;
> > > > > > > > +     default:
> > > > > > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > > > > > +             return;
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > > > > > >
> > > > > > > >   /*
> > > > > > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > > > > > index 516754caeaf9..c7704d8ee354 100644
> > > > > > > > --- a/arch/arm/lib/cache.c
> > > > > > > > +++ b/arch/arm/lib/cache.c
> > > > > > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > > > > > >
> > > > > > > >       return 0;
> > > > > > > >   }
> > > > > > > > +
> > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > >
> > > > > > > I would prefer if the weak function would return -ENOSYS indicating the
> > > > > > > missing implementation and the real function would return 0 in case of
> > > > > > > success or an error code on failure. This way the EFI protocol could set
> > > > > > > the return code if the architecture does not provide support for setting
> > > > > > > the attributes the passed addresses are invalid.
> > > > > >
> > > > > > Sure I'll change that in v2
> > > > >
> > > > > Instead of a weak function could you define an API for it, including
> > > > > structs for the information, a command to adjust it, tests, etc? This
> > > > > needs a little more thought.
> > > > >
> > > > > How about adding to cpu_ops and the information there? The cpu_func.h
> > > > > which is supposed to be deprecated.
> > > >
> > > > Looking at it, I think it's easier as well. I can add move that
> > > > function in cpu_ops, so it becomes NULL for other architectures. But
> > > > since this patchset is doing enough already, I'll just move that. In
> > > > the future, we can move all the cache* and mapping* functions there as
> > > > well
> > >
> > > That sounds great, thank you. I suspect that RISC-V will follow your lead here.
> >
> > I wrote the code but cpu uclass isn't even close to doing what we want.
> > Grepping for defined boards with a cpu uclass driver for arm gives me
> > back < 20 boards. Given the fact we already found 3 bugs with this
> > patch applied, I prefer to have a wider coverage and fix U-Boot.
> > I've uploaded the uclass version here [0], once more boards decide to
> > use it I will be happy to switch to that, but unfortunately for now,
> > I'll am obliged to keep the weak function definition.
>
> Please at least post the patches as an RFC.
>
> We had the same issue with EFI_LOADER back in the day (not wanting to
> depend on CONFIG_DM) and it has cost us a lot of time.
>
> Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
> way you want to go, I'd be happy to do a precursor series to deal with
> the fallout.

That's not the problem. I can easily add a select of CPU_CONFIG on
that new Kconfig.

The problem is that I  grep 20 boards supporting it, and out of those
20 boards only 6-7 are usable. The rest have cpu compatible nodes that
don't exist on any DT. e.g grep for 'xlnx,microblaze-7.10'. It's
defined as a 'cpu driver' but there are no DTs. As a result that cpu
dm pointer the code relies on, never gets created.

So the result is that this code will never execute unless you run on
very few boards. I've already sent a link to code that does use DM and
'works', so if and when boards adopt it I can resend it.

Thanks
/Ilias

>
> >
> > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/fix_memory_permissions_uclass?ref_type=heads
>
> Regards,
> Simon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Simon Glass]

Hi Ilias,

On Thu, 6 Feb 2025 at 09:21, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 6 Feb 2025 at 17:48, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 6 Feb 2025 at 08:16, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > On Thu, 6 Feb 2025 at 14:58, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >
> > > > > > > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > > > > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > > > > > > binary. Add a weak function that can be used across architectures to change
> > > > > > > > > the page permissions
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > > > ---
> > > > > > > > >   arch/arc/lib/cache.c           |  2 ++
> > > > > > > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > > > > > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > > > > > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > > > > > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > > > > > > >   arch/arm/lib/cache.c           |  2 ++
> > > > > > > > >   arch/m68k/lib/cache.c          |  2 ++
> > > > > > > > >   arch/nios2/lib/cache.c         |  2 ++
> > > > > > > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > > > > > > >   arch/riscv/lib/cache.c         |  2 ++
> > > > > > > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > > > > > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > > > > > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > > > > > > >   13 files changed, 59 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > > > > > > index 5169fc627fa5..5c79243d7223 100644
> > > > > > > > > --- a/arch/arc/lib/cache.c
> > > > > > > > > +++ b/arch/arc/lib/cache.c
> > > > > > > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > > > > > > >
> > > > > > > > >       __ic_entire_invalidate();
> > > > > > > > >   }
> > > > > > > > > +
> > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > > > > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > > > > > > >       dcache_enable();
> > > > > > > > >   #endif
> > > > > > > > >   }
> > > > > > > > > +
> > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > > > > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > > > > > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > > > > > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > > > > > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > > > > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > > > > > > index b6d08b7aad73..458a214e9577 100644
> > > > > > > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > > > > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > > > > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > > > > > > >       dcache_enable();
> > > > > > > > >   #endif
> > > > > > > > >   }
> > > > > > > > > +
> > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > > index 670379e17b7a..1cf3870177ee 100644
> > > > > > > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > > > > > > >       __asm_invalidate_tlb_all();
> > > > > > > > >   }
> > > > > > > > >
> > > > > > > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > > > > > > +{
> > > > > > > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > > > > > > +
> > > > > > > > > +     switch (perm) {
> > > > > > > > > +     case MMU_ATTR_RO:
> > > > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > > > > > > +             break;
> > > > > > > > > +     case MMU_ATTR_RX:
> > > > > > > > > +             attrs |= PTE_BLOCK_RO;
> > > > > > > > > +             break;
> > > > > > > > > +     case MMU_ATTR_RW:
> > > > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > > > > > > +             break;
> > > > > > > > > +     default:
> > > > > > > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > > > > > > +             return;
> > > > > > > > > +     }
> > > > > > > > > +
> > > > > > > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > > > > > > >
> > > > > > > > >   /*
> > > > > > > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > > > > > > index 516754caeaf9..c7704d8ee354 100644
> > > > > > > > > --- a/arch/arm/lib/cache.c
> > > > > > > > > +++ b/arch/arm/lib/cache.c
> > > > > > > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > > > > > > >
> > > > > > > > >       return 0;
> > > > > > > > >   }
> > > > > > > > > +
> > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > >
> > > > > > > > I would prefer if the weak function would return -ENOSYS indicating the
> > > > > > > > missing implementation and the real function would return 0 in case of
> > > > > > > > success or an error code on failure. This way the EFI protocol could set
> > > > > > > > the return code if the architecture does not provide support for setting
> > > > > > > > the attributes the passed addresses are invalid.
> > > > > > >
> > > > > > > Sure I'll change that in v2
> > > > > >
> > > > > > Instead of a weak function could you define an API for it, including
> > > > > > structs for the information, a command to adjust it, tests, etc? This
> > > > > > needs a little more thought.
> > > > > >
> > > > > > How about adding to cpu_ops and the information there? The cpu_func.h
> > > > > > which is supposed to be deprecated.
> > > > >
> > > > > Looking at it, I think it's easier as well. I can add move that
> > > > > function in cpu_ops, so it becomes NULL for other architectures. But
> > > > > since this patchset is doing enough already, I'll just move that. In
> > > > > the future, we can move all the cache* and mapping* functions there as
> > > > > well
> > > >
> > > > That sounds great, thank you. I suspect that RISC-V will follow your lead here.
> > >
> > > I wrote the code but cpu uclass isn't even close to doing what we want.
> > > Grepping for defined boards with a cpu uclass driver for arm gives me
> > > back < 20 boards. Given the fact we already found 3 bugs with this
> > > patch applied, I prefer to have a wider coverage and fix U-Boot.
> > > I've uploaded the uclass version here [0], once more boards decide to
> > > use it I will be happy to switch to that, but unfortunately for now,
> > > I'll am obliged to keep the weak function definition.
> >
> > Please at least post the patches as an RFC.
> >
> > We had the same issue with EFI_LOADER back in the day (not wanting to
> > depend on CONFIG_DM) and it has cost us a lot of time.
> >
> > Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
> > way you want to go, I'd be happy to do a precursor series to deal with
> > the fallout.
>
> That's not the problem. I can easily add a select of CPU_CONFIG on
> that new Kconfig.
>
> The problem is that I  grep 20 boards supporting it, and out of those
> 20 boards only 6-7 are usable. The rest have cpu compatible nodes that
> don't exist on any DT. e.g grep for 'xlnx,microblaze-7.10'. It's
> defined as a 'cpu driver' but there are no DTs. As a result that cpu
> dm pointer the code relies on, never gets created.
>
> So the result is that this code will never execute unless you run on
> very few boards. I've already sent a link to code that does use DM and
> 'works', so if and when boards adopt it I can resend it.

So perhaps very few boards care? Those that do want this new feature
could enable CPU.

You could even add both interfaces and encourage people to use the CPU
one. At least then people who are trying to DTRT can do so.

I don't like this approach to developing new features - hacking in a
non-DM API so that the code is used more. This is what happened with
EFI_LOADER too. But it's your tree and you obviously don't agree, so
go for it!

Regards,
Simon



>
> Thanks
> /Ilias
>
> >
> > >
> > > [0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/fix_memory_permissions_uclass?ref_type=heads
> >
> > Regards,
> > Simon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 10432 bytes --]

On Sun, Feb 09, 2025 at 07:35:31AM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Thu, 6 Feb 2025 at 09:21, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Thu, 6 Feb 2025 at 17:48, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Thu, 6 Feb 2025 at 08:16, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > On Thu, 6 Feb 2025 at 14:58, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ilias,
> > > > >
> > > > > On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Ilias,
> > > > > > >
> > > > > > > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Hi Heinrich,
> > > > > > > >
> > > > > > > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > >
> > > > > > > > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > > > > > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > > > > > > > binary. Add a weak function that can be used across architectures to change
> > > > > > > > > > the page permissions
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > >   arch/arc/lib/cache.c           |  2 ++
> > > > > > > > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > > > > > > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > > > > > > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > > > > > > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > > > > > > > >   arch/arm/lib/cache.c           |  2 ++
> > > > > > > > > >   arch/m68k/lib/cache.c          |  2 ++
> > > > > > > > > >   arch/nios2/lib/cache.c         |  2 ++
> > > > > > > > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > > > > > > > >   arch/riscv/lib/cache.c         |  2 ++
> > > > > > > > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > > > > > > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > > > > > > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > > > > > > > >   13 files changed, 59 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > > > > > > > index 5169fc627fa5..5c79243d7223 100644
> > > > > > > > > > --- a/arch/arc/lib/cache.c
> > > > > > > > > > +++ b/arch/arc/lib/cache.c
> > > > > > > > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > > > > > > > >
> > > > > > > > > >       __ic_entire_invalidate();
> > > > > > > > > >   }
> > > > > > > > > > +
> > > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > > > > > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > > > > > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > > > > > > > >       dcache_enable();
> > > > > > > > > >   #endif
> > > > > > > > > >   }
> > > > > > > > > > +
> > > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > > > > > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > > > > > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > > > > > > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > > > > > > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > > > > > > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > > > > > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > > > > > > > index b6d08b7aad73..458a214e9577 100644
> > > > > > > > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > > > > > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > > > > > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > > > > > > > >       dcache_enable();
> > > > > > > > > >   #endif
> > > > > > > > > >   }
> > > > > > > > > > +
> > > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > > > index 670379e17b7a..1cf3870177ee 100644
> > > > > > > > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > > > > > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > > > > > > > >       __asm_invalidate_tlb_all();
> > > > > > > > > >   }
> > > > > > > > > >
> > > > > > > > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > > > > > > > +{
> > > > > > > > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > > > > > > > +
> > > > > > > > > > +     switch (perm) {
> > > > > > > > > > +     case MMU_ATTR_RO:
> > > > > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > > > > > > > +             break;
> > > > > > > > > > +     case MMU_ATTR_RX:
> > > > > > > > > > +             attrs |= PTE_BLOCK_RO;
> > > > > > > > > > +             break;
> > > > > > > > > > +     case MMU_ATTR_RW:
> > > > > > > > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > > > > > > > +             break;
> > > > > > > > > > +     default:
> > > > > > > > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > > > > > > > +             return;
> > > > > > > > > > +     }
> > > > > > > > > > +
> > > > > > > > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > > > > > > > >
> > > > > > > > > >   /*
> > > > > > > > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > > > > > > > index 516754caeaf9..c7704d8ee354 100644
> > > > > > > > > > --- a/arch/arm/lib/cache.c
> > > > > > > > > > +++ b/arch/arm/lib/cache.c
> > > > > > > > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > > > > > > > >
> > > > > > > > > >       return 0;
> > > > > > > > > >   }
> > > > > > > > > > +
> > > > > > > > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > > > > > >
> > > > > > > > > I would prefer if the weak function would return -ENOSYS indicating the
> > > > > > > > > missing implementation and the real function would return 0 in case of
> > > > > > > > > success or an error code on failure. This way the EFI protocol could set
> > > > > > > > > the return code if the architecture does not provide support for setting
> > > > > > > > > the attributes the passed addresses are invalid.
> > > > > > > >
> > > > > > > > Sure I'll change that in v2
> > > > > > >
> > > > > > > Instead of a weak function could you define an API for it, including
> > > > > > > structs for the information, a command to adjust it, tests, etc? This
> > > > > > > needs a little more thought.
> > > > > > >
> > > > > > > How about adding to cpu_ops and the information there? The cpu_func.h
> > > > > > > which is supposed to be deprecated.
> > > > > >
> > > > > > Looking at it, I think it's easier as well. I can add move that
> > > > > > function in cpu_ops, so it becomes NULL for other architectures. But
> > > > > > since this patchset is doing enough already, I'll just move that. In
> > > > > > the future, we can move all the cache* and mapping* functions there as
> > > > > > well
> > > > >
> > > > > That sounds great, thank you. I suspect that RISC-V will follow your lead here.
> > > >
> > > > I wrote the code but cpu uclass isn't even close to doing what we want.
> > > > Grepping for defined boards with a cpu uclass driver for arm gives me
> > > > back < 20 boards. Given the fact we already found 3 bugs with this
> > > > patch applied, I prefer to have a wider coverage and fix U-Boot.
> > > > I've uploaded the uclass version here [0], once more boards decide to
> > > > use it I will be happy to switch to that, but unfortunately for now,
> > > > I'll am obliged to keep the weak function definition.
> > >
> > > Please at least post the patches as an RFC.
> > >
> > > We had the same issue with EFI_LOADER back in the day (not wanting to
> > > depend on CONFIG_DM) and it has cost us a lot of time.
> > >
> > > Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
> > > way you want to go, I'd be happy to do a precursor series to deal with
> > > the fallout.
> >
> > That's not the problem. I can easily add a select of CPU_CONFIG on
> > that new Kconfig.
> >
> > The problem is that I  grep 20 boards supporting it, and out of those
> > 20 boards only 6-7 are usable. The rest have cpu compatible nodes that
> > don't exist on any DT. e.g grep for 'xlnx,microblaze-7.10'. It's
> > defined as a 'cpu driver' but there are no DTs. As a result that cpu
> > dm pointer the code relies on, never gets created.
> >
> > So the result is that this code will never execute unless you run on
> > very few boards. I've already sent a link to code that does use DM and
> > 'works', so if and when boards adopt it I can resend it.
> 
> So perhaps very few boards care? Those that do want this new feature
> could enable CPU.
> 
> You could even add both interfaces and encourage people to use the CPU
> one. At least then people who are trying to DTRT can do so.
> 
> I don't like this approach to developing new features - hacking in a
> non-DM API so that the code is used more. This is what happened with
> EFI_LOADER too. But it's your tree and you obviously don't agree, so
> go for it!

Can you PLEASE stop implying that the mainline tree is the Linaro tree?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5692 bytes --]

On Thu, Feb 06, 2025 at 05:30:43AM -0700, Simon Glass wrote:
> Hi Ilias,
> 
> On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Heinrich,
> >
> > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 2/5/25 08:16, Ilias Apalodimas wrote:
> > > > For armv8 we are adding proper page permissions for the relocated U-Boot
> > > > binary. Add a weak function that can be used across architectures to change
> > > > the page permissions
> > > >
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > >   arch/arc/lib/cache.c           |  2 ++
> > > >   arch/arm/cpu/arm926ejs/cache.c |  2 ++
> > > >   arch/arm/cpu/armv7/cache_v7.c  |  1 +
> > > >   arch/arm/cpu/armv7m/cache.c    |  2 ++
> > > >   arch/arm/cpu/armv8/cache_v8.c  | 22 ++++++++++++++++++++++
> > > >   arch/arm/lib/cache.c           |  2 ++
> > > >   arch/m68k/lib/cache.c          |  2 ++
> > > >   arch/nios2/lib/cache.c         |  2 ++
> > > >   arch/powerpc/lib/cache.c       |  2 ++
> > > >   arch/riscv/lib/cache.c         |  2 ++
> > > >   arch/sh/cpu/sh4/cache.c        |  2 ++
> > > >   arch/xtensa/lib/cache.c        |  2 ++
> > > >   include/cpu_func.h             | 16 ++++++++++++++++
> > > >   13 files changed, 59 insertions(+)
> > > >
> > > > diff --git a/arch/arc/lib/cache.c b/arch/arc/lib/cache.c
> > > > index 5169fc627fa5..5c79243d7223 100644
> > > > --- a/arch/arc/lib/cache.c
> > > > +++ b/arch/arc/lib/cache.c
> > > > @@ -819,3 +819,5 @@ void sync_n_cleanup_cache_all(void)
> > > >
> > > >       __ic_entire_invalidate();
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/arm926ejs/cache.c b/arch/arm/cpu/arm926ejs/cache.c
> > > > index 5b87a3af91b2..857311b3dfad 100644
> > > > --- a/arch/arm/cpu/arm926ejs/cache.c
> > > > +++ b/arch/arm/cpu/arm926ejs/cache.c
> > > > @@ -88,3 +88,5 @@ void enable_caches(void)
> > > >       dcache_enable();
> > > >   #endif
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/armv7/cache_v7.c b/arch/arm/cpu/armv7/cache_v7.c
> > > > index d11420d2fdd0..14c9be77db8d 100644
> > > > --- a/arch/arm/cpu/armv7/cache_v7.c
> > > > +++ b/arch/arm/cpu/armv7/cache_v7.c
> > > > @@ -209,3 +209,4 @@ __weak void v7_outer_cache_flush_all(void) {}
> > > >   __weak void v7_outer_cache_inval_all(void) {}
> > > >   __weak void v7_outer_cache_flush_range(u32 start, u32 end) {}
> > > >   __weak void v7_outer_cache_inval_range(u32 start, u32 end) {}
> > > > +__weak void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/armv7m/cache.c b/arch/arm/cpu/armv7m/cache.c
> > > > index b6d08b7aad73..458a214e9577 100644
> > > > --- a/arch/arm/cpu/armv7m/cache.c
> > > > +++ b/arch/arm/cpu/armv7m/cache.c
> > > > @@ -370,3 +370,5 @@ void enable_caches(void)
> > > >       dcache_enable();
> > > >   #endif
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > > > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > > > index 670379e17b7a..1cf3870177ee 100644
> > > > --- a/arch/arm/cpu/armv8/cache_v8.c
> > > > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > > > @@ -1028,6 +1028,28 @@ skip_break:
> > > >       __asm_invalidate_tlb_all();
> > > >   }
> > > >
> > > > +void pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm)
> > > > +{
> > > > +     u64 attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) | PTE_BLOCK_INNER_SHARE | PTE_TYPE_VALID;
> > > > +
> > > > +     switch (perm) {
> > > > +     case MMU_ATTR_RO:
> > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN | PTE_BLOCK_RO;
> > > > +             break;
> > > > +     case MMU_ATTR_RX:
> > > > +             attrs |= PTE_BLOCK_RO;
> > > > +             break;
> > > > +     case MMU_ATTR_RW:
> > > > +             attrs |= PTE_BLOCK_PXN | PTE_BLOCK_UXN;
> > > > +             break;
> > > > +     default:
> > > > +             log_err("Unknown attribute %llx\n", perm);
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     mmu_change_region_attr(addr, size, attrs, false);
> > > > +}
> > > > +
> > > >   #else       /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > > >
> > > >   /*
> > > > diff --git a/arch/arm/lib/cache.c b/arch/arm/lib/cache.c
> > > > index 516754caeaf9..c7704d8ee354 100644
> > > > --- a/arch/arm/lib/cache.c
> > > > +++ b/arch/arm/lib/cache.c
> > > > @@ -170,3 +170,5 @@ __weak int arm_reserve_mmu(void)
> > > >
> > > >       return 0;
> > > >   }
> > > > +
> > > > +void __weak pgprot_set_attrs(phys_addr_t addr, size_t size, u64 perm) {}
> > >
> > > I would prefer if the weak function would return -ENOSYS indicating the
> > > missing implementation and the real function would return 0 in case of
> > > success or an error code on failure. This way the EFI protocol could set
> > > the return code if the architecture does not provide support for setting
> > > the attributes the passed addresses are invalid.
> >
> > Sure I'll change that in v2
> 
> Instead of a weak function could you define an API for it, including
> structs for the information, a command to adjust it, tests, etc? This
> needs a little more thought.

This is an API. It's just not part of DM because it's frankly
inappropriate to be configuring part of the MMU ASAP only once we've got
our device tree parsed. DM and a uclass is not always the right
abstraction.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

On Thu, Feb 06, 2025 at 08:47:47AM -0700, Simon Glass wrote:

[snip]
> Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
> way you want to go, I'd be happy to do a precursor series to deal with
> the fallout.

I'm not sure what EFI_LOADER has to do with the generic security feature
of enforcing permissions on pages. That's something we want everywhere
that can enable it as it's a good defensive security measure and also
catches code bugs.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Simon Glass]

Hi Tom,

On Sun, 9 Feb 2025 at 09:39, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Feb 06, 2025 at 08:47:47AM -0700, Simon Glass wrote:
>
> [snip]
> > Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
> > way you want to go, I'd be happy to do a precursor series to deal with
> > the fallout.
>
> I'm not sure what EFI_LOADER has to do with the generic security feature
> of enforcing permissions on pages. That's something we want everywhere
> that can enable it as it's a good defensive security measure and also
> catches code bugs.

Yes, it's a good thing to have. I assumed it was related to EFI
because of all the mention of EFI, SetVirtualAddressMap() and the
like.

It doesn't have to be DM. I was reacting to the idea that we cannot
add it to the CPU driver because hardly any boards have one. How about
mapping arch-specific stuff to generic functions, like we try to do
with the CPU uclass. The enforcement happens before initr_dm()
although I suppose it could be moved later, or a CPU driver could be
started up before relocation. Or just don't use a CPU driver, use
something else.

WIth all the pain I've just been through with the EFI link scripts, I
would have rather seen some effort to follow the existing convention,
e.g. text_start rather than start_text. We already have
__image_copy_start - there is so much arch-specific variability here
already.

Anyway, I'll stay away from this series in future.

Regards,
Simon

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Heinrich Schuchardt]

Am 9. Februar 2025 21:15:53 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Tom,
>
>On Sun, 9 Feb 2025 at 09:39, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Thu, Feb 06, 2025 at 08:47:47AM -0700, Simon Glass wrote:
>>
>> [snip]
>> > Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
>> > way you want to go, I'd be happy to do a precursor series to deal with
>> > the fallout.
>>
>> I'm not sure what EFI_LOADER has to do with the generic security feature
>> of enforcing permissions on pages. That's something we want everywhere
>> that can enable it as it's a good defensive security measure and also
>> catches code bugs.
>
>Yes, it's a good thing to have. I assumed it was related to EFI
>because of all the mention of EFI, SetVirtualAddressMap() and the
>like.
>
>It doesn't have to be DM. I was reacting to the idea that we cannot
>add it to the CPU driver because hardly any boards have one. How about
>mapping arch-specific stuff to generic functions, like we try to do
>with the CPU uclass. The enforcement happens before initr_dm()
>although I suppose it could be moved later, or a CPU driver could be
>started up before relocation. Or just don't use a CPU driver, use
>something else.
>
>WIth all the pain I've just been through with the EFI link scripts, I
>would have rather seen some effort to follow the existing convention,
>e.g. text_start rather than start_text. We already have
>__image_copy_start - there is so much arch-specific variability here
>already.

Like we did for the EFI linker scripts we should standardize the u-boot binary linker scripts by using a common linker script include.

Best regards

Heinrich


>
>Anyway, I'll stay away from this series in future.
>
>Regards,
>Simon

Re: [PATCH v1 1/6] meminfo: add memory details for armv8

[Caleb Connolly]

On 2/5/25 07:16, Ilias Apalodimas wrote:
> Upcoming patches are mapping memory with RO, RW^X etc permsissions.
> Fix the meminfo command to display them properly
> 
> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>   arch/arm/cpu/armv8/cache_v8.c    | 26 +++++++++++++++++++++++---
>   arch/arm/include/asm/armv8/mmu.h |  2 ++
>   cmd/meminfo.c                    |  5 +++++
>   3 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> index 5d6953ffedd1..c4b3da4a8da7 100644
> --- a/arch/arm/cpu/armv8/cache_v8.c
> +++ b/arch/arm/cpu/armv8/cache_v8.c
> @@ -421,7 +421,7 @@ static int count_ranges(void)
>   	return count;
>   }
>   
> -#define ALL_ATTRS (3 << 8 | PMD_ATTRINDX_MASK)
> +#define ALL_ATTRS (3 << 8 | PMD_ATTRMASK)
>   #define PTE_IS_TABLE(pte, level) (pte_type(&(pte)) == PTE_TYPE_TABLE && (level) < 3)
>   
>   enum walker_state {
> @@ -568,6 +568,20 @@ static void pretty_print_table_attrs(u64 pte)
>   static void pretty_print_block_attrs(u64 pte)
>   {
>   	u64 attrs = pte & PMD_ATTRINDX_MASK;
> +	u64 perm_attrs = pte & PMD_ATTRMASK;
> +	char mem_attrs[16] = { 0 };
> +	int cnt = 0;
> +
> +	if (perm_attrs & PTE_BLOCK_PXN)
> +		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "PXN ");
> +	if (perm_attrs & PTE_BLOCK_UXN)
> +		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "UXN ");
> +	if (perm_attrs & PTE_BLOCK_RO)
> +		cnt += snprintf(mem_attrs + cnt, sizeof(mem_attrs) - cnt, "RO");
> +	if (!mem_attrs[0])
> +		snprintf(mem_attrs, sizeof(mem_attrs), "RWX ");
> +
> +	printf(" | %-10s", mem_attrs);
>   
>   	switch (attrs) {
>   	case PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE):
> @@ -613,6 +627,7 @@ static void print_pte(u64 pte, int level)
>   {
>   	if (PTE_IS_TABLE(pte, level)) {
>   		printf(" %-5s", "Table");
> +		printf(" %-12s", "|");
>   		pretty_print_table_attrs(pte);
>   	} else {
>   		pretty_print_pte_type(pte);
> @@ -642,9 +657,9 @@ static bool pagetable_print_entry(u64 start_attrs, u64 end, int va_bits, int lev
>   
>   	printf("%*s", indent * 2, "");
>   	if (PTE_IS_TABLE(start_attrs, level))
> -		printf("[%#011llx]%14s", _addr, "");
> +		printf("[%#016llx]%19s", _addr, "");
>   	else
> -		printf("[%#011llx - %#011llx]", _addr, end);
> +		printf("[%#016llx - %#016llx]", _addr, end);
>   
>   	printf("%*s | ", (3 - level) * 2, "");
>   	print_pte(start_attrs, level);
> @@ -1112,3 +1127,8 @@ void __weak enable_caches(void)
>   	icache_enable();
>   	dcache_enable();
>   }
> +
> +void arch_dump_mem_attrs(void)
> +{
> +	dump_pagetable(gd->arch.tlb_addr, get_tcr(NULL, NULL));
> +}
> diff --git a/arch/arm/include/asm/armv8/mmu.h b/arch/arm/include/asm/armv8/mmu.h
> index 0ab681c893d3..6af8cd111a44 100644
> --- a/arch/arm/include/asm/armv8/mmu.h
> +++ b/arch/arm/include/asm/armv8/mmu.h
> @@ -66,6 +66,7 @@
>   #define PTE_BLOCK_NG		(1 << 11)
>   #define PTE_BLOCK_PXN		(UL(1) << 53)
>   #define PTE_BLOCK_UXN		(UL(1) << 54)
> +#define PTE_BLOCK_RO            (UL(1) << 7)
>   
>   /*
>    * AttrIndx[2:0]
> @@ -75,6 +76,7 @@
>   #define PMD_ATTRMASK		(PTE_BLOCK_PXN		| \
>   				 PTE_BLOCK_UXN		| \
>   				 PMD_ATTRINDX_MASK	| \
> +				 PTE_BLOCK_RO		| \
>   				 PTE_TYPE_VALID)
>   
>   /*
> diff --git a/cmd/meminfo.c b/cmd/meminfo.c
> index 5e83d61c2dd3..3915e2bbb268 100644
> --- a/cmd/meminfo.c
> +++ b/cmd/meminfo.c
> @@ -15,6 +15,10 @@
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> +void __weak arch_dump_mem_attrs(void)
> +{
> +}
> +
>   static void print_region(const char *name, ulong base, ulong size, ulong *uptop)
>   {
>   	ulong end = base + size;
> @@ -54,6 +58,7 @@ static int do_meminfo(struct cmd_tbl *cmdtp, int flag, int argc,
>   
>   	puts("DRAM:  ");
>   	print_size(gd->ram_size, "\n");
> +	arch_dump_mem_attrs();
>   
>   	if (!IS_ENABLED(CONFIG_CMD_MEMINFO_MAP))
>   		return 0;

-- 
Caleb (they/them)

Re: [PATCH v1 5/6] treewide: Add a function to change page permissions

[Ilias Apalodimas]

Hi Heinrich

On Sun, Feb 09, 2025 at 09:27:36PM +0100, Heinrich Schuchardt wrote:
> Am 9. Februar 2025 21:15:53 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Tom,
> >
> >On Sun, 9 Feb 2025 at 09:39, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Thu, Feb 06, 2025 at 08:47:47AM -0700, Simon Glass wrote:
> >>
> >> [snip]
> >> > Perhaps make EFI_LOADER select CPU, or depend on CPU? If that's the
> >> > way you want to go, I'd be happy to do a precursor series to deal with
> >> > the fallout.
> >>
> >> I'm not sure what EFI_LOADER has to do with the generic security feature
> >> of enforcing permissions on pages. That's something we want everywhere
> >> that can enable it as it's a good defensive security measure and also
> >> catches code bugs.
> >
> >Yes, it's a good thing to have. I assumed it was related to EFI
> >because of all the mention of EFI, SetVirtualAddressMap() and the
> >like.
> >
> >It doesn't have to be DM. I was reacting to the idea that we cannot
> >add it to the CPU driver because hardly any boards have one. How about
> >mapping arch-specific stuff to generic functions, like we try to do
> >with the CPU uclass. The enforcement happens before initr_dm()
> >although I suppose it could be moved later, or a CPU driver could be
> >started up before relocation. Or just don't use a CPU driver, use
> >something else.
> >
> >WIth all the pain I've just been through with the EFI link scripts, I
> >would have rather seen some effort to follow the existing convention,
> >e.g. text_start rather than start_text. We already have
> >__image_copy_start - there is so much arch-specific variability here
> >already.
>
> Like we did for the EFI linker scripts we should standardize the u-boot binary linker scripts by using a common linker script include.

Yes, I already started this, but due to the efi_runtime complexity I need
to fix that weird split we have for .text_start, .efi_runtime, .text split first.

I have an idea of how we can fix it and link the EFI code in a way that all
relocatable objects of the .efi_runtime are self contained. But I'll send a
v2 as is first and work on the linker scripts afterwards

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
>
> >
> >Anyway, I'll stay away from this series in future.
> >
> >Regards,
> >Simon
>