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

[Tom Rini <trini@konsulko.com>]

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

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

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

-- 
Tom

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