| Message ID | 20250205071714.635518-6-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix page permission on arm64 architectures | expand |
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 > *
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 > > * >
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 > > > * > >
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 >>>> * >>>
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
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
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
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
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
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
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
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?
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.
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.
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
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
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 *
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(+)