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

[Simon Glass]

Hi Ilias,

On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
<ilias.apalodimas 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.

Regards,
SImon