| Message ID | 20200624075226.92728-13-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 1 | expand |
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
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 <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? 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 > >
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");
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(-)