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
    *