Re: [PATCH v3 08/19] pc-bios/s390-ccw: Remove panics from ECKD IPL path

[Thomas Huth <thuth@redhat.com>]

On 08/10/2024 03.15, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Remove panic-on-error from ECKD block device IPL 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/s390-ccw.h |   1 +
>   pc-bios/s390-ccw/bootmap.c  | 183 ++++++++++++++++++++++++------------
>   2 files changed, 126 insertions(+), 58 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index cbd92f3671..7516e96a14 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -31,6 +31,7 @@ typedef unsigned long long u64;
>   #define EBUSY   2
>   #define ENODEV  3
>   #define EINVAL  4
> +#define ENOENT  5
>   
>   #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 7984de62fe..266b38c034 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -145,14 +145,17 @@ static block_number_t load_eckd_segments(block_number_t blk, bool ldipl,
>       bool more_data;
>   
>       memset(_bprs, FREE_SPACE_FILLER, sizeof(_bprs));
> -    read_block(blk, bprs, "BPRS read failed");
> +    if (virtio_read(blk, bprs)) {
> +        puts("BPRS read failed");
> +        return -EIO;
> +    }

load_eckd_segments() returns a value of type block_number_t which is an 
unsigned type, so returning a negative error value will likely not work as 
expected...

...
> @@ -317,21 +352,28 @@ static void run_eckd_boot_script(block_number_t bmt_block_nr,
>   
>           do {
>               block_nr = load_eckd_segments(block_nr, ldipl, &address);
> -        } while (block_nr != -1);
> +        } while (block_nr >= 0);

... for example here: block_nr is also of type block_number_t, so the while 
condition is also true for "negative" error codes.

> +        if (block_nr != -ENOENT && block_nr < 0) {

"block_nr < 0" likely also always evaluates to "false".

> +            return ldipl ? 0 : -EIO;
> +        }
>       }
>   
>       if (ldipl && bms->entry[i].type != BOOT_SCRIPT_EXEC) {
>           /* Abort LD-IPL and retry as CCW-IPL */
> -        return;
> +        return 0;
>       }
>   
> -    IPL_assert(bms->entry[i].type == BOOT_SCRIPT_EXEC,
> -               "Unknown script entry type");
> +    if (bms->entry[i].type != BOOT_SCRIPT_EXEC) {
> +        puts("Unknown script entry type");
> +        return -EINVAL;
> +    }
>       write_reset_psw(bms->entry[i].address.load_address); /* no return */
>       jump_to_IPL_code(0); /* no return */
> +    return 1;
>   }
...
> @@ -441,11 +490,14 @@ static block_number_t eckd_find_bmt(ExtEckdBlockPtr *ptr)
>       BootRecord *br;
>   
>       blockno = gen_eckd_block_num(ptr, 0);
> -    read_block(blockno, tmp_sec, "Cannot read boot record");
> +    if (virtio_read(blockno, tmp_sec)) {
> +        puts("Cannot read boot record");
> +        return -EIO;

Same problem here: Returning a negative error code via unsigned 
block_number_t ....

...
> @@ -490,34 +545,46 @@ static void ipl_eckd(void)
>           ldipl_bmt = eckd_find_bmt((ExtEckdBlockPtr *)&vlbl->f.br);
>           if (ldipl_bmt) {

... will cause trouble here.

>               puts("List-Directed");
> -            /* LD-IPL does not use the S1B bock, just make it NULL */
> -            run_eckd_boot_script(ldipl_bmt, NULL_BLOCK_NR);
> -            /* Only return in error, retry as CCW-IPL */
> +            /*
> +             * LD-IPL does not use the S1B bock, just make it NULL_BLOCK_NR.
> +             * In some failure cases retry IPL before aborting.
> +             */
> +            if (run_eckd_boot_script(ldipl_bmt, NULL_BLOCK_NR)) {
> +                return -EIO;
> +            }
> +            /* Non-fatal error, retry as CCW-IPL */
>               printf("Retrying IPL ");
>               print_eckd_msg();
>           }
>           memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> -        read_block(2, vtoc, "Cannot read block 2");
> +        if (virtio_read(2, vtoc)) {
> +            puts("Cannot read block 2");
> +            return -EIO;
> +        }
>       }
>   
>       /* Not list-directed */
>       if (magic_match(vtoc->magic, VOL1_MAGIC)) {
> -        ipl_eckd_cdl(); /* may return in error */
> +        if (ipl_eckd_cdl()) {
> +            return 1;
> +        }
>       }
>   
>       if (magic_match(vtoc->magic, CMS1_MAGIC)) {
> -        ipl_eckd_ldl(ECKD_CMS); /* no return */
> +        return ipl_eckd_ldl(ECKD_CMS);
>       }
>       if (magic_match(vtoc->magic, LNX1_MAGIC)) {
> -        ipl_eckd_ldl(ECKD_LDL); /* no return */
> +        return ipl_eckd_ldl(ECKD_LDL);
>       }
>   
> -    ipl_eckd_ldl(ECKD_LDL_UNLABELED); /* it still may return */
> +    if (ipl_eckd_ldl(ECKD_LDL_UNLABELED)) {
> +        return 1;
> +    }
>       /*
>        * Ok, it is not a LDL by any means.
>        * It still might be a CDL with zero record keys for IPL1 and IPL2
>        */
> -    ipl_eckd_cdl();
> +    return ipl_eckd_cdl();
>   }

  Thomas