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

Hi Artur,

On Wed, 2025-03-12 at 00:40 +0100, Artur Rojek wrote:
> On 2025-03-11 18:28, John Paul Adrian Glaubitz wrote:
> > Hi Artur,
> 
> Hey Adrian,
> thanks for looking into this patch.

Sure. I just want to understand what's going on before signing it with "Reviewed-by",
I wouldn't dare that without fully understanding what the proposed change does ;-).

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

Makes sense.

> 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 = .;

OK, that's really odd. So, the __bss_stop would be moved to the desired
position but the section itself still remains small? What exactly does the
linker fill the region with? Sounds very strange.

> 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).

Looking at the actual kernel image with readelf is a very good suggestion!

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

OK. FWIW, do you understand what SBSS is for? I couldn't find any explanation
for it.

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

That's absolutely sensible as this avoids having to update the firmware.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913