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

2023-06-27 Thread 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   0x00018000
  io_new_mask:
  .quad   0x00018000
+
+.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

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

2023-06-26 Thread 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   0x00018000
  io_new_mask:
  .quad   0x00018000
+
+.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

2023-06-26 Thread 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   0x00018000
  io_new_mask:
  .quad   0x00018000
+
+.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

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

2023-06-26 Thread 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 
---
 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   0x00018000
 io_new_mask:
 .quad   0x00018000
+
+.bss
+
+.align  16
+.lcomm  stack,STACK_SIZE
-- 
2.39.3