[PATCH v3 0/7] pc-bios/s390-ccw: Fixes and improvements for start.S (and other files)

[PATCH v3 0/7] pc-bios/s390-ccw: Fixes and improvements for start.S (and other files)

[Thomas Huth]

tl;dr: Please review the final patch - it's the only one that really changed.

This is a respin of the currently pending s390-ccw bios patches. Compared
to v2, I've included other pending patches for the s390-ccw bios this time,
so the series grew a little bit.

The only real code change compared to the previous version is in the
final patch: Instead of using ".bss" as start address of the area that
should be cleared, it now loads __bss_start indirectly instead. That
way we should be safe in case there will be another segment between
__bss_start and .bss one day (like .sbss on other architectures). See
The comments in https://bugzilla.redhat.com/show_bug.cgi?id=2216662
for more information.

Old cover letter follows:

------------------------------- 8< ------------------------------------

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 final 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 other patches patches:

- Juan recently noticed unnecessary types in the s390-ccw bios (patch 1)
  which I continued to improve in patch 2
- Recent binutils complain about executable stack - fixed in patch 3
- Indentation is a mixture between tabs and spaces in start.S (patch 4)
- We do not set up a stack frame for the main() function, which could
  cause memory corruption (patch 5)
- The stack is declared in multiple places, though it's only needed
  in start.S (patch 6)

v3:
- Included other pending s390-ccw bios patches
- Changed the commit message of the "__u*" clean-up patch
- Changed the final patch to load __bss_start indirectly instead

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

Juan Quintela (1):
  s390-ccw: Getting rid of ulong

Thomas Huth (6):
  pc-bios/s390-ccw: Get rid of the the __u* types
  pc-bios/s390-ccw/Makefile: Use -z noexecstack to silence linker
    warning
  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/cio.h           | 232 +++++++++++++++----------------
 pc-bios/s390-ccw/helper.h        |   2 +-
 pc-bios/s390-ccw/s390-ccw.h      |  12 +-
 pc-bios/s390-ccw/virtio-scsi.h   |   2 +-
 pc-bios/s390-ccw/virtio.h        |   4 +-
 pc-bios/s390-ccw/main.c          |   1 -
 pc-bios/s390-ccw/netmain.c       |   1 -
 pc-bios/s390-ccw/virtio-blkdev.c |  12 +-
 pc-bios/s390-ccw/virtio-scsi.c   |   4 +-
 pc-bios/s390-ccw/virtio.c        |  12 +-
 pc-bios/s390-ccw/Makefile        |   2 +-
 pc-bios/s390-ccw/start.S         | 149 +++++++++++---------
 tests/tcg/s390x/head64.S         |   7 +-
 13 files changed, 220 insertions(+), 220 deletions(-)

-- 
2.39.3

[PATCH v3 1/7] s390-ccw: Getting rid of ulong

[Thomas Huth]

From: Juan Quintela <quintela@redhat.com>

Any good reason why this still exist?
I can understand u* and __u* to be linux kernel like, but ulong?

Signed-off-by: Juan Quintela <quintela@redhat.com>
Message-Id: <20230510143925.4094-4-quintela@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/helper.h        |  2 +-
 pc-bios/s390-ccw/s390-ccw.h      |  7 +++----
 pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
 pc-bios/s390-ccw/virtio.h        |  4 ++--
 pc-bios/s390-ccw/virtio-blkdev.c | 12 ++++++------
 pc-bios/s390-ccw/virtio-scsi.c   |  4 ++--
 pc-bios/s390-ccw/virtio.c        | 12 ++++++------
 7 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index 3d0731c4c6..8e3dfcb6d6 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -38,7 +38,7 @@ static inline void yield(void)
 
 static inline void sleep(unsigned int seconds)
 {
-    ulong target = get_time_seconds() + seconds;
+    unsigned long target = get_time_seconds() + seconds;
 
     while (get_time_seconds() < target) {
         yield();
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index b88e0550ab..f849fba74b 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -17,7 +17,6 @@ typedef unsigned char      u8;
 typedef unsigned short     u16;
 typedef unsigned int       u32;
 typedef unsigned long long u64;
-typedef unsigned long      ulong;
 typedef unsigned char      __u8;
 typedef unsigned short     __u16;
 typedef unsigned int       __u32;
@@ -67,11 +66,11 @@ void sclp_get_loadparm_ascii(char *loadparm);
 int sclp_read(char *str, size_t count);
 
 /* virtio.c */
-unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
-                                 ulong subchan_id, void *load_addr);
+unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
+                                 unsigned long subchan_id, void *load_addr);
 bool virtio_is_supported(SubChannelId schid);
 int virtio_blk_setup_device(SubChannelId schid);
-int virtio_read(ulong sector, void *load_addr);
+int virtio_read(unsigned long sector, void *load_addr);
 
 /* bootmap.c */
 void zipl_load(void);
diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-ccw/virtio-scsi.h
index e6b6cd4815..c5612e16a2 100644
--- a/pc-bios/s390-ccw/virtio-scsi.h
+++ b/pc-bios/s390-ccw/virtio-scsi.h
@@ -68,7 +68,7 @@ static inline bool virtio_scsi_response_ok(const VirtioScsiCmdResp *r)
 }
 
 int virtio_scsi_read_many(VDev *vdev,
-                          ulong sector, void *load_addr, int sec_num);
+                          unsigned long sector, void *load_addr, int sec_num);
 int virtio_scsi_setup_device(SubChannelId schid);
 
 #endif /* VIRTIO_SCSI_H */
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index e657d381ec..85bd9d1695 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -190,14 +190,14 @@ int virtio_get_block_size(void);
 uint8_t virtio_get_heads(void);
 uint8_t virtio_get_sectors(void);
 uint64_t virtio_get_blocks(void);
-int virtio_read_many(ulong sector, void *load_addr, int sec_num);
+int virtio_read_many(unsigned long sector, void *load_addr, int sec_num);
 
 #define VIRTIO_SECTOR_SIZE 512
 #define VIRTIO_ISO_BLOCK_SIZE 2048
 #define VIRTIO_SCSI_BLOCK_SIZE 512
 #define VIRTIO_DASD_DEFAULT_BLOCK_SIZE 4096
 
-static inline ulong virtio_sector_adjust(ulong sector)
+static inline unsigned long virtio_sector_adjust(unsigned long sector)
 {
     return sector * (virtio_get_block_size() / VIRTIO_SECTOR_SIZE);
 }
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 794f99b42c..a81207b52e 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -16,7 +16,7 @@
 #define VIRTIO_BLK_F_GEOMETRY   (1 << 4)
 #define VIRTIO_BLK_F_BLK_SIZE   (1 << 6)
 
-static int virtio_blk_read_many(VDev *vdev, ulong sector, void *load_addr,
+static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_addr,
                                 int sec_num)
 {
     VirtioBlkOuthdr out_hdr;
@@ -49,7 +49,7 @@ static int virtio_blk_read_many(VDev *vdev, ulong sector, void *load_addr,
     return status;
 }
 
-int virtio_read_many(ulong sector, void *load_addr, int sec_num)
+int virtio_read_many(unsigned long sector, void *load_addr, int sec_num)
 {
     VDev *vdev = virtio_get_device();
 
@@ -63,14 +63,14 @@ int virtio_read_many(ulong sector, void *load_addr, int sec_num)
     return -1;
 }
 
-unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
-                                 ulong subchan_id, void *load_addr)
+unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
+                                 unsigned long subchan_id, void *load_addr)
 {
     u8 status;
     int sec = rec_list1;
     int sec_num = ((rec_list2 >> 32) & 0xffff) + 1;
     int sec_len = rec_list2 >> 48;
-    ulong addr = (ulong)load_addr;
+    unsigned long addr = (unsigned long)load_addr;
 
     if (sec_len != virtio_get_block_size()) {
         return -1;
@@ -86,7 +86,7 @@ unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
     return addr;
 }
 
-int virtio_read(ulong sector, void *load_addr)
+int virtio_read(unsigned long sector, void *load_addr)
 {
     return virtio_read_many(sector, load_addr, 1);
 }
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index dcce696a33..d1a84b937c 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -150,7 +150,7 @@ static bool scsi_report_luns(VDev *vdev, void *data, uint32_t data_size)
 }
 
 static bool scsi_read_10(VDev *vdev,
-                         ulong sector, int sectors, void *data,
+                         unsigned long sector, int sectors, void *data,
                          unsigned int data_size)
 {
     ScsiCdbRead10 cdb = {
@@ -269,7 +269,7 @@ static int virtio_scsi_locate_device(VDev *vdev)
 }
 
 int virtio_scsi_read_many(VDev *vdev,
-                          ulong sector, void *load_addr, int sec_num)
+                          unsigned long sector, void *load_addr, int sec_num)
 {
     int sector_count;
     int f = vdev->blk_factor;
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index f37510f312..5edd058d88 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -48,10 +48,10 @@ VirtioDevType virtio_get_device_type(void)
 static long kvm_hypercall(unsigned long nr, unsigned long param1,
                           unsigned long param2, unsigned long param3)
 {
-    register ulong r_nr asm("1") = nr;
-    register ulong r_param1 asm("2") = param1;
-    register ulong r_param2 asm("3") = param2;
-    register ulong r_param3 asm("4") = param3;
+    register unsigned long r_nr asm("1") = nr;
+    register unsigned long r_param1 asm("2") = param1;
+    register unsigned long r_param2 asm("3") = param2;
+    register unsigned long r_param3 asm("4") = param3;
     register long retval asm("2");
 
     asm volatile ("diag %%r2,%%r4,0x500"
@@ -145,7 +145,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
         vr->avail->ring[vr->avail->idx % vr->num] = vr->next_idx;
     }
 
-    vr->desc[vr->next_idx].addr = (ulong)p;
+    vr->desc[vr->next_idx].addr = (unsigned long)p;
     vr->desc[vr->next_idx].len = len;
     vr->desc[vr->next_idx].flags = flags & ~VRING_HIDDEN_IS_CHAIN;
     vr->desc[vr->next_idx].next = vr->next_idx;
@@ -182,7 +182,7 @@ int vr_poll(VRing *vr)
  */
 int vring_wait_reply(void)
 {
-    ulong target_second = get_time_seconds() + vdev.wait_reply_timeout;
+    unsigned long target_second = get_time_seconds() + vdev.wait_reply_timeout;
 
     /* Wait for any queue to be updated by the host */
     do {
-- 
2.39.3

[PATCH v3 2/7] pc-bios/s390-ccw: Get rid of the the __u* types

[Thomas Huth]

The types starting with double underscores have likely been
introduced into the s390-ccw bios to be able to re-use structs
from the Linux kernel in the past, but the corresponding structs
in cio.h have been changed there a long time ago already to not
use the variants with the double underscores anymore:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/drivers/s390/cio/cio.h?id=cd6b4f27b9bb2a

So it would be good to replace these in the s390-ccw bios now, too.

Message-Id: <20230627114101.122231-1-thuth@redhat.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/cio.h      | 232 ++++++++++++++++++------------------
 pc-bios/s390-ccw/s390-ccw.h |   4 -
 2 files changed, 116 insertions(+), 120 deletions(-)

diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index 88a88adfd2..8b18153deb 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -17,32 +17,32 @@
  * path management control word
  */
 struct pmcw {
-    __u32 intparm;      /* interruption parameter */
-    __u32 qf:1;         /* qdio facility */
-    __u32 w:1;
-    __u32 isc:3;        /* interruption subclass */
-    __u32 res5:3;       /* reserved zeros */
-    __u32 ena:1;        /* enabled */
-    __u32 lm:2;         /* limit mode */
-    __u32 mme:2;        /* measurement-mode enable */
-    __u32 mp:1;         /* multipath mode */
-    __u32 tf:1;         /* timing facility */
-    __u32 dnv:1;        /* device number valid */
-    __u32 dev:16;       /* device number */
-    __u8  lpm;          /* logical path mask */
-    __u8  pnom;         /* path not operational mask */
-    __u8  lpum;         /* last path used mask */
-    __u8  pim;          /* path installed mask */
-    __u16 mbi;          /* measurement-block index */
-    __u8  pom;          /* path operational mask */
-    __u8  pam;          /* path available mask */
-    __u8  chpid[8];     /* CHPID 0-7 (if available) */
-    __u32 unused1:8;    /* reserved zeros */
-    __u32 st:3;         /* subchannel type */
-    __u32 unused2:18;   /* reserved zeros */
-    __u32 mbfc:1;       /* measurement block format control */
-    __u32 xmwme:1;      /* extended measurement word mode enable */
-    __u32 csense:1;     /* concurrent sense; can be enabled ...*/
+    u32 intparm;        /* interruption parameter */
+    u32 qf:1;           /* qdio facility */
+    u32 w:1;
+    u32 isc:3;          /* interruption subclass */
+    u32 res5:3;         /* reserved zeros */
+    u32 ena:1;          /* enabled */
+    u32 lm:2;           /* limit mode */
+    u32 mme:2;          /* measurement-mode enable */
+    u32 mp:1;           /* multipath mode */
+    u32 tf:1;           /* timing facility */
+    u32 dnv:1;          /* device number valid */
+    u32 dev:16;         /* device number */
+    u8  lpm;            /* logical path mask */
+    u8  pnom;           /* path not operational mask */
+    u8  lpum;           /* last path used mask */
+    u8  pim;            /* path installed mask */
+    u16 mbi;            /* measurement-block index */
+    u8  pom;            /* path operational mask */
+    u8  pam;            /* path available mask */
+    u8  chpid[8];       /* CHPID 0-7 (if available) */
+    u32 unused1:8;      /* reserved zeros */
+    u32 st:3;           /* subchannel type */
+    u32 unused2:18;     /* reserved zeros */
+    u32 mbfc:1;         /* measurement block format control */
+    u32 xmwme:1;        /* extended measurement word mode enable */
+    u32 csense:1;       /* concurrent sense; can be enabled ...*/
                         /*  ... per MSCH, however, if facility */
                         /*  ... is not installed, this results */
                         /*  ... in an operand exception.       */
@@ -50,24 +50,24 @@ struct pmcw {
 
 /* Target SCHIB configuration. */
 struct schib_config {
-    __u64 mba;
-    __u32 intparm;
-    __u16 mbi;
-    __u32 isc:3;
-    __u32 ena:1;
-    __u32 mme:2;
-    __u32 mp:1;
-    __u32 csense:1;
-    __u32 mbfc:1;
+    u64 mba;
+    u32 intparm;
+    u16 mbi;
+    u32 isc:3;
+    u32 ena:1;
+    u32 mme:2;
+    u32 mp:1;
+    u32 csense:1;
+    u32 mbfc:1;
 } __attribute__ ((packed));
 
 struct scsw {
-    __u16 flags;
-    __u16 ctrl;
-    __u32 cpa;
-    __u8 dstat;
-    __u8 cstat;
-    __u16 count;
+    u16 flags;
+    u16 ctrl;
+    u32 cpa;
+    u8 dstat;
+    u8 cstat;
+    u16 count;
 } __attribute__ ((packed));
 
 /* Function Control */
@@ -117,42 +117,42 @@ struct scsw {
 typedef struct schib {
     struct pmcw pmcw;     /* path management control word */
     struct scsw scsw;     /* subchannel status word */
-    __u64 mba;            /* measurement block address */
-    __u8 mda[4];          /* model dependent area */
+    u64 mba;              /* measurement block address */
+    u8 mda[4];            /* model dependent area */
 } __attribute__ ((packed, aligned(4))) Schib;
 
 typedef struct subchannel_id {
     union {
         struct {
-            __u16 cssid:8;
-            __u16 reserved:4;
-            __u16 m:1;
-            __u16 ssid:2;
-            __u16 one:1;
+            u16 cssid:8;
+            u16 reserved:4;
+            u16 m:1;
+            u16 ssid:2;
+            u16 one:1;
         };
-        __u16 sch_id;
+        u16 sch_id;
     };
-    __u16 sch_no;
+    u16 sch_no;
 } __attribute__ ((packed, aligned(4))) SubChannelId;
 
 struct chsc_header {
-    __u16 length;
-    __u16 code;
+    u16 length;
+    u16 code;
 } __attribute__((packed));
 
 typedef struct chsc_area_sda {
     struct chsc_header request;
-    __u8 reserved1:4;
-    __u8 format:4;
-    __u8 reserved2;
-    __u16 operation_code;
-    __u32 reserved3;
-    __u32 reserved4;
-    __u32 operation_data_area[252];
+    u8 reserved1:4;
+    u8 format:4;
+    u8 reserved2;
+    u16 operation_code;
+    u32 reserved3;
+    u32 reserved4;
+    u32 operation_data_area[252];
     struct chsc_header response;
-    __u32 reserved5:4;
-    __u32 format2:4;
-    __u32 reserved6:24;
+    u32 reserved5:4;
+    u32 format2:4;
+    u32 reserved6:24;
 } __attribute__((packed)) ChscAreaSda;
 
 /*
@@ -160,37 +160,37 @@ typedef struct chsc_area_sda {
  */
 struct tpi_info {
     struct subchannel_id schid;
-    __u32 intparm;      /* interruption parameter */
-    __u32 adapter_IO:1;
-    __u32 reserved2:1;
-    __u32 isc:3;
-    __u32 reserved3:12;
-    __u32 int_type:3;
-    __u32 reserved4:12;
+    u32 intparm;      /* interruption parameter */
+    u32 adapter_IO:1;
+    u32 reserved2:1;
+    u32 isc:3;
+    u32 reserved3:12;
+    u32 int_type:3;
+    u32 reserved4:12;
 } __attribute__ ((packed, aligned(4)));
 
 /* channel command word (format 0) */
 typedef struct ccw0 {
-    __u8 cmd_code;
-    __u32 cda:24;
-    __u32 chainData:1;
-    __u32 chain:1;
-    __u32 sli:1;
-    __u32 skip:1;
-    __u32 pci:1;
-    __u32 ida:1;
-    __u32 suspend:1;
-    __u32 mida:1;
-    __u8 reserved;
-    __u16 count;
+    u8 cmd_code;
+    u32 cda:24;
+    u32 chainData:1;
+    u32 chain:1;
+    u32 sli:1;
+    u32 skip:1;
+    u32 pci:1;
+    u32 ida:1;
+    u32 suspend:1;
+    u32 mida:1;
+    u8 reserved;
+    u16 count;
 } __attribute__ ((packed, aligned(8))) Ccw0;
 
 /* channel command word (format 1) */
 typedef struct ccw1 {
-    __u8 cmd_code;
-    __u8 flags;
-    __u16 count;
-    __u32 cda;
+    u8 cmd_code;
+    u8 flags;
+    u16 count;
+    u32 cda;
 } __attribute__ ((packed, aligned(8))) Ccw1;
 
 /* do_cio() CCW formats */
@@ -234,31 +234,31 @@ typedef struct ccw1 {
  * Command-mode operation request block
  */
 typedef struct cmd_orb {
-    __u32 intparm;    /* interruption parameter */
-    __u32 key:4;      /* flags, like key, suspend control, etc. */
-    __u32 spnd:1;     /* suspend control */
-    __u32 res1:1;     /* reserved */
-    __u32 mod:1;      /* modification control */
-    __u32 sync:1;     /* synchronize control */
-    __u32 fmt:1;      /* format control */
-    __u32 pfch:1;     /* prefetch control */
-    __u32 isic:1;     /* initial-status-interruption control */
-    __u32 alcc:1;     /* address-limit-checking control */
-    __u32 ssic:1;     /* suppress-suspended-interr. control */
-    __u32 res2:1;     /* reserved */
-    __u32 c64:1;      /* IDAW/QDIO 64 bit control  */
-    __u32 i2k:1;      /* IDAW 2/4kB block size control */
-    __u32 lpm:8;      /* logical path mask */
-    __u32 ils:1;      /* incorrect length */
-    __u32 zero:6;     /* reserved zeros */
-    __u32 orbx:1;     /* ORB extension control */
-    __u32 cpa;    /* channel program address */
+    u32 intparm;    /* interruption parameter */
+    u32 key:4;      /* flags, like key, suspend control, etc. */
+    u32 spnd:1;     /* suspend control */
+    u32 res1:1;     /* reserved */
+    u32 mod:1;      /* modification control */
+    u32 sync:1;     /* synchronize control */
+    u32 fmt:1;      /* format control */
+    u32 pfch:1;     /* prefetch control */
+    u32 isic:1;     /* initial-status-interruption control */
+    u32 alcc:1;     /* address-limit-checking control */
+    u32 ssic:1;     /* suppress-suspended-interr. control */
+    u32 res2:1;     /* reserved */
+    u32 c64:1;      /* IDAW/QDIO 64 bit control  */
+    u32 i2k:1;      /* IDAW 2/4kB block size control */
+    u32 lpm:8;      /* logical path mask */
+    u32 ils:1;      /* incorrect length */
+    u32 zero:6;     /* reserved zeros */
+    u32 orbx:1;     /* ORB extension control */
+    u32 cpa;        /* channel program address */
 }  __attribute__ ((packed, aligned(4))) CmdOrb;
 
 struct ciw {
-    __u8 type;
-    __u8 command;
-    __u16 count;
+    u8 type;
+    u8 command;
+    u16 count;
 };
 
 #define CU_TYPE_UNKNOWN         0x0000
@@ -271,12 +271,12 @@ struct ciw {
  */
 typedef struct senseid {
     /* common part */
-    __u8  reserved;   /* always 0x'FF' */
-    __u16 cu_type;    /* control unit type */
-    __u8  cu_model;   /* control unit model */
-    __u16 dev_type;   /* device type */
-    __u8  dev_model;  /* device model */
-    __u8  unused;     /* padding byte */
+    u8  reserved;   /* always 0x'FF' */
+    u16 cu_type;    /* control unit type */
+    u8  cu_model;   /* control unit model */
+    u16 dev_type;   /* device type */
+    u8  dev_model;  /* device model */
+    u8  unused;     /* padding byte */
     /* extended part */
     struct ciw ciw[62];
 }  __attribute__ ((packed, aligned(4))) SenseId;
@@ -342,9 +342,9 @@ typedef struct SenseDataEckdDasd {
 /* interruption response block */
 typedef struct irb {
     struct scsw scsw;
-    __u32 esw[5];
-    __u32 ecw[8];
-    __u32 emw[8];
+    u32 esw[5];
+    u32 ecw[8];
+    u32 emw[8];
 }  __attribute__ ((packed, aligned(4))) Irb;
 
 /* Used for SEEK ccw commands */
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index f849fba74b..f68a832718 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -17,10 +17,6 @@ typedef unsigned char      u8;
 typedef unsigned short     u16;
 typedef unsigned int       u32;
 typedef unsigned long long u64;
-typedef unsigned char      __u8;
-typedef unsigned short     __u16;
-typedef unsigned int       __u32;
-typedef unsigned long long __u64;
 
 #define true 1
 #define false 0
-- 
2.39.3

[PATCH v3 3/7] pc-bios/s390-ccw/Makefile: Use -z noexecstack to silence linker warning

[Thomas Huth]

Recent versions of ld complain when linking the s390-ccw bios:

 /usr/bin/ld: warning: start.o: missing .note.GNU-stack section implies
              executable stack
 /usr/bin/ld: NOTE: This behaviour is deprecated and will be removed in
              a future version of the linker

We can silence the warning by telling the linker to mark the stack
as not executable.

Message-Id: <20230622130822.396793-1-thuth@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 2e8cc015aa..acfcd1e71a 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -55,7 +55,7 @@ config-cc.mak: Makefile
 	    $(call cc-option,-march=z900,-march=z10)) 3> config-cc.mak
 -include config-cc.mak
 
-LDFLAGS += -Wl,-pie -nostdlib
+LDFLAGS += -Wl,-pie -nostdlib -z noexecstack
 
 build-all: s390-ccw.img s390-netboot.img
 
-- 
2.39.3

[PATCH v3 4/7] 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>
Message-Id: <20230627074703.99608-2-thuth@redhat.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
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 v3 5/7] 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>
Message-Id: <20230627074703.99608-3-thuth@redhat.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/start.S | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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

[PATCH v3 6/7] 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.

Message-Id: <20230627074703.99608-4-thuth@redhat.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
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 ++++++
 tests/tcg/s390x/head64.S    | 7 ++-----
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index f68a832718..c977a52b50 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -50,7 +50,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 abd6fe6639..429a2b30a1 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -121,3 +121,9 @@ external_new_mask:
     .quad   0x0000000180000000
 io_new_mask:
     .quad   0x0000000180000000
+
+.bss
+    .align  8
+stack:
+    .space  STACK_SIZE
+    .size   stack,STACK_SIZE
diff --git a/tests/tcg/s390x/head64.S b/tests/tcg/s390x/head64.S
index c6f36dfea4..4fe288388a 100644
--- 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
-- 
2.39.3

[PATCH v3 7/7] 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 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(-)

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

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

[Claudio Imbrenda]

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:

Re: [PATCH v3 1/7] s390-ccw: Getting rid of ulong

[Claudio Imbrenda]

On Thu, 29 Jun 2023 12:48:15 +0200
Thomas Huth <thuth@redhat.com> wrote:

> From: Juan Quintela <quintela@redhat.com>
> 
> Any good reason why this still exist?
> I can understand u* and __u* to be linux kernel like, but ulong?

shorter code? ¯\_(ツ)_/¯

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Message-Id: <20230510143925.4094-4-quintela@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

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

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/helper.h        |  2 +-
>  pc-bios/s390-ccw/s390-ccw.h      |  7 +++----
>  pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
>  pc-bios/s390-ccw/virtio.h        |  4 ++--
>  pc-bios/s390-ccw/virtio-blkdev.c | 12 ++++++------
>  pc-bios/s390-ccw/virtio-scsi.c   |  4 ++--
>  pc-bios/s390-ccw/virtio.c        | 12 ++++++------
>  7 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
> index 3d0731c4c6..8e3dfcb6d6 100644
> --- a/pc-bios/s390-ccw/helper.h
> +++ b/pc-bios/s390-ccw/helper.h
> @@ -38,7 +38,7 @@ static inline void yield(void)
>  
>  static inline void sleep(unsigned int seconds)
>  {
> -    ulong target = get_time_seconds() + seconds;
> +    unsigned long target = get_time_seconds() + seconds;
>  
>      while (get_time_seconds() < target) {
>          yield();
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index b88e0550ab..f849fba74b 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -17,7 +17,6 @@ typedef unsigned char      u8;
>  typedef unsigned short     u16;
>  typedef unsigned int       u32;
>  typedef unsigned long long u64;
> -typedef unsigned long      ulong;
>  typedef unsigned char      __u8;
>  typedef unsigned short     __u16;
>  typedef unsigned int       __u32;
> @@ -67,11 +66,11 @@ void sclp_get_loadparm_ascii(char *loadparm);
>  int sclp_read(char *str, size_t count);
>  
>  /* virtio.c */
> -unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
> -                                 ulong subchan_id, void *load_addr);
> +unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
> +                                 unsigned long subchan_id, void *load_addr);
>  bool virtio_is_supported(SubChannelId schid);
>  int virtio_blk_setup_device(SubChannelId schid);
> -int virtio_read(ulong sector, void *load_addr);
> +int virtio_read(unsigned long sector, void *load_addr);
>  
>  /* bootmap.c */
>  void zipl_load(void);
> diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-ccw/virtio-scsi.h
> index e6b6cd4815..c5612e16a2 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.h
> +++ b/pc-bios/s390-ccw/virtio-scsi.h
> @@ -68,7 +68,7 @@ static inline bool virtio_scsi_response_ok(const VirtioScsiCmdResp *r)
>  }
>  
>  int virtio_scsi_read_many(VDev *vdev,
> -                          ulong sector, void *load_addr, int sec_num);
> +                          unsigned long sector, void *load_addr, int sec_num);
>  int virtio_scsi_setup_device(SubChannelId schid);
>  
>  #endif /* VIRTIO_SCSI_H */
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index e657d381ec..85bd9d1695 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -190,14 +190,14 @@ int virtio_get_block_size(void);
>  uint8_t virtio_get_heads(void);
>  uint8_t virtio_get_sectors(void);
>  uint64_t virtio_get_blocks(void);
> -int virtio_read_many(ulong sector, void *load_addr, int sec_num);
> +int virtio_read_many(unsigned long sector, void *load_addr, int sec_num);
>  
>  #define VIRTIO_SECTOR_SIZE 512
>  #define VIRTIO_ISO_BLOCK_SIZE 2048
>  #define VIRTIO_SCSI_BLOCK_SIZE 512
>  #define VIRTIO_DASD_DEFAULT_BLOCK_SIZE 4096
>  
> -static inline ulong virtio_sector_adjust(ulong sector)
> +static inline unsigned long virtio_sector_adjust(unsigned long sector)
>  {
>      return sector * (virtio_get_block_size() / VIRTIO_SECTOR_SIZE);
>  }
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> index 794f99b42c..a81207b52e 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -16,7 +16,7 @@
>  #define VIRTIO_BLK_F_GEOMETRY   (1 << 4)
>  #define VIRTIO_BLK_F_BLK_SIZE   (1 << 6)
>  
> -static int virtio_blk_read_many(VDev *vdev, ulong sector, void *load_addr,
> +static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_addr,
>                                  int sec_num)
>  {
>      VirtioBlkOuthdr out_hdr;
> @@ -49,7 +49,7 @@ static int virtio_blk_read_many(VDev *vdev, ulong sector, void *load_addr,
>      return status;
>  }
>  
> -int virtio_read_many(ulong sector, void *load_addr, int sec_num)
> +int virtio_read_many(unsigned long sector, void *load_addr, int sec_num)
>  {
>      VDev *vdev = virtio_get_device();
>  
> @@ -63,14 +63,14 @@ int virtio_read_many(ulong sector, void *load_addr, int sec_num)
>      return -1;
>  }
>  
> -unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
> -                                 ulong subchan_id, void *load_addr)
> +unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
> +                                 unsigned long subchan_id, void *load_addr)
>  {
>      u8 status;
>      int sec = rec_list1;
>      int sec_num = ((rec_list2 >> 32) & 0xffff) + 1;
>      int sec_len = rec_list2 >> 48;
> -    ulong addr = (ulong)load_addr;
> +    unsigned long addr = (unsigned long)load_addr;
>  
>      if (sec_len != virtio_get_block_size()) {
>          return -1;
> @@ -86,7 +86,7 @@ unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
>      return addr;
>  }
>  
> -int virtio_read(ulong sector, void *load_addr)
> +int virtio_read(unsigned long sector, void *load_addr)
>  {
>      return virtio_read_many(sector, load_addr, 1);
>  }
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
> index dcce696a33..d1a84b937c 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -150,7 +150,7 @@ static bool scsi_report_luns(VDev *vdev, void *data, uint32_t data_size)
>  }
>  
>  static bool scsi_read_10(VDev *vdev,
> -                         ulong sector, int sectors, void *data,
> +                         unsigned long sector, int sectors, void *data,
>                           unsigned int data_size)
>  {
>      ScsiCdbRead10 cdb = {
> @@ -269,7 +269,7 @@ static int virtio_scsi_locate_device(VDev *vdev)
>  }
>  
>  int virtio_scsi_read_many(VDev *vdev,
> -                          ulong sector, void *load_addr, int sec_num)
> +                          unsigned long sector, void *load_addr, int sec_num)
>  {
>      int sector_count;
>      int f = vdev->blk_factor;
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index f37510f312..5edd058d88 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -48,10 +48,10 @@ VirtioDevType virtio_get_device_type(void)
>  static long kvm_hypercall(unsigned long nr, unsigned long param1,
>                            unsigned long param2, unsigned long param3)
>  {
> -    register ulong r_nr asm("1") = nr;
> -    register ulong r_param1 asm("2") = param1;
> -    register ulong r_param2 asm("3") = param2;
> -    register ulong r_param3 asm("4") = param3;
> +    register unsigned long r_nr asm("1") = nr;
> +    register unsigned long r_param1 asm("2") = param1;
> +    register unsigned long r_param2 asm("3") = param2;
> +    register unsigned long r_param3 asm("4") = param3;
>      register long retval asm("2");
>  
>      asm volatile ("diag %%r2,%%r4,0x500"
> @@ -145,7 +145,7 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
>          vr->avail->ring[vr->avail->idx % vr->num] = vr->next_idx;
>      }
>  
> -    vr->desc[vr->next_idx].addr = (ulong)p;
> +    vr->desc[vr->next_idx].addr = (unsigned long)p;
>      vr->desc[vr->next_idx].len = len;
>      vr->desc[vr->next_idx].flags = flags & ~VRING_HIDDEN_IS_CHAIN;
>      vr->desc[vr->next_idx].next = vr->next_idx;
> @@ -182,7 +182,7 @@ int vr_poll(VRing *vr)
>   */
>  int vring_wait_reply(void)
>  {
> -    ulong target_second = get_time_seconds() + vdev.wait_reply_timeout;
> +    unsigned long target_second = get_time_seconds() + vdev.wait_reply_timeout;
>  
>      /* Wait for any queue to be updated by the host */
>      do {

Re: [PATCH v3 1/7] s390-ccw: Getting rid of ulong

[Philippe Mathieu-Daudé]

On 29/6/23 12:48, Thomas Huth wrote:
> From: Juan Quintela <quintela@redhat.com>
> 
> Any good reason why this still exist?
> I can understand u* and __u* to be linux kernel like, but ulong?
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> Message-Id: <20230510143925.4094-4-quintela@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   pc-bios/s390-ccw/helper.h        |  2 +-
>   pc-bios/s390-ccw/s390-ccw.h      |  7 +++----
>   pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
>   pc-bios/s390-ccw/virtio.h        |  4 ++--
>   pc-bios/s390-ccw/virtio-blkdev.c | 12 ++++++------
>   pc-bios/s390-ccw/virtio-scsi.c   |  4 ++--
>   pc-bios/s390-ccw/virtio.c        | 12 ++++++------
>   7 files changed, 21 insertions(+), 22 deletions(-)

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

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

[Thomas Huth]

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

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

[Claudio Imbrenda]

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