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

[Thomas Huth <thuth@redhat.com>]

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