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

[Thomas Huth]

On 27/09/2024 19.15, Jared Rossi wrote:
> 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);

This IPL_assert() made sure that virtio_read_many() returned 0 (for success)...

> > > +    if (virtio_read(block_offset, buf)) {

... so the new code here checks that virtio_read() (which returns the same 
error code as virtio_read_many()) does *not* return 0 to signal that there 
was an error...

> > > +        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!");

... and this IPL_assert() also checks that virtio_read_many() returns 0 for 
success...

> > > +    if (!virtio_read_many(block_offset, load_addr, blks_to_load)) {

... but this code here checks that virtio_read_many() now returns 0 to 
signal that there is an error...

Either I need more coffee or one of the two if-conditions is wrong...?

> > 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.

Why is one of the two if-statements using a "!" and why is one without it?

> > > +        puts("Failed to read boot image!");
> > > +        return 1;
> > > +    }
> > > +    return 0;
> > >   }
> > >     #define ISO9660_MAX_DIR_DEPTH 8
...
> > > @@ -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.

I agree, I also don't see a compelling reason for keeping read_iso_sector() 
anymore.

 Thomas