[PATCH 0/4] pc-bios/s390-ccw: Fixes and improvements for start.S

[PATCH 0/4] pc-bios/s390-ccw: Fixes and improvements for start.S

[Thomas Huth]

Main motivation of this series was a bug that showed up when compiling
with Clang 16 and binutils 2.40 (which has been reported in Fedora ELN, see
https://bugzilla.redhat.com/show_bug.cgi?id=2216662). This is fixed in
the fourth patch.

While working on this issue, I came accross some other issues which I
address in the first three patches:

- Indentation is a mixture between tabs and spaces in start.S (patch 1)
- We do not set up a stack frame for the main() function, which could
  cause memory corruption (patch 2)
- The stack is declared in multiple places, though it's only needed
  in start.S (patch 3)

Thomas Huth (4):
  pc-bios/s390-ccw: Fix indentation in start.S
  pc-bios/s390-ccw: Provide space for initial stack frame in start.S
  pc-bios/s390-ccw: Move the stack array into start.S
  pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

 pc-bios/s390-ccw/s390-ccw.h |   1 -
 pc-bios/s390-ccw/main.c     |   1 -
 pc-bios/s390-ccw/netmain.c  |   1 -
 pc-bios/s390-ccw/start.S    | 143 +++++++++++++++++++-----------------
 4 files changed, 75 insertions(+), 71 deletions(-)

-- 
2.39.3

[PATCH 1/4] pc-bios/s390-ccw: Fix indentation in start.S

[Thomas Huth]

start.S is currently indented with a mixture of spaces and tabs, which
is quite ugly. QEMU coding style says indentation should be 4 spaces,
and this is also what we are using in the assembler files in the
tests/tcg/s390x/ folder already, so let's adjust start.S accordingly.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/start.S | 136 +++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 68 deletions(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 6072906df4..d29de09cc6 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -10,37 +10,37 @@
  * directory.
  */
 
-        .globl _start
+    .globl _start
 _start:
 
-	larl   %r15, stack + 0x8000	/* Set up stack */
+    larl    %r15,stack + 0x8000     /* Set up stack */
 
-	/* clear bss */
-	larl %r2, __bss_start
-	larl %r3, _end
-	slgr %r3, %r2		/* get sizeof bss */
-	ltgr	%r3,%r3 	/* bss empty? */
-	jz	done
-	aghi	%r3,-1
-	srlg	%r4,%r3,8	/* how many 256 byte chunks? */
-	ltgr	%r4,%r4
-	lgr	%r1,%r2
-	jz	remainder
+    /* clear bss */
+    larl    %r2,__bss_start
+    larl    %r3,_end
+    slgr    %r3,%r2    /* get sizeof bss */
+    ltgr    %r3,%r3    /* bss empty? */
+    jz      done
+    aghi    %r3,-1
+    srlg    %r4,%r3,8  /* how many 256 byte chunks? */
+    ltgr    %r4,%r4
+    lgr     %r1,%r2
+    jz      remainder
 loop:
-	xc	0(256,%r1),0(%r1)
-	la	%r1,256(%r1)
-	brctg	%r4,loop
+    xc      0(256,%r1),0(%r1)
+    la      %r1,256(%r1)
+    brctg   %r4,loop
 remainder:
-	larl	%r2,memsetxc
-	ex	%r3,0(%r2)
+    larl    %r2,memsetxc
+    ex      %r3,0(%r2)
 done:
-        /* set up a pgm exception disabled wait psw */
-        larl	%r2, disabled_wait_psw
-        mvc	0x01d0(16), 0(%r2)
-        j      main		/* And call C */
+    /* set up a pgm exception disabled wait psw */
+    larl    %r2,disabled_wait_psw
+    mvc     0x01d0(16),0(%r2)
+    j       main       /* And call C */
 
 memsetxc:
-	xc	0(1,%r1),0(%r1)
+    xc      0(1,%r1),0(%r1)
 
 
 /*
@@ -48,11 +48,11 @@ memsetxc:
  *
  * stops the current guest cpu.
  */
-	.globl disabled_wait
+    .globl disabled_wait
 disabled_wait:
-	larl	%r1,disabled_wait_psw
-	lpswe	0(%r1)
-1:	j	1b
+    larl    %r1,disabled_wait_psw
+    lpswe   0(%r1)
+1:  j       1b
 
 
 /*
@@ -60,61 +60,61 @@ disabled_wait:
  *
  * eats one sclp interrupt
  */
-        .globl consume_sclp_int
+    .globl consume_sclp_int
 consume_sclp_int:
-        /* enable service interrupts in cr0 */
-        stctg   %c0,%c0,0(%r15)
-        oi      6(%r15),0x2
-        lctlg   %c0,%c0,0(%r15)
-        /* prepare external call handler */
-        larl %r1, external_new_code
-        stg %r1, 0x1b8
-        larl %r1, external_new_mask
-        mvc 0x1b0(8),0(%r1)
-        /* load enabled wait PSW */
-        larl %r1, enabled_wait_psw
-        lpswe 0(%r1)
+    /* enable service interrupts in cr0 */
+    stctg   %c0,%c0,0(%r15)
+    oi      6(%r15),0x2
+    lctlg   %c0,%c0,0(%r15)
+    /* prepare external call handler */
+    larl    %r1,external_new_code
+    stg     %r1,0x1b8
+    larl    %r1,external_new_mask
+    mvc     0x1b0(8),0(%r1)
+    /* load enabled wait PSW */
+    larl    %r1,enabled_wait_psw
+    lpswe   0(%r1)
 
 /*
  * void consume_io_int(void)
  *
  * eats one I/O interrupt
  */
-        .globl consume_io_int
+    .globl consume_io_int
 consume_io_int:
-        /* enable I/O interrupts in cr6 */
-        stctg %c6,%c6,0(%r15)
-        oi    4(%r15), 0xff
-        lctlg %c6,%c6,0(%r15)
-        /* prepare i/o call handler */
-        larl  %r1, io_new_code
-        stg   %r1, 0x1f8
-        larl  %r1, io_new_mask
-        mvc   0x1f0(8),0(%r1)
-        /* load enabled wait PSW */
-        larl  %r1, enabled_wait_psw
-        lpswe 0(%r1)
+    /* enable I/O interrupts in cr6 */
+    stctg   %c6,%c6,0(%r15)
+    oi      4(%r15), 0xff
+    lctlg   %c6,%c6,0(%r15)
+    /* prepare i/o call handler */
+    larl    %r1,io_new_code
+    stg     %r1,0x1f8
+    larl    %r1,io_new_mask
+    mvc     0x1f0(8),0(%r1)
+    /* load enabled wait PSW */
+    larl    %r1,enabled_wait_psw
+    lpswe   0(%r1)
 
 external_new_code:
-        /* disable service interrupts in cr0 */
-        stctg   %c0,%c0,0(%r15)
-        ni      6(%r15),0xfd
-        lctlg   %c0,%c0,0(%r15)
-        br      %r14
+    /* disable service interrupts in cr0 */
+    stctg   %c0,%c0,0(%r15)
+    ni      6(%r15),0xfd
+    lctlg   %c0,%c0,0(%r15)
+    br      %r14
 
 io_new_code:
-        /* disable I/O interrupts in cr6 */
-        stctg %c6,%c6,0(%r15)
-        ni    4(%r15), 0x00
-        lctlg %c6,%c6,0(%r15)
-        br    %r14
+    /* disable I/O interrupts in cr6 */
+    stctg   %c6,%c6,0(%r15)
+    ni      4(%r15),0x00
+    lctlg   %c6,%c6,0(%r15)
+    br      %r14
 
-        .align  8
+    .align  8
 disabled_wait_psw:
-        .quad   0x0002000180000000,0x0000000000000000
+    .quad   0x0002000180000000,0x0000000000000000
 enabled_wait_psw:
-        .quad   0x0302000180000000,0x0000000000000000
+    .quad   0x0302000180000000,0x0000000000000000
 external_new_mask:
-        .quad   0x0000000180000000
+    .quad   0x0000000180000000
 io_new_mask:
-        .quad   0x0000000180000000
+    .quad   0x0000000180000000
-- 
2.39.3

Re: [PATCH 1/4] pc-bios/s390-ccw: Fix indentation in start.S

[Cédric Le Goater]

On 6/26/23 15:21, Thomas Huth wrote:
> start.S is currently indented with a mixture of spaces and tabs, which
> is quite ugly. QEMU coding style says indentation should be 4 spaces,
> and this is also what we are using in the assembler files in the
> tests/tcg/s390x/ folder already, so let's adjust start.S accordingly.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   pc-bios/s390-ccw/start.S | 136 +++++++++++++++++++--------------------
>   1 file changed, 68 insertions(+), 68 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 6072906df4..d29de09cc6 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -10,37 +10,37 @@
>    * directory.
>    */
>   
> -        .globl _start
> +    .globl _start
>   _start:
>   
> -	larl   %r15, stack + 0x8000	/* Set up stack */
> +    larl    %r15,stack + 0x8000     /* Set up stack */
>   
> -	/* clear bss */
> -	larl %r2, __bss_start
> -	larl %r3, _end
> -	slgr %r3, %r2		/* get sizeof bss */
> -	ltgr	%r3,%r3 	/* bss empty? */
> -	jz	done
> -	aghi	%r3,-1
> -	srlg	%r4,%r3,8	/* how many 256 byte chunks? */
> -	ltgr	%r4,%r4
> -	lgr	%r1,%r2
> -	jz	remainder
> +    /* clear bss */
> +    larl    %r2,__bss_start
> +    larl    %r3,_end
> +    slgr    %r3,%r2    /* get sizeof bss */
> +    ltgr    %r3,%r3    /* bss empty? */
> +    jz      done
> +    aghi    %r3,-1
> +    srlg    %r4,%r3,8  /* how many 256 byte chunks? */
> +    ltgr    %r4,%r4
> +    lgr     %r1,%r2
> +    jz      remainder
>   loop:
> -	xc	0(256,%r1),0(%r1)
> -	la	%r1,256(%r1)
> -	brctg	%r4,loop
> +    xc      0(256,%r1),0(%r1)
> +    la      %r1,256(%r1)
> +    brctg   %r4,loop
>   remainder:
> -	larl	%r2,memsetxc
> -	ex	%r3,0(%r2)
> +    larl    %r2,memsetxc
> +    ex      %r3,0(%r2)
>   done:
> -        /* set up a pgm exception disabled wait psw */
> -        larl	%r2, disabled_wait_psw
> -        mvc	0x01d0(16), 0(%r2)
> -        j      main		/* And call C */
> +    /* set up a pgm exception disabled wait psw */
> +    larl    %r2,disabled_wait_psw
> +    mvc     0x01d0(16),0(%r2)
> +    j       main       /* And call C */
>   
>   memsetxc:
> -	xc	0(1,%r1),0(%r1)
> +    xc      0(1,%r1),0(%r1)
>   
>   
>   /*
> @@ -48,11 +48,11 @@ memsetxc:
>    *
>    * stops the current guest cpu.
>    */
> -	.globl disabled_wait
> +    .globl disabled_wait
>   disabled_wait:
> -	larl	%r1,disabled_wait_psw
> -	lpswe	0(%r1)
> -1:	j	1b
> +    larl    %r1,disabled_wait_psw
> +    lpswe   0(%r1)
> +1:  j       1b
>   
>   
>   /*
> @@ -60,61 +60,61 @@ disabled_wait:
>    *
>    * eats one sclp interrupt
>    */
> -        .globl consume_sclp_int
> +    .globl consume_sclp_int
>   consume_sclp_int:
> -        /* enable service interrupts in cr0 */
> -        stctg   %c0,%c0,0(%r15)
> -        oi      6(%r15),0x2
> -        lctlg   %c0,%c0,0(%r15)
> -        /* prepare external call handler */
> -        larl %r1, external_new_code
> -        stg %r1, 0x1b8
> -        larl %r1, external_new_mask
> -        mvc 0x1b0(8),0(%r1)
> -        /* load enabled wait PSW */
> -        larl %r1, enabled_wait_psw
> -        lpswe 0(%r1)
> +    /* enable service interrupts in cr0 */
> +    stctg   %c0,%c0,0(%r15)
> +    oi      6(%r15),0x2
> +    lctlg   %c0,%c0,0(%r15)
> +    /* prepare external call handler */
> +    larl    %r1,external_new_code
> +    stg     %r1,0x1b8
> +    larl    %r1,external_new_mask
> +    mvc     0x1b0(8),0(%r1)
> +    /* load enabled wait PSW */
> +    larl    %r1,enabled_wait_psw
> +    lpswe   0(%r1)
>   
>   /*
>    * void consume_io_int(void)
>    *
>    * eats one I/O interrupt
>    */
> -        .globl consume_io_int
> +    .globl consume_io_int
>   consume_io_int:
> -        /* enable I/O interrupts in cr6 */
> -        stctg %c6,%c6,0(%r15)
> -        oi    4(%r15), 0xff
> -        lctlg %c6,%c6,0(%r15)
> -        /* prepare i/o call handler */
> -        larl  %r1, io_new_code
> -        stg   %r1, 0x1f8
> -        larl  %r1, io_new_mask
> -        mvc   0x1f0(8),0(%r1)
> -        /* load enabled wait PSW */
> -        larl  %r1, enabled_wait_psw
> -        lpswe 0(%r1)
> +    /* enable I/O interrupts in cr6 */
> +    stctg   %c6,%c6,0(%r15)
> +    oi      4(%r15), 0xff
> +    lctlg   %c6,%c6,0(%r15)
> +    /* prepare i/o call handler */
> +    larl    %r1,io_new_code
> +    stg     %r1,0x1f8
> +    larl    %r1,io_new_mask
> +    mvc     0x1f0(8),0(%r1)
> +    /* load enabled wait PSW */
> +    larl    %r1,enabled_wait_psw
> +    lpswe   0(%r1)
>   
>   external_new_code:
> -        /* disable service interrupts in cr0 */
> -        stctg   %c0,%c0,0(%r15)
> -        ni      6(%r15),0xfd
> -        lctlg   %c0,%c0,0(%r15)
> -        br      %r14
> +    /* disable service interrupts in cr0 */
> +    stctg   %c0,%c0,0(%r15)
> +    ni      6(%r15),0xfd
> +    lctlg   %c0,%c0,0(%r15)
> +    br      %r14
>   
>   io_new_code:
> -        /* disable I/O interrupts in cr6 */
> -        stctg %c6,%c6,0(%r15)
> -        ni    4(%r15), 0x00
> -        lctlg %c6,%c6,0(%r15)
> -        br    %r14
> +    /* disable I/O interrupts in cr6 */
> +    stctg   %c6,%c6,0(%r15)
> +    ni      4(%r15),0x00
> +    lctlg   %c6,%c6,0(%r15)
> +    br      %r14
>   
> -        .align  8
> +    .align  8
>   disabled_wait_psw:
> -        .quad   0x0002000180000000,0x0000000000000000
> +    .quad   0x0002000180000000,0x0000000000000000
>   enabled_wait_psw:
> -        .quad   0x0302000180000000,0x0000000000000000
> +    .quad   0x0302000180000000,0x0000000000000000
>   external_new_mask:
> -        .quad   0x0000000180000000
> +    .quad   0x0000000180000000
>   io_new_mask:
> -        .quad   0x0000000180000000
> +    .quad   0x0000000180000000

[PATCH 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Thomas Huth]

Providing the space of a stack frame is the duty of the caller,
so we should reserve 160 bytes before jumping into the main function.
Otherwise the main() function might write past the stack array.

While we're at it, add a proper STACK_SIZE macro for the stack size
instead of using magic numbers (this is also required for the following
patch).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/start.S | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index d29de09cc6..29b0a9ece0 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -10,10 +10,12 @@
  * directory.
  */
 
+#define STACK_SIZE 0x8000
+
     .globl _start
 _start:
 
-    larl    %r15,stack + 0x8000     /* Set up stack */
+    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
 
     /* clear bss */
     larl    %r2,__bss_start
-- 
2.39.3

Re: [PATCH 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Cédric Le Goater]

On 6/26/23 15:21, Thomas Huth wrote:
> Providing the space of a stack frame is the duty of the caller,
> so we should reserve 160 bytes before jumping into the main function.
> Otherwise the main() function might write past the stack array.
> 
> While we're at it, add a proper STACK_SIZE macro for the stack size
> instead of using magic numbers (this is also required for the following
> patch).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

> ---
>   pc-bios/s390-ccw/start.S | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index d29de09cc6..29b0a9ece0 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -10,10 +10,12 @@
>    * directory.
>    */
>   
> +#define STACK_SIZE 0x8000
> +
>       .globl _start
>   _start:
>   
> -    larl    %r15,stack + 0x8000     /* Set up stack */
> +    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
>   
>       /* clear bss */
>       larl    %r2,__bss_start

Re: [PATCH 2/4] pc-bios/s390-ccw: Provide space for initial stack frame in start.S

[Christian Borntraeger]

Am 26.06.23 um 15:21 schrieb Thomas Huth:
> Providing the space of a stack frame is the duty of the caller,
> so we should reserve 160 bytes before jumping into the main function.
> Otherwise the main() function might write past the stack array.
> 
> While we're at it, add a proper STACK_SIZE macro for the stack size
> instead of using magic numbers (this is also required for the following
> patch).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   pc-bios/s390-ccw/start.S | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index d29de09cc6..29b0a9ece0 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -10,10 +10,12 @@
>    * directory.
>    */
>   
> +#define STACK_SIZE 0x8000
> +
>       .globl _start
>   _start:
>   
> -    larl    %r15,stack + 0x8000     /* Set up stack */
> +    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

[PATCH 3/4] pc-bios/s390-ccw: Move the stack array into start.S

[Thomas Huth]

The stack array is only referenced from the start-up code (which is
shared between the s390-ccw.img and the s390-netboot.img), but it is
currently declared twice, once in main.c and once in netmain.c.
It makes more sense to declare this in start.S instead - which will
also be helpful in the next patch, since we need to mention the .bss
section in start.S in that patch.

While we're at it, let's also drop the huge alignment of the stack,
since there is no technical requirement for aligning it to page
boundaries.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/s390-ccw.h | 1 -
 pc-bios/s390-ccw/main.c     | 1 -
 pc-bios/s390-ccw/netmain.c  | 1 -
 pc-bios/s390-ccw/start.S    | 5 +++++
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index b88e0550ab..91afcbbca9 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -55,7 +55,6 @@ void consume_io_int(void);
 /* main.c */
 void write_subsystem_identification(void);
 void write_iplb_location(void);
-extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 unsigned int get_loadparm_index(void);
 void main(void);
 
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index a2def83e82..5506798098 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -17,7 +17,6 @@
 #include "virtio-scsi.h"
 #include "dasd-ipl.h"
 
-char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 static SubChannelId blk_schid = { .one = 1 };
 static char loadparm_str[LOADPARM_LEN + 1];
 QemuIplParameters qipl;
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 056e93a818..5cd619b2d6 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -50,7 +50,6 @@ void write_iplb_location(void) {}
 /* STSI 3.2.2 offset of first vmdb + offset of uuid inside vmdb */
 #define STSI322_VMDB_UUID_OFFSET ((8 + 12) * 4)
 
-char stack[PAGE_SIZE * 8] __attribute__((aligned(PAGE_SIZE)));
 IplParameterBlock iplb __attribute__((aligned(PAGE_SIZE)));
 static char cfgbuf[2048];
 
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 29b0a9ece0..47ef6e8aa8 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -120,3 +120,8 @@ external_new_mask:
     .quad   0x0000000180000000
 io_new_mask:
     .quad   0x0000000180000000
+
+.bss
+
+    .align  16
+    .lcomm  stack,STACK_SIZE
-- 
2.39.3

Re: [PATCH 3/4] pc-bios/s390-ccw: Move the stack array into start.S

[Christian Borntraeger]

Am 26.06.23 um 15:21 schrieb Thomas Huth:

> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 29b0a9ece0..47ef6e8aa8 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -120,3 +120,8 @@ external_new_mask:
>       .quad   0x0000000180000000
>   io_new_mask:
>       .quad   0x0000000180000000
> +
> +.bss
> +
> +    .align  16
> +    .lcomm  stack,STACK_SIZE

IIRC, the ELF ABI defines the stack to be 8 byte aligned, but 16 certainly does not hurt.

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Re: [PATCH 3/4] pc-bios/s390-ccw: Move the stack array into start.S

[Richard Henderson]

On 6/26/23 16:54, Christian Borntraeger wrote:
> 
> Am 26.06.23 um 15:21 schrieb Thomas Huth:
> 
>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>> index 29b0a9ece0..47ef6e8aa8 100644
>> --- a/pc-bios/s390-ccw/start.S
>> +++ b/pc-bios/s390-ccw/start.S
>> @@ -120,3 +120,8 @@ external_new_mask:
>>       .quad   0x0000000180000000
>>   io_new_mask:
>>       .quad   0x0000000180000000
>> +
>> +.bss
>> +
>> +    .align  16
>> +    .lcomm  stack,STACK_SIZE
> 
> IIRC, the ELF ABI defines the stack to be 8 byte aligned, but 16 certainly does not hurt.

This doesn't do what you think it does.

.lcomm produces a COMMON symbol, which is merged with .bss at link time.  Thus the .align 
does nothing.  Even switching to .bss section does nothing here.

You want

.bss
	.align 16
stack:
	.space	STACK_SIZE
	.size	stack, STACK_SIZE


r~

Re: [PATCH 3/4] pc-bios/s390-ccw: Move the stack array into start.S

[Thomas Huth]

On 26/06/2023 17.25, Richard Henderson wrote:
> On 6/26/23 16:54, Christian Borntraeger wrote:
>>
>> Am 26.06.23 um 15:21 schrieb Thomas Huth:
>>
>>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>>> index 29b0a9ece0..47ef6e8aa8 100644
>>> --- a/pc-bios/s390-ccw/start.S
>>> +++ b/pc-bios/s390-ccw/start.S
>>> @@ -120,3 +120,8 @@ external_new_mask:
>>>       .quad   0x0000000180000000
>>>   io_new_mask:
>>>       .quad   0x0000000180000000
>>> +
>>> +.bss
>>> +
>>> +    .align  16
>>> +    .lcomm  stack,STACK_SIZE
>>
>> IIRC, the ELF ABI defines the stack to be 8 byte aligned, but 16 certainly 
>> does not hurt.
> 
> This doesn't do what you think it does.
> 
> .lcomm produces a COMMON symbol, which is merged with .bss at link time.  
> Thus the .align does nothing.  Even switching to .bss section does nothing 
> here.
> 
> You want
> 
> .bss
>      .align 16
> stack:
>      .space    STACK_SIZE
>      .size    stack, STACK_SIZE

Oh, thanks! I'll fix it in v2.

  Thomas

[PATCH 4/4] pc-bios/s390-ccw: Don't use __bss_start with the "larl" instruction

[Thomas Huth]

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 using the real start address of the .bss section instead.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2216662
Reported-by: Miroslav Rezanina <mrezanin@redhat.com>
Suggested-by: Nick Clifton <nickc@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/start.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 47ef6e8aa8..6747d4c600 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -18,7 +18,7 @@ _start:
     larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
 
     /* clear bss */
-    larl    %r2,__bss_start
+    larl    %r2,.bss
     larl    %r3,_end
     slgr    %r3,%r2    /* get sizeof bss */
     ltgr    %r3,%r3    /* bss empty? */
-- 
2.39.3