Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable
On 02/09/2020 10.46, Janosch Frank wrote: > On 9/1/20 6:59 PM, Thomas Huth wrote: >> On 31/08/2020 17.09, Janosch Frank wrote: >>> If a blob provides a reset PSW then we should use it instead of >>> branching to the PSW address and using our own mask. >>> >>> Signed-off-by: Janosch Frank >>> --- >>> pc-bios/s390-ccw/bootmap.c | 3 ++- >>> pc-bios/s390-ccw/jump2ipl.c | 22 +- >>> pc-bios/s390-ccw/s390-ccw.h | 1 + >>> 3 files changed, 20 insertions(+), 6 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >>> index 8747c4ea26..5a03b1eb8b 100644 >>> --- a/pc-bios/s390-ccw/bootmap.c >>> +++ b/pc-bios/s390-ccw/bootmap.c >>> @@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte) >>> IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC >>> entry"); >>> >>> /* should not return */ >>> -jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR); >>> +write_reset_psw(entry->compdat.load_psw); >>> +jump_to_IPL_code(0); >>> } >>> >>> static void ipl_scsi(void) >>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >>> index b6aad32def..5b8352d257 100644 >>> --- a/pc-bios/s390-ccw/jump2ipl.c >>> +++ b/pc-bios/s390-ccw/jump2ipl.c >>> @@ -12,15 +12,21 @@ >>> >>> #define KERN_IMAGE_START 0x01UL >>> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >>> +#define RESET_PSW ((uint64_t)_to_IPL_addr | RESET_PSW_MASK) >>> >>> static uint64_t *reset_psw = 0, save_psw, ipl_continue; >>> >>> +void write_reset_psw(uint64_t psw) >>> +{ >>> +*reset_psw = psw; >>> +} >>> + >>> static void jump_to_IPL_addr(void) >>> { >>> __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; >>> >>> /* Restore reset PSW */ >>> -*reset_psw = save_psw; >>> +write_reset_psw(save_psw); >>> >>> ipl(); >>> /* should not return */ >>> @@ -43,9 +49,10 @@ 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. >>> */ >>> -save_psw = *reset_psw; >>> -*reset_psw = (uint64_t) _to_IPL_addr; >>> -*reset_psw |= RESET_PSW_MASK; >>> +if (address) { >>> +save_psw = *reset_psw; >>> +write_reset_psw(RESET_PSW); >>> +} >>> ipl_continue = address; >>> debug_print_int("set IPL addr to", ipl_continue); >> >> In case you respin this series, I think I'd move the "ipl_continue = >> address" into the if-statement, too, and change the debug_print_int line >> to use address instead of ipl_continue. > > Hmm, my intention was to always have something printed. > But I guess it would make more sense to print the reset psw addr in the > ~address case. I meant to only move the "ipl_continue = address" line and keep the debug_print_int() at its current place (you just have to replace ipl_continue there). But you're right, it would make more sense to print the PSW at address 0 in that case instead. Thomas
Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable
On 9/1/20 6:59 PM, Thomas Huth wrote: > On 31/08/2020 17.09, Janosch Frank wrote: >> If a blob provides a reset PSW then we should use it instead of >> branching to the PSW address and using our own mask. >> >> Signed-off-by: Janosch Frank >> --- >> pc-bios/s390-ccw/bootmap.c | 3 ++- >> pc-bios/s390-ccw/jump2ipl.c | 22 +- >> pc-bios/s390-ccw/s390-ccw.h | 1 + >> 3 files changed, 20 insertions(+), 6 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >> index 8747c4ea26..5a03b1eb8b 100644 >> --- a/pc-bios/s390-ccw/bootmap.c >> +++ b/pc-bios/s390-ccw/bootmap.c >> @@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte) >> IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC >> entry"); >> >> /* should not return */ >> -jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR); >> +write_reset_psw(entry->compdat.load_psw); >> +jump_to_IPL_code(0); >> } >> >> static void ipl_scsi(void) >> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c >> index b6aad32def..5b8352d257 100644 >> --- a/pc-bios/s390-ccw/jump2ipl.c >> +++ b/pc-bios/s390-ccw/jump2ipl.c >> @@ -12,15 +12,21 @@ >> >> #define KERN_IMAGE_START 0x01UL >> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) >> +#define RESET_PSW ((uint64_t)_to_IPL_addr | RESET_PSW_MASK) >> >> static uint64_t *reset_psw = 0, save_psw, ipl_continue; >> >> +void write_reset_psw(uint64_t psw) >> +{ >> +*reset_psw = psw; >> +} >> + >> static void jump_to_IPL_addr(void) >> { >> __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; >> >> /* Restore reset PSW */ >> -*reset_psw = save_psw; >> +write_reset_psw(save_psw); >> >> ipl(); >> /* should not return */ >> @@ -43,9 +49,10 @@ 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. >> */ >> -save_psw = *reset_psw; >> -*reset_psw = (uint64_t) _to_IPL_addr; >> -*reset_psw |= RESET_PSW_MASK; >> +if (address) { >> +save_psw = *reset_psw; >> +write_reset_psw(RESET_PSW); >> +} >> ipl_continue = address; >> debug_print_int("set IPL addr to", ipl_continue); > > In case you respin this series, I think I'd move the "ipl_continue = > address" into the if-statement, too, and change the debug_print_int line > to use address instead of ipl_continue. Hmm, my intention was to always have something printed. But I guess it would make more sense to print the reset psw addr in the ~address case. > >> @@ -77,7 +84,12 @@ void jump_to_low_kernel(void) >> >> /* Trying to get PSW at zero address */ >> if (*((uint64_t *)0) & RESET_PSW_MASK) { >> -jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR); >> +/* >> + * Surely nobody will try running directly from lowcore, so >> + * let's use 0 as an indication that we want to load the reset >> + * psw at 0x0 and not jump to the entry. >> + */ >> +jump_to_IPL_code(0); >> } >> >> /* No other option left, so use the Linux kernel start address */ >> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h >> index 36b884cced..7090720422 100644 >> --- a/pc-bios/s390-ccw/s390-ccw.h >> +++ b/pc-bios/s390-ccw/s390-ccw.h >> @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr); >> void zipl_load(void); >> >> /* jump2ipl.c */ >> +void write_reset_psw(uint64_t psw); >> void jump_to_IPL_code(uint64_t address); >> void jump_to_low_kernel(void); > > Looks sane to me: > > Reviewed-by: Thomas Huth > > signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable
On 31/08/2020 17.09, Janosch Frank wrote: > If a blob provides a reset PSW then we should use it instead of > branching to the PSW address and using our own mask. > > Signed-off-by: Janosch Frank > --- > pc-bios/s390-ccw/bootmap.c | 3 ++- > pc-bios/s390-ccw/jump2ipl.c | 22 +- > pc-bios/s390-ccw/s390-ccw.h | 1 + > 3 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index 8747c4ea26..5a03b1eb8b 100644 > --- a/pc-bios/s390-ccw/bootmap.c > +++ b/pc-bios/s390-ccw/bootmap.c > @@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte) > IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC > entry"); > > /* should not return */ > -jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR); > +write_reset_psw(entry->compdat.load_psw); > +jump_to_IPL_code(0); > } > > static void ipl_scsi(void) > diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c > index b6aad32def..5b8352d257 100644 > --- a/pc-bios/s390-ccw/jump2ipl.c > +++ b/pc-bios/s390-ccw/jump2ipl.c > @@ -12,15 +12,21 @@ > > #define KERN_IMAGE_START 0x01UL > #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) > +#define RESET_PSW ((uint64_t)_to_IPL_addr | RESET_PSW_MASK) > > static uint64_t *reset_psw = 0, save_psw, ipl_continue; > > +void write_reset_psw(uint64_t psw) > +{ > +*reset_psw = psw; > +} > + > static void jump_to_IPL_addr(void) > { > __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; > > /* Restore reset PSW */ > -*reset_psw = save_psw; > +write_reset_psw(save_psw); > > ipl(); > /* should not return */ > @@ -43,9 +49,10 @@ 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. > */ > -save_psw = *reset_psw; > -*reset_psw = (uint64_t) _to_IPL_addr; > -*reset_psw |= RESET_PSW_MASK; > +if (address) { > +save_psw = *reset_psw; > +write_reset_psw(RESET_PSW); > +} > ipl_continue = address; > debug_print_int("set IPL addr to", ipl_continue); In case you respin this series, I think I'd move the "ipl_continue = address" into the if-statement, too, and change the debug_print_int line to use address instead of ipl_continue. > @@ -77,7 +84,12 @@ void jump_to_low_kernel(void) > > /* Trying to get PSW at zero address */ > if (*((uint64_t *)0) & RESET_PSW_MASK) { > -jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR); > +/* > + * Surely nobody will try running directly from lowcore, so > + * let's use 0 as an indication that we want to load the reset > + * psw at 0x0 and not jump to the entry. > + */ > +jump_to_IPL_code(0); > } > > /* No other option left, so use the Linux kernel start address */ > diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h > index 36b884cced..7090720422 100644 > --- a/pc-bios/s390-ccw/s390-ccw.h > +++ b/pc-bios/s390-ccw/s390-ccw.h > @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr); > void zipl_load(void); > > /* jump2ipl.c */ > +void write_reset_psw(uint64_t psw); > void jump_to_IPL_code(uint64_t address); > void jump_to_low_kernel(void); Looks sane to me: Reviewed-by: Thomas Huth
[PATCH v3 3/5] pc-bios: s390x: Use reset PSW if avaliable
If a blob provides a reset PSW then we should use it instead of branching to the PSW address and using our own mask. Signed-off-by: Janosch Frank --- pc-bios/s390-ccw/bootmap.c | 3 ++- pc-bios/s390-ccw/jump2ipl.c | 22 +- pc-bios/s390-ccw/s390-ccw.h | 1 + 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c index 8747c4ea26..5a03b1eb8b 100644 --- a/pc-bios/s390-ccw/bootmap.c +++ b/pc-bios/s390-ccw/bootmap.c @@ -515,7 +515,8 @@ static void zipl_run(ScsiBlockPtr *pte) IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry"); /* should not return */ -jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR); +write_reset_psw(entry->compdat.load_psw); +jump_to_IPL_code(0); } static void ipl_scsi(void) diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c index b6aad32def..5b8352d257 100644 --- a/pc-bios/s390-ccw/jump2ipl.c +++ b/pc-bios/s390-ccw/jump2ipl.c @@ -12,15 +12,21 @@ #define KERN_IMAGE_START 0x01UL #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64) +#define RESET_PSW ((uint64_t)_to_IPL_addr | RESET_PSW_MASK) static uint64_t *reset_psw = 0, save_psw, ipl_continue; +void write_reset_psw(uint64_t psw) +{ +*reset_psw = psw; +} + static void jump_to_IPL_addr(void) { __attribute__((noreturn)) void (*ipl)(void) = (void *)ipl_continue; /* Restore reset PSW */ -*reset_psw = save_psw; +write_reset_psw(save_psw); ipl(); /* should not return */ @@ -43,9 +49,10 @@ 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. */ -save_psw = *reset_psw; -*reset_psw = (uint64_t) _to_IPL_addr; -*reset_psw |= RESET_PSW_MASK; +if (address) { +save_psw = *reset_psw; +write_reset_psw(RESET_PSW); +} ipl_continue = address; debug_print_int("set IPL addr to", ipl_continue); @@ -77,7 +84,12 @@ void jump_to_low_kernel(void) /* Trying to get PSW at zero address */ if (*((uint64_t *)0) & RESET_PSW_MASK) { -jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR); +/* + * Surely nobody will try running directly from lowcore, so + * let's use 0 as an indication that we want to load the reset + * psw at 0x0 and not jump to the entry. + */ +jump_to_IPL_code(0); } /* No other option left, so use the Linux kernel start address */ diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h index 36b884cced..7090720422 100644 --- a/pc-bios/s390-ccw/s390-ccw.h +++ b/pc-bios/s390-ccw/s390-ccw.h @@ -78,6 +78,7 @@ int virtio_read(ulong sector, void *load_addr); void zipl_load(void); /* jump2ipl.c */ +void write_reset_psw(uint64_t psw); void jump_to_IPL_code(uint64_t address); void jump_to_low_kernel(void); -- 2.25.1