On Sun, Feb 09, 2025 at 07:35:31AM -0700, Simon Glass wrote:
> 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!Can you PLEASE stop implying that the mainline tree is the Linaro tree? -- Tom
signature.asc
Description: PGP signature
