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

Hi Artur,

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?

> +	}
> +	__bss_stop = .;
>  	_end = . ;
>  
>  	STABS_DEBUG

Thanks,
Adrian

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