On 6/25/20 2:58 PM, Thomas Huth wrote: > 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 <fran...@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?
Sure, that would make it way better to read. > >> - 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. This is a RFC and will need a lot of testing. I guess I'll move patch 11 and 12 of this series into a new one and also fix some more boot related stuff so this becomes less maze like. > > Thomas > >
signature.asc
Description: OpenPGP digital signature