1. Patchew
  2. Xen
  3. View series

[PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too

Marek Marczykowski-Górecki posted 1 patch 3 weeks, 5 days ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20250109131515.1757764-1-marmarek@invisiblethingslab.com
xen/arch/x86/boot/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Marek Marczykowski-Górecki 3 weeks, 5 days ago 
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(-)

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
-- 
2.46.0
Re: [PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Jan Beulich 3 weeks, 5 days ago 
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
Re: [PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Marek Marczykowski-Górecki 3 weeks, 5 days ago 
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)

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Andrew Cooper 3 weeks, 5 days ago 
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
Re: [PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Frediano Ziglio 3 weeks, 5 days ago 
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
Re: [PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Andrew Cooper 3 weeks, 5 days ago 
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
Re: [PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Frediano Ziglio 3 weeks, 5 days ago 
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
Re: [PATCH] x86/boot: adjust text gap/diff to work with 64-bytes alignment too
Posted by Jan Beulich 3 weeks, 5 days ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.