diff mbox series

[v4,07/19] pc-bios/s390-ccw: Remove panics from ISO IPL path

Message ID 20241017014748.829029-8-jrossi@linux.ibm.com (mailing list archive)
State New
Headers show
Series s390x: Add Full Boot Order Support | expand

Commit Message

Jared Rossi Oct. 17, 2024, 1:47 a.m. UTC
From: Jared Rossi <jrossi@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 <jrossi@linux.ibm.com>
---
 pc-bios/s390-ccw/bootmap.h  | 15 +++----
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 pc-bios/s390-ccw/bootmap.c  | 82 ++++++++++++++++++++++++-------------
 3 files changed, 61 insertions(+), 37 deletions(-)

Comments

Thomas Huth Oct. 17, 2024, 7:38 a.m. UTC | #1
On 17/10/2024 03.47, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@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 <jrossi@linux.ibm.com>
> ---
...
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 414c3f1b47..5477cfe228 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 (virtio_read(bswap32(s->load_rba), magic_sec)) {
> +        puts("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);
> @@ -692,28 +694,35 @@ static uint32_t sec_offset[ISO9660_MAX_DIR_DEPTH];
>   /* Remained directory space in bytes */
>   static uint32_t dir_rem[ISO9660_MAX_DIR_DEPTH];
>   
> -static inline uint32_t iso_get_file_size(uint32_t load_rba)
> +static inline long iso_get_file_size(uint32_t load_rba)
>   {
>       IsoVolDesc *vd = (IsoVolDesc *)sec;
>       IsoDirHdr *cur_record = &vd->vd.primary.rootdir;
>       uint8_t *temp = sec + ISO_SECTOR_SIZE;
>       int level = 0;
>   
> -    read_iso_sector(ISO_PRIMARY_VD_SECTOR, sec,
> -                    "Failed to read ISO primary descriptor");
> +    if (virtio_read(ISO_PRIMARY_VD_SECTOR, sec)) {
> +        puts("Failed to read ISO primary descriptor");
> +        return -EIO;
> +    }
> +
>       sec_loc[0] = iso_733_to_u32(cur_record->ext_loc);
>       dir_rem[0] = 0;
>       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;
> +            }
>               if (dir_rem[level] == 0) {
>                   /* Skip self and parent records */
>                   dir_rem[level] = iso_733_to_u32(cur_record->data_len) -
> @@ -758,8 +767,10 @@ static inline uint32_t iso_get_file_size(uint32_t load_rba)
>           if (dir_rem[level] == 0) {
>               /* Nothing remaining */
>               level--;
> -            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;
> +            }
>           }
>       }
>   
> @@ -774,7 +785,7 @@ static void load_iso_bc_entry(IsoBcSection *load)
>        * is padded and ISO_SECTOR_SIZE bytes aligned
>        */
>       uint32_t blks_to_load = bswap16(s.sector_count) >> ET_SECTOR_SHIFT;
> -    uint32_t real_size = iso_get_file_size(bswap32(s.load_rba));
> +    long real_size = iso_get_file_size(bswap32(s.load_rba));
>   
>       if (real_size) {
>           /* Round up blocks to load */

I think this check should now be changed to "real_size > 0" ?
And in the "else" path, the function should return immediately?

  Thomas
diff mbox series

Patch

diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 4a7d8a91f1..09f4e6fb40 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -385,17 +385,14 @@  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,
-                                   const char *errmsg)
-{
-    IPL_assert(virtio_read_many(block_offset, buf, 1) == 0, errmsg);
-}
-
-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)) {
+        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 6abb34e563..3e844abd71 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..5477cfe228 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 (virtio_read(bswap32(s->load_rba), magic_sec)) {
+        puts("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);
@@ -692,28 +694,35 @@  static uint32_t sec_offset[ISO9660_MAX_DIR_DEPTH];
 /* Remained directory space in bytes */
 static uint32_t dir_rem[ISO9660_MAX_DIR_DEPTH];
 
-static inline uint32_t iso_get_file_size(uint32_t load_rba)
+static inline long iso_get_file_size(uint32_t load_rba)
 {
     IsoVolDesc *vd = (IsoVolDesc *)sec;
     IsoDirHdr *cur_record = &vd->vd.primary.rootdir;
     uint8_t *temp = sec + ISO_SECTOR_SIZE;
     int level = 0;
 
-    read_iso_sector(ISO_PRIMARY_VD_SECTOR, sec,
-                    "Failed to read ISO primary descriptor");
+    if (virtio_read(ISO_PRIMARY_VD_SECTOR, sec)) {
+        puts("Failed to read ISO primary descriptor");
+        return -EIO;
+    }
+
     sec_loc[0] = iso_733_to_u32(cur_record->ext_loc);
     dir_rem[0] = 0;
     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;
+            }
             if (dir_rem[level] == 0) {
                 /* Skip self and parent records */
                 dir_rem[level] = iso_733_to_u32(cur_record->data_len) -
@@ -758,8 +767,10 @@  static inline uint32_t iso_get_file_size(uint32_t load_rba)
         if (dir_rem[level] == 0) {
             /* Nothing remaining */
             level--;
-            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;
+            }
         }
     }
 
@@ -774,7 +785,7 @@  static void load_iso_bc_entry(IsoBcSection *load)
      * is padded and ISO_SECTOR_SIZE bytes aligned
      */
     uint32_t blks_to_load = bswap16(s.sector_count) >> ET_SECTOR_SHIFT;
-    uint32_t real_size = iso_get_file_size(bswap32(s.load_rba));
+    long real_size = iso_get_file_size(bswap32(s.load_rba));
 
     if (real_size) {
         /* Round up blocks to load */
@@ -784,9 +795,11 @@  static void load_iso_bc_entry(IsoBcSection *load)
         puts("ISO boot image size could not be verified");
     }
 
-    read_iso_boot_image(bswap32(s.load_rba),
+    if (read_iso_boot_image(bswap32(s.load_rba),
                         (void *)((uint64_t)bswap16(s.load_segment)),
-                        blks_to_load);
+                        blks_to_load)) {
+        return;
+    }
 
     jump_to_low_kernel();
 }
@@ -809,17 +822,18 @@  static uint32_t find_iso_bc(void)
                 return bswap32(et->bc_offset);
             }
         }
-        read_iso_sector(block_num++, sec,
-                        "Failed to read ISO volume descriptor");
+        if (virtio_read(block_num++, sec)) {
+            puts("Failed to read ISO volume descriptor");
+            return 0;
+        }
     }
 
     return 0;
 }
 
-static IsoBcSection *find_iso_bc_entry(void)
+static IsoBcSection *find_iso_bc_entry(uint32_t offset)
 {
     IsoBcEntry *e = (IsoBcEntry *)sec;
-    uint32_t offset = find_iso_bc();
     int i;
     unsigned int loadparm = get_loadparm_index();
 
@@ -827,11 +841,13 @@  static IsoBcSection *find_iso_bc_entry(void)
         return NULL;
     }
 
-    read_iso_sector(offset, sec, "Failed to read El Torito boot catalog");
+    if (virtio_read(offset, sec)) {
+        puts("Failed to read El Torito boot catalog");
+        return NULL;
+    }
 
     if (!is_iso_bc_valid(e)) {
         /* The validation entry is mandatory */
-        panic("No valid boot catalog found!\n");
         return NULL;
     }
 
@@ -851,19 +867,25 @@  static IsoBcSection *find_iso_bc_entry(void)
         }
     }
 
-    panic("No suitable boot entry found on ISO-9660 media!\n");
-
     return NULL;
 }
 
-static void ipl_iso_el_torito(void)
+static int ipl_iso_el_torito(void)
 {
-    IsoBcSection *s = find_iso_bc_entry();
+    uint32_t offset = find_iso_bc();
+    if (!offset) {
+        return 0;
+    }
+
+    IsoBcSection *s = find_iso_bc_entry(offset);
 
     if (s) {
-        load_iso_bc_entry(s);
-        /* no return */
+        load_iso_bc_entry(s); /* only return in error */
+        return 1;
     }
+
+    puts("No suitable boot entry found on ISO-9660 media!");
+    return -EIO;
 }
 
 /**
@@ -893,7 +915,9 @@  static void zipl_load_vblk(void)
         if (blksize != VIRTIO_ISO_BLOCK_SIZE) {
             virtio_assume_iso9660();
         }
-        ipl_iso_el_torito();
+        if (ipl_iso_el_torito()) {
+            return;
+        }
     }
 
     if (blksize != VIRTIO_DASD_DEFAULT_BLOCK_SIZE) {
@@ -907,7 +931,9 @@  static void zipl_load_vscsi(void)
 {
     if (virtio_get_block_size() == VIRTIO_ISO_BLOCK_SIZE) {
         /* Is it an ISO image in non-CD drive? */
-        ipl_iso_el_torito();
+        if (ipl_iso_el_torito()) {
+            return;
+        }
     }
 
     puts("Using guessed DASD geometry.");