Hi Ilias,
On Thu, 6 Feb 2025 at 08:16, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> On Thu, 6 Feb 2025 at 14:58, Simon Glass <s...@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 6 Feb 2025 at 05:52, Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Thu, 6 Feb 2025 at 14:30, Simon Glass <s...@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Wed, 5 Feb 2025 at 09:54, Ilias Apalodimas
> > > > <ilias.apalodi...@linaro.org> wrote:
> > > > >
> > > > > Hi Heinrich,
> > > > >
> > > > > On Wed, 5 Feb 2025 at 18:48, Heinrich Schuchardt <xypron.g...@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.apalodi...@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
