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

[Thomas Huth <thuth@redhat.com>]

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