Re: [PATCH v4 09/19] pc-bios/s390-ccw: Remove panics from SCSI IPL path

[Thomas Huth <thuth@redhat.com>]

On 17/10/2024 03.47, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Remove panic-on-error from virtio-scsi 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/iplb.h          |   2 +
>   pc-bios/s390-ccw/bootmap.c       |  88 +++++++++++++-----
>   pc-bios/s390-ccw/jump2ipl.c      |   1 +
>   pc-bios/s390-ccw/main.c          |   2 +-
>   pc-bios/s390-ccw/virtio-blkdev.c |   4 +-
>   pc-bios/s390-ccw/virtio-scsi.c   | 147 +++++++++++++++++++++----------
>   6 files changed, 172 insertions(+), 72 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 3758698468..639fa34919 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -94,6 +94,8 @@ struct QemuIplParameters {
>   typedef struct QemuIplParameters QemuIplParameters;
>   
>   extern QemuIplParameters qipl;
> +extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
> +extern bool have_iplb;

Why do you move these to the header file now here? You don't seem to be 
using these in this patch? Should it be done in a later patch?

Also the "extern IplParameterBlock iplb" is already available in this header 
file, no need to add it again.

...
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
> index 6b4a1caf8a..32fa81a247 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -26,7 +26,7 @@ static uint8_t scsi_inquiry_std_response[256];
>   static ScsiInquiryEvpdPages scsi_inquiry_evpd_pages_response;
>   static ScsiInquiryEvpdBl scsi_inquiry_evpd_bl_response;
>   
> -static inline void vs_assert(bool term, const char **msgs)
> +static inline bool vs_assert(bool term, const char **msgs)
>   {
>       if (!term) {
>           int i = 0;
> @@ -35,11 +35,13 @@ static inline void vs_assert(bool term, const char **msgs)
>           while (msgs[i]) {
>               printf("%s", msgs[i++]);
>           }
> -        panic(" !\n");
> +        puts(" !");
>       }
> +
> +    return term;
>   }
>   
> -static void virtio_scsi_verify_response(VirtioScsiCmdResp *resp,
> +static bool virtio_scsi_verify_response(VirtioScsiCmdResp *resp,
>                                           const char *title)
>   {
>       const char *mr[] = {
> @@ -56,8 +58,12 @@ static void virtio_scsi_verify_response(VirtioScsiCmdResp *resp,
>           0
>       };
>   
> -    vs_assert(resp->response == VIRTIO_SCSI_S_OK, mr);
> -    vs_assert(resp->status == CDB_STATUS_GOOD, ms);
> +    if (!vs_assert(resp->response == VIRTIO_SCSI_S_OK, mr) ||
> +                !vs_assert(resp->status == CDB_STATUS_GOOD, ms)) {
> +        return false;
> +    }
> +
> +    return true;

Could be simplified to:

     return vs_assert(resp->response == VIRTIO_SCSI_S_OK, mr) &&
            vs_assert(resp->status == CDB_STATUS_GOOD, ms);

>   }
>   
...
> @@ -110,12 +123,13 @@ static bool scsi_inquiry(VDev *vdev, uint8_t evpd, uint8_t page,
>           { data, data_size, VRING_DESC_F_WRITE },
>       };
>   
> -    vs_run("inquiry", inquiry, vdev, &cdb, sizeof(cdb), data, data_size);
> +    int ret = vs_run("inquiry", inquiry,
> +            vdev, &cdb, sizeof(cdb), data, data_size);

Please indent the second line with the "(" in the previous line.

> -    return virtio_scsi_response_ok(&resp);
> +    return ret ? ret : virtio_scsi_response_ok(&resp);
>   }
...
>           if (r->lun_list_len == 0) {
>               printf("no LUNs for target 0x%X\n", target);
>               continue;
> @@ -283,7 +306,9 @@ int virtio_scsi_read_many(VDev *vdev,
>           data_size = sector_count * virtio_get_block_size() * f;
>           if (!scsi_read_10(vdev, sector * f, sector_count * f, load_addr,
>                             data_size)) {
> -            virtio_scsi_verify_response(&resp, "virtio-scsi:read_many");
> +            if (!virtio_scsi_verify_response(&resp, "virtio-scsi:read_many")) {
> +                return 1;
> +            }
>           }
>           load_addr += data_size;
>           sector += sector_count;
> @@ -352,11 +377,16 @@ static int virtio_scsi_setup(VDev *vdev)
>               uint8_t code = resp.sense[0] & SCSI_SENSE_CODE_MASK;
>               uint8_t sense_key = resp.sense[2] & SCSI_SENSE_KEY_MASK;
>   
> -            IPL_assert(resp.sense_len != 0, "virtio-scsi:setup: no SENSE data");
> +            if (resp.sense_len == 0) {
> +                puts("virtio-scsi: setup: no SENSE data");
> +                return -EINVAL;
> +            }
>   
> -            IPL_assert(retry_test_unit_ready && code == 0x70 &&
> -                       sense_key == SCSI_SENSE_KEY_UNIT_ATTENTION,
> -                       "virtio-scsi:setup: cannot retry");
> +            if (!retry_test_unit_ready || code != 0x70 ||
> +                       sense_key != SCSI_SENSE_KEY_UNIT_ATTENTION) {
> +                puts("virtio-scsi:setup: cannot retry");
> +                return -EIO;
> +            }
>   
>               /* retry on CHECK_CONDITION/UNIT_ATTENTION as it
>                * may not designate a real error, but it may be
> @@ -367,16 +397,22 @@ static int virtio_scsi_setup(VDev *vdev)
>               continue;
>           }
>   
> -        virtio_scsi_verify_response(&resp, "virtio-scsi:setup");
> +        if (!virtio_scsi_verify_response(&resp, "virtio-scsi:setup")) {
> +            return 1;

Phew, there a bunch of places now that return "1" for e.g. response OK, but 
this one here is now using "1" for error? ... that's quite confusing. Could 
we maybe standardize on using negative values for error codes? (and 1/0 or 
true/false only for functions that return OK/not OK ?), i.e. use a negative 
error code here (returning -1 is also fine for me)?

> +        }
>       }
>   
>       /* read and cache SCSI INQUIRY response */
> -    if (!scsi_inquiry(vdev,
> +    ret = scsi_inquiry(vdev,
>                         SCSI_INQUIRY_STANDARD,
>                         SCSI_INQUIRY_STANDARD_NONE,
>                         scsi_inquiry_std_response,
> -                      sizeof(scsi_inquiry_std_response))) {
> -        virtio_scsi_verify_response(&resp, "virtio-scsi:setup:inquiry");
> +                      sizeof(scsi_inquiry_std_response));
> +    if (ret < 1) {
> +        if (ret != 0 || !virtio_scsi_verify_response(&resp,
> +                "virtio-scsi:setup:inquiry")) {
> +            return 1;

dito, use a negative error code here?

> +        }
>       }
>   
>       if (virtio_scsi_inquiry_response_is_cdrom(scsi_inquiry_std_response)) {
> @@ -385,12 +421,16 @@ static int virtio_scsi_setup(VDev *vdev)
>           vdev->scsi_block_size = VIRTIO_ISO_BLOCK_SIZE;
>       }
>   
> -    if (!scsi_inquiry(vdev,
> +    ret = scsi_inquiry(vdev,
>                         SCSI_INQUIRY_EVPD,
>                         SCSI_INQUIRY_EVPD_SUPPORTED_PAGES,
>                         evpd,
> -                      sizeof(*evpd))) {
> -        virtio_scsi_verify_response(&resp, "virtio-scsi:setup:supported_pages");
> +                      sizeof(*evpd));
> +    if (ret < 1) {
> +        if (ret != 0 || !virtio_scsi_verify_response(&resp,
> +                "virtio-scsi:setup:supported_pages")) {
> +            return 1;

dito

> +        }
>       }
>   
>       debug_print_int("EVPD length", evpd->page_length);
> @@ -402,12 +442,16 @@ static int virtio_scsi_setup(VDev *vdev)
>               continue;
>           }
>   
> -        if (!scsi_inquiry(vdev,
> +        ret = scsi_inquiry(vdev,
>                             SCSI_INQUIRY_EVPD,
>                             SCSI_INQUIRY_EVPD_BLOCK_LIMITS,
>                             evpd_bl,
> -                          sizeof(*evpd_bl))) {
> -            virtio_scsi_verify_response(&resp, "virtio-scsi:setup:blocklimits");
> +                          sizeof(*evpd_bl));
> +        if (ret < 1) {
> +            if (ret != 0 || !virtio_scsi_verify_response(&resp,
> +                    "virtio-scsi:setup:blocklimits")) {
> +                return 1;

dito

> +            }
>           }
>   
>           debug_print_int("max transfer", evpd_bl->max_transfer);
> @@ -423,8 +467,12 @@ static int virtio_scsi_setup(VDev *vdev)
>       vdev->max_transfer = MIN_NON_ZERO(VIRTIO_SCSI_MAX_SECTORS,
>                                         vdev->max_transfer);
>   
> -    if (!scsi_read_capacity(vdev, data, data_size)) {
> -        virtio_scsi_verify_response(&resp, "virtio-scsi:setup:read_capacity");
> +    ret = scsi_read_capacity(vdev, data, data_size);
> +    if (ret < 1) {
> +        if (ret != 0 || !virtio_scsi_verify_response(&resp,
> +                "virtio-scsi:setup:read_capacity")) {
> +            return 1;

dito

> +        }
>       }
>       scsi_parse_capacity_report(data, &vdev->scsi_last_block,
>                                  (uint32_t *) &vdev->scsi_block_size);
> @@ -439,10 +487,15 @@ int virtio_scsi_setup_device(SubChannelId schid)
>       vdev->schid = schid;
>       virtio_setup_ccw(vdev);
>   
> -    IPL_assert(vdev->config.scsi.sense_size == VIRTIO_SCSI_SENSE_SIZE,
> -               "Config: sense size mismatch");
> -    IPL_assert(vdev->config.scsi.cdb_size == VIRTIO_SCSI_CDB_SIZE,
> -               "Config: CDB size mismatch");
> +    if (vdev->config.scsi.sense_size != VIRTIO_SCSI_SENSE_SIZE) {
> +        puts("Config: sense size mismatch");
> +        return -EINVAL;
> +    }
> +
> +    if (vdev->config.scsi.cdb_size != VIRTIO_SCSI_CDB_SIZE) {
> +        puts("Config: CDB size mismatch");
> +        return -EINVAL;
> +    }
>   
>       puts("Using virtio-scsi.");
>   

  Thomas