Re: [RFC v5 12/12] pc-bios: s390x: Cleanup jump to ipl code

[Thomas Huth <thuth@redhat.com>]

On 24/06/2020 09.52, 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;

I wonder whether there was a reason for having ipl_continue in the 
lowcore instead of using a simple static function pointer... Christian, 
do you remember?

>   static void jump_to_IPL_2(void)
>   {
> -    ResetInfo *current = 0;
> +    uint64_t *psw_current = 0;

Mhh, what about using uint64_t *psw_current = (uint64_t *)lowcore 
instead, to make it more more explicit?

> -    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;

dito.

> -    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");
> 

The patch sounds like a good idea to me ... but since this code proofed 
to be very fragile in the past, let's wait for Christian to say whether 
there was a good reason for ipl_continue in the lowcore or not.

  Thomas