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

[Thomas Huth]

On 17/06/2024 16.49, Christian Borntraeger wrote:
> 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?

If there is an easy way, this could maybe an option, but in the long run, 
I'd really prefer if we'd merge the binaries and get rid of such tricks, 
since this makes the code flow quite hard to understand and maybe also more 
difficult to debug if you run into problems later.

 Thomas