Re: [PATCH 1/2] sh: align .bss section padding to 8-byte boundary

Hi Artur,

On Wed, 12 Mar 2025 at 00:40, Artur Rojek <[email protected]> wrote:
> On 2025-03-11 18:28, John Paul Adrian Glaubitz wrote:
> > I'm currently trying to review this patch, but I'm not 100% sure how it
> > this change helps grows the .bss section, see below. Maybe you can help
> > me understand what's happening.
> >
> > On Sun, 2025-02-16 at 18:55 +0100, Artur Rojek wrote:
> >> J2 based devices expect to find a devicetree blob at the end of the
> >> bss
> >> section. As of a77725a9a3c5, libfdt enforces 8-byte alignment for the
> >> dtb, causing J2 devices to fail early in sh_fdt_init.
> >>
> >> As J2 loader firmware calculates the dtb location based on the kernel
> >> image .bss section size, rather than the __bss_stop symbol offset, the
> >> required alignment can't be enforced with BSS_SECTION(0, PAGE_SIZE,
> >> 8).
> >> Instead, inline modified version of the above macro, which grows .bss
> >> by the required size.
> >>
> >> While this change affects all existing SH boards, it should be benign
> >> on
> >> platforms which don't need this alignment.
> >>
> >> Signed-off-by: Artur Rojek <[email protected]>
> >> ---
> >>  arch/sh/kernel/vmlinux.lds.S | 15 ++++++++++++++-
> >>  1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/sh/kernel/vmlinux.lds.S
> >> b/arch/sh/kernel/vmlinux.lds.S
> >> index 9644fe187a3f..008c30289eaa 100644
> >> --- a/arch/sh/kernel/vmlinux.lds.S
> >> +++ b/arch/sh/kernel/vmlinux.lds.S
> >> @@ -71,7 +71,20 @@ SECTIONS
> >>
> >>      . = ALIGN(PAGE_SIZE);
> >>      __init_end = .;
> >> -    BSS_SECTION(0, PAGE_SIZE, 4)
> >> +    __bss_start = .;
> >> +    SBSS(0)
> >> +    . = ALIGN(PAGE_SIZE);
> >
> > What this effectively does is removing ". = ALIGN(sbss_align);" first
> > from BSS_SECTION().
> >
> > Then it inserts ". = ALIGN(PAGE_SIZE);" after the "SBSS(0)".
> >
> > If I understand this correctly, SBSS() inserts a zero-padding and if
> > I'm not mistaken,
> > inserting ". = ALIGN(PAGE_SIZE);" will cause this padding to grow to at
> > least PAGE_SIZE
> > due the alignment.
> >
> > Is this correct?
> >
> >> +    .bss : AT(ADDR(.bss) - LOAD_OFFSET) {
> >> +            BSS_FIRST_SECTIONS
> >> +            . = ALIGN(PAGE_SIZE);
> >> +            *(.bss..page_aligned)
> >> +            . = ALIGN(PAGE_SIZE);
> >> +            *(.dynbss)
> >> +            *(BSS_MAIN)
> >> +            *(COMMON)
> >> +            . = ALIGN(8);
> >
> > If my understanding above is correct, why do we will need an additional
> > ". = ALIGN(8)"
> > here?
>
> I'll tackle both of the above questions at once.
> I'm by no means an expert at GNU Linker syntax, but the intention of
> this patch is to put . = ALIGN(8) inside the .bss : { ... } section
> definition, so that the section itself grows by the requested padding.
>
> In the original BSS_SECTION(0, PAGE_SIZE, 4), the last argument inserts
> a 4 byte padding after the closing brace of .bss section definition,
> causing the __bss_stop symbol offset to grow, but not the .bss section
> itself:
>
> #define BSS_SECTION(sbss_align, bss_align, stop_align)                  \
>         . = ALIGN(sbss_align);                                          \
>         __bss_start = .;                                                \
>         SBSS(sbss_align)                                                \
>         BSS(bss_align)                                                  \
>         . = ALIGN(stop_align);                                          \
>         __bss_stop = .;
>
> TurtleBoard loader is only concerned with the .bss section size - it
> doesn't care about any symbol offsets - and hence this seemingly cryptic
> change (you can display the section size information with
> readelf -t kernel_image).
> The rest of the changes are simply to "inline" the BSS() macro (as I
> needed to access that closing brace), and the former sbss_align,
> bss_align (that's your PAGE_SIZE) and stop_align arguments are passed
> accordingly, the same way they used to be passed before. The only
> visible effect should be the move of ALIGN(stop_align) inside of .bss
> section definition, and the change of stop_align value from 4 to 8.
>
> Arguably the TurtleBoard loader should read the __bss_stop symbol offset
> instead, but in this patch I'm trying to solve the issue from kernel's
> point of view.

What about moving (or duplicating, e.g. sbss_align alignment is
done before and after __bss_start)  the stop_align alignment
from BSS_SECTION() into BSS() instead, i.e. just changing
include/asm-generic/vmlinux.lds.h for everyone?  I don't think that
would hurt any platforms, while fixing the issue for good.
IMHO it is a bit strange that the size of the bss section can differ
from __bss_stop - __bss_start.
One last question though: what about sbss? How does the TurtleBoard
loader handle that?  __bss_stop - __bss_start is not the size of bss,
but the sum of the sizes of sbss and bss, plus extra alignment in
between. The latter might cause trouble, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds