[PATCH v4 0/4] pc-bios: s390x: Cleanup part 2

[PATCH v4 0/4] 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

CI:
https://gitlab.com/frankja/qemu/-/pipelines/198568601

v4:
	* Dropped ext/io new PSW patch to speed up review

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 (4):
  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: Go into disabled wait when encountering a PGM
    exception

 pc-bios/s390-ccw/bootmap.c  |  9 ++++---
 pc-bios/s390-ccw/bootmap.h  |  7 +++++-
 pc-bios/s390-ccw/jump2ipl.c | 47 ++++++++++++++++++++-----------------
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 pc-bios/s390-ccw/start.S    |  5 +++-
 5 files changed, 43 insertions(+), 26 deletions(-)

-- 
2.25.1

[PATCH v4 1/4] 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 v4 2/4] 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>
Reviewed-by: Thomas Huth <thuth@redhat.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 v4 3/4] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/bootmap.c  |  6 ++++--
 pc-bios/s390-ccw/jump2ipl.c | 26 +++++++++++++++++++-------
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 8747c4ea26..88bd12d5d7 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -273,7 +273,8 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
 
     IPL_assert(bms->entry[i].type == BOOT_SCRIPT_EXEC,
                "Unknown script entry type");
-    jump_to_IPL_code(bms->entry[i].address.load_address); /* no return */
+    write_reset_psw(bms->entry[i].address.load_address); /* no return */
+    jump_to_IPL_code(0); /* no return */
 }
 
 static void ipl_eckd_cdl(void)
@@ -515,7 +516,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..63afa4a349 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,11 +49,12 @@ 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;
-    ipl_continue = address;
-    debug_print_int("set IPL addr to", ipl_continue);
+    if (address) {
+        save_psw = *reset_psw;
+        write_reset_psw(RESET_PSW);
+        ipl_continue = address;
+    }
+    debug_print_int("set IPL addr to", address ? address : *reset_psw & PSW_MASK_SHORT_ADDR);
 
     /* Ensure the guest output starts fresh */
     sclp_print("\n");
@@ -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 v4 4/4] 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/start.S | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index ce519300a1..4d5ad21653 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -34,7 +34,10 @@ remainder:
 	larl	%r2,memsetxc
 	ex	%r3,0(%r2)
 done:
-	j      main		/* And call C */
+        /* set up a pgm exception disabled wait psw */
+        larl	%r2, disabled_wait_psw
+        mvc	0x01d0(16), 0(%r2)
+        j      main		/* And call C */
 
 memsetxc:
 	xc	0(1,%r1),0(%r1)
-- 
2.25.1

Re: [PATCH v4 0/4] pc-bios: s390x: Cleanup part 2

[Thomas Huth]

On 06/10/2020 11.42, Janosch Frank wrote:
> 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
> 
> CI:
> https://gitlab.com/frankja/qemu/-/pipelines/198568601
> 
> v4:
> 	* Dropped ext/io new PSW patch to speed up review

Thanks, I've queued the patches to my s390-ccw-bios branch:

  https://gitlab.com/huth/qemu/-/commits/s390-ccw-bios/

   Thomas

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

[Thomas Huth]

On 06/10/2020 11.42, 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>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   pc-bios/s390-ccw/bootmap.c  |  6 ++++--
>   pc-bios/s390-ccw/jump2ipl.c | 26 +++++++++++++++++++-------
>   pc-bios/s390-ccw/s390-ccw.h |  1 +
>   3 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 8747c4ea26..88bd12d5d7 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -273,7 +273,8 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
>   
>       IPL_assert(bms->entry[i].type == BOOT_SCRIPT_EXEC,
>                  "Unknown script entry type");
> -    jump_to_IPL_code(bms->entry[i].address.load_address); /* no return */
> +    write_reset_psw(bms->entry[i].address.load_address); /* no return */
> +    jump_to_IPL_code(0); /* no return */
>   }
>   
>   static void ipl_eckd_cdl(void)
> @@ -515,7 +516,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..63afa4a349 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,11 +49,12 @@ 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;
> -    ipl_continue = address;
> -    debug_print_int("set IPL addr to", ipl_continue);
> +    if (address) {
> +        save_psw = *reset_psw;
> +        write_reset_psw(RESET_PSW);
> +        ipl_continue = address;
> +    }
> +    debug_print_int("set IPL addr to", address ? address : *reset_psw & PSW_MASK_SHORT_ADDR);

checkpatch.pl complains about a very long line here...
I'll change this line to use the Elvis operator, so that is a little bit 
short, ok?

  Thomas