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

[PATCH v2 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. I checked with "objdump" that the change is fine, indeed.

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)

v2:
- Use ".space" instead of ".lcomm" in the third patch to make sure
  that the alignment is really taken into consideration (thanks Richard)
- Alignment of 8 should be enough in the third patch (thank Christian)
- Added Reviewed-bys from v1

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    | 144 +++++++++++++++++++-----------------
 4 files changed, 76 insertions(+), 71 deletions(-)

-- 
2.39.3

[PATCH v2 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.

Reviewed-by: Cédric Le Goater <clg@redhat.com>
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

[PATCH v2 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).

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
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

[PATCH v2 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    | 6 ++++++
 4 files changed, 6 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..111dea261b 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -120,3 +120,9 @@ external_new_mask:
     .quad   0x0000000180000000
 io_new_mask:
     .quad   0x0000000180000000
+
+.bss
+    .align  8
+stack:
+    .space  STACK_SIZE
+    .size   stack,STACK_SIZE
-- 
2.39.3

[PATCH v2 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 111dea261b..a63c4e3ff2 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

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

[Marc Hartmayer]

Thomas Huth <thuth@redhat.com> writes:

> 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).
>
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 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 */
                                         ^^^
                                         You can also add a macro for this
                                         - e.g. STACK_FRAME_SIZE.

Besides that,
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>

>  
>      /* clear bss */
>      larl    %r2,__bss_start
> -- 
> 2.39.3
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen 
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Philippe Mathieu-Daudé]

On 27/6/23 10:26, Marc Hartmayer wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> 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).
>>
>> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   pc-bios/s390-ccw/start.S | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)


>> +#define STACK_SIZE 0x8000
>> +
>>       .globl _start
>>   _start:
>>   
>> -    larl    %r15,stack + 0x8000     /* Set up stack */
>> +    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
>                                           ^^^
>                                           You can also add a macro for this
>                                           - e.g. STACK_FRAME_SIZE.

Yes please :) No need to respin.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Besides that,
> Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
=

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

[Philippe Mathieu-Daudé]

On 27/6/23 09:47, 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.
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   pc-bios/s390-ccw/start.S | 136 +++++++++++++++++++--------------------
>   1 file changed, 68 insertions(+), 68 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:00 +0200
Thomas Huth <thuth@redhat.com> 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.
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.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

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

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:01 +0200
Thomas Huth <thuth@redhat.com> 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).
> 
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>


with Marc's suggestion applied:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.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

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

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:02 +0200
Thomas Huth <thuth@redhat.com> wrote:

> 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>

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.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    | 6 ++++++
>  4 files changed, 6 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..111dea261b 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -120,3 +120,9 @@ external_new_mask:
>      .quad   0x0000000180000000
>  io_new_mask:
>      .quad   0x0000000180000000
> +
> +.bss
> +    .align  8
> +stack:
> +    .space  STACK_SIZE
> +    .size   stack,STACK_SIZE

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

[Claudio Imbrenda]

On Tue, 27 Jun 2023 09:47:03 +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 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 111dea261b..a63c4e3ff2 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

since we are here, do you have guarantees that _end is always correctly
aligned?

if so:

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

>      slgr    %r3,%r2    /* get sizeof bss */
>      ltgr    %r3,%r3    /* bss empty? */

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

[Eric Farman]

On Tue, 2023-06-27 at 09:47 +0200, 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.
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/start.S | 136 +++++++++++++++++++------------------
> --
>  1 file changed, 68 insertions(+), 68 deletions(-)

Oh, that looks nice. Thank you.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

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

[Eric Farman]

On Tue, 2023-06-27 at 11:14 +0200, Philippe Mathieu-Daudé wrote:
> On 27/6/23 10:26, Marc Hartmayer wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> > 
> > > 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).
> > > 
> > > Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> > > Reviewed-by: Cédric Le Goater <clg@redhat.com>
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   pc-bios/s390-ccw/start.S | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> > > +#define STACK_SIZE 0x8000
> > > +
> > >       .globl _start
> > >   _start:
> > >   
> > > -    larl    %r15,stack + 0x8000     /* Set up stack */
> > > +    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
> >                                           ^^^
> >                                           You can also add a macro
> > for this
> >                                           - e.g. STACK_FRAME_SIZE.
> 
> Yes please :) No need to respin.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> > Besides that,
> > Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> =

Ditto :)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

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

[Eric Farman]

On Tue, 2023-06-27 at 09:47 +0200, Thomas Huth wrote:
> 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    | 6 ++++++
>  4 files changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

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

[Eric Farman]

On Tue, 2023-06-27 at 09:47 +0200, Thomas Huth 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 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(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

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

[Thomas Huth]

On 27/06/2023 11.29, Claudio Imbrenda wrote:
> On Tue, 27 Jun 2023 09:47:03 +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 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 111dea261b..a63c4e3ff2 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
> 
> since we are here, do you have guarantees that _end is always correctly
> aligned?

Yes, you can see the internal linker script by running
"ld --verbose", and there I get:

   ...
   .data1          : { *(.data1) }
   _edata = .; PROVIDE (edata = .);
   . = .;
   __bss_start = .;
   .bss            :
   {
    *(.dynbss)
    *(.bss .bss.* .gnu.linkonce.b.*)
    *(COMMON)
    /* Align here to ensure that the .bss section occupies space up to
       _end.  Align after .bss to ensure correct alignment even if the
       .bss section disappears because there are no input sections.
       FIXME: Why do we need it? When there is no .bss section, we do not
       pad the .data section.  */
    . = ALIGN(. != 0 ? 64 / 8 : 1);
   }
   . = ALIGN(64 / 8);
   . = SEGMENT_START("ldata-segment", .);
   . = ALIGN(64 / 8);
   _end = .; PROVIDE (end = .);
   . = DATA_SEGMENT_END (.);
   ...

As you can see, there is no alignment in front of
__bss_start, but there is alignment in front of
__end.

> if so:
> 
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Thanks!

  Thomas

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

[Thomas Huth]

On 27/06/2023 11.14, Philippe Mathieu-Daudé wrote:
> On 27/6/23 10:26, Marc Hartmayer wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>>
>>> 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).
>>>
>>> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   pc-bios/s390-ccw/start.S | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
>>> +#define STACK_SIZE 0x8000
>>> +
>>>       .globl _start
>>>   _start:
>>> -    larl    %r15,stack + 0x8000     /* Set up stack */
>>> +    larl    %r15,stack + STACK_SIZE - 160   /* Set up stack */
>>                                           ^^^
>>                                           You can also add a macro for this
>>                                           - e.g. STACK_FRAME_SIZE.
> 
> Yes please :) No need to respin.

Ok, I'll add:

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

Thanks,
  Thomas

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

[Thomas Huth]

On 27/06/2023 09.47, Thomas Huth wrote:
> 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    | 6 ++++++
>   4 files changed, 6 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..111dea261b 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -120,3 +120,9 @@ external_new_mask:
>       .quad   0x0000000180000000
>   io_new_mask:
>       .quad   0x0000000180000000
> +
> +.bss
> +    .align  8
> +stack:
> +    .space  STACK_SIZE
> +    .size   stack,STACK_SIZE

Sigh, I missed that one of the tcg tests includes start.S ...
I'll include this hunk on top of this patch to get it fixed:

diff --git a/tests/tcg/s390x/head64.S b/tests/tcg/s390x/head64.S
--- a/tests/tcg/s390x/head64.S
+++ b/tests/tcg/s390x/head64.S
@@ -8,6 +8,8 @@
  #include "../../../pc-bios/s390-ccw/start.S"
  #undef main
  
+.text
+
  main_pre:
      aghi %r15,-160                     /* reserve stack for C code */
      brasl %r14,sclp_setup
@@ -24,8 +26,3 @@ success_psw:
      .quad 0x2000180000000,0xfff        /* see is_special_wait_psw() */
  failure_psw:
      .quad 0x2000180000000,0            /* disabled wait */
-
-    .section .bss
-    .align 0x1000
-stack:
-    .skip 0x8000

  Thomas