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

[Artur Rojek]

On 2025-03-11 18:28, John Paul Adrian Glaubitz wrote:
> Hi Artur,

Hey Adrian,
thanks for looking into this patch.

> 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 <contact@xxxxxxxxxxxxxx>
> > ---
> >  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.

Cheers,
Artur

> > +	}
> > +	__bss_stop = .;
> >  	_end = . ;
> >
> >  	STABS_DEBUG
>
> Thanks,
> Adrian