Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic

[Christian Borntraeger <borntraeger@linux.ibm.com>]

Am 05.06.24 um 15:37 schrieb Thomas Huth:
> On 29/05/2024 17.43, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> On a panic during IPL (i.e. a device failed to boot) check for another device
>> to boot from, as indicated by the presence of an unused IPLB.
>>
>> If an IPLB is successfully loaded, then jump to the start of BIOS, restarting
>> IPL using the updated IPLB.  Otherwise enter disabled wait.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>>   docs/system/bootindex.rst         | 7 ++++---
>>   docs/system/s390x/bootdevices.rst | 9 ++++++---
>>   pc-bios/s390-ccw/s390-ccw.h       | 6 ++++++
>>   3 files changed, 16 insertions(+), 6 deletions(-)
> 
> Could you please split the documentation changes into a separate patch in v2 ? ... I think that would be cleaner.
> 
>> diff --git a/docs/system/bootindex.rst b/docs/system/bootindex.rst
>> index 8b057f812f..de597561bd 100644
>> --- a/docs/system/bootindex.rst
>> +++ b/docs/system/bootindex.rst
>> @@ -50,9 +50,10 @@ Limitations
>>   Some firmware has limitations on which devices can be considered for
>>   booting.  For instance, the PC BIOS boot specification allows only one
>> -disk to be bootable.  If boot from disk fails for some reason, the BIOS
>> -won't retry booting from other disk.  It can still try to boot from
>> -floppy or net, though.
>> +disk to be bootable, except for on s390x machines. If boot from disk fails for
>> +some reason, the BIOS won't retry booting from other disk.  It can still try to
>> +boot from floppy or net, though.  In the case of s390x, the BIOS will try up to
>> +8 total devices, any number of which may be disks.
> 
> Since the old text was already talking about "PC BIOS", I'd rather leave that paragraph as it is (maybe just replace "PC BIOS" with "x86 PC BIOS"), and add a separate paragraph afterwards about s390x instead.
> 
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index c977a52b50..de3d1f0d5a 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -43,6 +43,7 @@ typedef unsigned long long u64;
>>   #include "iplb.h"
>>   /* start.s */
>> +extern char _start[];
>>   void disabled_wait(void) __attribute__ ((__noreturn__));
>>   void consume_sclp_int(void);
>>   void consume_io_int(void);
>> @@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
>>   static inline void panic(const char *string)
>>   {
>>       sclp_print(string);
>> +    if (load_next_iplb()) {
>> +        sclp_print("\nTrying next boot device...");
>> +        jump_to_IPL_code((long)_start);
>> +    }
>> +
>>       disabled_wait();
>>   }
> 
> Honestly, I am unsure whether this is a really cool idea or a very ugly hack ... but I think I tend towards the latter, sorry. Jumping back to the startup code might cause various problem, e.g. pre-initialized variables don't get their values reset, causing different behavior when the s390-ccw bios runs a function a second time this way. 

We jump back to _start and to me it looks like that this code does the resetting of bss segment.
So anything that has a zero value this should be fine. But static variables != 0 are indeed tricky.
As far as I can see we do have some of those :-(

So instead of jumping, is there a way that remember somewhere at which device we are and then trigger a re-ipl to reload the BIOS?