[PATCH v3 0/5] pc-bios: s390x: Cleanup part 2

[PATCH v3 0/5] pc-bios: s390x: Cleanup part 2

[Janosch Frank]

So, here are a few more cleanup patches mostly cleaning up ipl code
and some of the assembly.

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

v3:
	* Split PSW save rework again
	* Added noreturn annotation
	* Minor cleanup

v2:
	* Fixed psw saving in use reset psw patch (thanks Jason)
	* Dropped a lot of patches which weren't strictly necessary
	* Added disabled wait patch
	* Added RFC PSW save patch

Janosch Frank (5):
  pc-bios: s390x: Fix bootmap.c zipl component entry data handling
  pc-bios: s390x: Save PSW rework
  pc-bios: s390x: Use reset PSW if avaliable
  pc-bios: s390x: Save io and external new PSWs before overwriting them
  pc-bios: s390x: Go into disabled wait when encountering a PGM
    exception

 pc-bios/s390-ccw/bootmap.c  |  6 ++--
 pc-bios/s390-ccw/bootmap.h  |  7 +++-
 pc-bios/s390-ccw/jump2ipl.c | 51 +++++++++++++++++------------
 pc-bios/s390-ccw/netmain.c  |  3 ++
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 pc-bios/s390-ccw/start.S    | 65 +++++++++++++++++++++++++++----------
 6 files changed, 92 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH v3 1/5] pc-bios: s390x: Fix bootmap.c zipl component entry data handling

[Janosch Frank]

The two main types of zipl component entries are execute and
load/data. The last member of the component entry struct therefore
denotes either a PSW or an address. Let's make this a bit more clear
by introducing a union and cleaning up the code that uses that struct
member.

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

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

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

[PATCH v3 2/5] pc-bios: s390x: Save PSW rework

[Janosch Frank]

We don't need to save the ipl_continue variable in lowcore and have it
limited to 32 bits because of the lowcore layout. Let's move it to a
new 64 bit variable and get rid of the reset info struct.

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

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

[PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable

[Janosch Frank]

If a blob provides a reset PSW then we should use it instead of
branching to the PSW address and using our own mask.

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

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 8747c4ea26..5a03b1eb8b 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte)
     IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
 
     /* should not return */
-    jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
+    write_reset_psw(entry->compdat.load_psw);
+    jump_to_IPL_code(0);
 }
 
 static void ipl_scsi(void)
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index b6aad32def..5b8352d257 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -12,15 +12,21 @@
 
 #define KERN_IMAGE_START 0x010000UL
 #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
+#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
 
 static uint64_t *reset_psw = 0, save_psw, ipl_continue;
 
+void write_reset_psw(uint64_t psw)
+{
+    *reset_psw = psw;
+}
+
 static void jump_to_IPL_addr(void)
 {
     __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue;
 
     /* Restore reset PSW */
-    *reset_psw = save_psw;
+    write_reset_psw(save_psw);
 
     ipl();
     /* should not return */
@@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address)
      * content of non-BIOS memory after we loaded the guest, so we
      * save the original content and restore it in jump_to_IPL_2.
      */
-    save_psw = *reset_psw;
-    *reset_psw = (uint64_t) &jump_to_IPL_addr;
-    *reset_psw |= RESET_PSW_MASK;
+    if (address) {
+        save_psw = *reset_psw;
+        write_reset_psw(RESET_PSW);
+    }
     ipl_continue = address;
     debug_print_int("set IPL addr to", ipl_continue);
 
@@ -77,7 +84,12 @@ 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)) & PSW_MASK_SHORT_ADDR);
+        /*
+         * Surely nobody will try running directly from lowcore, so
+         * let's use 0 as an indication that we want to load the reset
+         * psw at 0x0 and not jump to the entry.
+         */
+        jump_to_IPL_code(0);
     }
 
     /* No other option left, so use the Linux kernel start address */
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 36b884cced..7090720422 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr);
 void zipl_load(void);
 
 /* jump2ipl.c */
+void write_reset_psw(uint64_t psw);
 void jump_to_IPL_code(uint64_t address);
 void jump_to_low_kernel(void);
 
-- 
2.25.1

[PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them

[Janosch Frank]

Currently we always overwrite the mentioned exception new PSWs before
loading the enabled wait PSW. Let's save the PSW before overwriting
and restore it right before starting the loaded kernel.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 pc-bios/s390-ccw/jump2ipl.c |  4 +++
 pc-bios/s390-ccw/netmain.c  |  3 ++
 pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
 3 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 5b8352d257..bb94ba7550 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -14,6 +14,7 @@
 #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
 #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
 
+extern uint64_t psw_save_io[], psw_save_ext[];
 static uint64_t *reset_psw = 0, save_psw, ipl_continue;
 
 void write_reset_psw(uint64_t psw)
@@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address)
     /* Ensure the guest output starts fresh */
     sclp_print("\n");
 
+    memcpy(&lowcore->io_new_psw, psw_save_io, 16);
+    memcpy(&lowcore->external_new_psw, psw_save_ext, 16);
+
     /*
      * HACK ALERT.
      * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 056e93a818..74ef28fbc6 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -32,6 +32,7 @@
 #include <time.h>
 #include <pxelinux.h>
 
+#include "s390-arch.h"
 #include "s390-ccw.h"
 #include "cio.h"
 #include "virtio.h"
@@ -43,6 +44,8 @@
 extern char _start[];
 void write_iplb_location(void) {}
 
+LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
+
 #define KERNEL_ADDR             ((void *)0L)
 #define KERNEL_MAX_SIZE         ((long)_start)
 #define ARCH_COMMAND_LINE_SIZE  896              /* Taken from Linux kernel */
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index ce519300a1..939aac3a7c 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -34,7 +34,17 @@ remainder:
 	larl	%r2,memsetxc
 	ex	%r3,0(%r2)
 done:
-	j      main		/* And call C */
+        /* prepare i/o call handler */
+        larl  %r1, io_new_code
+        larl  %r2, io_new_psw
+        stg   %r1, 8(%r2)
+        mvc   0x1f0(16),0(%r2)
+        /* prepare external call handler */
+        larl  %r1, external_new_code
+        larl  %r2, external_new_psw
+        stg   %r1, 8(%r2)
+        mvc   0x1b0(16),0(%r2)
+        j      main		/* And call C */
 
 memsetxc:
 	xc	0(1,%r1),0(%r1)
@@ -64,13 +74,16 @@ consume_sclp_int:
         oi      6(%r15),0x2
         lctlg   %c0,%c0,0(%r15)
         /* prepare external call handler */
-        larl %r1, external_new_code
-        stg %r1, 0x1b8
-        larl %r1, external_new_mask
-        mvc 0x1b0(8),0(%r1)
-        /* load enabled wait PSW */
-        larl %r1, enabled_wait_psw
-        lpswe 0(%r1)
+        larl  %r1, external_new_psw
+        lghi  %r2, 0x1b0
+        /* Is the BIOS' external new PSW already set? */
+        clc   0(16, %r1), 0(%r2)
+        je    load_ewait
+        /* No, save old PSW and write BIOS PSW */
+        larl  %r3, psw_save_ext
+        mvc   0(16, %r3), 0x1b0
+        mvc   0x1b0(16),0(%r1)
+        j     load_ewait
 
 /*
  * void consume_io_int(void)
@@ -84,11 +97,20 @@ consume_io_int:
         oi    4(%r15), 0xff
         lctlg %c6,%c6,0(%r15)
         /* prepare i/o call handler */
-        larl  %r1, io_new_code
-        stg   %r1, 0x1f8
-        larl  %r1, io_new_mask
-        mvc   0x1f0(8),0(%r1)
-        /* load enabled wait PSW */
+        larl  %r1, io_new_psw
+        lghi  %r2, 0x1f0
+        /* Is the BIOS' PSW already set? */
+        larl  %r3, load_ewait
+        clc   0(16, %r1), 0(%r2)
+        bcr   8, %r3
+        /* No, save old PSW and write BIOS PSW */
+        larl  %r3, psw_save_io
+        mvc   0(16, %r3), 0x1f0
+        mvc   0x1f0(16),0(%r1)
+        j     load_ewait
+
+load_ewait:
+        /* PSW is the correct one, time to load the enabled wait PSW */
         larl  %r1, enabled_wait_psw
         lpswe 0(%r1)
 
@@ -107,11 +129,17 @@ io_new_code:
         br    %r14
 
         .align  8
+        .globl psw_save_io
+        .globl psw_save_ext
 disabled_wait_psw:
         .quad   0x0002000180000000,0x0000000000000000
 enabled_wait_psw:
         .quad   0x0302000180000000,0x0000000000000000
-external_new_mask:
-        .quad   0x0000000180000000
-io_new_mask:
-        .quad   0x0000000180000000
+external_new_psw:
+        .quad   0x0000000180000000,0
+io_new_psw:
+        .quad   0x0000000180000000,0
+psw_save_io:
+        .quad   0,0
+psw_save_ext:
+        .quad   0,0
-- 
2.25.1

[PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception

[Janosch Frank]

Let's setup a PGM PSW, so we won't load 0s when a program exception
happens. Instead we'll load a disabled wait PSW.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw/start.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 939aac3a7c..775b45baeb 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -44,6 +44,9 @@ done:
         larl  %r2, external_new_psw
         stg   %r1, 8(%r2)
         mvc   0x1b0(16),0(%r2)
+        /* set up a pgm exception disabled wait psw */
+        larl  %r2, disabled_wait_psw
+        mvc   0x01d0(16), 0(%r2)
         j      main		/* And call C */
 
 memsetxc:
-- 
2.25.1

Re: [PATCH v3 2/5] pc-bios: s390x: Save PSW rework

[Thomas Huth]

On 31/08/2020 17.09, Janosch Frank wrote:
> We don't need to save the ipl_continue variable in lowcore and have it
> limited to 32 bits because of the lowcore layout. Let's move it to a
> new 64 bit variable and get rid of the reset info struct.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 767012bf0c..b6aad32def 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -13,20 +13,17 @@
>  #define KERN_IMAGE_START 0x010000UL
>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>  
> -typedef struct ResetInfo {
> -    uint64_t ipl_psw;
> -    uint32_t ipl_continue;
> -} ResetInfo;
> +static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>  
> -static ResetInfo save;
> -
> -static void jump_to_IPL_2(void)
> +static void jump_to_IPL_addr(void)
>  {
> -    ResetInfo *current = 0;
> +    __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue;
>  
> -    void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
> -    *current = save;
> -    ipl(); /* should not return */
> +    /* Restore reset PSW */
> +    *reset_psw = save_psw;
> +
> +    ipl();
> +    /* should not return */
>  }
>  
>  void jump_to_IPL_code(uint64_t address)
> @@ -46,15 +43,11 @@ void jump_to_IPL_code(uint64_t address)
>       * content of non-BIOS memory after we loaded the guest, so we
>       * save the original content and restore it in jump_to_IPL_2.
>       */
> -    ResetInfo *current = 0;
> -
> -    save = *current;
> -
> -    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
> -    current->ipl_psw |= RESET_PSW_MASK;
> -    current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
> -
> -    debug_print_int("set IPL addr to", current->ipl_continue);
> +    save_psw = *reset_psw;
> +    *reset_psw = (uint64_t) &jump_to_IPL_addr;
> +    *reset_psw |= RESET_PSW_MASK;
> +    ipl_continue = address;
> +    debug_print_int("set IPL addr to", ipl_continue);
>  
>      /* Ensure the guest output starts fresh */
>      sclp_print("\n");

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

Re: [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2

[no-reply]

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



Hi,

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

Type: series
Message-id: 20200831150910.317171-1-frankja@linux.ibm.com
Subject: [PATCH v3 0/5] pc-bios: s390x: Cleanup part 2

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

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200831150910.317171-1-frankja@linux.ibm.com -> patchew/20200831150910.317171-1-frankja@linux.ibm.com
Switched to a new branch 'test'
62c6c18 pc-bios: s390x: Go into disabled wait when encountering a PGM exception
28a68e4 pc-bios: s390x: Save io and external new PSWs before overwriting them
58aec9e pc-bios: s390x: Use reset PSW if avaliable
b4cf040 pc-bios: s390x: Save PSW rework
30bab1a pc-bios: s390x: Fix bootmap.c zipl component entry data handling

=== OUTPUT BEGIN ===
1/5 Checking commit 30bab1ae7536 (pc-bios: s390x: Fix bootmap.c zipl component entry data handling)
2/5 Checking commit b4cf040a1e53 (pc-bios: s390x: Save PSW rework)
3/5 Checking commit 58aec9efd157 (pc-bios: s390x: Use reset PSW if avaliable)
4/5 Checking commit 28a68e4503b1 (pc-bios: s390x: Save io and external new PSWs before overwriting them)
ERROR: externs should be avoided in .c files
#22: FILE: pc-bios/s390-ccw/jump2ipl.c:17:
+extern uint64_t psw_save_io[], psw_save_ext[];

total: 1 errors, 0 warnings, 118 lines checked

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

5/5 Checking commit 62c6c187c90f (pc-bios: s390x: Go into disabled wait when encountering a PGM exception)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200831150910.317171-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 v3 3/5] pc-bios: s390x: Use reset PSW if avaliable

[Thomas Huth]

On 31/08/2020 17.09, Janosch Frank wrote:
> If a blob provides a reset PSW then we should use it instead of
> branching to the PSW address and using our own mask.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/bootmap.c  |  3 ++-
>  pc-bios/s390-ccw/jump2ipl.c | 22 +++++++++++++++++-----
>  pc-bios/s390-ccw/s390-ccw.h |  1 +
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 8747c4ea26..5a03b1eb8b 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte)
>      IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>  
>      /* should not return */
> -    jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
> +    write_reset_psw(entry->compdat.load_psw);
> +    jump_to_IPL_code(0);
>  }
>  
>  static void ipl_scsi(void)
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index b6aad32def..5b8352d257 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -12,15 +12,21 @@
>  
>  #define KERN_IMAGE_START 0x010000UL
>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
> +#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
>  
>  static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>  
> +void write_reset_psw(uint64_t psw)
> +{
> +    *reset_psw = psw;
> +}
> +
>  static void jump_to_IPL_addr(void)
>  {
>      __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue;
>  
>      /* Restore reset PSW */
> -    *reset_psw = save_psw;
> +    write_reset_psw(save_psw);
>  
>      ipl();
>      /* should not return */
> @@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address)
>       * content of non-BIOS memory after we loaded the guest, so we
>       * save the original content and restore it in jump_to_IPL_2.
>       */
> -    save_psw = *reset_psw;
> -    *reset_psw = (uint64_t) &jump_to_IPL_addr;
> -    *reset_psw |= RESET_PSW_MASK;
> +    if (address) {
> +        save_psw = *reset_psw;
> +        write_reset_psw(RESET_PSW);
> +    }
>      ipl_continue = address;
>      debug_print_int("set IPL addr to", ipl_continue);

In case you respin this series, I think I'd move the "ipl_continue =
address" into the if-statement, too, and change the debug_print_int line
to use address instead of ipl_continue.

> @@ -77,7 +84,12 @@ 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)) & PSW_MASK_SHORT_ADDR);
> +        /*
> +         * Surely nobody will try running directly from lowcore, so
> +         * let's use 0 as an indication that we want to load the reset
> +         * psw at 0x0 and not jump to the entry.
> +         */
> +        jump_to_IPL_code(0);
>      }
>  
>      /* No other option left, so use the Linux kernel start address */
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 36b884cced..7090720422 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr);
>  void zipl_load(void);
>  
>  /* jump2ipl.c */
> +void write_reset_psw(uint64_t psw);
>  void jump_to_IPL_code(uint64_t address);
>  void jump_to_low_kernel(void);

Looks sane to me:

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

Re: [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them

[Thomas Huth]

On 31/08/2020 17.09, Janosch Frank wrote:
> Currently we always overwrite the mentioned exception new PSWs before
> loading the enabled wait PSW. Let's save the PSW before overwriting
> and restore it right before starting the loaded kernel.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c |  4 +++
>  pc-bios/s390-ccw/netmain.c  |  3 ++
>  pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
>  3 files changed, 52 insertions(+), 17 deletions(-)

Patch looks basically fine to me, I just got some questions for my
understanding below...

> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 5b8352d257..bb94ba7550 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -14,6 +14,7 @@
>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>  #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
>  
> +extern uint64_t psw_save_io[], psw_save_ext[];
>  static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>  
>  void write_reset_psw(uint64_t psw)
> @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address)
>      /* Ensure the guest output starts fresh */
>      sclp_print("\n");
>  
> +    memcpy(&lowcore->io_new_psw, psw_save_io, 16);
> +    memcpy(&lowcore->external_new_psw, psw_save_ext, 16);
> +
>      /*
>       * HACK ALERT.
>       * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index 056e93a818..74ef28fbc6 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -32,6 +32,7 @@
>  #include <time.h>
>  #include <pxelinux.h>
>  
> +#include "s390-arch.h"
>  #include "s390-ccw.h"
>  #include "cio.h"
>  #include "virtio.h"
> @@ -43,6 +44,8 @@
>  extern char _start[];
>  void write_iplb_location(void) {}
>  
> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
> +
>  #define KERNEL_ADDR             ((void *)0L)
>  #define KERNEL_MAX_SIZE         ((long)_start)
>  #define ARCH_COMMAND_LINE_SIZE  896              /* Taken from Linux kernel */
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index ce519300a1..939aac3a7c 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -34,7 +34,17 @@ remainder:
>  	larl	%r2,memsetxc
>  	ex	%r3,0(%r2)
>  done:
> -	j      main		/* And call C */
> +        /* prepare i/o call handler */
> +        larl  %r1, io_new_code
> +        larl  %r2, io_new_psw
> +        stg   %r1, 8(%r2)
> +        mvc   0x1f0(16),0(%r2)
> +        /* prepare external call handler */
> +        larl  %r1, external_new_code
> +        larl  %r2, external_new_psw
> +        stg   %r1, 8(%r2)

Can't you specify the external_new_code and io_new_code in the
external_new_psw / io_new_psw directly? Or is our relocation code not
good enough for this?

> +        mvc   0x1b0(16),0(%r2)
> +        j      main		/* And call C */
>  
>  memsetxc:
>  	xc	0(1,%r1),0(%r1)
> @@ -64,13 +74,16 @@ consume_sclp_int:
>          oi      6(%r15),0x2
>          lctlg   %c0,%c0,0(%r15)
>          /* prepare external call handler */
> -        larl %r1, external_new_code
> -        stg %r1, 0x1b8
> -        larl %r1, external_new_mask
> -        mvc 0x1b0(8),0(%r1)
> -        /* load enabled wait PSW */
> -        larl %r1, enabled_wait_psw
> -        lpswe 0(%r1)
> +        larl  %r1, external_new_psw
> +        lghi  %r2, 0x1b0
> +        /* Is the BIOS' external new PSW already set? */
> +        clc   0(16, %r1), 0(%r2)
> +        je    load_ewait
> +        /* No, save old PSW and write BIOS PSW */
> +        larl  %r3, psw_save_ext
> +        mvc   0(16, %r3), 0x1b0
> +        mvc   0x1b0(16),0(%r1)
> +        j     load_ewait
>  
>  /*
>   * void consume_io_int(void)
> @@ -84,11 +97,20 @@ consume_io_int:
>          oi    4(%r15), 0xff
>          lctlg %c6,%c6,0(%r15)
>          /* prepare i/o call handler */
> -        larl  %r1, io_new_code
> -        stg   %r1, 0x1f8
> -        larl  %r1, io_new_mask
> -        mvc   0x1f0(8),0(%r1)
> -        /* load enabled wait PSW */
> +        larl  %r1, io_new_psw
> +        lghi  %r2, 0x1f0
> +        /* Is the BIOS' PSW already set? */
> +        larl  %r3, load_ewait
> +        clc   0(16, %r1), 0(%r2)
> +        bcr   8, %r3

Why not a "je load_ewait" again, like in the consume_sclp_int handler?

> +        /* No, save old PSW and write BIOS PSW */
> +        larl  %r3, psw_save_io
> +        mvc   0(16, %r3), 0x1f0
> +        mvc   0x1f0(16),0(%r1)
> +        j     load_ewait
> +
> +load_ewait:
> +        /* PSW is the correct one, time to load the enabled wait PSW */
>          larl  %r1, enabled_wait_psw
>          lpswe 0(%r1)
>  
> @@ -107,11 +129,17 @@ io_new_code:
>          br    %r14
>  
>          .align  8
> +        .globl psw_save_io
> +        .globl psw_save_ext
>  disabled_wait_psw:
>          .quad   0x0002000180000000,0x0000000000000000
>  enabled_wait_psw:
>          .quad   0x0302000180000000,0x0000000000000000
> -external_new_mask:
> -        .quad   0x0000000180000000
> -io_new_mask:
> -        .quad   0x0000000180000000
> +external_new_psw:
> +        .quad   0x0000000180000000,0
> +io_new_psw:
> +        .quad   0x0000000180000000,0
> +psw_save_io:
> +        .quad   0,0
> +psw_save_ext:
> +        .quad   0,0
> 

In case you respin, could you maybe add some local #defines for 0x1f0
and 0x1b0 ?

 Thomas

Re: [PATCH v3 5/5] pc-bios: s390x: Go into disabled wait when encountering a PGM exception

[Thomas Huth]

On 31/08/2020 17.09, Janosch Frank wrote:
> Let's setup a PGM PSW, so we won't load 0s when a program exception
> happens. Instead we'll load a disabled wait PSW.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/start.S | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 939aac3a7c..775b45baeb 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -44,6 +44,9 @@ done:
>          larl  %r2, external_new_psw
>          stg   %r1, 8(%r2)
>          mvc   0x1b0(16),0(%r2)
> +        /* set up a pgm exception disabled wait psw */
> +        larl  %r2, disabled_wait_psw
> +        mvc   0x01d0(16), 0(%r2)
>          j      main		/* And call C */
>  
>  memsetxc:
> 

Sounds like a good idea.

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

Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable

[Janosch Frank]

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

On 9/1/20 6:59 PM, Thomas Huth wrote:
> On 31/08/2020 17.09, Janosch Frank wrote:
>> If a blob provides a reset PSW then we should use it instead of
>> branching to the PSW address and using our own mask.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/bootmap.c  |  3 ++-
>>  pc-bios/s390-ccw/jump2ipl.c | 22 +++++++++++++++++-----
>>  pc-bios/s390-ccw/s390-ccw.h |  1 +
>>  3 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 8747c4ea26..5a03b1eb8b 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte)
>>      IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>>  
>>      /* should not return */
>> -    jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
>> +    write_reset_psw(entry->compdat.load_psw);
>> +    jump_to_IPL_code(0);
>>  }
>>  
>>  static void ipl_scsi(void)
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index b6aad32def..5b8352d257 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -12,15 +12,21 @@
>>  
>>  #define KERN_IMAGE_START 0x010000UL
>>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>> +#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
>>  
>>  static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>  
>> +void write_reset_psw(uint64_t psw)
>> +{
>> +    *reset_psw = psw;
>> +}
>> +
>>  static void jump_to_IPL_addr(void)
>>  {
>>      __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue;
>>  
>>      /* Restore reset PSW */
>> -    *reset_psw = save_psw;
>> +    write_reset_psw(save_psw);
>>  
>>      ipl();
>>      /* should not return */
>> @@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address)
>>       * content of non-BIOS memory after we loaded the guest, so we
>>       * save the original content and restore it in jump_to_IPL_2.
>>       */
>> -    save_psw = *reset_psw;
>> -    *reset_psw = (uint64_t) &jump_to_IPL_addr;
>> -    *reset_psw |= RESET_PSW_MASK;
>> +    if (address) {
>> +        save_psw = *reset_psw;
>> +        write_reset_psw(RESET_PSW);
>> +    }
>>      ipl_continue = address;
>>      debug_print_int("set IPL addr to", ipl_continue);
> 
> In case you respin this series, I think I'd move the "ipl_continue =
> address" into the if-statement, too, and change the debug_print_int line
> to use address instead of ipl_continue.

Hmm, my intention was to always have something printed.
But I guess it would make more sense to print the reset psw addr in the
~address case.


> 
>> @@ -77,7 +84,12 @@ 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)) & PSW_MASK_SHORT_ADDR);
>> +        /*
>> +         * Surely nobody will try running directly from lowcore, so
>> +         * let's use 0 as an indication that we want to load the reset
>> +         * psw at 0x0 and not jump to the entry.
>> +         */
>> +        jump_to_IPL_code(0);
>>      }
>>  
>>      /* No other option left, so use the Linux kernel start address */
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 36b884cced..7090720422 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr);
>>  void zipl_load(void);
>>  
>>  /* jump2ipl.c */
>> +void write_reset_psw(uint64_t psw);
>>  void jump_to_IPL_code(uint64_t address);
>>  void jump_to_low_kernel(void);
> 
> Looks sane to me:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 



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

Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable

[Thomas Huth]

On 02/09/2020 10.46, Janosch Frank wrote:
> On 9/1/20 6:59 PM, Thomas Huth wrote:
>> On 31/08/2020 17.09, Janosch Frank wrote:
>>> If a blob provides a reset PSW then we should use it instead of
>>> branching to the PSW address and using our own mask.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  pc-bios/s390-ccw/bootmap.c  |  3 ++-
>>>  pc-bios/s390-ccw/jump2ipl.c | 22 +++++++++++++++++-----
>>>  pc-bios/s390-ccw/s390-ccw.h |  1 +
>>>  3 files changed, 20 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>>> index 8747c4ea26..5a03b1eb8b 100644
>>> --- a/pc-bios/s390-ccw/bootmap.c
>>> +++ b/pc-bios/s390-ccw/bootmap.c
>>> @@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte)
>>>      IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>>>  
>>>      /* should not return */
>>> -    jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
>>> +    write_reset_psw(entry->compdat.load_psw);
>>> +    jump_to_IPL_code(0);
>>>  }
>>>  
>>>  static void ipl_scsi(void)
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index b6aad32def..5b8352d257 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -12,15 +12,21 @@
>>>  
>>>  #define KERN_IMAGE_START 0x010000UL
>>>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>> +#define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
>>>  
>>>  static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>>  
>>> +void write_reset_psw(uint64_t psw)
>>> +{
>>> +    *reset_psw = psw;
>>> +}
>>> +
>>>  static void jump_to_IPL_addr(void)
>>>  {
>>>      __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue;
>>>  
>>>      /* Restore reset PSW */
>>> -    *reset_psw = save_psw;
>>> +    write_reset_psw(save_psw);
>>>  
>>>      ipl();
>>>      /* should not return */
>>> @@ -43,9 +49,10 @@ void jump_to_IPL_code(uint64_t address)
>>>       * content of non-BIOS memory after we loaded the guest, so we
>>>       * save the original content and restore it in jump_to_IPL_2.
>>>       */
>>> -    save_psw = *reset_psw;
>>> -    *reset_psw = (uint64_t) &jump_to_IPL_addr;
>>> -    *reset_psw |= RESET_PSW_MASK;
>>> +    if (address) {
>>> +        save_psw = *reset_psw;
>>> +        write_reset_psw(RESET_PSW);
>>> +    }
>>>      ipl_continue = address;
>>>      debug_print_int("set IPL addr to", ipl_continue);
>>
>> In case you respin this series, I think I'd move the "ipl_continue =
>> address" into the if-statement, too, and change the debug_print_int line
>> to use address instead of ipl_continue.
> 
> Hmm, my intention was to always have something printed.
> But I guess it would make more sense to print the reset psw addr in the
> ~address case.

I meant to only move the "ipl_continue = address" line and keep the
debug_print_int() at its current place (you just have to replace
ipl_continue there). But you're right, it would make more sense to print
the PSW at address 0 in that case instead.

 Thomas

Re: [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them

[Janosch Frank]

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

On 9/1/20 7:22 PM, Thomas Huth wrote:
> On 31/08/2020 17.09, Janosch Frank wrote:
>> Currently we always overwrite the mentioned exception new PSWs before
>> loading the enabled wait PSW. Let's save the PSW before overwriting
>> and restore it right before starting the loaded kernel.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/jump2ipl.c |  4 +++
>>  pc-bios/s390-ccw/netmain.c  |  3 ++
>>  pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
>>  3 files changed, 52 insertions(+), 17 deletions(-)
> 
> Patch looks basically fine to me, I just got some questions for my
> understanding below...
> 
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index 5b8352d257..bb94ba7550 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -14,6 +14,7 @@
>>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>  #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
>>  
>> +extern uint64_t psw_save_io[], psw_save_ext[];
>>  static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>  
>>  void write_reset_psw(uint64_t psw)
>> @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address)
>>      /* Ensure the guest output starts fresh */
>>      sclp_print("\n");
>>  
>> +    memcpy(&lowcore->io_new_psw, psw_save_io, 16);
>> +    memcpy(&lowcore->external_new_psw, psw_save_ext, 16);
>> +
>>      /*
>>       * HACK ALERT.
>>       * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2
>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>> index 056e93a818..74ef28fbc6 100644
>> --- a/pc-bios/s390-ccw/netmain.c
>> +++ b/pc-bios/s390-ccw/netmain.c
>> @@ -32,6 +32,7 @@
>>  #include <time.h>
>>  #include <pxelinux.h>
>>  
>> +#include "s390-arch.h"
>>  #include "s390-ccw.h"
>>  #include "cio.h"
>>  #include "virtio.h"
>> @@ -43,6 +44,8 @@
>>  extern char _start[];
>>  void write_iplb_location(void) {}
>>  
>> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>> +
>>  #define KERNEL_ADDR             ((void *)0L)
>>  #define KERNEL_MAX_SIZE         ((long)_start)
>>  #define ARCH_COMMAND_LINE_SIZE  896              /* Taken from Linux kernel */
>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>> index ce519300a1..939aac3a7c 100644
>> --- a/pc-bios/s390-ccw/start.S
>> +++ b/pc-bios/s390-ccw/start.S
>> @@ -34,7 +34,17 @@ remainder:
>>  	larl	%r2,memsetxc
>>  	ex	%r3,0(%r2)
>>  done:
>> -	j      main		/* And call C */
>> +        /* prepare i/o call handler */
>> +        larl  %r1, io_new_code
>> +        larl  %r2, io_new_psw
>> +        stg   %r1, 8(%r2)
>> +        mvc   0x1f0(16),0(%r2)
>> +        /* prepare external call handler */
>> +        larl  %r1, external_new_code
>> +        larl  %r2, external_new_psw
>> +        stg   %r1, 8(%r2)
> 
> Can't you specify the external_new_code and io_new_code in the
> external_new_psw / io_new_psw directly? Or is our relocation code not
> good enough for this?

Initially I had some problems with this. I just had another try and it
seems to work well, but as the testing infrastructure doesn't really
work, I can't vouch for that.

> 
>> +        mvc   0x1b0(16),0(%r2)
>> +        j      main		/* And call C */
>>  
>>  memsetxc:
>>  	xc	0(1,%r1),0(%r1)
>> @@ -64,13 +74,16 @@ consume_sclp_int:
>>          oi      6(%r15),0x2
>>          lctlg   %c0,%c0,0(%r15)
>>          /* prepare external call handler */
>> -        larl %r1, external_new_code
>> -        stg %r1, 0x1b8
>> -        larl %r1, external_new_mask
>> -        mvc 0x1b0(8),0(%r1)
>> -        /* load enabled wait PSW */
>> -        larl %r1, enabled_wait_psw
>> -        lpswe 0(%r1)
>> +        larl  %r1, external_new_psw
>> +        lghi  %r2, 0x1b0
>> +        /* Is the BIOS' external new PSW already set? */
>> +        clc   0(16, %r1), 0(%r2)
>> +        je    load_ewait
>> +        /* No, save old PSW and write BIOS PSW */
>> +        larl  %r3, psw_save_ext
>> +        mvc   0(16, %r3), 0x1b0
>> +        mvc   0x1b0(16),0(%r1)
>> +        j     load_ewait
>>  
>>  /*
>>   * void consume_io_int(void)
>> @@ -84,11 +97,20 @@ consume_io_int:
>>          oi    4(%r15), 0xff
>>          lctlg %c6,%c6,0(%r15)
>>          /* prepare i/o call handler */
>> -        larl  %r1, io_new_code
>> -        stg   %r1, 0x1f8
>> -        larl  %r1, io_new_mask
>> -        mvc   0x1f0(8),0(%r1)
>> -        /* load enabled wait PSW */
>> +        larl  %r1, io_new_psw
>> +        lghi  %r2, 0x1f0
>> +        /* Is the BIOS' PSW already set? */
>> +        larl  %r3, load_ewait
>> +        clc   0(16, %r1), 0(%r2)
>> +        bcr   8, %r3
> 
> Why not a "je load_ewait" again, like in the consume_sclp_int handler?

Great question

> 
>> +        /* No, save old PSW and write BIOS PSW */
>> +        larl  %r3, psw_save_io
>> +        mvc   0(16, %r3), 0x1f0
>> +        mvc   0x1f0(16),0(%r1)
>> +        j     load_ewait
>> +
>> +load_ewait:
>> +        /* PSW is the correct one, time to load the enabled wait PSW */
>>          larl  %r1, enabled_wait_psw
>>          lpswe 0(%r1)
>>  
>> @@ -107,11 +129,17 @@ io_new_code:
>>          br    %r14
>>  
>>          .align  8
>> +        .globl psw_save_io
>> +        .globl psw_save_ext
>>  disabled_wait_psw:
>>          .quad   0x0002000180000000,0x0000000000000000
>>  enabled_wait_psw:
>>          .quad   0x0302000180000000,0x0000000000000000
>> -external_new_mask:
>> -        .quad   0x0000000180000000
>> -io_new_mask:
>> -        .quad   0x0000000180000000
>> +external_new_psw:
>> +        .quad   0x0000000180000000,0
>> +io_new_psw:
>> +        .quad   0x0000000180000000,0
>> +psw_save_io:
>> +        .quad   0,0
>> +psw_save_ext:
>> +        .quad   0,0
>>
> 
> In case you respin, could you maybe add some local #defines for 0x1f0
> and 0x1b0 ?

At the top of this file?

> 
>  Thomas
> 



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

Re: [PATCH v3 4/5] pc-bios: s390x: Save io and external new PSWs before overwriting them

[Thomas Huth]

On 02/09/2020 12.30, Janosch Frank wrote:
> On 9/1/20 7:22 PM, Thomas Huth wrote:
>> On 31/08/2020 17.09, Janosch Frank wrote:
>>> Currently we always overwrite the mentioned exception new PSWs before
>>> loading the enabled wait PSW. Let's save the PSW before overwriting
>>> and restore it right before starting the loaded kernel.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  pc-bios/s390-ccw/jump2ipl.c |  4 +++
>>>  pc-bios/s390-ccw/netmain.c  |  3 ++
>>>  pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
>>>  3 files changed, 52 insertions(+), 17 deletions(-)
>>
>> Patch looks basically fine to me, I just got some questions for my
>> understanding below...
>>
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index 5b8352d257..bb94ba7550 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -14,6 +14,7 @@
>>>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>>  #define RESET_PSW ((uint64_t)&jump_to_IPL_addr | RESET_PSW_MASK)
>>>  
>>> +extern uint64_t psw_save_io[], psw_save_ext[];
>>>  static uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>>  
>>>  void write_reset_psw(uint64_t psw)
>>> @@ -59,6 +60,9 @@ void jump_to_IPL_code(uint64_t address)
>>>      /* Ensure the guest output starts fresh */
>>>      sclp_print("\n");
>>>  
>>> +    memcpy(&lowcore->io_new_psw, psw_save_io, 16);
>>> +    memcpy(&lowcore->external_new_psw, psw_save_ext, 16);
>>> +
>>>      /*
>>>       * HACK ALERT.
>>>       * We use the load normal reset to keep r15 unchanged. jump_to_IPL_2
>>> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
>>> index 056e93a818..74ef28fbc6 100644
>>> --- a/pc-bios/s390-ccw/netmain.c
>>> +++ b/pc-bios/s390-ccw/netmain.c
>>> @@ -32,6 +32,7 @@
>>>  #include <time.h>
>>>  #include <pxelinux.h>
>>>  
>>> +#include "s390-arch.h"
>>>  #include "s390-ccw.h"
>>>  #include "cio.h"
>>>  #include "virtio.h"
>>> @@ -43,6 +44,8 @@
>>>  extern char _start[];
>>>  void write_iplb_location(void) {}
>>>  
>>> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>>> +
>>>  #define KERNEL_ADDR             ((void *)0L)
>>>  #define KERNEL_MAX_SIZE         ((long)_start)
>>>  #define ARCH_COMMAND_LINE_SIZE  896              /* Taken from Linux kernel */
>>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>>> index ce519300a1..939aac3a7c 100644
>>> --- a/pc-bios/s390-ccw/start.S
>>> +++ b/pc-bios/s390-ccw/start.S
>>> @@ -34,7 +34,17 @@ remainder:
>>>  	larl	%r2,memsetxc
>>>  	ex	%r3,0(%r2)
>>>  done:
>>> -	j      main		/* And call C */
>>> +        /* prepare i/o call handler */
>>> +        larl  %r1, io_new_code
>>> +        larl  %r2, io_new_psw
>>> +        stg   %r1, 8(%r2)
>>> +        mvc   0x1f0(16),0(%r2)
>>> +        /* prepare external call handler */
>>> +        larl  %r1, external_new_code
>>> +        larl  %r2, external_new_psw
>>> +        stg   %r1, 8(%r2)
>>
>> Can't you specify the external_new_code and io_new_code in the
>> external_new_psw / io_new_psw directly? Or is our relocation code not
>> good enough for this?
> 
> Initially I had some problems with this. I just had another try and it
> seems to work well, but as the testing infrastructure doesn't really
> work, I can't vouch for that.

You could maybe dump the memory in both cases to see whether
external_new_psw and io_new_psw contain the same values before and after
the change?

>> In case you respin, could you maybe add some local #defines for 0x1f0
>> and 0x1b0 ?
> 
> At the top of this file?

Yes, please.

 Thomas