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.
>
> 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 */
