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

[PATCH 0/8] 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 based on my protvirt branch and are available here:
https://github.com/frankjaa/qemu/pull/new/cleanup_bios

Janosch Frank (8):
  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.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    | 25 ++++++----------------
 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 +++-------------
 15 files changed, 88 insertions(+), 94 deletions(-)
 create mode 100644 pc-bios/s390-ccw/time.h

-- 
2.25.1

[PATCH 1/8] 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 ce3815b2010d20cb..7925c33248836913 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 309ffa30d9922077..a8e2b1b6233735d8 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_milli() + val;
 }
 
 int get_timer(void)
 {
-    return dest_timer - get_timer_ms();
+    return dest_timer - get_time_milli();
 }
 
 int get_sec_ticks(void)
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 21f27e79906ea297..c5820e43aed143d0 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 0000000000000000..98f5acaa33b500bd
--- /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_milli(void)
+{
+    /* Bit 51 is incremented each microsecond */
+    return (get_clock() >> 12) / 1000;
+}
+
+static inline u64 get_time_seconds(void)
+{
+    return (get_time_milli()) / 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 ff7f4dad2597eeae..4de03728bb648be1 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 4fe4b9d261a6c88d..0620651da82d569d 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 fb40ca982853a8d7..43717b83d7b2a9b9 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 2/8] 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 aaa432deddb9242e..9ad794a789c47df2 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: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 4e65b411e1d890ba..8b912454c940a390 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 3/8] 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>
---
 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 0fc879bb8e8faac7..b932531e6f838405 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 5f36361c0223d434..73852029d4e92cd9 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 4/8] 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>
---
 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 4eba2510b045ff06..767012bf0c9f587e 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 73852029d4e92cd9..6da44d4436c75b55 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 5/8] 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>
---
 pc-bios/s390-ccw/main.c     | 7 -------
 pc-bios/s390-ccw/netmain.c  | 8 --------
 pc-bios/s390-ccw/s390-ccw.h | 7 ++++++-
 pc-bios/s390-ccw/start.S    | 5 +++--
 4 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 8b912454c940a390..146a50760bc70af7 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 a8e2b1b6233735d8..ca23f9bb19a3e04c 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 c5820e43aed143d0..b3dcfbc3c9b3814c 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -55,7 +55,6 @@ 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 aa8fceb19da2164a..35be141d8da38d07 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 6/8] 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>
---
 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 d13b7cbd1597bf2e..97205674e59aebb2 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 7/8] 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 b932531e6f838405..764ee89e92e3ae8d 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 8/8] 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>
---
 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 78d5bc74421b4173..b4e83a7e5deaa9dc 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 <= 0xfffffffful, "ptr2u32: ptr too large");
     return (uint32_t)(uint64_t)ptr;
 }
 
-- 
2.25.1

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

[no-reply]

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



Hi,

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

Subject: [PATCH 0/8] pc-bios: s390x: Cleanup part 1
Message-id: 20200324150847.10476-1-frankja@linux.ibm.com
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'
631f5fb pc-bios: s390x: Make u32 ptr check explicit
31a77a2 pc-bios: s390x: Replace 0x00 with 0x0 or 0
e7c3314 pc-bios: s390x: Use ebcdic2ascii table
67bf6cc pc-bios: s390x: Move panic() into header and add infinite loop
0430d47 pc-bios: s390x: Use PSW masks where possible
5dc5f58 pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant
49da8ad pc-bios: s390x: Get rid of magic offsets into the lowcore
61a8ff1 pc-bios: s390x: Consolidate timing functions into time.h

=== OUTPUT BEGIN ===
1/8 Checking commit 61a8ff1495d4 (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 1/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/8 Checking commit 49da8ad037ea (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: code indent should never use tabs
#30: FILE: pc-bios/s390-ccw/cio.h:128:
+^I    __u16: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 2/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/8 Checking commit 5dc5f5840149 (pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant)
4/8 Checking commit 0430d47d5125 (pc-bios: s390x: Use PSW masks where possible)
5/8 Checking commit 67bf6cc8c901 (pc-bios: s390x: Move panic() into header and add infinite loop)
6/8 Checking commit e7c33142a550 (pc-bios: s390x: Use ebcdic2ascii table)
7/8 Checking commit 31a77a28b0f3 (pc-bios: s390x: Replace 0x00 with 0x0 or 0)
8/8 Checking commit 631f5fb35427 (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/20200324150847.10476-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 1/8] pc-bios: s390x: Consolidate timing functions into time.h

[David Hildenbrand]

On 24.03.20 16:08, 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 ce3815b2010d20cb..7925c33248836913 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 309ffa30d9922077..a8e2b1b6233735d8 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_milli() + val;
>  }
>  
>  int get_timer(void)
>  {
> -    return dest_timer - get_timer_ms();
> +    return dest_timer - get_time_milli();
>  }
>  
>  int get_sec_ticks(void)
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 21f27e79906ea297..c5820e43aed143d0 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 0000000000000000..98f5acaa33b500bd
> --- /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_milli(void)

Milli Vanilli?

Please, just use common abbreviations. "ms" is certainly good enough.


-- 
Thanks,

David / dhildenb

Re: [PATCH 2/8] pc-bios: s390x: Get rid of magic offsets into the lowcore

[David Hildenbrand]

On 24.03.20 16:08, 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 aaa432deddb9242e..9ad794a789c47df2 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:4;

Broken indentation. Also, do we want to give that strange-looking
"__u32:4" a name ("reserved").

> +            __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 4e65b411e1d890ba..8b912454c940a390 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)
> 

Looks sane to me.

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/8] pc-bios: s390x: Get rid of magic offsets into the lowcore

[Janosch Frank]

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

On 3/27/20 11:25 AM, David Hildenbrand wrote:
> On 24.03.20 16:08, 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 aaa432deddb9242e..9ad794a789c47df2 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:4;
> 
> Broken indentation. Also, do we want to give that strange-looking
> "__u32:4" a name ("reserved").

Fixed indentation and renamed.

> 
>> +            __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 4e65b411e1d890ba..8b912454c940a390 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)
>>
> 
> Looks sane to me.
> 



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

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

[Cornelia Huck]

On Tue, 24 Mar 2020 11:08:44 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

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

They were also slightly different, so unifying them makes sense.

> 
> 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>
> ---
>  pc-bios/s390-ccw/main.c     | 7 -------
>  pc-bios/s390-ccw/netmain.c  | 8 --------
>  pc-bios/s390-ccw/s390-ccw.h | 7 ++++++-
>  pc-bios/s390-ccw/start.S    | 5 +++--
>  4 files changed, 9 insertions(+), 18 deletions(-)
> 

> @@ -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 aa8fceb19da2164a..35be141d8da38d07 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
>  
>  
>  /*

Possibly dumb question: Does checking code now figure out correctly
that code flow does not continue after disabled_wait()?

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

[Janosch Frank]

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

On 3/24/20 4:08 PM, Janosch Frank 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 based on my protvirt branch and are available here:
> https://github.com/frankjaa/qemu/pull/new/cleanup_bios

Ping
It's not urgent, but I don't want it to get buried and forgotten.

> 
> Janosch Frank (8):
>   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.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    | 25 ++++++----------------
>  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 +++-------------
>  15 files changed, 88 insertions(+), 94 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/time.h
> 



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

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

[David Hildenbrand]

On 29.04.20 14:11, Janosch Frank wrote:
> On 3/24/20 4:08 PM, Janosch Frank 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 based on my protvirt branch and are available here:
>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
> 
> Ping
> It's not urgent, but I don't want it to get buried and forgotten.
> 

Let's wait for Thomas, I currently don't have any capacity to review this.


-- 
Thanks,

David / dhildenb

Re: [PATCH 3/8] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant

[David Hildenbrand]

On 24.03.20 16:08, 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>
> ---
>  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 0fc879bb8e8faac7..b932531e6f838405 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 5f36361c0223d434..73852029d4e92cd9 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 {
> 

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

-- 
Thanks,

David / dhildenb

Re: [PATCH 4/8] pc-bios: s390x: Use PSW masks where possible

[David Hildenbrand]

On 24.03.20 16:08, 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>
> ---
>  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 4eba2510b045ff06..767012bf0c9f587e 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 73852029d4e92cd9..6da44d4436c75b55 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 */
> 


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

-- 
Thanks,

David / dhildenb

Re: [PATCH 6/8] pc-bios: s390x: Use ebcdic2ascii table

[David Hildenbrand]

On 24.03.20 16:08, 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?
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.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 d13b7cbd1597bf2e..97205674e59aebb2 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: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 24.03.20 16:08, Janosch Frank wrote:
> 0x00 looks odd, time to replace it with 0 or 0x0 (for pointers).

s/0x0/NULL/ ?

> 
> 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 b932531e6f838405..764ee89e92e3ae8d 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 8/8] pc-bios: s390x: Make u32 ptr check explicit

[David Hildenbrand]

On 24.03.20 16:08, 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>
> ---
>  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 78d5bc74421b4173..b4e83a7e5deaa9dc 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 <= 0xfffffffful, "ptr2u32: ptr too large");

wonder if we should even do ull.

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

>      return (uint32_t)(uint64_t)ptr;
>  }
>  
> 


-- 
Thanks,

David / dhildenb

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

[David Hildenbrand]

On 24.03.20 16:08, 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>
> ---
>  pc-bios/s390-ccw/main.c     | 7 -------
>  pc-bios/s390-ccw/netmain.c  | 8 --------
>  pc-bios/s390-ccw/s390-ccw.h | 7 ++++++-
>  pc-bios/s390-ccw/start.S    | 5 +++--
>  4 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 8b912454c940a390..146a50760bc70af7 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) { }
> -}

I remember there was a reason why to add the endless loop afterwards.
Maybe because some special machine checks can actually wake it up? Or
buggy hypervisor?

Anyhow, the kernel also does

__load_psw(psw);
while (1);

so it's best we keep that.


With the endless loop re-added

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

-- 
Thanks,

David / dhildenb

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

[Janosch Frank]

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

On 4/30/20 5:42 PM, David Hildenbrand wrote:
> On 24.03.20 16:08, 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>
>> ---
>>  pc-bios/s390-ccw/main.c     | 7 -------
>>  pc-bios/s390-ccw/netmain.c  | 8 --------
>>  pc-bios/s390-ccw/s390-ccw.h | 7 ++++++-
>>  pc-bios/s390-ccw/start.S    | 5 +++--
>>  4 files changed, 9 insertions(+), 18 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 8b912454c940a390..146a50760bc70af7 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) { }
>> -}
> 
> I remember there was a reason why to add the endless loop afterwards.
> Maybe because some special machine checks can actually wake it up? Or
> buggy hypervisor?
> 
> Anyhow, the kernel also does
> 
> __load_psw(psw);
> while (1);
> 
> so it's best we keep that.
> 
> 
> With the endless loop re-added

Well, I added a loop into the disabled_wait assembly and removed it from
the C code. It's even in the commit message.

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



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

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

[David Hildenbrand]

On 04.05.20 08:46, Janosch Frank wrote:
> On 4/30/20 5:42 PM, David Hildenbrand wrote:
>> On 24.03.20 16:08, 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>
>>> ---
>>>  pc-bios/s390-ccw/main.c     | 7 -------
>>>  pc-bios/s390-ccw/netmain.c  | 8 --------
>>>  pc-bios/s390-ccw/s390-ccw.h | 7 ++++++-
>>>  pc-bios/s390-ccw/start.S    | 5 +++--
>>>  4 files changed, 9 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>>> index 8b912454c940a390..146a50760bc70af7 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) { }
>>> -}
>>
>> I remember there was a reason why to add the endless loop afterwards.
>> Maybe because some special machine checks can actually wake it up? Or
>> buggy hypervisor?
>>
>> Anyhow, the kernel also does
>>
>> __load_psw(psw);
>> while (1);
>>
>> so it's best we keep that.
>>
>>
>> With the endless loop re-added
> 
> Well, I added a loop into the disabled_wait assembly and removed it from
> the C code. It's even in the commit message.

Whops, missed that, looks good to me then!


-- 
Thanks,

David / dhildenb

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

[Cornelia Huck]

On Thu, 30 Apr 2020 16:50:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 29.04.20 14:11, Janosch Frank wrote:
> > On 3/24/20 4:08 PM, Janosch Frank 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 based on my protvirt branch and are available here:
> >> https://github.com/frankjaa/qemu/pull/new/cleanup_bios  
> > 
> > Ping
> > It's not urgent, but I don't want it to get buried and forgotten.
> >   
> 
> Let's wait for Thomas, I currently don't have any capacity to review this.
> 
> 

From the feedback, I assume there will be a v2 anyway?

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

[Janosch Frank]

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

On 5/14/20 12:59 PM, Cornelia Huck wrote:
> On Thu, 30 Apr 2020 16:50:25 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 29.04.20 14:11, Janosch Frank wrote:
>>> On 3/24/20 4:08 PM, Janosch Frank 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 based on my protvirt branch and are available here:
>>>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios  
>>>
>>> Ping
>>> It's not urgent, but I don't want it to get buried and forgotten.
>>>   
>>
>> Let's wait for Thomas, I currently don't have any capacity to review this.
>>
>>
> 
> From the feedback, I assume there will be a v2 anyway?
> 

I'll send out a new version soonish(TM)



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

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

[Janosch Frank]

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

On 4/7/20 9:25 AM, Cornelia Huck wrote:
> On Tue, 24 Mar 2020 11:08:44 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> panic() was defined for the ccw and net bios, i.e. twice, so it's
>> cleaner to rather put it into the header.
> 
> They were also slightly different, so unifying them makes sense.
> 
>>
>> 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>
>> ---
>>  pc-bios/s390-ccw/main.c     | 7 -------
>>  pc-bios/s390-ccw/netmain.c  | 8 --------
>>  pc-bios/s390-ccw/s390-ccw.h | 7 ++++++-
>>  pc-bios/s390-ccw/start.S    | 5 +++--
>>  4 files changed, 9 insertions(+), 18 deletions(-)
>>
> 
>> @@ -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 aa8fceb19da2164a..35be141d8da38d07 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
>>  
>>  
>>  /*
> 
> Possibly dumb question: Does checking code now figure out correctly
> that code flow does not continue after disabled_wait()?
> 

Which checking code?
I could certainly add "__attribute__ ((__noreturn__))" if needed.


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

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

[Janosch Frank]

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

On 4/30/20 5:36 PM, David Hildenbrand wrote:
> On 24.03.20 16:08, Janosch Frank wrote:
>> 0x00 looks odd, time to replace it with 0 or 0x0 (for pointers).
> 
> s/0x0/NULL/ ?

I'd like to avoid NULL if I refer to offset 0 of the memory and only use
it to indicate that I purposely do not assign any specific value to a
pointer.


> 
>>
>> 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 b932531e6f838405..764ee89e92e3ae8d 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 */
>>
> 
> 



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

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

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 2111 bytes --]

On Thu, 14 May 2020 13:27:20 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 4/7/20 9:25 AM, Cornelia Huck wrote:
> > On Tue, 24 Mar 2020 11:08:44 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> panic() was defined for the ccw and net bios, i.e. twice, so it's
> >> cleaner to rather put it into the header.  
> > 
> > They were also slightly different, so unifying them makes sense.
> >   
> >>
> >> 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>
> >> ---
> >>  pc-bios/s390-ccw/main.c     | 7 -------
> >>  pc-bios/s390-ccw/netmain.c  | 8 --------
> >>  pc-bios/s390-ccw/s390-ccw.h | 7 ++++++-
> >>  pc-bios/s390-ccw/start.S    | 5 +++--
> >>  4 files changed, 9 insertions(+), 18 deletions(-)
> >>  
> >   
> >> @@ -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 aa8fceb19da2164a..35be141d8da38d07 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
> >>  
> >>  
> >>  /*  
> > 
> > Possibly dumb question: Does checking code now figure out correctly
> > that code flow does not continue after disabled_wait()?
> >   
> 
> Which checking code?

Things like e.g. Coverity.

> I could certainly add "__attribute__ ((__noreturn__))" if needed.

Probably would not hurt.

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