[PATCH v2 0/9] pc-bios: s390x: Cleanup part 1

[PATCH v2 0/9] 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

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

Janosch Frank (9):
  pc-bios: s390x: cio.c cleanup and compile fix
  pc-bios: s390x: Consolidate timing functions into time.h
  pc-bios: s390x: Get rid of magic offsets into the lowcore
  pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant
  pc-bios: s390x: Use PSW masks where possible
  pc-bios: s390x: Move panic() into header and add infinite loop
  pc-bios: s390x: Use ebcdic2ascii table
  pc-bios: s390x: Replace 0x00 with 0x0 or 0
  pc-bios: s390x: Make u32 ptr check explicit

 pc-bios/s390-ccw/bootmap.c     |  4 +---
 pc-bios/s390-ccw/cio.c         | 36 +++++++++++++++----------------
 pc-bios/s390-ccw/cio.h         | 17 +++++++++------
 pc-bios/s390-ccw/dasd-ipl.c    | 17 +++++++--------
 pc-bios/s390-ccw/helper.h      |  2 +-
 pc-bios/s390-ccw/jump2ipl.c    | 10 ++++-----
 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/start.S       |  5 +++--
 pc-bios/s390-ccw/time.h        | 39 ++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/virtio-net.c  |  1 +
 pc-bios/s390-ccw/virtio-scsi.c |  1 +
 pc-bios/s390-ccw/virtio.c      | 18 +++-------------
 16 files changed, 107 insertions(+), 113 deletions(-)
 create mode 100644 pc-bios/s390-ccw/time.h

-- 
2.25.1

[PATCH v2 1/9] 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
compalining 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>
---
 pc-bios/s390-ccw/cio.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
index 339ec5fbe7..63301ebb58 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,
+        .count = sizeof(sense_data),
+        .flags = CCW_FLAG_SLI,
+        .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,13 @@ static void print_irb_err(Irb *irb)
  */
 static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
 {
-    CmdOrb orb = {};
+    CmdOrb orb = {
+        .fmt = fmt,
+        .pfch = 1,	/* QEMU's cio implementation requires prefetch */
+        .c64 = 1,	/* QEMU's cio implementation requires 64-bit idaws */
+        .lpm = 0xFF,	/* All paths allowed */
+        .cpa = ccw_addr,
+    };
     int rc;
 
     IPL_assert(fmt == 0 || fmt == 1, "Invalid ccw format");
@@ -324,12 +330,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 v2 2/9] 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>
---
 pc-bios/s390-ccw/menu.c        |  1 +
 pc-bios/s390-ccw/netmain.c     | 15 +++----------
 pc-bios/s390-ccw/s390-ccw.h    | 18 ----------------
 pc-bios/s390-ccw/time.h        | 39 ++++++++++++++++++++++++++++++++++
 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, 48 insertions(+), 45 deletions(-)
 create mode 100644 pc-bios/s390-ccw/time.h

diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
index ce3815b201..7925c33248 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 "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..73def8de4f 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 "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..c5820e43ae 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);
@@ -144,24 +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
 
-static inline void sleep(unsigned int seconds)
-{
-    ulong target = get_second() + seconds;
-
-    while (get_second() < target) {
-        yield();
-    }
-}
-
 static inline void IPL_assert(bool term, const char *message)
 {
     if (!term) {
diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h
new file mode 100644
index 0000000000..899de83ae7
--- /dev/null
+++ b/pc-bios/s390-ccw/time.h
@@ -0,0 +1,39 @@
+#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;
+}
+
+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/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index ff7f4dad25..4de03728bb 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 "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..0620651da8 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 "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..43717b83d7 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 "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 v2 3/9] 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>
---
 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..1ce5344b85 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 v2 4/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant

[Janosch Frank]

ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
actually 64 bit addressing.

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

diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..b932531e6f 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -229,7 +229,6 @@ 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;   /* ...          */
+    pswl->mask |= PSW_MASK_64;   /* Force 64 bit addressing */
     jump_to_low_kernel();
 }
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 v2 5/9] pc-bios: s390x: Use PSW masks where possible

[Janosch Frank]

Let's move some of the PSW mask defines into s390-arch.h and use them
in jump2ipl.c

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 v2 6/9] 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>
---
 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 73def8de4f..ef7ce1e01d 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..35be141d8d 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 v2 7/9] 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 coul look up the ascii value instead?

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@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 v2 8/9] pc-bios: s390x: Replace 0x00 with 0x0 or 0

[Janosch Frank]

0x00 looks odd, time to replace it with 0 or 0x0 (for pointers).

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

diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index b932531e6f..764ee89e92 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -98,18 +98,18 @@ static int run_dynamic_ccw_program(SubChannelId schid, uint16_t cutype,
 
 static void make_readipl(void)
 {
-    Ccw0 *ccwIplRead = (Ccw0 *)0x00;
+    Ccw0 *ccwIplRead = (Ccw0 *)0x0;
 
     /* Create Read IPL ccw at address 0 */
     ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
-    ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */
+    ccwIplRead->cda = 0x0; /* Read into address 0x00 in main memory */
     ccwIplRead->chain = 0; /* Chain flag */
     ccwIplRead->count = 0x18; /* Read 0x18 bytes of data */
 }
 
 static void run_readipl(SubChannelId schid, uint16_t cutype)
 {
-    if (do_cio(schid, cutype, 0x00, CCW_FMT0)) {
+    if (do_cio(schid, cutype, 0x0, CCW_FMT0)) {
         panic("dasd-ipl: Failed to run Read IPL channel program\n");
     }
 }
@@ -133,10 +133,10 @@ static void check_ipl2(uint32_t ipl2_addr)
 {
     Ccw0 *ccw = u32toptr(ipl2_addr);
 
-    if (ipl2_addr == 0x00) {
+    if (ipl2_addr == 0) {
         panic("IPL2 address invalid. Is this disk really bootable?\n");
     }
-    if (ccw->cmd_code == 0x00) {
+    if (ccw->cmd_code == 0) {
         panic("IPL2 ccw data invalid. Is this disk really bootable?\n");
     }
 }
@@ -161,7 +161,7 @@ static void ipl1_fixup(void)
     memcpy(ccwRead, (void *)0x08, 16);
 
     /* Disable chaining so we don't TIC to IPL2 channel program */
-    ccwRead->chain = 0x00;
+    ccwRead->chain = 0;
 
     ccwSeek->cmd_code = CCW_CMD_DASD_SEEK;
     ccwSeek->cda = ptr2u32(seekData);
@@ -206,7 +206,7 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
  */
 void dasd_ipl(SubChannelId schid, uint16_t cutype)
 {
-    PSWLegacy *pswl = (PSWLegacy *) 0x00;
+    PSWLegacy *pswl = (PSWLegacy *) 0x0;
     uint32_t ipl2_addr;
 
     /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
-- 
2.25.1

[PATCH v2 9/9] 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>
---
 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 78d5bc7442..8a50ec91bb 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -18,7 +18,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

Re: [PATCH v2 0/9] pc-bios: s390x: Cleanup part 1

[no-reply]

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



Hi,

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

Message-id: 20200514123729.156283-1-frankja@linux.ibm.com
Subject: [PATCH v2 0/9] pc-bios: s390x: Cleanup part 1
Type: series

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

Switched to a new branch 'test'
5049518 pc-bios: s390x: Make u32 ptr check explicit
10aee2e pc-bios: s390x: Replace 0x00 with 0x0 or 0
168421e pc-bios: s390x: Use ebcdic2ascii table
51051af pc-bios: s390x: Move panic() into header and add infinite loop
9cf4c1c pc-bios: s390x: Use PSW masks where possible
e886137 pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant
b10800a pc-bios: s390x: Get rid of magic offsets into the lowcore
a105085 pc-bios: s390x: Consolidate timing functions into time.h
3a83ebd pc-bios: s390x: cio.c cleanup and compile fix

=== OUTPUT BEGIN ===
1/9 Checking commit 3a83ebd32644 (pc-bios: s390x: cio.c cleanup and compile fix)
ERROR: code indent should never use tabs
#66: FILE: pc-bios/s390-ccw/cio.c:319:
+        .pfch = 1,^I/* QEMU's cio implementation requires prefetch */$

ERROR: code indent should never use tabs
#67: FILE: pc-bios/s390-ccw/cio.c:320:
+        .c64 = 1,^I/* QEMU's cio implementation requires 64-bit idaws */$

ERROR: code indent should never use tabs
#68: FILE: pc-bios/s390-ccw/cio.c:321:
+        .lpm = 0xFF,^I/* All paths allowed */$

total: 3 errors, 0 warnings, 63 lines checked

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

2/9 Checking commit a105085b3794 (pc-bios: s390x: Consolidate timing functions into time.h)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#102: 
new file mode 100644

total: 0 errors, 1 warnings, 167 lines checked

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

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

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

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

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

total: 5 errors, 0 warnings, 37 lines checked

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

4/9 Checking commit e886137b0e3c (pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant)
5/9 Checking commit 9cf4c1c4626a (pc-bios: s390x: Use PSW masks where possible)
6/9 Checking commit 51051afe9a9e (pc-bios: s390x: Move panic() into header and add infinite loop)
7/9 Checking commit 168421e3c66e (pc-bios: s390x: Use ebcdic2ascii table)
8/9 Checking commit 10aee2ea877c (pc-bios: s390x: Replace 0x00 with 0x0 or 0)
9/9 Checking commit 5049518e6552 (pc-bios: s390x: Make u32 ptr check explicit)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200514123729.156283-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 v2 1/9] pc-bios: s390x: cio.c cleanup and compile fix

[David Hildenbrand]

On 14.05.20 14:37, Janosch Frank wrote:
> Let's initialize the structs at the beginning to ease reading and also
> zeroing all other fields. This also makes the compiler stop
> compalining about sense_id_ccw.flags being ored into when it's not

s/compalining/complaining/

> initialized.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/cio.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
> index 339ec5fbe7..63301ebb58 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,
> +        .count = sizeof(sense_data),
> +        .flags = CCW_FLAG_SLI,
> +        .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,13 @@ static void print_irb_err(Irb *irb)
>   */
>  static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
>  {
> -    CmdOrb orb = {};
> +    CmdOrb orb = {
> +        .fmt = fmt,
> +        .pfch = 1,	/* QEMU's cio implementation requires prefetch */
> +        .c64 = 1,	/* QEMU's cio implementation requires 64-bit idaws */

Maybe just document this on top (all comments combined)

/*
 * QEMU's CIO implementation requires prefetch and 64-bit idaws. We
 * allow all paths.
 */

Or get rid of the tabs ;)

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

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 2/9] pc-bios: s390x: Consolidate timing functions into time.h

[David Hildenbrand]

[...]

>  
> -static inline void yield(void)
> -{
> -    asm volatile ("diag 0,0,0x44"
> -                  : :
> -                  : "memory", "cc");
> -}
> -

Nit: Looks weird that yield() is moved to time.h

Wonder if there is a better fit for this function.

>  #define MAX_SECTOR_SIZE 4096
>  
> -static inline void sleep(unsigned int seconds)
> -{
> -    ulong target = get_second() + seconds;
> -
> -    while (get_second() < target) {
> -        yield();
> -    }
> -}
> -
>  static inline void IPL_assert(bool term, const char *message)
>  {
>      if (!term) {
> diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h
> new file mode 100644
> index 0000000000..899de83ae7
> --- /dev/null
> +++ b/pc-bios/s390-ccw/time.h
> @@ -0,0 +1,39 @@
> +#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;

return get_time_ms() / 1000;



-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/9] pc-bios: s390x: cio.c cleanup and compile fix

[Janosch Frank]

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

On 5/18/20 1:52 PM, David Hildenbrand wrote:
> On 14.05.20 14:37, Janosch Frank wrote:
>> Let's initialize the structs at the beginning to ease reading and also
>> zeroing all other fields. This also makes the compiler stop
>> compalining about sense_id_ccw.flags being ored into when it's not
> 
> s/compalining/complaining/
> 
>> initialized.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/cio.c | 36 ++++++++++++++++++------------------
>>  1 file changed, 18 insertions(+), 18 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
>> index 339ec5fbe7..63301ebb58 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,
>> +        .count = sizeof(sense_data),
>> +        .flags = CCW_FLAG_SLI,
>> +        .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,13 @@ static void print_irb_err(Irb *irb)
>>   */
>>  static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
>>  {
>> -    CmdOrb orb = {};
>> +    CmdOrb orb = {
>> +        .fmt = fmt,
>> +        .pfch = 1,	/* QEMU's cio implementation requires prefetch */
>> +        .c64 = 1,	/* QEMU's cio implementation requires 64-bit idaws */
> 
> Maybe just document this on top (all comments combined)
> 
> /*
>  * QEMU's CIO implementation requires prefetch and 64-bit idaws. We
>  * allow all paths.
>  */
> 
> Or get rid of the tabs ;)
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Will do, thanks!



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

Re: [PATCH v2 2/9] pc-bios: s390x: Consolidate timing functions into time.h

[Janosch Frank]

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

On 5/18/20 2:01 PM, David Hildenbrand wrote:
> [...]
> 
>>  
>> -static inline void yield(void)
>> -{
>> -    asm volatile ("diag 0,0,0x44"
>> -                  : :
>> -                  : "memory", "cc");
>> -}
>> -
> 
> Nit: Looks weird that yield() is moved to time.h
> 
> Wonder if there is a better fit for this functi
helper.h or maybe into the libc?

> 
>>  #define MAX_SECTOR_SIZE 4096
>>  
>> -static inline void sleep(unsigned int seconds)
>> -{
>> -    ulong target = get_second() + seconds;
>> -
>> -    while (get_second() < target) {
>> -        yield();
>> -    }
>> -}
>> -
>>  static inline void IPL_assert(bool term, const char *message)
>>  {
>>      if (!term) {
>> diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h
>> new file mode 100644
>> index 0000000000..899de83ae7
>> --- /dev/null
>> +++ b/pc-bios/s390-ccw/time.h
>> @@ -0,0 +1,39 @@
>> +#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;
> 
> return get_time_ms() / 1000;

Sure



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

Re: [PATCH v2 2/9] pc-bios: s390x: Consolidate timing functions into time.h

[David Hildenbrand]

On 18.05.20 14:09, Janosch Frank wrote:
> On 5/18/20 2:01 PM, David Hildenbrand wrote:
>> [...]
>>
>>>  
>>> -static inline void yield(void)
>>> -{
>>> -    asm volatile ("diag 0,0,0x44"
>>> -                  : :
>>> -                  : "memory", "cc");
>>> -}
>>> -
>>
>> Nit: Looks weird that yield() is moved to time.h
>>
>> Wonder if there is a better fit for this functi
> helper.h or maybe into the libc?

Maybe helper.h if it doesn't result in too much hassle.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 3/9] pc-bios: s390x: Get rid of magic offsets into the lowcore

[David Hildenbrand]

On 14.05.20 14:37, 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>
> ---
>  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..1ce5344b85 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)
> 

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

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 8/9] pc-bios: s390x: Replace 0x00 with 0x0 or 0

[David Hildenbrand]

On 14.05.20 14:37, Janosch Frank wrote:
> 0x00 looks odd, time to replace it with 0 or 0x0 (for pointers).

"(for addresses)" maybe?

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

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/dasd-ipl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index b932531e6f..764ee89e92 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -98,18 +98,18 @@ static int run_dynamic_ccw_program(SubChannelId schid, uint16_t cutype,
>  
>  static void make_readipl(void)
>  {
> -    Ccw0 *ccwIplRead = (Ccw0 *)0x00;
> +    Ccw0 *ccwIplRead = (Ccw0 *)0x0;
>  
>      /* Create Read IPL ccw at address 0 */
>      ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
> -    ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */
> +    ccwIplRead->cda = 0x0; /* Read into address 0x00 in main memory */
>      ccwIplRead->chain = 0; /* Chain flag */
>      ccwIplRead->count = 0x18; /* Read 0x18 bytes of data */
>  }
>  
>  static void run_readipl(SubChannelId schid, uint16_t cutype)
>  {
> -    if (do_cio(schid, cutype, 0x00, CCW_FMT0)) {
> +    if (do_cio(schid, cutype, 0x0, CCW_FMT0)) {
>          panic("dasd-ipl: Failed to run Read IPL channel program\n");
>      }
>  }
> @@ -133,10 +133,10 @@ static void check_ipl2(uint32_t ipl2_addr)
>  {
>      Ccw0 *ccw = u32toptr(ipl2_addr);
>  
> -    if (ipl2_addr == 0x00) {
> +    if (ipl2_addr == 0) {
>          panic("IPL2 address invalid. Is this disk really bootable?\n");
>      }
> -    if (ccw->cmd_code == 0x00) {
> +    if (ccw->cmd_code == 0) {
>          panic("IPL2 ccw data invalid. Is this disk really bootable?\n");
>      }
>  }
> @@ -161,7 +161,7 @@ static void ipl1_fixup(void)
>      memcpy(ccwRead, (void *)0x08, 16);
>  
>      /* Disable chaining so we don't TIC to IPL2 channel program */
> -    ccwRead->chain = 0x00;
> +    ccwRead->chain = 0;
>  
>      ccwSeek->cmd_code = CCW_CMD_DASD_SEEK;
>      ccwSeek->cda = ptr2u32(seekData);
> @@ -206,7 +206,7 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
>   */
>  void dasd_ipl(SubChannelId schid, uint16_t cutype)
>  {
> -    PSWLegacy *pswl = (PSWLegacy *) 0x00;
> +    PSWLegacy *pswl = (PSWLegacy *) 0x0;
>      uint32_t ipl2_addr;
>  
>      /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 0/9] pc-bios: s390x: Cleanup part 1

[Cornelia Huck]

On Thu, 14 May 2020 08:37:20 -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
> 
> v2:
> 	* Included cio fixup to get rid of compile errors...
> 	* Minor cosmetic fixes found by review
> 
> Janosch Frank (9):
>   pc-bios: s390x: cio.c cleanup and compile fix
>   pc-bios: s390x: Consolidate timing functions into time.h
>   pc-bios: s390x: Get rid of magic offsets into the lowcore
>   pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant
>   pc-bios: s390x: Use PSW masks where possible
>   pc-bios: s390x: Move panic() into header and add infinite loop
>   pc-bios: s390x: Use ebcdic2ascii table
>   pc-bios: s390x: Replace 0x00 with 0x0 or 0
>   pc-bios: s390x: Make u32 ptr check explicit
> 
>  pc-bios/s390-ccw/bootmap.c     |  4 +---
>  pc-bios/s390-ccw/cio.c         | 36 +++++++++++++++----------------
>  pc-bios/s390-ccw/cio.h         | 17 +++++++++------
>  pc-bios/s390-ccw/dasd-ipl.c    | 17 +++++++--------
>  pc-bios/s390-ccw/helper.h      |  2 +-
>  pc-bios/s390-ccw/jump2ipl.c    | 10 ++++-----
>  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/start.S       |  5 +++--
>  pc-bios/s390-ccw/time.h        | 39 ++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/virtio-net.c  |  1 +
>  pc-bios/s390-ccw/virtio-scsi.c |  1 +
>  pc-bios/s390-ccw/virtio.c      | 18 +++-------------
>  16 files changed, 107 insertions(+), 113 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/time.h
> 

I assume Christian/Thomas will pick that one up once the last wrinkles
have been ironed out?

Re: [PATCH v2 1/9] pc-bios: s390x: cio.c cleanup and compile fix

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> Let's initialize the structs at the beginning to ease reading and also
> zeroing all other fields. This also makes the compiler stop
> compalining 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>
> ---
>  pc-bios/s390-ccw/cio.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)

With the nits mentioned by David fixed:

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

Re: [PATCH v2 2/9] pc-bios: s390x: Consolidate timing functions into time.h

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> Let's consolidate timing related functions into one header.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/menu.c        |  1 +
>  pc-bios/s390-ccw/netmain.c     | 15 +++----------
>  pc-bios/s390-ccw/s390-ccw.h    | 18 ----------------
>  pc-bios/s390-ccw/time.h        | 39 ++++++++++++++++++++++++++++++++++
>  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, 48 insertions(+), 45 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/time.h
> 
> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
> index ce3815b201..7925c33248 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 "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..73def8de4f 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 "time.h"

netmain.c already contains a #include <time.h> which pulls in the header
with the same name from libnet in SLOF ... so I wonder why you don't run
into trouble here, I'd expect that your local time.h now rather gets
included twice. Did you rebuild s390-netboot.img without problems?
Anyway, time.h is maybe not the best name for a new header here...

 Thomas

Re: [PATCH v2 3/9] pc-bios: s390x: Get rid of magic offsets into the lowcore

[Thomas Huth]

On 14/05/2020 14.37, 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>
> ---
>  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..1ce5344b85 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;

Wrong indentation for that sch_no line? Should only be 4 spaces,
shouldn't it?

>  } __attribute__ ((packed, aligned(4))) SubChannelId;

 Thomas

Re: [PATCH v2 4/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
> actually 64 bit addressing.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  pc-bios/s390-ccw/dasd-ipl.c  | 3 +--
>  pc-bios/s390-ccw/s390-arch.h | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index 0fc879bb8e..b932531e6f 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -229,7 +229,6 @@ 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;   /* ...          */
> +    pswl->mask |= PSW_MASK_64;   /* Force 64 bit addressing */

This is not only a rename (as announced in the subject), but also a
change in behavior since you now do not change pswl->addr anymore. So
this is even a bug fix? Could you please mention this in the patch
description, too?

 Thanks,
  Thomas

Re: [PATCH v2 4/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant

[Thomas Huth]

On 21/05/2020 07.44, Thomas Huth wrote:
> On 14/05/2020 14.37, Janosch Frank wrote:
>> ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
>> actually 64 bit addressing.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  pc-bios/s390-ccw/dasd-ipl.c  | 3 +--
>>  pc-bios/s390-ccw/s390-arch.h | 2 +-
>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>> index 0fc879bb8e..b932531e6f 100644
>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>> @@ -229,7 +229,6 @@ 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;   /* ...          */
>> +    pswl->mask |= PSW_MASK_64;   /* Force 64 bit addressing */
> 
> This is not only a rename (as announced in the subject), but also a
> change in behavior since you now do not change pswl->addr anymore. So
> this is even a bug fix? Could you please mention this in the patch
> description, too?

Ah, wait, pswl is of type PSWLegacy, and ->mask and ->addr are of type
uint32_t here! So it seems wrong to use a 64-bit value for mask here,
doesn't it?

 Thomas

Re: [PATCH v2 5/9] pc-bios: s390x: Use PSW masks where possible

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> Let's move some of the PSW mask defines into s390-arch.h and use them
> in jump2ipl.c
> 
> 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

Please also mention that new define in the patch description.

 Thomas

Re: [PATCH v2 6/9] pc-bios: s390x: Move panic() into header and add infinite loop

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> 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>
> ---
>  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(-)

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

Re: [PATCH v2 7/9] pc-bios: s390x: Use ebcdic2ascii table

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> Why should we do conversion of a ebcdic value if we have a handy table
> where we coul look up the ascii value instead?

s/coul/could/

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@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;
>      }
> 

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

Re: [PATCH v2 8/9] pc-bios: s390x: Replace 0x00 with 0x0 or 0

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> 0x00 looks odd, time to replace it with 0 or 0x0 (for pointers).
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/dasd-ipl.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index b932531e6f..764ee89e92 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -98,18 +98,18 @@ static int run_dynamic_ccw_program(SubChannelId schid, uint16_t cutype,
>  
>  static void make_readipl(void)
>  {
> -    Ccw0 *ccwIplRead = (Ccw0 *)0x00;
> +    Ccw0 *ccwIplRead = (Ccw0 *)0x0;
>  
>      /* Create Read IPL ccw at address 0 */
>      ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
> -    ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */
> +    ccwIplRead->cda = 0x0; /* Read into address 0x00 in main memory */
>      ccwIplRead->chain = 0; /* Chain flag */
>      ccwIplRead->count = 0x18; /* Read 0x18 bytes of data */
>  }
>  
>  static void run_readipl(SubChannelId schid, uint16_t cutype)
>  {
> -    if (do_cio(schid, cutype, 0x00, CCW_FMT0)) {
> +    if (do_cio(schid, cutype, 0x0, CCW_FMT0)) {
>          panic("dasd-ipl: Failed to run Read IPL channel program\n");
>      }
>  }
> @@ -133,10 +133,10 @@ static void check_ipl2(uint32_t ipl2_addr)
>  {
>      Ccw0 *ccw = u32toptr(ipl2_addr);
>  
> -    if (ipl2_addr == 0x00) {
> +    if (ipl2_addr == 0) {
>          panic("IPL2 address invalid. Is this disk really bootable?\n");
>      }
> -    if (ccw->cmd_code == 0x00) {
> +    if (ccw->cmd_code == 0) {
>          panic("IPL2 ccw data invalid. Is this disk really bootable?\n");
>      }
>  }
> @@ -161,7 +161,7 @@ static void ipl1_fixup(void)
>      memcpy(ccwRead, (void *)0x08, 16);
>  
>      /* Disable chaining so we don't TIC to IPL2 channel program */
> -    ccwRead->chain = 0x00;
> +    ccwRead->chain = 0;
>  
>      ccwSeek->cmd_code = CCW_CMD_DASD_SEEK;
>      ccwSeek->cda = ptr2u32(seekData);
> @@ -206,7 +206,7 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
>   */
>  void dasd_ipl(SubChannelId schid, uint16_t cutype)
>  {
> -    PSWLegacy *pswl = (PSWLegacy *) 0x00;
> +    PSWLegacy *pswl = (PSWLegacy *) 0x0;
>      uint32_t ipl2_addr;
>  
>      /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
> 

Not sure whether this is really worth the churn ... but in case you insist:

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

Re: [PATCH v2 9/9] pc-bios: s390x: Make u32 ptr check explicit

[Thomas Huth]

On 14/05/2020 14.37, Janosch Frank wrote:
> 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>
> ---
>  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 78d5bc7442..8a50ec91bb 100644
> --- a/pc-bios/s390-ccw/helper.h
> +++ b/pc-bios/s390-ccw/helper.h
> @@ -18,7 +18,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;
>  }
>  
> 

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

Re: [PATCH v2 4/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant

[Janosch Frank]

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

On 5/21/20 7:47 AM, Thomas Huth wrote:
> On 21/05/2020 07.44, Thomas Huth wrote:
>> On 14/05/2020 14.37, Janosch Frank wrote:
>>> ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
>>> actually 64 bit addressing.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  pc-bios/s390-ccw/dasd-ipl.c  | 3 +--
>>>  pc-bios/s390-ccw/s390-arch.h | 2 +-
>>>  2 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>>> index 0fc879bb8e..b932531e6f 100644
>>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>>> @@ -229,7 +229,6 @@ 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;   /* ...          */
>>> +    pswl->mask |= PSW_MASK_64;   /* Force 64 bit addressing */
>>
>> This is not only a rename (as announced in the subject), but also a
>> change in behavior since you now do not change pswl->addr anymore. So
>> this is even a bug fix? Could you please mention this in the patch
>> description, too?
> 
> Ah, wait, pswl is of type PSWLegacy, and ->mask and ->addr are of type
> uint32_t here! So it seems wrong to use a 64-bit value for mask here,
> doesn't it?

Absolutely, how did that even compile?

I'm tempted to just make it a unsigned long ptr instead. The legacy PSW
struct doesn't seem to be used correctly anyway, the lowcore one is in
fact never used and this is the only other occurrence.


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

Re: [PATCH v2 5/9] pc-bios: s390x: Use PSW masks where possible

[Janosch Frank]

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

On 5/21/20 7:50 AM, Thomas Huth wrote:
> On 14/05/2020 14.37, Janosch Frank wrote:
>> Let's move some of the PSW mask defines into s390-arch.h and use them
>> in jump2ipl.c
>>
>> 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
> 
> Please also mention that new define in the patch description.
> 
>  Thomas
> 
> 
pc-bios: s390x: Use PSW masks where possible and introduce
PSW_MASK_SHORT_ADDR

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>



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

Re: [PATCH v2 2/9] pc-bios: s390x: Consolidate timing functions into time.h

[Janosch Frank]

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

On 5/20/20 9:53 PM, Thomas Huth wrote:
> On 14/05/2020 14.37, Janosch Frank wrote:
>> Let's consolidate timing related functions into one header.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/menu.c        |  1 +
>>  pc-bios/s390-ccw/netmain.c     | 15 +++----------
>>  pc-bios/s390-ccw/s390-ccw.h    | 18 ----------------
>>  pc-bios/s390-ccw/time.h        | 39 ++++++++++++++++++++++++++++++++++
>>  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, 48 insertions(+), 45 deletions(-)
>>  create mode 100644 pc-bios/s390-ccw/time.h
>>
>> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c
>> index ce3815b201..7925c33248 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 "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..73def8de4f 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 "time.h"
> 
> netmain.c already contains a #include <time.h> which pulls in the header
> with the same name from libnet in SLOF ... so I wonder why you don't run
> into trouble here, I'd expect that your local time.h now rather gets
> included twice. Did you rebuild s390-netboot.img without problems?
> Anyway, time.h is maybe not the best name for a new header here...
> 
>  Thomas
> 

That doesn't seem to be a problem when compiling.
However we could just move all of the function into helper.h as it's
really not a lot of code especially after splitting it up into helper.h
and time.h.

Or we could use s390-time.h


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