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

[PATCH v2 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


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: 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/s390-ccw.h |  1 +
 pc-bios/s390-ccw/start.S    | 65 +++++++++++++++++++++++++++----------
 5 files changed, 91 insertions(+), 39 deletions(-)

-- 
2.25.1

[PATCH v2 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 v2 2/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>
---
 pc-bios/s390-ccw/bootmap.c  |  1 +
 pc-bios/s390-ccw/jump2ipl.c | 48 ++++++++++++++++++++++---------------
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 3 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 8747c4ea26..0df9b3781d 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -515,6 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
     IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
 
     /* should not return */
+    write_reset_psw(entry->compdat.load_psw);
     jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
 }
 
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 767012bf0c..143d027bf7 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -13,20 +13,28 @@
 #define KERN_IMAGE_START 0x010000UL
 #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
 
-typedef struct ResetInfo {
-    uint64_t ipl_psw;
-    uint32_t ipl_continue;
-} ResetInfo;
-
-static ResetInfo save;
+uint64_t *reset_psw = 0, save_psw, ipl_continue;
 
 static void jump_to_IPL_2(void)
 {
-    ResetInfo *current = 0;
+    /* Restore reset PSW and io and external new PSWs */
+    *reset_psw = save_psw;
 
-    void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
-    *current = save;
-    ipl(); /* should not return */
+    /* No reset PSW, let's jump instead. */
+    if (ipl_continue) {
+        void (*ipl)(void) = (void *) (uint64_t) ipl_continue;
+        ipl();
+    }
+
+    /* Reset PSW available, let's load it */
+    asm volatile ("lpsw 0(%0)\n"
+        :  : "a" (0):);
+    /* should not return */
+}
+
+void write_reset_psw(uint64_t psw)
+{
+    *reset_psw = psw;
 }
 
 void jump_to_IPL_code(uint64_t address)
@@ -46,15 +54,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.
      */
-    ResetInfo *current = 0;
+    save_psw = *reset_psw;
+    *reset_psw = (uint64_t) &jump_to_IPL_2;
+    *reset_psw |= RESET_PSW_MASK;
+    ipl_continue = address;
 
-    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);
+    debug_print_int("set IPL addr to", ipl_continue);
 
     /* Ensure the guest output starts fresh */
     sclp_print("\n");
@@ -84,7 +89,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

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

[Thomas Huth]

On 27/08/2020 11.31, 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  |  1 +
>  pc-bios/s390-ccw/jump2ipl.c | 48 ++++++++++++++++++++++---------------
>  pc-bios/s390-ccw/s390-ccw.h |  1 +
>  3 files changed, 31 insertions(+), 19 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 8747c4ea26..0df9b3781d 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -515,6 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>      IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>  
>      /* should not return */
> +    write_reset_psw(entry->compdat.load_psw);
>      jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);

Shouldn't that be jump_to_IPL_code(0) now? Wouldn't it be cleaner to
have a jump_to_IPL_PSW() function instead?

>  }
>  
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 767012bf0c..143d027bf7 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -13,20 +13,28 @@
>  #define KERN_IMAGE_START 0x010000UL
>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>  
> -typedef struct ResetInfo {
> -    uint64_t ipl_psw;
> -    uint32_t ipl_continue;
> -} ResetInfo;
> -
> -static ResetInfo save;
> +uint64_t *reset_psw = 0, save_psw, ipl_continue;

I think the patch would be better readable if you'd split it in two -
first the ResetInfo rework, then the use-reset-PSW-if-available stuff ?

>  static void jump_to_IPL_2(void)
>  {
> -    ResetInfo *current = 0;
> +    /* Restore reset PSW and io and external new PSWs */
> +    *reset_psw = save_psw;

The comment talks about io and external new PSWs ... but where is it in
the code?

> -    void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
> -    *current = save;
> -    ipl(); /* should not return */
> +    /* No reset PSW, let's jump instead. */
> +    if (ipl_continue) {
> +        void (*ipl)(void) = (void *) (uint64_t) ipl_continue;

Is it possible to mark a function pointer with __attribute__((noreturn)) ?

> +        ipl();
> +    }
> +
> +    /* Reset PSW available, let's load it */
> +    asm volatile ("lpsw 0(%0)\n"
> +        :  : "a" (0):);

I've never tried. but maybe you could add __attribute__((noreturn)) to
an inline asm statement, too?

> +    /* should not return */
> +}
> +
> +void write_reset_psw(uint64_t psw)
> +{
> +    *reset_psw = psw;
>  }
>  
>  void jump_to_IPL_code(uint64_t address)
> @@ -46,15 +54,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.
>       */
> -    ResetInfo *current = 0;
> +    save_psw = *reset_psw;
> +    *reset_psw = (uint64_t) &jump_to_IPL_2;
> +    *reset_psw |= RESET_PSW_MASK;
> +    ipl_continue = address;
>  
> -    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);
> +    debug_print_int("set IPL addr to", ipl_continue);
>  
>      /* Ensure the guest output starts fresh */
>      sclp_print("\n");
> @@ -84,7 +89,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);
>  
> 

 Thomas

[PATCH RFC v2 3/4] 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>
---

Maybe we should rather statically allocate a lowcore so we don't dirty
0x0 at all.

---
 pc-bios/s390-ccw/jump2ipl.c |  3 ++
 pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
 2 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 143d027bf7..a44f3ab5b3 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -13,12 +13,15 @@
 #define KERN_IMAGE_START 0x010000UL
 #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
 
+extern uint64_t *psw_save_io, *psw_save_ext;
 uint64_t *reset_psw = 0, save_psw, ipl_continue;
 
 static void jump_to_IPL_2(void)
 {
     /* Restore reset PSW and io and external new PSWs */
     *reset_psw = save_psw;
+    memcpy((void *)0x1f0, psw_save_io, 16);
+    memcpy((void *)0x1b0, psw_save_ext, 16);
 
     /* No reset PSW, let's jump instead. */
     if (ipl_continue) {
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

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

[Thomas Huth]

On 27/08/2020 11.31, 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>
> ---
> 
> Maybe we should rather statically allocate a lowcore so we don't dirty
> 0x0 at all.
> 
> ---
>  pc-bios/s390-ccw/jump2ipl.c |  3 ++
>  pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
>  2 files changed, 48 insertions(+), 17 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 143d027bf7..a44f3ab5b3 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -13,12 +13,15 @@
>  #define KERN_IMAGE_START 0x010000UL
>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>  
> +extern uint64_t *psw_save_io, *psw_save_ext;

I think that should be

 extern uint64_t psw_save_io[], psw_save_ext[];

instead ... otherwise you'll end up with some funny bugs here, won't you?

>  uint64_t *reset_psw = 0, save_psw, ipl_continue;
>  
>  static void jump_to_IPL_2(void)
>  {
>      /* Restore reset PSW and io and external new PSWs */

Ok, now the comment makes sense :-)

>      *reset_psw = save_psw;
> +    memcpy((void *)0x1f0, psw_save_io, 16);
> +    memcpy((void *)0x1b0, psw_save_ext, 16);

Could you use &lowcore->external_new_psw and &lowcore->io_new_psw
instead of the magic numbers?

 Thomas

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

[Janosch Frank]

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

On 8/27/20 2:52 PM, Thomas Huth wrote:
> On 27/08/2020 11.31, 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>
>> ---
>>
>> Maybe we should rather statically allocate a lowcore so we don't dirty
>> 0x0 at all.
>>
>> ---
>>  pc-bios/s390-ccw/jump2ipl.c |  3 ++
>>  pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
>>  2 files changed, 48 insertions(+), 17 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index 143d027bf7..a44f3ab5b3 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -13,12 +13,15 @@
>>  #define KERN_IMAGE_START 0x010000UL
>>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>  
>> +extern uint64_t *psw_save_io, *psw_save_ext;
> 
> I think that should be
> 
>  extern uint64_t psw_save_io[], psw_save_ext[];
> 
> instead ... otherwise you'll end up with some funny bugs here, won't you?

What kind of bugs are you expecting?

> 
>>  uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>  
>>  static void jump_to_IPL_2(void)
>>  {
>>      /* Restore reset PSW and io and external new PSWs */
> 
> Ok, now the comment makes sense :-)
>>      *reset_psw = save_psw;
>> +    memcpy((void *)0x1f0, psw_save_io, 16);
>> +    memcpy((void *)0x1b0, psw_save_ext, 16);
> 
> Could you use &lowcore->external_new_psw and &lowcore->io_new_psw
> instead of the magic numbers?

I can, but that means that I need to declare lowcore in netmain.c as
well as including s390-arch.h

> 
>  Thomas
> 
> 
> 



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

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

[Thomas Huth]

On 27/08/2020 16.30, Janosch Frank wrote:
> On 8/27/20 2:52 PM, Thomas Huth wrote:
>> On 27/08/2020 11.31, 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>
>>> ---
>>>
>>> Maybe we should rather statically allocate a lowcore so we don't dirty
>>> 0x0 at all.
>>>
>>> ---
>>>  pc-bios/s390-ccw/jump2ipl.c |  3 ++
>>>  pc-bios/s390-ccw/start.S    | 62 +++++++++++++++++++++++++++----------
>>>  2 files changed, 48 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>>> index 143d027bf7..a44f3ab5b3 100644
>>> --- a/pc-bios/s390-ccw/jump2ipl.c
>>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>>> @@ -13,12 +13,15 @@
>>>  #define KERN_IMAGE_START 0x010000UL
>>>  #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>>  
>>> +extern uint64_t *psw_save_io, *psw_save_ext;
>>
>> I think that should be
>>
>>  extern uint64_t psw_save_io[], psw_save_ext[];
>>
>> instead ... otherwise you'll end up with some funny bugs here, won't you?
> 
> What kind of bugs are you expecting?

Well, "extern uint64_t var[];" and "extern uint64_t *var;" are two
different kind of things. One is an array, one is a pointer variable.
Looking at your assembler code, you obviously tried to declare an array
there, not a pointer variable.

Have a try with this test program:

 #include <string.h>

 extern unsigned long *var;

 void main(void)
 {
	asm volatile (" nop ; nop ; nop "); /* marker */
	memcpy((void *)0x1f0, var, 16);
	asm volatile (" nop ; nop ; nop "); /* marker */
 }

After compiling that with -O2, and disassembling the corresponding .o
file, I get this code between the nops:

   c:	c4 18 00 00 00 00 	lgrl	%r1,c <main+0xc>
			e: R_390_PC32DBL	var+0x2
  12:	e7 00 10 00 00 06 	vl	%v0,0(%r1)
  18:	e7 00 01 f0 00 0e 	vst	%v0,496

The "lgrl %r1,var" is likely not what you wanted here.

If you now replace the "*var" with "var[]", you get this instead:

   c:	c0 10 00 00 00 00 	larl	%r1,c <main+0xc>
			e: R_390_PC32DBL	var+0x2
  12:	e7 00 10 00 00 06 	vl	%v0,0(%r1)
  18:	e7 00 01 f0 00 0e 	vst	%v0,496

"larl" looks better now, doesn't it?

>>
>>>  uint64_t *reset_psw = 0, save_psw, ipl_continue;
>>>  
>>>  static void jump_to_IPL_2(void)
>>>  {
>>>      /* Restore reset PSW and io and external new PSWs */
>>
>> Ok, now the comment makes sense :-)
>>>      *reset_psw = save_psw;
>>> +    memcpy((void *)0x1f0, psw_save_io, 16);
>>> +    memcpy((void *)0x1b0, psw_save_ext, 16);
>>
>> Could you use &lowcore->external_new_psw and &lowcore->io_new_psw
>> instead of the magic numbers?
> 
> I can, but that means that I need to declare lowcore in netmain.c as
> well as including s390-arch.h

If that does not cause any other big hurdles, I think I'd prefer that
instead of using magic numbers.

 Thanks,
  Thomas

[PATCH v2 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>
---
 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 v2 4/4] pc-bios: s390x: Go into disabled wait when encountering a PGM exception

[Christian Borntraeger]

On 27.08.20 11:31, 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.

Makes sense

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>


> 
> Signed-off-by: Janosch Frank <frankja@linux.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:
>