| Message ID | 20250109131515.1757764-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/boot: adjust text gap/diff to work with 64-bytes alignment too | expand |
On 09.01.2025 14:15, Marek Marczykowski-Górecki wrote: > Xen compiled with -mtune=generic has .text alignment set to 64-bytes. > Having text_diff non-64-bytes-aligned breaks stuff: > > Traceback (most recent call last): > File "/builddir/build/BUILD/xen-4.20.0-build/xen-4.20.0-rc0/xen/./tools/combine_two_binaries.py", line 96, in <module> > raise Exception('File sizes do not match') > Exception: File sizes do not match: 70160 != 4080 + 66048 > > Adjust the numbers as suggested by Frediano to work with 64-bytes and > even 128-bytes alignment. And then breaking at 256? As indicated on Matrix, imo we should be able to cope with anything up to at least PAGE_SIZE. Or we should reduce .text alignment before linking. > Suggested-by: Frediano Ziglio <frediano.ziglio@cloud.com> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> No Fixes: tag? Jan
On 09/01/2025 1:23 pm, Jan Beulich wrote: > On 09.01.2025 14:15, Marek Marczykowski-Górecki wrote: >> Xen compiled with -mtune=generic has .text alignment set to 64-bytes. >> Having text_diff non-64-bytes-aligned breaks stuff: >> >> Traceback (most recent call last): >> File "/builddir/build/BUILD/xen-4.20.0-build/xen-4.20.0-rc0/xen/./tools/combine_two_binaries.py", line 96, in <module> >> raise Exception('File sizes do not match') >> Exception: File sizes do not match: 70160 != 4080 + 66048 >> >> Adjust the numbers as suggested by Frediano to work with 64-bytes and >> even 128-bytes alignment. > And then breaking at 256? As indicated on Matrix, imo we should be able to > cope with anything up to at least PAGE_SIZE. Or we should reduce .text > alignment before linking. Do you have a concrete proposal on how to do this? Because if not, we've had 2 downstreams hit by this build failure, and we probably ought to take this patch as a stopgap fix, and see about doing the better fix for 4.20. ~Andrew
On Thu, Jan 09, 2025 at 02:23:27PM +0100, Jan Beulich wrote:
> No Fixes: tag?
I guess this would be appropriate:
Fixes: aa9045e77130 ("x86/boot: Rework how 32bit C is linked/included for early boot")
(but not relevant for backports, since that's only in 4.20 release
cycle)
On 09.01.2025 14:44, Andrew Cooper wrote: > On 09/01/2025 1:23 pm, Jan Beulich wrote: >> On 09.01.2025 14:15, Marek Marczykowski-Górecki wrote: >>> Xen compiled with -mtune=generic has .text alignment set to 64-bytes. >>> Having text_diff non-64-bytes-aligned breaks stuff: >>> >>> Traceback (most recent call last): >>> File "/builddir/build/BUILD/xen-4.20.0-build/xen-4.20.0-rc0/xen/./tools/combine_two_binaries.py", line 96, in <module> >>> raise Exception('File sizes do not match') >>> Exception: File sizes do not match: 70160 != 4080 + 66048 >>> >>> Adjust the numbers as suggested by Frediano to work with 64-bytes and >>> even 128-bytes alignment. >> And then breaking at 256? As indicated on Matrix, imo we should be able to >> cope with anything up to at least PAGE_SIZE. Or we should reduce .text >> alignment before linking. > > Do you have a concrete proposal on how to do this? The latter suggestion would involve objcopy's --set-section-alignment, the former would involve using constants with the low 12 bits clear. > Because if not, we've had 2 downstreams hit by this build failure, and > we probably ought to take this patch as a stopgap fix, and see about > doing the better fix for 4.20. 4.21 that is, I guess. I never like such very much, because it's too likely that it'll then be forgotten. Jan
On Thu, Jan 9, 2025 at 1:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 09/01/2025 1:23 pm, Jan Beulich wrote: > > On 09.01.2025 14:15, Marek Marczykowski-Górecki wrote: > >> Xen compiled with -mtune=generic has .text alignment set to 64-bytes. > >> Having text_diff non-64-bytes-aligned breaks stuff: > >> > >> Traceback (most recent call last): > >> File "/builddir/build/BUILD/xen-4.20.0-build/xen-4.20.0-rc0/xen/./tools/combine_two_binaries.py", line 96, in <module> > >> raise Exception('File sizes do not match') > >> Exception: File sizes do not match: 70160 != 4080 + 66048 > >> > >> Adjust the numbers as suggested by Frediano to work with 64-bytes and > >> even 128-bytes alignment. > > And then breaking at 256? As indicated on Matrix, imo we should be able to > > cope with anything up to at least PAGE_SIZE. Or we should reduce .text > > alignment before linking. > > Do you have a concrete proposal on how to do this? > > Because if not, we've had 2 downstreams hit by this build failure, and > we probably ought to take this patch as a stopgap fix, and see about > doing the better fix for 4.20. > I agree with this, merge this and then leave the improvements for follow up(s). Yesterday I checked the output object file (built-in-32.o) to find that the output alignment is 1 byte, which is obviously wrong! There's no current requirement for page alignment. As page management is done in both 32 and 64 bit code, pages are allocated in other parts of the sources and handled correctly. Personally I prefer incremental development with sensible and self-contained improvements kept separate, instead of adding changes and changes because somebody touches a piece of code that can be improved. Usually any piece of code can be improved. Frediano
On 09/01/2025 3:30 pm, Frediano Ziglio wrote: > On Thu, Jan 9, 2025 at 1:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 09/01/2025 1:23 pm, Jan Beulich wrote: >>> On 09.01.2025 14:15, Marek Marczykowski-Górecki wrote: >>>> Xen compiled with -mtune=generic has .text alignment set to 64-bytes. >>>> Having text_diff non-64-bytes-aligned breaks stuff: >>>> >>>> Traceback (most recent call last): >>>> File "/builddir/build/BUILD/xen-4.20.0-build/xen-4.20.0-rc0/xen/./tools/combine_two_binaries.py", line 96, in <module> >>>> raise Exception('File sizes do not match') >>>> Exception: File sizes do not match: 70160 != 4080 + 66048 >>>> >>>> Adjust the numbers as suggested by Frediano to work with 64-bytes and >>>> even 128-bytes alignment. >>> And then breaking at 256? As indicated on Matrix, imo we should be able to >>> cope with anything up to at least PAGE_SIZE. Or we should reduce .text >>> alignment before linking. >> Do you have a concrete proposal on how to do this? >> >> Because if not, we've had 2 downstreams hit by this build failure, and >> we probably ought to take this patch as a stopgap fix, and see about >> doing the better fix for 4.20. >> > I agree with this, merge this and then leave the improvements for follow up(s). > > Yesterday I checked the output object file (built-in-32.o) to find > that the output alignment is 1 byte, which is obviously wrong! No so. It's perfectly easy to end up with 1 byte alignment on .text, commonly with -Os. e.g. In my build, reloc-trampoline.32.o has: Sections: Idx Name Size VMA LMA File off Algn 0 .group 00000008 00000000 00000000 00000034 2**2 CONTENTS, READONLY, GROUP, LINK_ONCE_DISCARD 1 .text 00000076 00000000 00000000 0000003c 2**0 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE 2 .data 00000000 00000000 00000000 000000b2 2**0 CONTENTS, ALLOC, LOAD, DATA 3 .bss 00000000 00000000 00000000 000000b2 2**0 ALLOC 4 .text.__x86.get_pc_thunk.bx 00000004 00000000 00000000 000000b2 2**0 CONTENTS, ALLOC, LOAD, READONLY, CODE 5 .comment 00000020 00000000 00000000 000000b6 2**0 CONTENTS, READONLY 6 .note.GNU-stack 00000000 00000000 00000000 000000d6 2**0 CONTENTS, READONLY and this is entirely reasonable IMO. That said, the fact that built-in-32.S does not have at least one .align directive probably is an issue. x86 will cope, and there's nothing interesting in this code that's going to choke on being misaligned. ~Andrew
On Thu, Jan 9, 2025 at 4:03 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 09/01/2025 3:30 pm, Frediano Ziglio wrote: > > On Thu, Jan 9, 2025 at 1:44 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> On 09/01/2025 1:23 pm, Jan Beulich wrote: > >>> On 09.01.2025 14:15, Marek Marczykowski-Górecki wrote: > >>>> Xen compiled with -mtune=generic has .text alignment set to 64-bytes. > >>>> Having text_diff non-64-bytes-aligned breaks stuff: > >>>> > >>>> Traceback (most recent call last): > >>>> File "/builddir/build/BUILD/xen-4.20.0-build/xen-4.20.0-rc0/xen/./tools/combine_two_binaries.py", line 96, in <module> > >>>> raise Exception('File sizes do not match') > >>>> Exception: File sizes do not match: 70160 != 4080 + 66048 > >>>> > >>>> Adjust the numbers as suggested by Frediano to work with 64-bytes and > >>>> even 128-bytes alignment. > >>> And then breaking at 256? As indicated on Matrix, imo we should be able to > >>> cope with anything up to at least PAGE_SIZE. Or we should reduce .text > >>> alignment before linking. > >> Do you have a concrete proposal on how to do this? > >> > >> Because if not, we've had 2 downstreams hit by this build failure, and > >> we probably ought to take this patch as a stopgap fix, and see about > >> doing the better fix for 4.20. > >> > > I agree with this, merge this and then leave the improvements for follow up(s). > > > > Yesterday I checked the output object file (built-in-32.o) to find > > that the output alignment is 1 byte, which is obviously wrong! > > No so. It's perfectly easy to end up with 1 byte alignment on .text, > commonly with -Os. > > e.g. In my build, reloc-trampoline.32.o has: > > Sections: > Idx Name Size VMA LMA File off Algn > 0 .group 00000008 00000000 00000000 00000034 2**2 > CONTENTS, READONLY, GROUP, LINK_ONCE_DISCARD > 1 .text 00000076 00000000 00000000 0000003c 2**0 > CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE > 2 .data 00000000 00000000 00000000 000000b2 2**0 > CONTENTS, ALLOC, LOAD, DATA > 3 .bss 00000000 00000000 00000000 000000b2 2**0 > ALLOC > 4 .text.__x86.get_pc_thunk.bx 00000004 00000000 00000000 000000b2 2**0 > CONTENTS, ALLOC, LOAD, READONLY, CODE > 5 .comment 00000020 00000000 00000000 000000b6 2**0 > CONTENTS, READONLY > 6 .note.GNU-stack 00000000 00000000 00000000 000000d6 2**0 > CONTENTS, READONLY > > and this is entirely reasonable IMO. > > That said, the fact that built-in-32.S does not have at least one .align > directive probably is an issue. > > x86 will cope, and there's nothing interesting in this code that's going > to choke on being misaligned. > What about something like https://github.com/freddy77/xen/commit/299a1fd70a84e9b52b84d59daff6878a3c42a595 ? Frediano
diff --git a/xen/arch/x86/boot/Makefile b/xen/arch/x86/boot/Makefile index d45787665907..80c32163fbbd 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 := 0x010240 +text_diff := 0x608040 $(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
Xen compiled with -mtune=generic has .text alignment set to 64-bytes. Having text_diff non-64-bytes-aligned breaks stuff: Traceback (most recent call last): File "/builddir/build/BUILD/xen-4.20.0-build/xen-4.20.0-rc0/xen/./tools/combine_two_binaries.py", line 96, in <module> raise Exception('File sizes do not match') Exception: File sizes do not match: 70160 != 4080 + 66048 Adjust the numbers as suggested by Frediano to work with 64-bytes and even 128-bytes alignment. Suggested-by: Frediano Ziglio <frediano.ziglio@cloud.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- xen/arch/x86/boot/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)