Re: [PATCH 12/18] pc-bios/s390-ccw: Enable failed IPL to return after error

[Thomas Huth <thuth@redhat.com>]

On 27/09/2024 02.51, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Remove panic-on-error from IPL functions such that a return code is propagated
> back to the main IPL calling function (rather than terminating immediately),
> which facilitates possible error recovery in the future.
> 
> A select few panics remain, which indicate fatal non-devices errors that must
> result in termination.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> 
> ---
...
> @@ -1054,16 +1073,19 @@ void zipl_load(void)
>   
>       if (vdev->is_cdrom) {
>           ipl_iso_el_torito();
> -        panic("\n! Cannot IPL this ISO image !\n");
> +        puts("Failed to IPL this ISO image!");
> +        return;
>       }
>   
>       if (virtio_get_device_type() == VIRTIO_ID_NET) {
>           netmain();
> -        panic("\n! Cannot IPL from this network !\n");
> +        puts("Failed to IPL from this network!");
> +        return;
>       }
>   
>       if (ipl_scsi()) {
> -        panic("\n! Cannot IPL this device !\n");
> +        puts("Failed to IPL from this device!");

I'd maybe say "Failed to IPL from this SCSI device" now, just to make sure 
that it is easier to match the message with one of the boot device types later?

> +        return;
>       }
...
>   void jump_to_low_kernel(void)
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 2345432abb..f818bd7210 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -77,6 +77,9 @@ static int is_dev_possibly_bootable(int dev_no, int sch_no)
>   
>       enable_subchannel(blk_schid);
>       cutype = cu_type(blk_schid);
> +    if (cutype == CU_TYPE_UNKNOWN) {
> +        return -EIO;
> +    }
>   
>       /*
>        * Note: we always have to run virtio_is_supported() here to make
> @@ -194,10 +197,10 @@ static void boot_setup(void)
>       have_iplb = store_iplb(&iplb);
>   }
>   
> -static void find_boot_device(void)
> +static bool find_boot_device(void)
>   {
>       VDev *vdev = virtio_get_device();
> -    bool found;
> +    bool found = false;
>   
>       switch (iplb.pbt) {
>       case S390_IPL_TYPE_CCW:
> @@ -215,10 +218,10 @@ static void find_boot_device(void)
>           found = find_subch(iplb.scsi.devno);
>           break;
>       default:
> -        panic("List-directed IPL not supported yet!\n");
> +        puts("Invalid IPLB");

Maybe rather say "Unsupported IPLB" ? At least the original message sounds 
like it was rather something that has not been implemented yet, and not 
something that is wrong on the disk...?

>       }
>   
> -    IPL_assert(found, "Boot device not found\n");
> +    return found;
>   }

...
>   unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
> @@ -73,13 +73,13 @@ unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list
>       unsigned long addr = (unsigned long)load_addr;
>   
>       if (sec_len != virtio_get_block_size()) {
> -        return -1;
> +        return 0;
>       }
>   
>       printf(".");
>       status = virtio_read_many(sec, (void *)addr, sec_num);
>       if (status) {
> -        panic("I/O Error");
> +        return 0;
>       }
>       addr += sec_num * virtio_get_block_size();

Ah, here's the fix for virtio_load_direct() ... since you changed the call 
site in patch 09 already, I think you should move this hunk to patch 09, too.

> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 8c6b0a8a92..e3fdb95b3c 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -217,16 +217,19 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
>       return 0;
>   }
>   
> -void virtio_setup_ccw(VDev *vdev)
> +int virtio_setup_ccw(VDev *vdev)
>   {
> -    int i, rc, cfg_size = 0;
> +    int i, cfg_size = 0;
>       uint8_t status;
>       struct VirtioFeatureDesc {
>           uint32_t features;
>           uint8_t index;
>       } __attribute__((packed)) feats;
>   
> -    IPL_assert(virtio_is_supported(vdev->schid), "PE");
> +    if (!virtio_is_supported(vdev->schid)) {
> +        puts("PE");

Do you remember what "PE" means here? ... might be a good opportunity to fix 
this error message as well...

> +        return -ENODEV;
> +    }

  Thomas