| Message ID | 20200715094045.381984-3-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 2 | expand |
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> >
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(-)