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

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

[Frediano Ziglio]

Output file didn't have correct alignment.
Allows alignment into data or code up to 2mb.
Intermediate object files are kept in order to copy alignment
from object produced by the linker and final object (produced
by combine_two_binaries.py script).

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/arch/x86/boot/Makefile        | 12 ++++++++----
 xen/tools/combine_two_binaries.py |  7 ++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

Changes since v1:
- Improve comments and description.

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 13d4583173..a56d8a7e0f 100644
--- a/xen/arch/x86/boot/Makefile
+++ b/xen/arch/x86/boot/Makefile
@@ -40,8 +40,12 @@ 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
+# As gap will affect the output section size it should not be huge to avoid the
+# creation of huge files.
+# The sum of gap and diff will affect the possible alignment so should be a
+# multiple of the possible alignment.
+text_gap := 0x01c240
+text_diff := 0x7e3dc0
 
 $(obj)/build32.base.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff)
 $(obj)/build32.offset.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff) -DAPPLY_OFFSET
@@ -69,7 +73,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)
 
 quiet_cmd_combine = GEN     $@
 cmd_combine = \
@@ -80,6 +83,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 +94,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
diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
index 581e57cbc0..8e587c24fb 100755
--- a/xen/tools/combine_two_binaries.py
+++ b/xen/tools/combine_two_binaries.py
@@ -26,6 +26,10 @@ parser.add_argument('--text-diff', dest='text_diff',
                     required=True,
                     type=auto_int,
                     help='Difference between code section start')
+parser.add_argument('--align', dest='align',
+                    default=2,
+                    type=auto_int,
+                    help='Alignment in power of 2')
 parser.add_argument('--output', dest='output',
                     help='Output file')
 parser.add_argument('--map', dest='mapfile',
@@ -93,7 +97,7 @@ if size1 > size2:
     file1, file2 = file2, file1
     size1, size2 = size2, size1
 if size2 != size1 + gap:
-    raise Exception('File sizes do not match')
+    raise Exception('File sizes do not match %d != %d + %d' % (size2, size1, gap))
 del size2
 
 file1.seek(0, 0)
@@ -219,6 +223,7 @@ print('''/*
  * File autogenerated by combine_two_binaries.py DO NOT EDIT
  */''', file=out)
 print('\t' + args.section_header, file=out)
+print('\t.p2align\t' + str(args.align), file=out)
 print('obj32_start:', file=out)
 output(out)
 print('\n\t.section .note.GNU-stack,"",@progbits', file=out)
-- 
2.34.1

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

[Frediano Ziglio]

ping

On Tue, Jan 14, 2025 at 11:54 AM Frediano Ziglio
<frediano.ziglio@cloud.com> wrote:
>
> Output file didn't have correct alignment.
> Allows alignment into data or code up to 2mb.
> Intermediate object files are kept in order to copy alignment
> from object produced by the linker and final object (produced
> by combine_two_binaries.py script).
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
> ---
>  xen/arch/x86/boot/Makefile        | 12 ++++++++----
>  xen/tools/combine_two_binaries.py |  7 ++++++-
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> Changes since v1:
> - Improve comments and description.
>
> diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
> index 13d4583173..a56d8a7e0f 100644
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -40,8 +40,12 @@ 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
> +# As gap will affect the output section size it should not be huge to avoid the
> +# creation of huge files.
> +# The sum of gap and diff will affect the possible alignment so should be a
> +# multiple of the possible alignment.
> +text_gap := 0x01c240
> +text_diff := 0x7e3dc0
>
>  $(obj)/build32.base.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff)
>  $(obj)/build32.offset.lds: AFLAGS-y += -DGAP=$(text_gap) -DTEXT_DIFF=$(text_diff) -DAPPLY_OFFSET
> @@ -69,7 +73,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)
>
>  quiet_cmd_combine = GEN     $@
>  cmd_combine = \
> @@ -80,6 +83,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 +94,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
> diff --git a/xen/tools/combine_two_binaries.py b/xen/tools/combine_two_binaries.py
> index 581e57cbc0..8e587c24fb 100755
> --- a/xen/tools/combine_two_binaries.py
> +++ b/xen/tools/combine_two_binaries.py
> @@ -26,6 +26,10 @@ parser.add_argument('--text-diff', dest='text_diff',
>                      required=True,
>                      type=auto_int,
>                      help='Difference between code section start')
> +parser.add_argument('--align', dest='align',
> +                    default=2,
> +                    type=auto_int,
> +                    help='Alignment in power of 2')
>  parser.add_argument('--output', dest='output',
>                      help='Output file')
>  parser.add_argument('--map', dest='mapfile',
> @@ -93,7 +97,7 @@ if size1 > size2:
>      file1, file2 = file2, file1
>      size1, size2 = size2, size1
>  if size2 != size1 + gap:
> -    raise Exception('File sizes do not match')
> +    raise Exception('File sizes do not match %d != %d + %d' % (size2, size1, gap))
>  del size2
>
>  file1.seek(0, 0)
> @@ -219,6 +223,7 @@ print('''/*
>   * File autogenerated by combine_two_binaries.py DO NOT EDIT
>   */''', file=out)
>  print('\t' + args.section_header, file=out)
> +print('\t.p2align\t' + str(args.align), file=out)
>  print('obj32_start:', file=out)
>  output(out)
>  print('\n\t.section .note.GNU-stack,"",@progbits', file=out)
> --
> 2.34.1
>

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

[Jan Beulich]

On 14.01.2025 12:54, Frediano Ziglio wrote:
> Output file didn't have correct alignment.

Coming back to this (as it's still in my "pending" folder), what does the above
mean? In how far was which alignment not correct for what purpose?

> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -40,8 +40,12 @@ 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
> +# As gap will affect the output section size it should not be huge to avoid the
> +# creation of huge files.
> +# The sum of gap and diff will affect the possible alignment so should be a
> +# multiple of the possible alignment.
> +text_gap := 0x01c240
> +text_diff := 0x7e3dc0

It remains unclear what "possible alignment" here is. Maybe that's the "up to
2mb" that the description talks about, but when looking at the file later the
description of this change will not be readily available.

> @@ -80,6 +83,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') \

I've tried this out manually on $(obj)/built-in-32.o, as $(obj)/built-in-32.base.o
isn't there without applying this patch. I'm observing two matching sections there,
i.e. I wonder whether the regex shouldn't be tightened. Not being fluent with sed
commands, I also wonder whether passing -n to the tool in favor of the d command
might not be better.

> @@ -90,4 +94,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

As before I think this is a separate bugfix anyway.

Jan