On 9/27/24 11:02 AM, Thomas Huth wrote:
On 27/09/2024 02.51, jro...@linux.ibm.com wrote:
From: Jared Rossi <jro...@linux.ibm.com>
Remove panic-on-error from IPL ISO El Torito specific functions so
that error
recovery may be possible in the future.
Functions that would previously panic now provide a return code.
Signed-off-by: Jared Rossi <jro...@linux.ibm.com>
---
pc-bios/s390-ccw/bootmap.h | 17 +++++++---
pc-bios/s390-ccw/s390-ccw.h | 1 +
pc-bios/s390-ccw/bootmap.c | 64 ++++++++++++++++++++++++-------------
3 files changed, 55 insertions(+), 27 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index bbe2c132aa..cb5346829b 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -385,17 +385,24 @@ static inline uint32_t iso_733_to_u32(uint64_t x)
#define ISO_PRIMARY_VD_SECTOR 16
-static inline void read_iso_sector(uint32_t block_offset, void *buf,
+static inline int read_iso_sector(uint32_t block_offset, void *buf,
const char *errmsg)
{
- IPL_assert(virtio_read_many(block_offset, buf, 1) == 0, errmsg);
+ if (virtio_read(block_offset, buf)) {
+ puts(errmsg);
+ return 1;
+ }
+ return 0;
}
-static inline void read_iso_boot_image(uint32_t block_offset, void
*load_addr,
+static inline int read_iso_boot_image(uint32_t block_offset, void
*load_addr,
uint32_t blks_to_load)
{
- IPL_assert(virtio_read_many(block_offset, load_addr,
blks_to_load) == 0,
- "Failed to read boot image!");
+ if (!virtio_read_many(block_offset, load_addr, blks_to_load)) {
That "!" looks wrong here? Or do I misunderstood the original
IPL_assert() condition?
Basically all of the IPL_assert() conditions become logically flipped,
but it is
intended. IPL_assert() panics if success condition is NOT met, but in
the new
version an error code is returned if an failure condition IS met, so we are
branching on the inverse condition.
+ puts("Failed to read boot image!");
+ return 1;
+ }
+ return 0;
}
#define ISO9660_MAX_DIR_DEPTH 8
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 0ed7eb8ade..cbd92f3671 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -30,6 +30,7 @@ typedef unsigned long long u64;
#define EIO 1
#define EBUSY 2
#define ENODEV 3
+#define EINVAL 4
#ifndef MIN
#define MIN(a, b) (((a) < (b)) ? (a) : (b))
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 414c3f1b47..31cf0f6d97 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -678,8 +678,10 @@ static bool
is_iso_bc_entry_compatible(IsoBcSection *s)
if (s->unused || !s->sector_count) {
return false;
}
- read_iso_sector(bswap32(s->load_rba), magic_sec,
- "Failed to read image sector 0");
+ if (read_iso_sector(bswap32(s->load_rba), magic_sec,
+ "Failed to read image sector 0")) {
+ return false;
+ }
/* Checking bytes 8 - 32 for S390 Linux magic */
return !memcmp(magic_sec + 8, linux_s390_magic, 24);
@@ -706,14 +708,18 @@ static inline uint32_t
iso_get_file_size(uint32_t load_rba)
sec_offset[0] = 0;
while (level >= 0) {
- IPL_assert(sec_offset[level] <= ISO_SECTOR_SIZE,
- "Directory tree structure violation");
+ if (sec_offset[level] > ISO_SECTOR_SIZE) {
+ puts("Directory tree structure violation");
+ return -EIO;
+ }
cur_record = (IsoDirHdr *)(temp + sec_offset[level]);
if (sec_offset[level] == 0) {
- read_iso_sector(sec_loc[level], temp,
- "Failed to read ISO directory");
+ if (virtio_read(sec_loc[level], temp)) {
+ puts("Failed to read ISO directory");
+ return -EIO;
+ }
Any reasons for switching from read_iso_sector() directly to
virtio_read() here?
I think this is just an oversight on my part. I had thought to remove the
read_iso_sector() function entirely since it is just a wrapper for
virtio_read() that becomes redundant once the panic is removed, but it looks
like I wasn't consistent with where I removed it. In my opinion we can
remove
read_iso_sector() and just call virtio_read(), but either way it should be
consistent, so I will standardize the calls. I don't see any compelling
reason
to keep the read_iso_sector() function since all it is doing is checking the
RC of virtio_read(), which we will want to check anyway to determine if
we need
to abort the IPL here.
Apart from that, patch looks fine for me, thanks for doing this
clean-up work!
Thomas
