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

[Thomas Huth <thuth@redhat.com>]

On 27/09/2024 02.51, 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>
> 
> ---
>   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?

> +        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?

Apart from that, patch looks fine for me, thanks for doing this clean-up work!

  Thomas