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)) {
+ 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.");
--
2.45.1On 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
© 2016 - 2024 Red Hat, Inc.