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

Frediano Ziglio posted 1 patch 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20250114095914.93226-1-frediano.ziglio@cloud.com
There is a newer version of this series
xen/arch/x86/boot/Makefile        | 8 ++++----
xen/tools/combine_two_binaries.py | 7 ++++++-
2 files changed, 10 insertions(+), 5 deletions(-)
[PATCH] x86/boot: Handle better alignment for 32 bit code
Posted by Frediano Ziglio 3 weeks ago
Output file didn't have correct alignment.
Allows alignment into data or code up to 2mb.

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

diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile
index 13d4583173..9a8ecba7aa 100644
--- 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
 
 $(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 +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)
 
 quiet_cmd_combine = GEN     $@
 cmd_combine = \
@@ -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
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] x86/boot: Handle better alignment for 32 bit code
Posted by Jan Beulich 3 weeks ago
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@cloud.com>

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