Re: [PATCH] x86/boot: Handle better alignment for 32 bit code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2025 10:59, Frediano Ziglio wrote:
> Output file didn't have correct alignment.
> Allows alignment into data or code up to 2mb.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

Afaic this is way too little of a description. For example, ...

> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -40,8 +40,8 @@ LD32 := $(LD) $(subst x86_64,i386,$(LDFLAGS_DIRECT))
>  # are affected by both text_diff and text_gap.  Ensure the sum of gap and 
> diff
>  # is greater than 2^16 so that any 16bit relocations if present in the object
>  # file turns into a build-time error.
> -text_gap := 0x010200
> -text_diff := 0x408020
> +text_gap := 0x01c240
> +text_diff := 0x7e3dc0

... how is anyone to derive how we ended up with these magic numbers?
What parts of them are arbitrary, and what parts are required to be the
way they are?

> @@ -69,7 +69,6 @@ $(obj)/built-in-32.%.bin: $(obj)/build32.%.lds 
> $(obj)/built-in-32.tmp.o
>       $(LD32) $(orphan-handling-y) -N -T $< -o $(@:bin=o) $(filter %.o,$^)
>       $(NM) -p --format=bsd $(@:bin=o) > $(@:bin=map)
>       $(OBJCOPY) -j .text -O binary $(@:bin=o) $@
> -     rm -f $(@:bin=o)

This looks like an unrelated change. It may be okay to have here, but
then it needs mentioning in the description.

> @@ -80,6 +79,7 @@ cmd_combine = \
>                --bin1      $(obj)/built-in-32.base.bin \
>                --bin2      $(obj)/built-in-32.offset.bin \
>                --map       $(obj)/built-in-32.base.map \
> +              --align     $(shell $(OBJDUMP) -h 
> $(obj)/built-in-32.base.o|sed '/text.*2\*\*/ {s/.*2\*\*//;p;}; d') \
>                --exports   cmdline_parse_early,reloc,reloc_trampoline32 \
>                --output    $@
>  
> @@ -90,4 +90,4 @@ $(obj)/built-in-32.S: $(obj)/built-in-32.base.bin 
> $(obj)/built-in-32.offset.bin
>                        $(srctree)/tools/combine_two_binaries.py FORCE
>       $(call if_changed,combine)
>  
> -clean-files := built-in-32.*.bin built-in-32.*.map build32.*.lds
> +clean-files := built-in-32.*.bin built-in-32.*.map built-in-32.*.o 
> build32.*.lds

This looks like it would have been needed already before, if the build
process was interrupted before the "rm" that you remove above.

Jan