[PATCH v1 5/6] treewide: Add a function to change page permissions
Ilias Apalodimas
ilias.apalodimas at linaro.org
Thu Feb 6 17:21:10 CET 2025
Hi Simon,
On Thu, 6 Feb 2025 at 17:48, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Ilias,
>
> On Thu, 6 Feb 2025 at 08:16, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
> >
> > 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.
>
> 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
More information about the U-Boot
mailing list