| Message ID | 20240927005117.1679506-8-jrossi@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add Full Boot Order Support | expand |
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
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 >
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
>>>> + 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...? > Hmm, I think you are sufficiently caffeinated... I agree that something is mixed up here. I suspect I got some wires crossed when I was doing the read_iso_sector() rework/removal. I will double check all of the IPL_assert() reworks and uniformly remove the read_iso_sector() calls so it is consistent in that regard. I’ll also take a closer look at my test coverage, because the mismatched error conditions should always cause incorrect IPL behavior here, which seems to indicate this code path was not properly exercised... Jared Rossi
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)) { + 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; + } if (dir_rem[level] == 0) { /* Skip self and parent records */ dir_rem[level] = iso_733_to_u32(cur_record->data_len) - @@ -784,9 +790,11 @@ static void load_iso_bc_entry(IsoBcSection *load) puts("ISO boot image size could not be verified"); } - read_iso_boot_image(bswap32(s.load_rba), + if (read_iso_boot_image(bswap32(s.load_rba), (void *)((uint64_t)bswap16(s.load_segment)), - blks_to_load); + blks_to_load)) { + return; + } jump_to_low_kernel(); } @@ -809,17 +817,18 @@ static uint32_t find_iso_bc(void) return bswap32(et->bc_offset); } } - read_iso_sector(block_num++, sec, - "Failed to read ISO volume descriptor"); + if (read_iso_sector(block_num++, sec, + "Failed to read ISO volume descriptor")) { + return 0; + } } return 0; } -static IsoBcSection *find_iso_bc_entry(void) +static IsoBcSection *find_iso_bc_entry(uint32_t offset) { IsoBcEntry *e = (IsoBcEntry *)sec; - uint32_t offset = find_iso_bc(); int i; unsigned int loadparm = get_loadparm_index(); @@ -827,11 +836,12 @@ static IsoBcSection *find_iso_bc_entry(void) return NULL; } - read_iso_sector(offset, sec, "Failed to read El Torito boot catalog"); + if (read_iso_sector(offset, sec, "Failed to read El Torito boot catalog")) { + return NULL; + } if (!is_iso_bc_valid(e)) { /* The validation entry is mandatory */ - panic("No valid boot catalog found!\n"); return NULL; } @@ -851,19 +861,25 @@ static IsoBcSection *find_iso_bc_entry(void) } } - panic("No suitable boot entry found on ISO-9660 media!\n"); - return NULL; } -static void ipl_iso_el_torito(void) +static int ipl_iso_el_torito(void) { - IsoBcSection *s = find_iso_bc_entry(); + uint32_t offset = find_iso_bc(); + if (!offset) { + return 0; + } + + IsoBcSection *s = find_iso_bc_entry(offset); if (s) { - load_iso_bc_entry(s); - /* no return */ + load_iso_bc_entry(s); /* only return in error */ + return 1; } + + puts("No suitable boot entry found on ISO-9660 media!"); + return -EIO; } /** @@ -893,7 +909,9 @@ static void zipl_load_vblk(void) if (blksize != VIRTIO_ISO_BLOCK_SIZE) { virtio_assume_iso9660(); } - ipl_iso_el_torito(); + if (ipl_iso_el_torito()) { + return; + } } if (blksize != VIRTIO_DASD_DEFAULT_BLOCK_SIZE) { @@ -907,7 +925,9 @@ static void zipl_load_vscsi(void) { if (virtio_get_block_size() == VIRTIO_ISO_BLOCK_SIZE) { /* Is it an ISO image in non-CD drive? */ - ipl_iso_el_torito(); + if (ipl_iso_el_torito()) { + return; + } } puts("Using guessed DASD geometry.");