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

[Jared Rossi]

On 9/27/24 11:02 AM, Thomas Huth wrote:
> 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?

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