| Message ID | 20201120160117.59366-3-farman@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | pc-bios/s390 fixes for reboot-to-vfio-ccw | expand |
On 20.11.20 17:01, Eric Farman wrote: > A Linux binary will have the string "S390EP" at address 0x10008, > which is important in getting the guest up off the ground. In the > case of a reboot (specifically chreipl going to a new device), > we should defer to the PSW at address zero for the new config, > which will re-write "S390EP" from the new image. > > Let's clear it out at this point so that a reipl to, say, a DASD > passthrough device drives the IPL path from scratch without disrupting > disrupting the order of operations for other boots. > > Rather than hardcoding the address of this magic (again), let's > define it somewhere so that the two users are visibly related. Hmmm, this might have side effects, e.g. if you do something like a kdump or kexec to a non-Linux binary that happens to have code at 0x10008, no? As far as I can tell, the problem should only happen for a ccw type IPL so why not [...] > --- a/pc-bios/s390-ccw/main.c > +++ b/pc-bios/s390-ccw/main.c > @@ -178,6 +178,12 @@ static void boot_setup(void) > memcpy(lpmsg + 10, loadparm_str, 8); > sclp_print(lpmsg); > > + /* > + * Clear out any potential S390EP magic (see jump_to_low_kernel()), > + * so we don't taint our decision-making process during a reboot. > + */ > + memset((char *)S390EP, 0, 6); move this into find_subch in here: ------------- snip --------------- case CU_TYPE_DASD_3990: case CU_TYPE_DASD_2107: return true; ------------- snip ---------------
On 23/11/2020 08.39, Christian Borntraeger wrote: > On 20.11.20 17:01, Eric Farman wrote: >> A Linux binary will have the string "S390EP" at address 0x10008, >> which is important in getting the guest up off the ground. In the >> case of a reboot (specifically chreipl going to a new device), >> we should defer to the PSW at address zero for the new config, >> which will re-write "S390EP" from the new image. >> >> Let's clear it out at this point so that a reipl to, say, a DASD >> passthrough device drives the IPL path from scratch without disrupting >> disrupting the order of operations for other boots. >> >> Rather than hardcoding the address of this magic (again), let's >> define it somewhere so that the two users are visibly related. > > > Hmmm, this might have side effects, e.g. if you do something like a kdump > or kexec to a non-Linux binary that happens to have code at 0x10008, no? Do these scenarios really go through the s390-ccw bios again, or do they rather bypass the bios and jump directly into the new kernel? > As far as I can tell, the problem should only happen for a ccw type IPL > so why not Not sure whether it really can only happen in these cases... for example, would it also be possible to reboot from a Linux kernel into a kvm-unit-test? ... these also do not have the S390EP magic, IIRC. > [...] >> --- a/pc-bios/s390-ccw/main.c >> +++ b/pc-bios/s390-ccw/main.c >> @@ -178,6 +178,12 @@ static void boot_setup(void) >> memcpy(lpmsg + 10, loadparm_str, 8); >> sclp_print(lpmsg); >> >> + /* >> + * Clear out any potential S390EP magic (see jump_to_low_kernel()), >> + * so we don't taint our decision-making process during a reboot. >> + */ >> + memset((char *)S390EP, 0, 6); > > > move this into find_subch > in here: > ------------- snip --------------- > case CU_TYPE_DASD_3990: > case CU_TYPE_DASD_2107: > return true; > ------------- snip --------------- > That would be is_dev_possibly_bootable() now? ... not sure whether this is the best location... maybe put it better at the beginning of dasd_ipl() instead? Thomas
On 23.11.20 09:05, Thomas Huth wrote: > On 23/11/2020 08.39, Christian Borntraeger wrote: >> On 20.11.20 17:01, Eric Farman wrote: >>> A Linux binary will have the string "S390EP" at address 0x10008, >>> which is important in getting the guest up off the ground. In the >>> case of a reboot (specifically chreipl going to a new device), >>> we should defer to the PSW at address zero for the new config, >>> which will re-write "S390EP" from the new image. >>> >>> Let's clear it out at this point so that a reipl to, say, a DASD >>> passthrough device drives the IPL path from scratch without disrupting >>> disrupting the order of operations for other boots. >>> >>> Rather than hardcoding the address of this magic (again), let's >>> define it somewhere so that the two users are visibly related. >> >> >> Hmmm, this might have side effects, e.g. if you do something like a kdump >> or kexec to a non-Linux binary that happens to have code at 0x10008, no? > > Do these scenarios really go through the s390-ccw bios again, or do they > rather bypass the bios and jump directly into the new kernel? Right they jump directly into the new kernel. So this patch could actually be "good enough" for 5.2 as is?
On 23/11/2020 09.07, Christian Borntraeger wrote: > > > On 23.11.20 09:05, Thomas Huth wrote: >> On 23/11/2020 08.39, Christian Borntraeger wrote: >>> On 20.11.20 17:01, Eric Farman wrote: >>>> A Linux binary will have the string "S390EP" at address 0x10008, >>>> which is important in getting the guest up off the ground. In the >>>> case of a reboot (specifically chreipl going to a new device), >>>> we should defer to the PSW at address zero for the new config, >>>> which will re-write "S390EP" from the new image. >>>> >>>> Let's clear it out at this point so that a reipl to, say, a DASD >>>> passthrough device drives the IPL path from scratch without disrupting >>>> disrupting the order of operations for other boots. >>>> >>>> Rather than hardcoding the address of this magic (again), let's >>>> define it somewhere so that the two users are visibly related. >>> >>> >>> Hmmm, this might have side effects, e.g. if you do something like a kdump >>> or kexec to a non-Linux binary that happens to have code at 0x10008, no? >> >> Do these scenarios really go through the s390-ccw bios again, or do they >> rather bypass the bios and jump directly into the new kernel? > > Right they jump directly into the new kernel. So this patch could actually > be "good enough" for 5.2 as is? I think it should be fine, yes. I'll give it a try with my usual s390-ccw bios tests, and if there are no regressions (and no other objections on the mailing list here), I'll prepare a pull request. Thomas
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c index fbae45b03c..b9c70d64a5 100644 --- a/pc-bios/s390-ccw/jump2ipl.c +++ b/pc-bios/s390-ccw/jump2ipl.c @@ -78,7 +78,7 @@ void jump_to_low_kernel(void) * kernel start address (when jumping to the PSW-at-zero address instead, * the kernel startup code fails when we booted from a network device). */ - if (!memcmp((char *)0x10008, "S390EP", 6)) { + if (!memcmp((char *)S390EP, "S390EP", 6)) { jump_to_IPL_code(KERN_IMAGE_START); } diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c index fc4bfaa455..5d2b7ba94d 100644 --- a/pc-bios/s390-ccw/main.c +++ b/pc-bios/s390-ccw/main.c @@ -178,6 +178,12 @@ static void boot_setup(void) memcpy(lpmsg + 10, loadparm_str, 8); sclp_print(lpmsg); + /* + * Clear out any potential S390EP magic (see jump_to_low_kernel()), + * so we don't taint our decision-making process during a reboot. + */ + memset((char *)S390EP, 0, 6); + have_iplb = store_iplb(&iplb); } diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h index 6da44d4436..a741488aaa 100644 --- a/pc-bios/s390-ccw/s390-arch.h +++ b/pc-bios/s390-ccw/s390-arch.h @@ -95,6 +95,9 @@ typedef struct LowCore { extern LowCore *lowcore; +/* Location of "S390EP" in a Linux binary (see arch/s390/boot/head.S) */ +#define S390EP 0x10008 + static inline void set_prefix(uint32_t address) { asm volatile("spx %0" : : "m" (address) : "memory");
A Linux binary will have the string "S390EP" at address 0x10008, which is important in getting the guest up off the ground. In the case of a reboot (specifically chreipl going to a new device), we should defer to the PSW at address zero for the new config, which will re-write "S390EP" from the new image. Let's clear it out at this point so that a reipl to, say, a DASD passthrough device drives the IPL path from scratch without disrupting disrupting the order of operations for other boots. Rather than hardcoding the address of this magic (again), let's define it somewhere so that the two users are visibly related. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- pc-bios/s390-ccw/jump2ipl.c | 2 +- pc-bios/s390-ccw/main.c | 6 ++++++ pc-bios/s390-ccw/s390-arch.h | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-)