[PATCH v1 5/6] treewide: Add a function to change page permissions
Ilias Apalodimas
ilias.apalodimas at linaro.org
Thu Feb 6 16:15:30 CET 2025
On Thu, 6 Feb 2025 at 14:58, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Thu, 6 Feb 2025 at 14:30, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > > <ilias.apalodimas at linaro.org> wrote:
> > > >
> > > > Hi Heinrich,
> > > >
> > > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.glpk at 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 at 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
More information about the U-Boot
mailing list