[PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Janosch Frank]

The bios is in dire need for a cleanup as there are still a lot of
magic constants being used throughout as well as duplicated code.

In the first part of this series we consolidate constants and
functions, as well as doing some minor cleanups and fixes.

The patches are available here:
https://github.com/frankjaa/qemu/pull/new/cleanup_bios

v5:
	* Fixed whitespace damage
	* Removed reset PSW mask changes in dasd-ipl.c
	* Added jump2ipl.c cleanup patches

v4:
	* Renamed time.h to s390-time.h
	* Fixed function names in sleep()
	* Changed order of sense_id_ccw initialization
	* Added missing include before sleep()

v3:
	* Dropped 0x00 to 0x0/0 patch
	* Moved some timing functions into helper.h instead of time.h
	* Fixed IPL psw manipulation in dasd-ipl.c
 	* Minor cosmetic fixes found by review

v2:
	* Included cio fixup to get rid of compile errors...
	* Minor cosmetic fixes found by review


Janosch Frank (12):
  pc-bios: s390x: cio.c cleanup and compile fix
  pc-bios: s390x: Consolidate timing functions into time.h
  pc-bios: s390x: Move sleep and yield to helper.h
  pc-bios: s390x: Get rid of magic offsets into the lowcore
  pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
  pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
  pc-bios: s390x: Use PSW masks where possible and introduce
    PSW_MASK_SHORT_ADDR
  pc-bios: s390x: Move panic() into header and add infinite loop
  pc-bios: s390x: Use ebcdic2ascii table
  pc-bios: s390x: Make u32 ptr check explicit
  pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
  pc-bios: s390x: Cleanup jump to ipl code

 pc-bios/s390-ccw/bootmap.c     |  9 ++++----
 pc-bios/s390-ccw/bootmap.h     |  2 +-
 pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
 pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
 pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
 pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
 pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
 pc-bios/s390-ccw/main.c        | 15 +++----------
 pc-bios/s390-ccw/menu.c        |  1 +
 pc-bios/s390-ccw/netmain.c     | 23 +++----------------
 pc-bios/s390-ccw/s390-arch.h   |  4 +++-
 pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
 pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
 pc-bios/s390-ccw/start.S       |  5 +++--
 pc-bios/s390-ccw/virtio-net.c  |  2 ++
 pc-bios/s390-ccw/virtio-scsi.c |  2 ++
 pc-bios/s390-ccw/virtio.c      | 18 +++------------
 17 files changed, 120 insertions(+), 125 deletions(-)
 create mode 100644 pc-bios/s390-ccw/s390-time.h

-- 
2.25.1

[PATCH v5 01/12] pc-bios: s390x: cio.c cleanup and compile fix

[Janosch Frank]

Let's initialize the structs at the beginning to ease reading and also
zeroing all other fields. This also makes the compiler stop
complaining about sense_id_ccw.flags being ored into when it's not
initialized.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
index 339ec5fbe7..83ca27ab41 100644
--- a/pc-bios/s390-ccw/cio.c
+++ b/pc-bios/s390-ccw/cio.c
@@ -49,13 +49,13 @@ void enable_subchannel(SubChannelId schid)
 
 uint16_t cu_type(SubChannelId schid)
 {
-    Ccw1 sense_id_ccw;
     SenseId sense_data;
-
-    sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
-    sense_id_ccw.cda = ptr2u32(&sense_data);
-    sense_id_ccw.count = sizeof(sense_data);
-    sense_id_ccw.flags |= CCW_FLAG_SLI;
+    Ccw1 sense_id_ccw = {
+        .cmd_code = CCW_CMD_SENSE_ID,
+        .flags = CCW_FLAG_SLI,
+        .count = sizeof(sense_data),
+        .cda = ptr2u32(&sense_data),
+    };
 
     if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
         panic("Failed to run SenseID CCw\n");
@@ -67,13 +67,13 @@ uint16_t cu_type(SubChannelId schid)
 int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
                  uint16_t data_size)
 {
-    Ccw1 senseCcw;
+    Ccw1 senseCcw = {
+        .cmd_code = CCW_CMD_BASIC_SENSE,
+        .count = data_size,
+        .cda = ptr2u32(sense_data),
+    };
     Irb irb;
 
-    senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
-    senseCcw.cda = ptr2u32(sense_data);
-    senseCcw.count = data_size;
-
     return __do_cio(schid, ptr2u32(&senseCcw), CCW_FMT1, &irb);
 }
 
@@ -314,7 +314,17 @@ static void print_irb_err(Irb *irb)
  */
 static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
 {
-    CmdOrb orb = {};
+    /*
+     * QEMU's CIO implementation requires prefetch and 64-bit idaws. We
+     * allow all paths.
+     */
+    CmdOrb orb = {
+        .fmt = fmt,
+        .pfch = 1,
+        .c64 = 1,
+        .lpm = 0xFF,
+        .cpa = ccw_addr,
+    };
     int rc;
 
     IPL_assert(fmt == 0 || fmt == 1, "Invalid ccw format");
@@ -324,12 +334,6 @@ static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
         IPL_assert(ccw_addr <= 0xFFFFFF - 8, "Invalid ccw address");
     }
 
-    orb.fmt = fmt;
-    orb.pfch = 1;  /* QEMU's cio implementation requires prefetch */
-    orb.c64 = 1;   /* QEMU's cio implementation requires 64-bit idaws */
-    orb.lpm = 0xFF; /* All paths allowed */
-    orb.cpa = ccw_addr;
-
     rc = ssch(schid, &orb);
     if (rc == 1 || rc == 2) {
         /* Subchannel status pending or busy. Eat status and ask for retry. */
-- 
2.25.1

[PATCH v5 02/12] pc-bios: s390x: Consolidate timing functions into time.h

[Janosch Frank]

Let's consolidate timing related functions into one header.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Acked-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/menu.c        |  1 +
 pc-bios/s390-ccw/netmain.c     | 15 +++------------
 pc-bios/s390-ccw/s390-ccw.h    |  8 ++++----
 pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++++++
 pc-bios/s390-ccw/virtio-net.c  |  1 +
 pc-bios/s390-ccw/virtio-scsi.c |  1 +
 pc-bios/s390-ccw/virtio.c      | 18 +++---------------
 7 files changed, 36 insertions(+), 31 deletions(-)
 create mode 100644 pc-bios/s390-ccw/s390-time.h

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index ce3815b201..de8260a5d6 100644
--- a/pc-bios/s390-ccw/menu.c
+++ b/pc-bios/s390-ccw/menu.c
@@ -12,6 +12,7 @@
 #include "libc.h"
 #include "s390-ccw.h"
 #include "sclp.h"
+#include "s390-time.h"
 
 #define KEYCODE_NO_INP '\0'
 #define KEYCODE_ESCAPE '\033'
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 309ffa30d9..f1ee63577a 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -35,6 +35,7 @@
 #include "s390-ccw.h"
 #include "cio.h"
 #include "virtio.h"
+#include "s390-time.h"
 
 #define DEFAULT_BOOT_RETRIES 10
 #define DEFAULT_TFTP_RETRIES 20
@@ -57,24 +58,14 @@ static SubChannelId net_schid = { .one = 1 };
 static uint8_t mac[6];
 static uint64_t dest_timer;
 
-static uint64_t get_timer_ms(void)
-{
-    uint64_t clk;
-
-    asm volatile(" stck %0 " : : "Q"(clk) : "memory");
-
-    /* Bit 51 is incremented each microsecond */
-    return (clk >> (63 - 51)) / 1000;
-}
-
 void set_timer(int val)
 {
-    dest_timer = get_timer_ms() + val;
+    dest_timer = get_time_ms() + val;
 }
 
 int get_timer(void)
 {
-    return dest_timer - get_timer_ms();
+    return dest_timer - get_time_ms();
 }
 
 int get_sec_ticks(void)
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 21f27e7990..fae1de363f 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -74,8 +74,6 @@ unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
 bool virtio_is_supported(SubChannelId schid);
 void virtio_blk_setup_device(SubChannelId schid);
 int virtio_read(ulong sector, void *load_addr);
-u64 get_clock(void);
-ulong get_second(void);
 
 /* bootmap.c */
 void zipl_load(void);
@@ -153,11 +151,13 @@ static inline void yield(void)
 
 #define MAX_SECTOR_SIZE 4096
 
+#include "s390-time.h"
+
 static inline void sleep(unsigned int seconds)
 {
-    ulong target = get_second() + seconds;
+    ulong target = get_time_seconds() + seconds;
 
-    while (get_second() < target) {
+    while (get_time_seconds() < target) {
         yield();
     }
 }
diff --git a/pc-bios/s390-ccw/s390-time.h b/pc-bios/s390-ccw/s390-time.h
new file mode 100644
index 0000000000..ed6d982371
--- /dev/null
+++ b/pc-bios/s390-ccw/s390-time.h
@@ -0,0 +1,23 @@
+#ifndef TIME_H
+#define TIME_H
+
+static inline u64 get_clock(void)
+{
+    u64 r;
+
+    asm volatile("stck %0" : "=Q" (r) : : "cc");
+    return r;
+}
+
+static inline u64 get_time_ms(void)
+{
+    /* Bit 51 is incremented each microsecond */
+    return (get_clock() >> 12) / 1000;
+}
+
+static inline u64 get_time_seconds(void)
+{
+    return get_time_ms() / 1000;
+}
+
+#endif
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index ff7f4dad25..a13f3b6fb9 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -19,6 +19,7 @@
 #include <ethernet.h>
 #include "s390-ccw.h"
 #include "virtio.h"
+#include "s390-time.h"
 
 #ifndef DEBUG_VIRTIO_NET
 #define DEBUG_VIRTIO_NET 0
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 4fe4b9d261..7bf0be4ffa 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -14,6 +14,7 @@
 #include "virtio.h"
 #include "scsi.h"
 #include "virtio-scsi.h"
+#include "s390-time.h"
 
 static ScsiDevice default_scsi_device;
 static VirtioScsiCmdReq req;
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index fb40ca9828..ab49840db8 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -15,6 +15,7 @@
 #include "virtio-scsi.h"
 #include "bswap.h"
 #include "helper.h"
+#include "s390-time.h"
 
 #define VRING_WAIT_REPLY_TIMEOUT 30
 
@@ -157,19 +158,6 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
     }
 }
 
-u64 get_clock(void)
-{
-    u64 r;
-
-    asm volatile("stck %0" : "=Q" (r) : : "cc");
-    return r;
-}
-
-ulong get_second(void)
-{
-    return (get_clock() >> 12) / 1000000;
-}
-
 int vr_poll(VRing *vr)
 {
     if (vr->used->idx == vr->used_idx) {
@@ -194,7 +182,7 @@ int vr_poll(VRing *vr)
  */
 int vring_wait_reply(void)
 {
-    ulong target_second = get_second() + vdev.wait_reply_timeout;
+    ulong target_second = get_time_seconds() + vdev.wait_reply_timeout;
 
     /* Wait for any queue to be updated by the host */
     do {
@@ -207,7 +195,7 @@ int vring_wait_reply(void)
         if (r) {
             return 0;
         }
-    } while (!vdev.wait_reply_timeout || (get_second() < target_second));
+    } while (!vdev.wait_reply_timeout || (get_time_seconds() < target_second));
 
     return 1;
 }
-- 
2.25.1

[PATCH v5 03/12] pc-bios: s390x: Move sleep and yield to helper.h

[Janosch Frank]

They are definitely helper functions.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 pc-bios/s390-ccw/helper.h      | 17 +++++++++++++++++
 pc-bios/s390-ccw/s390-ccw.h    | 18 ------------------
 pc-bios/s390-ccw/virtio-net.c  |  1 +
 pc-bios/s390-ccw/virtio-scsi.c |  1 +
 4 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index 78d5bc7442..32a453b634 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -14,6 +14,7 @@
 #define S390_CCW_HELPER_H
 
 #include "s390-ccw.h"
+#include "s390-time.h"
 
 /* Avoids compiler warnings when casting a pointer to a u32 */
 static inline uint32_t ptr2u32(void *ptr)
@@ -28,4 +29,20 @@ static inline void *u32toptr(uint32_t n)
     return (void *)(uint64_t)n;
 }
 
+static inline void yield(void)
+{
+    asm volatile ("diag 0,0,0x44"
+                  : :
+                  : "memory", "cc");
+}
+
+static inline void sleep(unsigned int seconds)
+{
+    ulong target = get_time_seconds() + seconds;
+
+    while (get_time_seconds() < target) {
+        yield();
+    }
+}
+
 #endif
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index fae1de363f..c5820e43ae 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -142,26 +142,8 @@ static inline void debug_print_addr(const char *desc, void *p)
 #define KVM_S390_VIRTIO_SET_STATUS      2
 #define KVM_S390_VIRTIO_CCW_NOTIFY      3
 
-static inline void yield(void)
-{
-    asm volatile ("diag 0,0,0x44"
-                  : :
-                  : "memory", "cc");
-}
-
 #define MAX_SECTOR_SIZE 4096
 
-#include "s390-time.h"
-
-static inline void sleep(unsigned int seconds)
-{
-    ulong target = get_time_seconds() + seconds;
-
-    while (get_time_seconds() < target) {
-        yield();
-    }
-}
-
 static inline void IPL_assert(bool term, const char *message)
 {
     if (!term) {
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index a13f3b6fb9..2fcb0a58c5 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -20,6 +20,7 @@
 #include "s390-ccw.h"
 #include "virtio.h"
 #include "s390-time.h"
+#include "helper.h"
 
 #ifndef DEBUG_VIRTIO_NET
 #define DEBUG_VIRTIO_NET 0
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 7bf0be4ffa..eddfb8a7ad 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -15,6 +15,7 @@
 #include "scsi.h"
 #include "virtio-scsi.h"
 #include "s390-time.h"
+#include "helper.h"
 
 static ScsiDevice default_scsi_device;
 static VirtioScsiCmdReq req;
-- 
2.25.1

[PATCH v5 04/12] pc-bios: s390x: Get rid of magic offsets into the lowcore

[Janosch Frank]

If we have a lowcore struct that has members for offsets that we want
to touch, why not use it?

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 pc-bios/s390-ccw/cio.h  | 17 +++++++++++------
 pc-bios/s390-ccw/main.c |  8 +++-----
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index aaa432dedd..1e5d4e92e1 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -122,12 +122,17 @@ typedef struct schib {
 } __attribute__ ((packed, aligned(4))) Schib;
 
 typedef struct subchannel_id {
-        __u32 cssid:8;
-        __u32:4;
-        __u32 m:1;
-        __u32 ssid:2;
-        __u32 one:1;
-        __u32 sch_no:16;
+    union {
+        struct {
+            __u16 cssid:8;
+            __u16 reserved:4;
+            __u16 m:1;
+            __u16 ssid:2;
+            __u16 one:1;
+        };
+        __u16 sch_id;
+    };
+    __u16 sch_no;
 } __attribute__ ((packed, aligned(4))) SubChannelId;
 
 struct chsc_header {
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 4e65b411e1..8b912454c9 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
  */
 void write_subsystem_identification(void)
 {
-    SubChannelId *schid = (SubChannelId *) 184;
-    uint32_t *zeroes = (uint32_t *) 188;
-
-    *schid = blk_schid;
-    *zeroes = 0;
+    lowcore->subchannel_id = blk_schid.sch_id;
+    lowcore->subchannel_nr = blk_schid.sch_no;
+    lowcore->io_int_parm = 0;
 }
 
 void write_iplb_location(void)
-- 
2.25.1

[PATCH v5 05/12] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes

[Janosch Frank]

jump_to_low_kernel() and the functions that it calls will already or
64 bit addressing into the reset psw mask when executing
jump_to_IPL_2() after the diag308 subcode 1.

The kernel proper is then branched to rather than doing a full PSW
change.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 pc-bios/s390-ccw/dasd-ipl.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..e8f2846740 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -206,7 +206,6 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
  */
 void dasd_ipl(SubChannelId schid, uint16_t cutype)
 {
-    PSWLegacy *pswl = (PSWLegacy *) 0x00;
     uint32_t ipl2_addr;
 
     /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
@@ -229,7 +228,5 @@ void dasd_ipl(SubChannelId schid, uint16_t cutype)
     run_ipl2(schid, cutype, ipl2_addr);
 
     /* Transfer control to the guest operating system */
-    pswl->mask |= PSW_MASK_EAMODE;   /* Force z-mode */
-    pswl->addr |= PSW_MASK_BAMODE;   /* ...          */
     jump_to_low_kernel();
 }
-- 
2.25.1

[PATCH v5 06/12] pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64

[Janosch Frank]

This constant enables 64 bit addressing, not the ESAME architecture,
so it shouldn't be named ZMODE.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 pc-bios/s390-ccw/s390-arch.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 5f36361c02..73852029d4 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -29,7 +29,7 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy size incorrect");
 #define PSW_MASK_WAIT       0x0002000000000000ULL
 #define PSW_MASK_EAMODE     0x0000000100000000ULL
 #define PSW_MASK_BAMODE     0x0000000080000000ULL
-#define PSW_MASK_ZMODE      (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
+#define PSW_MASK_64         (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
 
 /* Low core mapping */
 typedef struct LowCore {
-- 
2.25.1

[PATCH v5 07/12] pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR

[Janosch Frank]

Let's move some of the PSW mask defines into s390-arch.h and use them
in jump2ipl.c. Also let's introduce a new constant for the address
mask of 8 byte (short) PSWs.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 pc-bios/s390-ccw/jump2ipl.c  | 10 ++++------
 pc-bios/s390-ccw/s390-arch.h |  2 ++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 4eba2510b0..767012bf0c 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -8,12 +8,10 @@
 
 #include "libc.h"
 #include "s390-ccw.h"
+#include "s390-arch.h"
 
 #define KERN_IMAGE_START 0x010000UL
-#define PSW_MASK_64 0x0000000100000000ULL
-#define PSW_MASK_32 0x0000000080000000ULL
-#define PSW_MASK_SHORTPSW 0x0008000000000000ULL
-#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
+#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
 
 typedef struct ResetInfo {
     uint64_t ipl_psw;
@@ -54,7 +52,7 @@ void jump_to_IPL_code(uint64_t address)
 
     current->ipl_psw = (uint64_t) &jump_to_IPL_2;
     current->ipl_psw |= RESET_PSW_MASK;
-    current->ipl_continue = address & 0x7fffffff;
+    current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
 
     debug_print_int("set IPL addr to", current->ipl_continue);
 
@@ -86,7 +84,7 @@ void jump_to_low_kernel(void)
 
     /* Trying to get PSW at zero address */
     if (*((uint64_t *)0) & RESET_PSW_MASK) {
-        jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
+        jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR);
     }
 
     /* No other option left, so use the Linux kernel start address */
diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 73852029d4..6da44d4436 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -26,9 +26,11 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy size incorrect");
 
 /* s390 psw bit masks */
 #define PSW_MASK_IOINT      0x0200000000000000ULL
+#define PSW_MASK_SHORTPSW   0x0008000000000000ULL
 #define PSW_MASK_WAIT       0x0002000000000000ULL
 #define PSW_MASK_EAMODE     0x0000000100000000ULL
 #define PSW_MASK_BAMODE     0x0000000080000000ULL
+#define PSW_MASK_SHORT_ADDR 0x000000007fffffffULL
 #define PSW_MASK_64         (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
 
 /* Low core mapping */
-- 
2.25.1

[PATCH v5 08/12] pc-bios: s390x: Move panic() into header and add infinite loop

[Janosch Frank]

panic() was defined for the ccw and net bios, i.e. twice, so it's
cleaner to rather put it into the header.

Also let's add an infinite loop into the assembly of disabled_wait() so
the caller doesn't need to take care of it.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/main.c     | 7 -------
 pc-bios/s390-ccw/netmain.c  | 8 --------
 pc-bios/s390-ccw/s390-ccw.h | 9 +++++++--
 pc-bios/s390-ccw/start.S    | 5 +++--
 4 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 8b912454c9..146a50760b 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -46,13 +46,6 @@ void write_iplb_location(void)
     lowcore->ptr_iplb = ptr2u32(&iplb);
 }
 
-void panic(const char *string)
-{
-    sclp_print(string);
-    disabled_wait();
-    while (1) { }
-}
-
 unsigned int get_loadparm_index(void)
 {
     return atoui(loadparm_str);
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index f1ee63577a..056e93a818 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -439,14 +439,6 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
     return rc;
 }
 
-void panic(const char *string)
-{
-    sclp_print(string);
-    for (;;) {
-        disabled_wait();
-    }
-}
-
 void write_subsystem_identification(void)
 {
     SubChannelId *schid = (SubChannelId *) 184;
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index c5820e43ae..36b884cced 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -50,12 +50,11 @@ typedef unsigned long long __u64;
 #include "iplb.h"
 
 /* start.s */
-void disabled_wait(void);
+void disabled_wait(void) __attribute__ ((__noreturn__));
 void consume_sclp_int(void);
 void consume_io_int(void);
 
 /* main.c */
-void panic(const char *string);
 void write_subsystem_identification(void);
 void write_iplb_location(void);
 extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
@@ -91,6 +90,12 @@ bool menu_is_enabled_enum(void);
 
 #define MAX_BOOT_ENTRIES  31
 
+static inline void panic(const char *string)
+{
+    sclp_print(string);
+    disabled_wait();
+}
+
 static inline void fill_hex(char *out, unsigned char val)
 {
     const char hex[] = "0123456789abcdef";
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index aa8fceb19d..ce519300a1 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -47,8 +47,9 @@ memsetxc:
  */
 	.globl disabled_wait
 disabled_wait:
-        larl %r1,disabled_wait_psw
-        lpswe   0(%r1)
+	larl	%r1,disabled_wait_psw
+	lpswe	0(%r1)
+1:	j	1b
 
 
 /*
-- 
2.25.1

[PATCH v5 09/12] pc-bios: s390x: Use ebcdic2ascii table

[Janosch Frank]

Why should we do conversion of a ebcdic value if we have a handy table
where we could look up the ascii value instead?

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/bootmap.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index d13b7cbd15..97205674e5 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -328,9 +328,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
         msg[0] = '2';
         break;
     default:
-        msg[0] = vlbl->LDL_version;
-        msg[0] &= 0x0f; /* convert EBCDIC   */
-        msg[0] |= 0x30; /* to ASCII (digit) */
+        msg[0] = ebc2asc[vlbl->LDL_version];
         msg[1] = '?';
         break;
     }
-- 
2.25.1

[PATCH v5 10/12] pc-bios: s390x: Make u32 ptr check explicit

[Janosch Frank]

Let's make it a bit more clear that we check the full 64 bits to fit
into the 32 we return.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/helper.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index 32a453b634..dfcfea0ff0 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -19,7 +19,7 @@
 /* Avoids compiler warnings when casting a pointer to a u32 */
 static inline uint32_t ptr2u32(void *ptr)
 {
-    IPL_assert((uint64_t)ptr <= 0xffffffff, "ptr2u32: ptr too large");
+    IPL_assert((uint64_t)ptr <= 0xffffffffull, "ptr2u32: ptr too large");
     return (uint32_t)(uint64_t)ptr;
 }
 
-- 
2.25.1

[PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses

[Janosch Frank]

The component entries written by zipl contain short PSWs, not
addresses. Let's mask them and only pass the address part to
jump_to_IPL_code(uint64_t address) because it expects an address as
visible by the name of the argument.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 pc-bios/s390-ccw/bootmap.c | 5 +++--
 pc-bios/s390-ccw/bootmap.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 97205674e5..8547a140df 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -10,6 +10,7 @@
 
 #include "libc.h"
 #include "s390-ccw.h"
+#include "s390-arch.h"
 #include "bootmap.h"
 #include "virtio.h"
 #include "bswap.h"
@@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
     char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
 
     blockno = entry->data.blockno;
-    address = entry->load_address;
+    address = entry->psw & PSW_MASK_SHORT_ADDR;
 
     debug_print_int("loading segment at block", blockno);
     debug_print_int("addr", address);
@@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
     IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
 
     /* should not return */
-    jump_to_IPL_code(entry->load_address);
+    jump_to_IPL_code(entry->psw & PSW_MASK_SHORT_ADDR);
 }
 
 static void ipl_scsi(void)
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 12a0166aae..e07f87e690 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -68,7 +68,7 @@ typedef struct ComponentEntry {
     ScsiBlockPtr data;
     uint8_t pad[7];
     uint8_t component_type;
-    uint64_t load_address;
+    uint64_t psw;
 } __attribute((packed)) ComponentEntry;
 
 typedef struct ComponentHeader {
-- 
2.25.1

[RFC v5 12/12] pc-bios: s390x: Cleanup jump to ipl code

[Janosch Frank]

jump_to_IPL_code takes a 64 bit address, masks it with the short psw
address mask and later branches to it using a full 64 bit register.

* As the masking is not necessary, let's remove it
* Without the mask we can save the ipl address to a static 64 bit
  function ptr as we later branch to it
* Let's also clean up the variable names and remove the now unneeded
  ResetInfo

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 767012bf0c..aef37cea76 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -13,20 +13,15 @@
 #define KERN_IMAGE_START 0x010000UL
 #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
 
-typedef struct ResetInfo {
-    uint64_t ipl_psw;
-    uint32_t ipl_continue;
-} ResetInfo;
-
-static ResetInfo save;
+static void (*ipl_continue)(void);
+static uint64_t psw_save;
 
 static void jump_to_IPL_2(void)
 {
-    ResetInfo *current = 0;
+    uint64_t *psw_current = 0;
 
-    void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
-    *current = save;
-    ipl(); /* should not return */
+    *psw_current = psw_save;
+    ipl_continue(); /* should not return */
 }
 
 void jump_to_IPL_code(uint64_t address)
@@ -46,15 +41,15 @@ void jump_to_IPL_code(uint64_t address)
      * content of non-BIOS memory after we loaded the guest, so we
      * save the original content and restore it in jump_to_IPL_2.
      */
-    ResetInfo *current = 0;
+    uint64_t *psw_current = 0;
 
-    save = *current;
+    psw_save = *psw_current;
 
-    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
-    current->ipl_psw |= RESET_PSW_MASK;
-    current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
+    *psw_current = (uint64_t) &jump_to_IPL_2;
+    *psw_current |= RESET_PSW_MASK;
+    ipl_continue = (void *)address;
 
-    debug_print_int("set IPL addr to", current->ipl_continue);
+    debug_print_int("set IPL addr to", (uint64_t)ipl_continue);
 
     /* Ensure the guest output starts fresh */
     sclp_print("\n");
-- 
2.25.1

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200624075226.92728-1-frankja@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1
Type: series
Message-id: 20200624075226.92728-1-frankja@linux.ibm.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200624075226.92728-1-frankja@linux.ibm.com -> patchew/20200624075226.92728-1-frankja@linux.ibm.com
Switched to a new branch 'test'
f7789d0 pc-bios: s390x: Cleanup jump to ipl code
8680ca2 pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
830483b pc-bios: s390x: Make u32 ptr check explicit
f313bb2 pc-bios: s390x: Use ebcdic2ascii table
00728a2 pc-bios: s390x: Move panic() into header and add infinite loop
d025a9c pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR
e33e9ff pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
c98ec6e pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
f22b573 pc-bios: s390x: Get rid of magic offsets into the lowcore
aafb016 pc-bios: s390x: Move sleep and yield to helper.h
855c1c9 pc-bios: s390x: Consolidate timing functions into time.h
b906f41 pc-bios: s390x: cio.c cleanup and compile fix

=== OUTPUT BEGIN ===
1/12 Checking commit b906f41df28f (pc-bios: s390x: cio.c cleanup and compile fix)
2/12 Checking commit 855c1c9be4fe (pc-bios: s390x: Consolidate timing functions into time.h)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#94: 
new file mode 100644

total: 0 errors, 1 warnings, 142 lines checked

Patch 2/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/12 Checking commit aafb016fdfe0 (pc-bios: s390x: Move sleep and yield to helper.h)
4/12 Checking commit f22b573bc73e (pc-bios: s390x: Get rid of magic offsets into the lowcore)
ERROR: spaces required around that ':' (ctx:VxV)
#30: FILE: pc-bios/s390-ccw/cio.h:127:
+            __u16 cssid:8;
                        ^

ERROR: spaces required around that ':' (ctx:VxV)
#31: FILE: pc-bios/s390-ccw/cio.h:128:
+            __u16 reserved:4;
                           ^

ERROR: spaces required around that ':' (ctx:VxV)
#32: FILE: pc-bios/s390-ccw/cio.h:129:
+            __u16 m:1;
                    ^

ERROR: spaces required around that ':' (ctx:VxV)
#33: FILE: pc-bios/s390-ccw/cio.h:130:
+            __u16 ssid:2;
                       ^

ERROR: spaces required around that ':' (ctx:VxV)
#34: FILE: pc-bios/s390-ccw/cio.h:131:
+            __u16 one:1;
                      ^

total: 5 errors, 0 warnings, 37 lines checked

Patch 4/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/12 Checking commit c98ec6eb101b (pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes)
6/12 Checking commit e33e9fff145a (pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64)
7/12 Checking commit d025a9ce4c67 (pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR)
8/12 Checking commit 00728a2fdf9e (pc-bios: s390x: Move panic() into header and add infinite loop)
9/12 Checking commit f313bb25948c (pc-bios: s390x: Use ebcdic2ascii table)
10/12 Checking commit 830483bf940d (pc-bios: s390x: Make u32 ptr check explicit)
11/12 Checking commit 8680ca210f71 (pc-bios: s390x: Fix bootmap.c passing PSWs as addresses)
12/12 Checking commit f7789d0e1906 (pc-bios: s390x: Cleanup jump to ipl code)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200624075226.92728-1-frankja@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Cornelia Huck]

On Wed, 24 Jun 2020 03:52:14 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> The bios is in dire need for a cleanup as there are still a lot of
> magic constants being used throughout as well as duplicated code.
> 
> In the first part of this series we consolidate constants and
> functions, as well as doing some minor cleanups and fixes.
> 
> The patches are available here:
> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
> 
> v5:
> 	* Fixed whitespace damage
> 	* Removed reset PSW mask changes in dasd-ipl.c
> 	* Added jump2ipl.c cleanup patches
> 
> v4:
> 	* Renamed time.h to s390-time.h
> 	* Fixed function names in sleep()
> 	* Changed order of sense_id_ccw initialization
> 	* Added missing include before sleep()
> 
> v3:
> 	* Dropped 0x00 to 0x0/0 patch
> 	* Moved some timing functions into helper.h instead of time.h
> 	* Fixed IPL psw manipulation in dasd-ipl.c
>  	* Minor cosmetic fixes found by review
> 
> v2:
> 	* Included cio fixup to get rid of compile errors...
> 	* Minor cosmetic fixes found by review
> 
> 
> Janosch Frank (12):
>   pc-bios: s390x: cio.c cleanup and compile fix
>   pc-bios: s390x: Consolidate timing functions into time.h
>   pc-bios: s390x: Move sleep and yield to helper.h
>   pc-bios: s390x: Get rid of magic offsets into the lowcore
>   pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>   pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>   pc-bios: s390x: Use PSW masks where possible and introduce
>     PSW_MASK_SHORT_ADDR
>   pc-bios: s390x: Move panic() into header and add infinite loop
>   pc-bios: s390x: Use ebcdic2ascii table
>   pc-bios: s390x: Make u32 ptr check explicit
>   pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>   pc-bios: s390x: Cleanup jump to ipl code
> 
>  pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>  pc-bios/s390-ccw/bootmap.h     |  2 +-
>  pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>  pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>  pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>  pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>  pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>  pc-bios/s390-ccw/main.c        | 15 +++----------
>  pc-bios/s390-ccw/menu.c        |  1 +
>  pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>  pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>  pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>  pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>  pc-bios/s390-ccw/start.S       |  5 +++--
>  pc-bios/s390-ccw/virtio-net.c  |  2 ++
>  pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>  pc-bios/s390-ccw/virtio.c      | 18 +++------------
>  17 files changed, 120 insertions(+), 125 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/s390-time.h
> 

Hm... what's the general status of this? Most of the patches have at
least one R-b/A-b already, I see.

Do the s390-ccw boot maintainers want to pick this (once the rest has
been looked at) and then send me a pull req, or should I pick it when
it is good to go? Softfreeze is less than two weeks away :)

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Thomas Huth]

On 24/06/2020 12.44, Cornelia Huck wrote:
> On Wed, 24 Jun 2020 03:52:14 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> The bios is in dire need for a cleanup as there are still a lot of
>> magic constants being used throughout as well as duplicated code.
>>
>> In the first part of this series we consolidate constants and
>> functions, as well as doing some minor cleanups and fixes.
>>
>> The patches are available here:
>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
>>
>> v5:
>> 	* Fixed whitespace damage
>> 	* Removed reset PSW mask changes in dasd-ipl.c
>> 	* Added jump2ipl.c cleanup patches
>>
>> v4:
>> 	* Renamed time.h to s390-time.h
>> 	* Fixed function names in sleep()
>> 	* Changed order of sense_id_ccw initialization
>> 	* Added missing include before sleep()
>>
>> v3:
>> 	* Dropped 0x00 to 0x0/0 patch
>> 	* Moved some timing functions into helper.h instead of time.h
>> 	* Fixed IPL psw manipulation in dasd-ipl.c
>>   	* Minor cosmetic fixes found by review
>>
>> v2:
>> 	* Included cio fixup to get rid of compile errors...
>> 	* Minor cosmetic fixes found by review
>>
>>
>> Janosch Frank (12):
>>    pc-bios: s390x: cio.c cleanup and compile fix
>>    pc-bios: s390x: Consolidate timing functions into time.h
>>    pc-bios: s390x: Move sleep and yield to helper.h
>>    pc-bios: s390x: Get rid of magic offsets into the lowcore
>>    pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>>    pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>>    pc-bios: s390x: Use PSW masks where possible and introduce
>>      PSW_MASK_SHORT_ADDR
>>    pc-bios: s390x: Move panic() into header and add infinite loop
>>    pc-bios: s390x: Use ebcdic2ascii table
>>    pc-bios: s390x: Make u32 ptr check explicit
>>    pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>>    pc-bios: s390x: Cleanup jump to ipl code
>>
>>   pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>>   pc-bios/s390-ccw/bootmap.h     |  2 +-
>>   pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>>   pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>>   pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>>   pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>>   pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>>   pc-bios/s390-ccw/main.c        | 15 +++----------
>>   pc-bios/s390-ccw/menu.c        |  1 +
>>   pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>>   pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>>   pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>>   pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>>   pc-bios/s390-ccw/start.S       |  5 +++--
>>   pc-bios/s390-ccw/virtio-net.c  |  2 ++
>>   pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>>   pc-bios/s390-ccw/virtio.c      | 18 +++------------
>>   17 files changed, 120 insertions(+), 125 deletions(-)
>>   create mode 100644 pc-bios/s390-ccw/s390-time.h
>>
> 
> Hm... what's the general status of this? Most of the patches have at
> least one R-b/A-b already, I see.
> 
> Do the s390-ccw boot maintainers want to pick this (once the rest has
> been looked at) and then send me a pull req, or should I pick it when
> it is good to go? Softfreeze is less than two weeks away :)

I'd like to review the missing parts and run my tests with the patches 
applied ... I'm just a little bit swamped right now, so please give me 
some more time...

  Thomas

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3983 bytes --]

On 6/24/20 12:46 PM, Thomas Huth wrote:
> On 24/06/2020 12.44, Cornelia Huck wrote:
>> On Wed, 24 Jun 2020 03:52:14 -0400
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> The bios is in dire need for a cleanup as there are still a lot of
>>> magic constants being used throughout as well as duplicated code.
>>>
>>> In the first part of this series we consolidate constants and
>>> functions, as well as doing some minor cleanups and fixes.
>>>
>>> The patches are available here:
>>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
>>>
>>> v5:
>>> 	* Fixed whitespace damage
>>> 	* Removed reset PSW mask changes in dasd-ipl.c
>>> 	* Added jump2ipl.c cleanup patches
>>>
>>> v4:
>>> 	* Renamed time.h to s390-time.h
>>> 	* Fixed function names in sleep()
>>> 	* Changed order of sense_id_ccw initialization
>>> 	* Added missing include before sleep()
>>>
>>> v3:
>>> 	* Dropped 0x00 to 0x0/0 patch
>>> 	* Moved some timing functions into helper.h instead of time.h
>>> 	* Fixed IPL psw manipulation in dasd-ipl.c
>>>   	* Minor cosmetic fixes found by review
>>>
>>> v2:
>>> 	* Included cio fixup to get rid of compile errors...
>>> 	* Minor cosmetic fixes found by review
>>>
>>>
>>> Janosch Frank (12):
>>>    pc-bios: s390x: cio.c cleanup and compile fix
>>>    pc-bios: s390x: Consolidate timing functions into time.h
>>>    pc-bios: s390x: Move sleep and yield to helper.h
>>>    pc-bios: s390x: Get rid of magic offsets into the lowcore
>>>    pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>>>    pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>>>    pc-bios: s390x: Use PSW masks where possible and introduce
>>>      PSW_MASK_SHORT_ADDR
>>>    pc-bios: s390x: Move panic() into header and add infinite loop
>>>    pc-bios: s390x: Use ebcdic2ascii table
>>>    pc-bios: s390x: Make u32 ptr check explicit
>>>    pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>>>    pc-bios: s390x: Cleanup jump to ipl code
>>>
>>>   pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>>>   pc-bios/s390-ccw/bootmap.h     |  2 +-
>>>   pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>>>   pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>>>   pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>>>   pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>>>   pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>>>   pc-bios/s390-ccw/main.c        | 15 +++----------
>>>   pc-bios/s390-ccw/menu.c        |  1 +
>>>   pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>>>   pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>>>   pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>>>   pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>>>   pc-bios/s390-ccw/start.S       |  5 +++--
>>>   pc-bios/s390-ccw/virtio-net.c  |  2 ++
>>>   pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>>>   pc-bios/s390-ccw/virtio.c      | 18 +++------------
>>>   17 files changed, 120 insertions(+), 125 deletions(-)
>>>   create mode 100644 pc-bios/s390-ccw/s390-time.h
>>>
>>
>> Hm... what's the general status of this? Most of the patches have at
>> least one R-b/A-b already, I see.
>>
>> Do the s390-ccw boot maintainers want to pick this (once the rest has
>> been looked at) and then send me a pull req, or should I pick it when
>> it is good to go? Softfreeze is less than two weeks away :)
> 
> I'd like to review the missing parts and run my tests with the patches 
> applied ... I'm just a little bit swamped right now, so please give me 
> some more time...
> 
>   Thomas

I'd suggest we hold off on the RFC patch since it makes testing all of
the boot methods necessary and I was only able to test direct, dasd
passthrough and virtio-ccw up to now. Time to build a testing environment...

I can move that patch into the next series for further discussion. It
was mostly added to prove the 64 bit PSW mask removal in patch #5.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Cornelia Huck]

On Wed, 24 Jun 2020 12:46:49 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 24/06/2020 12.44, Cornelia Huck wrote:
> > On Wed, 24 Jun 2020 03:52:14 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> The bios is in dire need for a cleanup as there are still a lot of
> >> magic constants being used throughout as well as duplicated code.
> >>
> >> In the first part of this series we consolidate constants and
> >> functions, as well as doing some minor cleanups and fixes.
> >>
> >> The patches are available here:
> >> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
> >>
> >> v5:
> >> 	* Fixed whitespace damage
> >> 	* Removed reset PSW mask changes in dasd-ipl.c
> >> 	* Added jump2ipl.c cleanup patches
> >>
> >> v4:
> >> 	* Renamed time.h to s390-time.h
> >> 	* Fixed function names in sleep()
> >> 	* Changed order of sense_id_ccw initialization
> >> 	* Added missing include before sleep()
> >>
> >> v3:
> >> 	* Dropped 0x00 to 0x0/0 patch
> >> 	* Moved some timing functions into helper.h instead of time.h
> >> 	* Fixed IPL psw manipulation in dasd-ipl.c
> >>   	* Minor cosmetic fixes found by review
> >>
> >> v2:
> >> 	* Included cio fixup to get rid of compile errors...
> >> 	* Minor cosmetic fixes found by review
> >>
> >>
> >> Janosch Frank (12):
> >>    pc-bios: s390x: cio.c cleanup and compile fix
> >>    pc-bios: s390x: Consolidate timing functions into time.h
> >>    pc-bios: s390x: Move sleep and yield to helper.h
> >>    pc-bios: s390x: Get rid of magic offsets into the lowcore
> >>    pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
> >>    pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
> >>    pc-bios: s390x: Use PSW masks where possible and introduce
> >>      PSW_MASK_SHORT_ADDR
> >>    pc-bios: s390x: Move panic() into header and add infinite loop
> >>    pc-bios: s390x: Use ebcdic2ascii table
> >>    pc-bios: s390x: Make u32 ptr check explicit
> >>    pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
> >>    pc-bios: s390x: Cleanup jump to ipl code
> >>
> >>   pc-bios/s390-ccw/bootmap.c     |  9 ++++----
> >>   pc-bios/s390-ccw/bootmap.h     |  2 +-
> >>   pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
> >>   pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
> >>   pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
> >>   pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
> >>   pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
> >>   pc-bios/s390-ccw/main.c        | 15 +++----------
> >>   pc-bios/s390-ccw/menu.c        |  1 +
> >>   pc-bios/s390-ccw/netmain.c     | 23 +++----------------
> >>   pc-bios/s390-ccw/s390-arch.h   |  4 +++-
> >>   pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
> >>   pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
> >>   pc-bios/s390-ccw/start.S       |  5 +++--
> >>   pc-bios/s390-ccw/virtio-net.c  |  2 ++
> >>   pc-bios/s390-ccw/virtio-scsi.c |  2 ++
> >>   pc-bios/s390-ccw/virtio.c      | 18 +++------------
> >>   17 files changed, 120 insertions(+), 125 deletions(-)
> >>   create mode 100644 pc-bios/s390-ccw/s390-time.h
> >>  
> > 
> > Hm... what's the general status of this? Most of the patches have at
> > least one R-b/A-b already, I see.
> > 
> > Do the s390-ccw boot maintainers want to pick this (once the rest has
> > been looked at) and then send me a pull req, or should I pick it when
> > it is good to go? Softfreeze is less than two weeks away :)  
> 
> I'd like to review the missing parts and run my tests with the patches 
> applied ... I'm just a little bit swamped right now, so please give me 
> some more time...

Sure, please don't feel hurried :)

Re: [PATCH v5 02/12] pc-bios: s390x: Consolidate timing functions into time.h

[David Hildenbrand]

On 24.06.20 09:52, Janosch Frank wrote:
> Let's consolidate timing related functions into one header.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Acked-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/menu.c        |  1 +
>  pc-bios/s390-ccw/netmain.c     | 15 +++------------
>  pc-bios/s390-ccw/s390-ccw.h    |  8 ++++----
>  pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++++++
>  pc-bios/s390-ccw/virtio-net.c  |  1 +
>  pc-bios/s390-ccw/virtio-scsi.c |  1 +
>  pc-bios/s390-ccw/virtio.c      | 18 +++---------------
>  7 files changed, 36 insertions(+), 31 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/s390-time.h

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v5 04/12] pc-bios: s390x: Get rid of magic offsets into the lowcore

[Thomas Huth]

On 24/06/2020 09.52, Janosch Frank wrote:
> If we have a lowcore struct that has members for offsets that we want
> to touch, why not use it?
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>   pc-bios/s390-ccw/cio.h  | 17 +++++++++++------
>   pc-bios/s390-ccw/main.c |  8 +++-----
>   2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index aaa432dedd..1e5d4e92e1 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
> @@ -122,12 +122,17 @@ typedef struct schib {
>   } __attribute__ ((packed, aligned(4))) Schib;
>   
>   typedef struct subchannel_id {
> -        __u32 cssid:8;
> -        __u32:4;
> -        __u32 m:1;
> -        __u32 ssid:2;
> -        __u32 one:1;
> -        __u32 sch_no:16;
> +    union {
> +        struct {
> +            __u16 cssid:8;
> +            __u16 reserved:4;
> +            __u16 m:1;
> +            __u16 ssid:2;
> +            __u16 one:1;
> +        };
> +        __u16 sch_id;
> +    };
> +    __u16 sch_no;
>   } __attribute__ ((packed, aligned(4))) SubChannelId;
>   
>   struct chsc_header {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 4e65b411e1..8b912454c9 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>    */
>   void write_subsystem_identification(void)
>   {
> -    SubChannelId *schid = (SubChannelId *) 184;
> -    uint32_t *zeroes = (uint32_t *) 188;
> -
> -    *schid = blk_schid;
> -    *zeroes = 0;
> +    lowcore->subchannel_id = blk_schid.sch_id;
> +    lowcore->subchannel_nr = blk_schid.sch_no;
> +    lowcore->io_int_parm = 0;
>   }

Ok, I have to admit that I had to read it multiple times (maybe not 
enough coffee the last time), but it looks fine to me now.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v5 05/12] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes

[Thomas Huth]

On 24/06/2020 09.52, Janosch Frank wrote:
> jump_to_low_kernel() and the functions that it calls will already or
> 64 bit addressing into the reset psw mask when executing
> jump_to_IPL_2() after the diag308 subcode 1.

Hmm, the jump_to_IPL_code() also sets the 64-bit addressing bits ... but 
jump_to_low_kernel() has some logic that tests for the bits before that 
function is called:

     /* Trying to get PSW at zero address */
     if (*((uint64_t *)0) & RESET_PSW_MASK) {
         jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
     }

Could it be that the code in dasd-ipl.c has been written with that 
if-statement in mind?

  Thomas


> The kernel proper is then branched to rather than doing a full PSW
> change.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/dasd-ipl.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index 0fc879bb8e..e8f2846740 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -206,7 +206,6 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
>    */
>   void dasd_ipl(SubChannelId schid, uint16_t cutype)
>   {
> -    PSWLegacy *pswl = (PSWLegacy *) 0x00;
>       uint32_t ipl2_addr;
>   
>       /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
> @@ -229,7 +228,5 @@ void dasd_ipl(SubChannelId schid, uint16_t cutype)
>       run_ipl2(schid, cutype, ipl2_addr);
>   
>       /* Transfer control to the guest operating system */
> -    pswl->mask |= PSW_MASK_EAMODE;   /* Force z-mode */
> -    pswl->addr |= PSW_MASK_BAMODE;   /* ...          */
>       jump_to_low_kernel();
>   }
> 

Re: [PATCH v5 06/12] pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64

[Thomas Huth]

On 24/06/2020 09.52, Janosch Frank wrote:
> This constant enables 64 bit addressing, not the ESAME architecture,
> so it shouldn't be named ZMODE.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/s390-arch.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
> index 5f36361c02..73852029d4 100644
> --- a/pc-bios/s390-ccw/s390-arch.h
> +++ b/pc-bios/s390-ccw/s390-arch.h
> @@ -29,7 +29,7 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy size incorrect");
>   #define PSW_MASK_WAIT       0x0002000000000000ULL
>   #define PSW_MASK_EAMODE     0x0000000100000000ULL
>   #define PSW_MASK_BAMODE     0x0000000080000000ULL
> -#define PSW_MASK_ZMODE      (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
> +#define PSW_MASK_64         (PSW_MASK_EAMODE | PSW_MASK_BAMODE)

I think you could maybe squash it with the next patch.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v5 05/12] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes

[Thomas Huth]

On 25/06/2020 12.57, Thomas Huth wrote:
> On 24/06/2020 09.52, Janosch Frank wrote:
>> jump_to_low_kernel() and the functions that it calls will already or
>> 64 bit addressing into the reset psw mask when executing
>> jump_to_IPL_2() after the diag308 subcode 1.
> 
> Hmm, the jump_to_IPL_code() also sets the 64-bit addressing bits ... but 
> jump_to_low_kernel() has some logic that tests for the bits before that 
> function is called:
> 
>      /* Trying to get PSW at zero address */
>      if (*((uint64_t *)0) & RESET_PSW_MASK) {
>          jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
>      }
> 
> Could it be that the code in dasd-ipl.c has been written with that 
> if-statement in mind?

... in that case, the code in dasd-ipl.c should maybe rather call 
jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff) directly instead of 
going through jump_to_low_kernel(), I guess.

Jason, do you remember the intention here?

  Thomas

Re: [PATCH v5 07/12] pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR

[Thomas Huth]

On 24/06/2020 09.52, Janosch Frank wrote:
> Let's move some of the PSW mask defines into s390-arch.h and use them
> in jump2ipl.c. Also let's introduce a new constant for the address
> mask of 8 byte (short) PSWs.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>   pc-bios/s390-ccw/jump2ipl.c  | 10 ++++------
>   pc-bios/s390-ccw/s390-arch.h |  2 ++
>   2 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses

[Thomas Huth]

On 24/06/2020 09.52, Janosch Frank wrote:
> The component entries written by zipl contain short PSWs, not
> addresses. Let's mask them and only pass the address part to
> jump_to_IPL_code(uint64_t address) because it expects an address as
> visible by the name of the argument.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/bootmap.c | 5 +++--
>   pc-bios/s390-ccw/bootmap.h | 2 +-
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 97205674e5..8547a140df 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -10,6 +10,7 @@
>   
>   #include "libc.h"
>   #include "s390-ccw.h"
> +#include "s390-arch.h"
>   #include "bootmap.h"
>   #include "virtio.h"
>   #include "bswap.h"
> @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
>       char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
>   
>       blockno = entry->data.blockno;
> -    address = entry->load_address;
> +    address = entry->psw & PSW_MASK_SHORT_ADDR;

Are you really sure about this one here? The address does not seem to be 
used for any of the jump_to_IPL() functions. And in the zipl sources, I 
can also see spots like this:

    entry->compdat.load_address = data.load_address;

... without any further short mask bits. So I somehow doubt that this 
change is really ok?

>       debug_print_int("loading segment at block", blockno);
>       debug_print_int("addr", address);
> @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>       IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>   
>       /* should not return */
> -    jump_to_IPL_code(entry->load_address);
> +    jump_to_IPL_code(entry->psw & PSW_MASK_SHORT_ADDR);

That one should be fine, I think.

>   }
>   
>   static void ipl_scsi(void)
> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> index 12a0166aae..e07f87e690 100644
> --- a/pc-bios/s390-ccw/bootmap.h
> +++ b/pc-bios/s390-ccw/bootmap.h
> @@ -68,7 +68,7 @@ typedef struct ComponentEntry {
>       ScsiBlockPtr data;
>       uint8_t pad[7];
>       uint8_t component_type;
> -    uint64_t load_address;
> +    uint64_t psw;

I'd recommend to keep the load_address name. It's the same name as used 
in the zipl sources, and as far as I can see, the field does not always 
contain a PSW.

>   } __attribute((packed)) ComponentEntry;
>   
>   typedef struct ComponentHeader {
> 

  Thomas

Re: [RFC v5 12/12] pc-bios: s390x: Cleanup jump to ipl code

[Thomas Huth]

On 24/06/2020 09.52, Janosch Frank wrote:
> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
> address mask and later branches to it using a full 64 bit register.
> 
> * As the masking is not necessary, let's remove it
> * Without the mask we can save the ipl address to a static 64 bit
>    function ptr as we later branch to it
> * Let's also clean up the variable names and remove the now unneeded
>    ResetInfo
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
>   1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 767012bf0c..aef37cea76 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -13,20 +13,15 @@
>   #define KERN_IMAGE_START 0x010000UL
>   #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>   
> -typedef struct ResetInfo {
> -    uint64_t ipl_psw;
> -    uint32_t ipl_continue;
> -} ResetInfo;
> -
> -static ResetInfo save;
> +static void (*ipl_continue)(void);
> +static uint64_t psw_save;

I wonder whether there was a reason for having ipl_continue in the 
lowcore instead of using a simple static function pointer... Christian, 
do you remember?

>   static void jump_to_IPL_2(void)
>   {
> -    ResetInfo *current = 0;
> +    uint64_t *psw_current = 0;

Mhh, what about using uint64_t *psw_current = (uint64_t *)lowcore 
instead, to make it more more explicit?

> -    void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
> -    *current = save;
> -    ipl(); /* should not return */
> +    *psw_current = psw_save;
> +    ipl_continue(); /* should not return */
>   }
>   
>   void jump_to_IPL_code(uint64_t address)
> @@ -46,15 +41,15 @@ void jump_to_IPL_code(uint64_t address)
>        * content of non-BIOS memory after we loaded the guest, so we
>        * save the original content and restore it in jump_to_IPL_2.
>        */
> -    ResetInfo *current = 0;
> +    uint64_t *psw_current = 0;

dito.

> -    save = *current;
> +    psw_save = *psw_current;
>   
> -    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
> -    current->ipl_psw |= RESET_PSW_MASK;
> -    current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
> +    *psw_current = (uint64_t) &jump_to_IPL_2;
> +    *psw_current |= RESET_PSW_MASK;
> +    ipl_continue = (void *)address;
>   
> -    debug_print_int("set IPL addr to", current->ipl_continue);
> +    debug_print_int("set IPL addr to", (uint64_t)ipl_continue);
>   
>       /* Ensure the guest output starts fresh */
>       sclp_print("\n");
> 

The patch sounds like a good idea to me ... but since this code proofed 
to be very fragile in the past, let's wait for Christian to say whether 
there was a good reason for ipl_continue in the lowcore or not.

  Thomas

Re: [PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3428 bytes --]

On 6/25/20 2:46 PM, Thomas Huth wrote:
> On 24/06/2020 09.52, Janosch Frank wrote:
>> The component entries written by zipl contain short PSWs, not
>> addresses. Let's mask them and only pass the address part to
>> jump_to_IPL_code(uint64_t address) because it expects an address as
>> visible by the name of the argument.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 5 +++--
>>   pc-bios/s390-ccw/bootmap.h | 2 +-
>>   2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 97205674e5..8547a140df 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -10,6 +10,7 @@
>>   
>>   #include "libc.h"
>>   #include "s390-ccw.h"
>> +#include "s390-arch.h"
>>   #include "bootmap.h"
>>   #include "virtio.h"
>>   #include "bswap.h"
>> @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
>>       char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
>>   
>>       blockno = entry->data.blockno;
>> -    address = entry->load_address;
>> +    address = entry->psw & PSW_MASK_SHORT_ADDR;
> 
> Are you really sure about this one here? The address does not seem to be 
> used for any of the jump_to_IPL() functions. And in the zipl sources, I 
> can also see spots like this:

This one slipped through and is indeed wrong.

> 
>     entry->compdat.load_address = data.load_address;
> 
> ... without any further short mask bits. So I somehow doubt that this 
> change is really ok?
> 
>>       debug_print_int("loading segment at block", blockno);
>>       debug_print_int("addr", address);
>> @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>>       IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>>   
>>       /* should not return */
>> -    jump_to_IPL_code(entry->load_address);
>> +    jump_to_IPL_code(entry->psw & PSW_MASK_SHORT_ADDR);
> 
> That one should be fine, I think.

Yes, as it is a execute type entry, this needs to be a PSW and therefore
needs to be masked.

> 
>>   }
>>   
>>   static void ipl_scsi(void)
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index 12a0166aae..e07f87e690 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -68,7 +68,7 @@ typedef struct ComponentEntry {
>>       ScsiBlockPtr data;
>>       uint8_t pad[7];
>>       uint8_t component_type;
>> -    uint64_t load_address;
>> +    uint64_t psw;
> 
> I'd recommend to keep the load_address name. It's the same name as used 
> in the zipl sources, and as far as I can see, the field does not always 
> contain a PSW.

The problem is that this is a union in zipl containing an address, psw
or signature header.

I guess we should also make it a union and use the proper members so it
is clear what we retrieve from the entry. If it is a PSW we need to mask
it if it is a component address masking might be a bad idea.

But I absolutely do not want to have this named PSW and then being used
like a normal address. It took me way too long to figure out why my
guest wasn't booting anymore.

Time for a new series of patches :)

> 
>>   } __attribute((packed)) ComponentEntry;
>>   
>>   typedef struct ComponentHeader {
>>
> 
>   Thomas
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC v5 12/12] pc-bios: s390x: Cleanup jump to ipl code

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 3325 bytes --]

On 6/25/20 2:58 PM, Thomas Huth wrote:
> On 24/06/2020 09.52, Janosch Frank wrote:
>> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
>> address mask and later branches to it using a full 64 bit register.
>>
>> * As the masking is not necessary, let's remove it
>> * Without the mask we can save the ipl address to a static 64 bit
>>    function ptr as we later branch to it
>> * Let's also clean up the variable names and remove the now unneeded
>>    ResetInfo
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
>>   1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index 767012bf0c..aef37cea76 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -13,20 +13,15 @@
>>   #define KERN_IMAGE_START 0x010000UL
>>   #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>   
>> -typedef struct ResetInfo {
>> -    uint64_t ipl_psw;
>> -    uint32_t ipl_continue;
>> -} ResetInfo;
>> -
>> -static ResetInfo save;
>> +static void (*ipl_continue)(void);
>> +static uint64_t psw_save;
> 
> I wonder whether there was a reason for having ipl_continue in the 
> lowcore instead of using a simple static function pointer... Christian, 
> do you remember?
> 
>>   static void jump_to_IPL_2(void)
>>   {
>> -    ResetInfo *current = 0;
>> +    uint64_t *psw_current = 0;
> 
> Mhh, what about using uint64_t *psw_current = (uint64_t *)lowcore 
> instead, to make it more more explicit?

Sure, that would make it way better to read.

> 
>> -    void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
>> -    *current = save;
>> -    ipl(); /* should not return */
>> +    *psw_current = psw_save;
>> +    ipl_continue(); /* should not return */
>>   }
>>   
>>   void jump_to_IPL_code(uint64_t address)
>> @@ -46,15 +41,15 @@ void jump_to_IPL_code(uint64_t address)
>>        * content of non-BIOS memory after we loaded the guest, so we
>>        * save the original content and restore it in jump_to_IPL_2.
>>        */
>> -    ResetInfo *current = 0;
>> +    uint64_t *psw_current = 0;
> 
> dito.
> 
>> -    save = *current;
>> +    psw_save = *psw_current;
>>   
>> -    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
>> -    current->ipl_psw |= RESET_PSW_MASK;
>> -    current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
>> +    *psw_current = (uint64_t) &jump_to_IPL_2;
>> +    *psw_current |= RESET_PSW_MASK;
>> +    ipl_continue = (void *)address;
>>   
>> -    debug_print_int("set IPL addr to", current->ipl_continue);
>> +    debug_print_int("set IPL addr to", (uint64_t)ipl_continue);
>>   
>>       /* Ensure the guest output starts fresh */
>>       sclp_print("\n");
>>
> 
> The patch sounds like a good idea to me ... but since this code proofed 
> to be very fragile in the past, let's wait for Christian to say whether 
> there was a good reason for ipl_continue in the lowcore or not.

This is a RFC and will need a lot of testing.
I guess I'll move patch 11 and 12 of this series into a new one and also
fix some more boot related stuff so this becomes less maze like.

> 
>   Thomas
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 01/12] pc-bios: s390x: cio.c cleanup and compile fix

[Cornelia Huck]

On Wed, 24 Jun 2020 03:52:15 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> Let's initialize the structs at the beginning to ease reading and also
> zeroing all other fields. This also makes the compiler stop
> complaining about sense_id_ccw.flags being ored into when it's not
> initialized.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 04/12] pc-bios: s390x: Get rid of magic offsets into the lowcore

[Cornelia Huck]

On Wed, 24 Jun 2020 03:52:18 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> If we have a lowcore struct that has members for offsets that we want
> to touch, why not use it?
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  pc-bios/s390-ccw/cio.h  | 17 +++++++++++------
>  pc-bios/s390-ccw/main.c |  8 +++-----
>  2 files changed, 14 insertions(+), 11 deletions(-)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1

[Thomas Huth]

On 24/06/2020 12.57, Janosch Frank wrote:
> On 6/24/20 12:46 PM, Thomas Huth wrote:
>> On 24/06/2020 12.44, Cornelia Huck wrote:
>>> On Wed, 24 Jun 2020 03:52:14 -0400
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> The bios is in dire need for a cleanup as there are still a lot of
>>>> magic constants being used throughout as well as duplicated code.
>>>>
>>>> In the first part of this series we consolidate constants and
>>>> functions, as well as doing some minor cleanups and fixes.
>>>>
>>>> The patches are available here:
>>>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
>>>>
>>>> v5:
>>>> 	* Fixed whitespace damage
>>>> 	* Removed reset PSW mask changes in dasd-ipl.c
>>>> 	* Added jump2ipl.c cleanup patches
>>>>
>>>> v4:
>>>> 	* Renamed time.h to s390-time.h
>>>> 	* Fixed function names in sleep()
>>>> 	* Changed order of sense_id_ccw initialization
>>>> 	* Added missing include before sleep()
>>>>
>>>> v3:
>>>> 	* Dropped 0x00 to 0x0/0 patch
>>>> 	* Moved some timing functions into helper.h instead of time.h
>>>> 	* Fixed IPL psw manipulation in dasd-ipl.c
>>>>    	* Minor cosmetic fixes found by review
>>>>
>>>> v2:
>>>> 	* Included cio fixup to get rid of compile errors...
>>>> 	* Minor cosmetic fixes found by review
>>>>
>>>>
>>>> Janosch Frank (12):
>>>>     pc-bios: s390x: cio.c cleanup and compile fix
>>>>     pc-bios: s390x: Consolidate timing functions into time.h
>>>>     pc-bios: s390x: Move sleep and yield to helper.h
>>>>     pc-bios: s390x: Get rid of magic offsets into the lowcore
>>>>     pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>>>>     pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>>>>     pc-bios: s390x: Use PSW masks where possible and introduce
>>>>       PSW_MASK_SHORT_ADDR
>>>>     pc-bios: s390x: Move panic() into header and add infinite loop
>>>>     pc-bios: s390x: Use ebcdic2ascii table
>>>>     pc-bios: s390x: Make u32 ptr check explicit
>>>>     pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>>>>     pc-bios: s390x: Cleanup jump to ipl code
>>>>
>>>>    pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>>>>    pc-bios/s390-ccw/bootmap.h     |  2 +-
>>>>    pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>>>>    pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>>>>    pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>>>>    pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>>>>    pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>>>>    pc-bios/s390-ccw/main.c        | 15 +++----------
>>>>    pc-bios/s390-ccw/menu.c        |  1 +
>>>>    pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>>>>    pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>>>>    pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>>>>    pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>>>>    pc-bios/s390-ccw/start.S       |  5 +++--
>>>>    pc-bios/s390-ccw/virtio-net.c  |  2 ++
>>>>    pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>>>>    pc-bios/s390-ccw/virtio.c      | 18 +++------------
>>>>    17 files changed, 120 insertions(+), 125 deletions(-)
>>>>    create mode 100644 pc-bios/s390-ccw/s390-time.h
>>>>
>>>
>>> Hm... what's the general status of this? Most of the patches have at
>>> least one R-b/A-b already, I see.
>>>
>>> Do the s390-ccw boot maintainers want to pick this (once the rest has
>>> been looked at) and then send me a pull req, or should I pick it when
>>> it is good to go? Softfreeze is less than two weeks away :)
>>
>> I'd like to review the missing parts and run my tests with the patches
>> applied ... I'm just a little bit swamped right now, so please give me
>> some more time...
>>
>>    Thomas
> 
> I'd suggest we hold off on the RFC patch since it makes testing all of
> the boot methods necessary and I was only able to test direct, dasd
> passthrough and virtio-ccw up to now. Time to build a testing environment...
> 
> I can move that patch into the next series for further discussion. It
> was mostly added to prove the 64 bit PSW mask removal in patch #5.

I think patches 1-4 and 6-10 are ready to go, I also tested them now 
with my usual manual regression tests and there was no issue.

For patch 5, I first like to get some feedback from Jason, to understand 
the original intention of the removed code there.

Patch 11 needs a rework.

And for patch 12, I'd appreciate some feedback from Christian.

I'm planning to send a s390-ccw pull request for the softfreeze on 
Thursday, so if you want to see 5, 11 or 12 included there, it would be 
good to get this done within the next two days (otherwise, I think it's 
also not too critical to delay them to QEMU 5.2 instead).

  Thomas