On Fri, 8 Mar 2024 at 16:14, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> On Fri, 8 Mar 2024 at 15:22, Ilias Apalodimas
> <ilias.apalodi...@linaro.org> wrote:
> >
> > Hi Sam,
> >
> > On Thu, 7 Mar 2024 at 08:50, Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> > >
> > >
> > >
> > > On Thu, 7 Mar 2024 at 00:19, Sam Edwards <cfswo...@gmail.com> wrote:
> > >>
> > >>
> > >>
> > >> On 3/6/24 02:13, Ilias Apalodimas wrote:
> > >> > Hi Sam,
> > >> >
> > >> > Again thank you for the elaborate review. This really helps a lot.
> > >> >
> > >> > On Wed, 6 Mar 2024 at 10:14, Sam Edwards <cfswo...@gmail.com> wrote:
> > >> >>
> > >> >>
> > >> >>
> > >> >> On 3/4/24 02:01, Ilias Apalodimas wrote:
> > >> >>> __efi_runtime_start/end are defined as c variables for arm7 only in
> > >> >>> order to force the compiler emit relative references. However,
> > >> >>> defining
> > >> >>> those within a section definition will do the same thing. On top of
> > >> >>> that
> > >> >>> the v8 linker scripts define it as a symbol.
> > >> >>>
> > >> >>> So let's remove the special sections from the linker scripts, the
> > >> >>> variable definitions from sections.c and define them as a symbols
> > >> >>> within
> > >> >>> the correct section.
> > >> >>>
> > >> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > >> >> Tested-by: Sam Edwards <cfswo...@gmail.com> # Binary output identical
> > >> >>
> > >> >> Thanks for the cleanup,
> > >> >> Sam
> > >> >>
> > >> >>> ---
> > >> >>> arch/arm/cpu/u-boot.lds | 12 +++---------
> > >> >>> arch/arm/lib/sections.c | 2 --
> > >> >>> arch/arm/mach-zynq/u-boot.lds | 12 +++---------
> > >> >>> include/asm-generic/sections.h | 1 +
> > >> >>> 4 files changed, 7 insertions(+), 20 deletions(-)
> > >> >>>
> > >> >>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> > >> >>> index 7c6e7891d360..df55bb716e35 100644
> > >> >>> --- a/arch/arm/cpu/u-boot.lds
> > >> >>> +++ b/arch/arm/cpu/u-boot.lds
> > >> >>> @@ -43,18 +43,12 @@ SECTIONS
> > >> >>> }
> > >> >>>
> > >> >>> /* This needs to come before *(.text*) */
> > >> >>> - .__efi_runtime_start : {
> > >> >>> - *(.__efi_runtime_start)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .efi_runtime : {
> > >> >>> + .efi_runtime ALIGN(4) : {
> > >> >>
> > >> >> Do we truly require the ALIGN(4)? If I understand correctly, by
> > >> >> default,
> > >> >> the linker calculates the alignment of an output section as the least
> > >> >> common multiple of the input sections' alignment requirements --
> > >> >> meaning
> > >> >> most (perhaps all) of our ALIGN()s today are redundant.
> > >> >
> > >> > I don't think we do. But I preserved those for a few reasons.
> > >> >
> > >> >> For the time
> > >> >> being, I'm in favor of merging existing `. = ALIGN(x)` into each
> > >> >> following section for clarity and to avoid the testing overhead of
> > >> >> removing them in the same patch as other changes. However, in the near
> > >> >> future (perhaps even as "near" as v2 of this series?), I'd also like
> > >> >> to
> > >> >> see a patch that eliminates unnecessary ALIGN()s altogether.
> > >> >> Introducing
> > >> >> additional ALIGN()s where we already know they aren't needed may be a
> > >> >> step away from that goal.
> > >> >
> > >> > So as you already mentioned the reason I preserved this is:
> > >> > - Reduce the testing overhead by preserving the same layout for now
> > >> > - In the future, I am playing around with the idea of mapping U-Boot
> > >> > (not SPL but the relocated full U-Boot) in sections with proper memory
> > >> > permissions (R), RW^X etc). In that case, we will need a 4k section
> > >> > alignment and we can repurpose the ALIGN(4/8) to
> > >> > ALIGN(CONSTANT(COMMONPAGESIZE)).
> > >> >
> > >> > Thoughts?
> > >>
> > >> Ah, right: I had forgotten for a moment that the ultimate goal was to
> > >> clean up the memory protection bit situation.
> > >>
> > >> Okay, as long as that future cleanup will also remove all unnecessary
> > >> uses of ALIGN() (i.e. any that don't result in an actual change in
> > >> memory layout) then that sounds great to me.
> > >
> > >
> > > Yep will do
> > >
> > >>
> > >> Reviewed-by: Sam Edwards <cfswo...@gmail.com>
> >
> > Thanks. However, I noticed the alignment rule was wrong
> > it needs to be '.efi_runtime : ALIGN(4)' instead of '.efi_runtime ALIGN(4)
> > :'.
> > The latter will define the start address instead of specifying the
> > alignment.
> > Due to what you mentioned above this (that we don't really need the
> > ALIGN(4) the resulting binary is unaffected.
>
> To be exact, it will work because ALIGN returns the current location
> counter aligned upward to the specified value.
But thinking about it a bit more, I'll just get rid of the redundant
ALIGN as you suggested. I don't think it makes sense to keep or force
the output section alignment just because in the future we want to fix
page permissions.
We can always re-introduce it when stricter (4k) alignment is required for that
Cheers
/Ilias
>
>
> >
> > Thanks
> > /Ilias
> >
> > >
> > >
> > >
> > >
> > > Thanks!
> > >
> > >>
> > >> Cheers,
> > >> Sam
> > >>
> > >> >
> > >> > Thanks
> > >> > /Ilias
> > >> >
> > >> >>
> > >> >>> + __efi_runtime_start = .;
> > >> >>> *(.text.efi_runtime*)
> > >> >>> *(.rodata.efi_runtime*)
> > >> >>> *(.data.efi_runtime*)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .__efi_runtime_stop : {
> > >> >>> - *(.__efi_runtime_stop)
> > >> >>> + __efi_runtime_stop = .;
> > >> >>> }
> > >> >>>
> > >> >>> .text_rest :
> > >> >>> diff --git a/arch/arm/lib/sections.c b/arch/arm/lib/sections.c
> > >> >>> index 1ee3dd3667ba..a4d4202e99f5 100644
> > >> >>> --- a/arch/arm/lib/sections.c
> > >> >>> +++ b/arch/arm/lib/sections.c
> > >> >>> @@ -25,6 +25,4 @@ char __secure_start[0]
> > >> >>> __section(".__secure_start");
> > >> >>> char __secure_end[0] __section(".__secure_end");
> > >> >>> char __secure_stack_start[0] __section(".__secure_stack_start");
> > >> >>> char __secure_stack_end[0] __section(".__secure_stack_end");
> > >> >>> -char __efi_runtime_start[0] __section(".__efi_runtime_start");
> > >> >>> -char __efi_runtime_stop[0] __section(".__efi_runtime_stop");
> > >> >>> char _end[0] __section(".__end");
> > >> >>> diff --git a/arch/arm/mach-zynq/u-boot.lds
> > >> >>> b/arch/arm/mach-zynq/u-boot.lds
> > >> >>> index 71dea4a1f60a..fcd0f42a7106 100644
> > >> >>> --- a/arch/arm/mach-zynq/u-boot.lds
> > >> >>> +++ b/arch/arm/mach-zynq/u-boot.lds
> > >> >>> @@ -22,18 +22,12 @@ SECTIONS
> > >> >>> }
> > >> >>>
> > >> >>> /* This needs to come before *(.text*) */
> > >> >>> - .__efi_runtime_start : {
> > >> >>> - *(.__efi_runtime_start)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .efi_runtime : {
> > >> >>> + .efi_runtime ALIGN(4) : {
> > >> >>
> > >> >> Ditto above
> > >> >>
> > >> >>> + __efi_runtime_start = .;
> > >> >>> *(.text.efi_runtime*)
> > >> >>> *(.rodata.efi_runtime*)
> > >> >>> *(.data.efi_runtime*)
> > >> >>> - }
> > >> >>> -
> > >> >>> - .__efi_runtime_stop : {
> > >> >>> - *(.__efi_runtime_stop)
> > >> >>> + __efi_runtime_stop = .;
> > >> >>> }
> > >> >>>
> > >> >>> .text_rest :
> > >> >>> diff --git a/include/asm-generic/sections.h
> > >> >>> b/include/asm-generic/sections.h
> > >> >>> index 60949200dd93..b6bca53db10d 100644
> > >> >>> --- a/include/asm-generic/sections.h
> > >> >>> +++ b/include/asm-generic/sections.h
> > >> >>> @@ -35,6 +35,7 @@ extern char __priv_data_start[], __priv_data_end[];
> > >> >>> extern char __ctors_start[], __ctors_end[];
> > >> >>>
> > >> >>> extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
> > >> >>> +extern char __efi_runtime_start[], __efi_runtime_stop[];
> > >> >>>
> > >> >>> /* function descriptor handling (if any). Override
> > >> >>> * in asm/sections.h */
