| Message ID | 20230629104821.194859-8-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-bios/s390-ccw: Fixes and improvements for start.S (and other files) | expand |
On Thu, 29 Jun 2023 12:48:21 +0200 Thomas Huth <thuth@redhat.com> wrote: > start.S currently cannot be compiled with Clang 16 and binutils 2.40: > > ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for > relocation R_390_PC32DBL > > According to the built-in linker script of ld, the symbol __bss_start > can actually point *before* the .bss section and does not need to have > any alignment, so in certain situations (like when using the internal > assembler of Clang), the __bss_start symbol can indeed be unaligned > and thus it is not suitable for being used with the "larl" instruction > that needs an address that is at least aligned to halfwords. > The problem went unnoticed so far since binutils <= 2.39 did not > check the alignment, but starting with binutils 2.40, such unaligned > addresses are now refused. > > Fix it by loading the address indirectly instead. what are the advantages of this solution compared to your previous one (i.e. align .bss) ? > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662 > Reported-by: Miroslav Rezanina <mrezanin@redhat.com> > Suggested-by: Andreas Krebbel <andreas.krebbel@de.ibm.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > pc-bios/s390-ccw/start.S | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S > index 429a2b30a1..061b06591c 100644 > --- a/pc-bios/s390-ccw/start.S > +++ b/pc-bios/s390-ccw/start.S > @@ -19,7 +19,8 @@ _start: > larl %r15,stack + STACK_SIZE - STACK_FRAME_SIZE /* Set up stack */ > > /* clear bss */ > - larl %r2,__bss_start > + larl %r2,bss_start_literal /* __bss_start might be unaligned ... */ > + lg %r2,0(%r2) /* ... so load it indirectly */ > larl %r3,_end > slgr %r3,%r2 /* get sizeof bss */ > ltgr %r3,%r3 /* bss empty? */ > @@ -45,7 +46,6 @@ done: > memsetxc: > xc 0(1,%r1),0(%r1) > > - > /* > * void disabled_wait(void) > * > @@ -113,6 +113,8 @@ io_new_code: > br %r14 > > .align 8 > +bss_start_literal: > + .quad __bss_start > disabled_wait_psw: > .quad 0x0002000180000000,0x0000000000000000 > enabled_wait_psw:
On 29/06/2023 12.58, Claudio Imbrenda wrote: > On Thu, 29 Jun 2023 12:48:21 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> start.S currently cannot be compiled with Clang 16 and binutils 2.40: >> >> ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for >> relocation R_390_PC32DBL >> >> According to the built-in linker script of ld, the symbol __bss_start >> can actually point *before* the .bss section and does not need to have >> any alignment, so in certain situations (like when using the internal >> assembler of Clang), the __bss_start symbol can indeed be unaligned >> and thus it is not suitable for being used with the "larl" instruction >> that needs an address that is at least aligned to halfwords. >> The problem went unnoticed so far since binutils <= 2.39 did not >> check the alignment, but starting with binutils 2.40, such unaligned >> addresses are now refused. >> >> Fix it by loading the address indirectly instead. > > what are the advantages of this solution compared to your previous one > (i.e. align .bss) ? __bss_start is supposed to point to an address that is before all bss-like segments. There are also segments like .sbss and .bss.plt on other architectures, see https://bugzilla.redhat.com/show_bug.cgi?id=2216662#c11 . Seems like we don't have them on s390x yet, so currently my previous patch is fine, too. But in case there will ever be an extension to the s390x ABI that introduces such additional segments, we have to switch back to __bss_start again. So it sounds slightly more future-proof to me to keep __bss_start here, even if we need a slightly more complex startup code here now. Thomas
On Thu, 29 Jun 2023 13:12:26 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 29/06/2023 12.58, Claudio Imbrenda wrote: > > On Thu, 29 Jun 2023 12:48:21 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> start.S currently cannot be compiled with Clang 16 and binutils 2.40: > >> > >> ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for > >> relocation R_390_PC32DBL > >> > >> According to the built-in linker script of ld, the symbol __bss_start > >> can actually point *before* the .bss section and does not need to have > >> any alignment, so in certain situations (like when using the internal > >> assembler of Clang), the __bss_start symbol can indeed be unaligned > >> and thus it is not suitable for being used with the "larl" instruction > >> that needs an address that is at least aligned to halfwords. > >> The problem went unnoticed so far since binutils <= 2.39 did not > >> check the alignment, but starting with binutils 2.40, such unaligned > >> addresses are now refused. > >> > >> Fix it by loading the address indirectly instead. > > > > what are the advantages of this solution compared to your previous one > > (i.e. align .bss) ? > > __bss_start is supposed to point to an address that is before all bss-like > segments. There are also segments like .sbss and .bss.plt on other > architectures, see https://bugzilla.redhat.com/show_bug.cgi?id=2216662#c11 . > Seems like we don't have them on s390x yet, so currently my previous patch > is fine, too. But in case there will ever be an extension to the s390x ABI > that introduces such additional segments, we have to switch back to > __bss_start again. So it sounds slightly more future-proof to me to keep > __bss_start here, even if we need a slightly more complex startup code here now. fair enough Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > Thomas > >
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S index 429a2b30a1..061b06591c 100644 --- a/pc-bios/s390-ccw/start.S +++ b/pc-bios/s390-ccw/start.S @@ -19,7 +19,8 @@ _start: larl %r15,stack + STACK_SIZE - STACK_FRAME_SIZE /* Set up stack */ /* clear bss */ - larl %r2,__bss_start + larl %r2,bss_start_literal /* __bss_start might be unaligned ... */ + lg %r2,0(%r2) /* ... so load it indirectly */ larl %r3,_end slgr %r3,%r2 /* get sizeof bss */ ltgr %r3,%r3 /* bss empty? */ @@ -45,7 +46,6 @@ done: memsetxc: xc 0(1,%r1),0(%r1) - /* * void disabled_wait(void) * @@ -113,6 +113,8 @@ io_new_code: br %r14 .align 8 +bss_start_literal: + .quad __bss_start disabled_wait_psw: .quad 0x0002000180000000,0x0000000000000000 enabled_wait_psw:
start.S currently cannot be compiled with Clang 16 and binutils 2.40: ld: start.o(.text+0x8): misaligned symbol `__bss_start' (0xc1e5) for relocation R_390_PC32DBL According to the built-in linker script of ld, the symbol __bss_start can actually point *before* the .bss section and does not need to have any alignment, so in certain situations (like when using the internal assembler of Clang), the __bss_start symbol can indeed be unaligned and thus it is not suitable for being used with the "larl" instruction that needs an address that is at least aligned to halfwords. The problem went unnoticed so far since binutils <= 2.39 did not check the alignment, but starting with binutils 2.40, such unaligned addresses are now refused. Fix it by loading the address indirectly instead. Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662 Reported-by: Miroslav Rezanina <mrezanin@redhat.com> Suggested-by: Andreas Krebbel <andreas.krebbel@de.ibm.com> Signed-off-by: Thomas Huth <thuth@redhat.com> --- pc-bios/s390-ccw/start.S | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)