Hi Ilias,
On Thu, 6 Feb 2025 at 09:21, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 6 Feb 2025 at 17:48, Simon Glass <s...@chromium.org> wrote:
> >
> > 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.
>
> 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.
So perhaps very few boards care? Those that do want this new feature
could enable CPU.
You could even add both interfaces and encourage people to use the CPU
one. At least then people who are trying to DTRT can do so.
I don't like this approach to developing new features - hacking in a
non-DM API so that the code is used more. This is what happened with
EFI_LOADER too. But it's your tree and you obviously don't agree, so
go for it!
Regards,
Simon
>
> Thanks
> /Ilias
>
> >
> > >
> > > [0]
> > > https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/fix_memory_permissions_uclass?ref_type=heads
> >
> > Regards,
> > Simon
