On 17/06/2024 01.44, Jared Rossi wrote:
On 6/7/24 1:57 AM, Thomas Huth wrote:
On 05/06/2024 16.48, Jared Rossi wrote:
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. Thus this
sounds very fragile. Could we please try to get things cleaned up
correctly, so that functions return with error codes instead of
panicking when we can continue with another boot device? Even if its
more work right now, I think this will be much more maintainable in the
future.
Thomas
Thanks Thomas, I appreciate your insight. Your hesitation is perfectly
understandable as well. My initial design was like you suggest, where
the functions return instead of panic, but the issue I ran into is that
netboot uses a separate image, which we jump in to at the start of IPL
from a network device (see zipl_load() in pc-bios/s390-ccw/bootmap.c). I
wasn't able to come up with a simple way to return to the main BIOS code
if a netboot fails other than by jumping back. So, it seems to me that
netboot kind of throws a monkeywrench into the basic idea of reworking
the panics into returns.
I'm open to suggestions on a better way to recover from a failed netboot,
and it's certainly possible I've overlooked something, but as far as I
can tell a jump is necessary in that particular case at least. Netboot
could perhaps be handled as a special case where the jump back is
permitted whereas other device types return, but I don't think that
actually solves the main issue.
What are your thoughts on this?
Yes, I agree that jumping is currently required to get back from the
netboot code. So if you could rework your patches in a way that limits the
jumping to a failed netboot, that would be acceptable, I think.
Apart from that: We originally decided to put the netboot code into a
separate binary since the required roms/SLOF module might not always have
been checked out (it needed to be done manually), so we were not able to
compile it in all cases. But nowadays, this is handled in a much nicer
way, the submodule is automatically checked out once you compile the
s390x-softmmu target and have a s390x compiler available, so I wonder
whether we should maybe do the next step and integrate the netboot code
into the main s390-ccw.img now? Anybody got an opinion on this?
Thomas
Hi Thomas,
I would generally defer the decision about integrating the netboot code to
someone with more insight than me, but for what it's worth, I am of the
opinion that if we want to rework all of panics into returns, then it would
make the most sense to also do the integration now so that we can avoid
using jump altogether. Unless I'm missing something simple, I don't think
the panic/return conversion will be trivial, and actually I think it will be
quite invasive since there are dozens of calls to panic and assert that will
need to be changed. It doesn't seem worthwhile to do all of these
conversions in order to avoid using jump, but then still being exposed to
possible problems caused by jumping due to netboot requiring it anyway.
Agreed, we should either do it right and merge the two binaries, or it does
not make too much sense to only partly convert the code.
I can look into merging the two binaries, but it might also take some time.
So for the time being, I'm fine if we include the panic-jumping hack for
now, we can still then clean it up later.
Thomas
