jump_to_IPL_code takes a 64 bit address, masks it with the short psw
address mask and later branches to it using a full 64 bit register.

* As the masking is not necessary, let's remove it
* Without the mask we can save the ipl address to a static 64 bit
  function ptr as we later branch to it
* Let's also clean up the variable names and remove the now unneeded
  ResetInfo

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

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 767012bf0c..aef37cea76 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -13,20 +13,15 @@
 #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;
+static void (*ipl_continue)(void);
+static uint64_t psw_save;
 
 static void jump_to_IPL_2(void)
 {
-    ResetInfo *current = 0;
+    uint64_t *psw_current = 0;
 
-    void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
-    *current = save;
-    ipl(); /* should not return */
+    *psw_current = psw_save;
+    ipl_continue(); /* should not return */
 }
 
 void jump_to_IPL_code(uint64_t address)
@@ -46,15 +41,15 @@ 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;
+    uint64_t *psw_current = 0;
 
-    save = *current;
+    psw_save = *psw_current;
 
-    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
-    current->ipl_psw |= RESET_PSW_MASK;
-    current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
+    *psw_current = (uint64_t) &jump_to_IPL_2;
+    *psw_current |= RESET_PSW_MASK;
+    ipl_continue = (void *)address;
 
-    debug_print_int("set IPL addr to", current->ipl_continue);
+    debug_print_int("set IPL addr to", (uint64_t)ipl_continue);
 
     /* Ensure the guest output starts fresh */
     sclp_print("\n");
-- 
2.25.1

On 15/07/2020 11.40, Janosch Frank wrote:
> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
> address mask and later branches to it using a full 64 bit register.
> 
> * As the masking is not necessary, let's remove it
> * Without the mask we can save the ipl address to a static 64 bit
>   function ptr as we later branch to it
> * Let's also clean up the variable names and remove the now unneeded
>   ResetInfo
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 767012bf0c..aef37cea76 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -13,20 +13,15 @@
>  #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;
> +static void (*ipl_continue)(void);
> +static uint64_t psw_save;

Christian, do you remember whether there was a reason that we saved the
"ipl_continue" in the low-core in the past?

The changes here look ok to me, but I still wonder why it has been more
"complicated" before...?

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

On 7/17/20 5:13 PM, Thomas Huth wrote:
> On 15/07/2020 11.40, Janosch Frank wrote:
>> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
>> address mask and later branches to it using a full 64 bit register.
>>
>> * As the masking is not necessary, let's remove it
>> * Without the mask we can save the ipl address to a static 64 bit
>>   function ptr as we later branch to it
>> * Let's also clean up the variable names and remove the now unneeded
>>   ResetInfo
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
>>  1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index 767012bf0c..aef37cea76 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -13,20 +13,15 @@
>>  #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;
>> +static void (*ipl_continue)(void);
>> +static uint64_t psw_save;
> 
> Christian, do you remember whether there was a reason that we saved the
> "ipl_continue" in the low-core in the past?
> 
> The changes here look ok to me, but I still wonder why it has been more
> "complicated" before...?

Unfortunately looking at 962982329029acb6651f81b47cb401e593bb62df where
this was introduced also doesn't clear that up.

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

Thanks!

On 17.07.20 17:13, Thomas Huth wrote:
> On 15/07/2020 11.40, Janosch Frank wrote:
>> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
>> address mask and later branches to it using a full 64 bit register.
>>
>> * As the masking is not necessary, let's remove it
>> * Without the mask we can save the ipl address to a static 64 bit
>>   function ptr as we later branch to it
>> * Let's also clean up the variable names and remove the now unneeded
>>   ResetInfo
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
>>  1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index 767012bf0c..aef37cea76 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -13,20 +13,15 @@
>>  #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;
>> +static void (*ipl_continue)(void);
>> +static uint64_t psw_save;
> 
> Christian, do you remember whether there was a reason that we saved the
> "ipl_continue" in the low-core in the past?
> 
> The changes here look ok to me, but I still wonder why it has been more
> "complicated" before...?

This construct was made to restore the memory outside of the loader to the
original content. The new code should also work I guess. 



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