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

jrossi@linux.ibm.com posted 19 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v3 07/19] pc-bios/s390-ccw: Remove panics from ISO IPL path
Posted by jrossi@linux.ibm.com 1 month, 2 weeks ago
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  | 78 ++++++++++++++++++++++++-------------
 3 files changed, 59 insertions(+), 35 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index bbe2c132aa..d639258eca 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 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..7984de62fe 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);
@@ -699,21 +701,28 @@ static inline uint32_t iso_get_file_size(uint32_t load_rba)
     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;
+            }
         }
     }
 
@@ -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.");
-- 
2.45.1
Re: [PATCH v3 07/19] pc-bios/s390-ccw: Remove panics from ISO IPL path
Posted by Thomas Huth 1 month, 2 weeks ago
On 08/10/2024 03.15, 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..7984de62fe 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);
> @@ -699,21 +701,28 @@ static inline uint32_t iso_get_file_size(uint32_t load_rba)
>       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;
> +    }

iso_get_file_size() seems to return an uint32_t value, so returning a 
negative error code won't work here. I think you either have to change the 
return type to signed "long" and fix the caller site, too, or return 0 for 
errors here?

  Thomas
Re: [PATCH v3 07/19] pc-bios/s390-ccw: Remove panics from ISO IPL path
Posted by Jared Rossi 1 month, 2 weeks ago

On 10/9/24 5:46 AM, Thomas Huth wrote:
> On 08/10/2024 03.15, 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..7984de62fe 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);
>> @@ -699,21 +701,28 @@ static inline uint32_t 
>> iso_get_file_size(uint32_t load_rba)
>>       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;
>> +    }
>
> iso_get_file_size() seems to return an uint32_t value, so returning a 
> negative error code won't work here. I think you either have to change 
> the return type to signed "long" and fix the caller site, too, or 
> return 0 for errors here?
>
>  Thomas
>

Fixed by using "long" as suggested.  Also found and fixed an error with 
real_size < 0 not being handled correctly in a later part of the code 
that used "if (real_size)"

     Jared