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

[Thomas Huth]

On 08/10/2024 03.15, 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>
> ---
...
> @@ -78,24 +84,30 @@ static void prepare_request(VDev *vdev, const void *cdb, 
> int cdb_size,
>       }
>   }
>   
> -static inline void vs_io_assert(bool term, const char *msg)
> +static inline bool vs_io_assert(bool term, const char *msg)
>   {
> -    if (!term) {
> -        virtio_scsi_verify_response(&resp, msg);
> +    if (!term && !virtio_scsi_verify_response(&resp, msg)) {
> +        return false;
>       }
> +
> +    return true;
>   }
>   
> -static void vs_run(const char *title, VirtioCmd *cmd, VDev *vdev,
> +static int vs_run(const char *title, VirtioCmd *cmd, VDev *vdev,
>                      const void *cdb, int cdb_size,
>                      void *data, uint32_t data_size)
>   {
>       prepare_request(vdev, cdb, cdb_size, data, data_size);
> -    vs_io_assert(virtio_run(vdev, VR_REQUEST, cmd) == 0, title);
> +    if (!vs_io_assert(virtio_run(vdev, VR_REQUEST, cmd) == 0, title)) {
> +        puts(title);

Should there be a "return" with a non-0 value here? ...

> +    }
> +
> +    return 0;
>   }
>   
>   /* SCSI protocol implementation routines */
>   
> -static bool scsi_inquiry(VDev *vdev, uint8_t evpd, uint8_t page,
> +static int scsi_inquiry(VDev *vdev, uint8_t evpd, uint8_t page,
>                            void *data, uint32_t data_size)
>   {
>       ScsiCdbInquiry cdb = {
> @@ -110,12 +122,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 cc = vs_run("inquiry", inquiry,
> +            vdev, &cdb, sizeof(cdb), data, data_size);

... since the caller site now obviously checks for a non-zero value!

> -    return virtio_scsi_response_ok(&resp);
> +    return cc ? cc : virtio_scsi_response_ok(&resp);
>   }
...
> @@ -207,7 +221,7 @@ static int virtio_scsi_locate_device(VDev *vdev)
>       static uint8_t data[16 + 8 * 63];
>       ScsiLunReport *r = (void *) data;
>       ScsiDevice *sdev = vdev->scsi_device;
> -    int i, luns;
> +    int i, cc, luns;
>   
>       /* QEMU has hardcoded channel #0 in many places.
>        * If this hardcoded value is ever changed, we'll need to add code for
> @@ -233,13 +247,21 @@ static int virtio_scsi_locate_device(VDev *vdev)
>           sdev->channel = channel;
>           sdev->target = target;
>           sdev->lun = 0;          /* LUN has to be 0 for REPORT LUNS */
> -        if (!scsi_report_luns(vdev, data, sizeof(data))) {
> +        cc = scsi_report_luns(vdev, data, sizeof(data));
> +        if (cc < 0) {
> +            return cc;
> +        }

By the way, calling a variable "cc" reminds me of the CC of the PSW, so I'd 
expect values from 0 to 3 for this variable. Obviously this was meant to 
convey negative error codes instead, so I'd like to suggest to rename that 
variable to "ret" or something similar instead.

> +        else if (cc == 0) {
>               if (resp.response == VIRTIO_SCSI_S_BAD_TARGET) {
>                   continue;
>               }
>               printf("target 0x%X", target);
> -            virtio_scsi_verify_response(&resp, "SCSI cannot report LUNs");
> +            if (!virtio_scsi_verify_response(&resp, "SCSI cannot report 
> LUNs")) {
> +                return -EIO;
> +            }
>           }

 Thomas