[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 09/19] pc-bios/s390-ccw: Remove panics from SCSI IPL path
|
From: |
Thomas Huth |
|
Subject: |
Re: [PATCH v4 09/19] pc-bios/s390-ccw: Remove panics from SCSI IPL path |
|
Date: |
Thu, 17 Oct 2024 12:28:48 +0200 |
|
User-agent: |
Mozilla Thunderbird |
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
- [PATCH v4 00/19] s390x: Add Full Boot Order Support, jrossi, 2024/10/16
- [PATCH v4 01/19] hw/s390x/ipl: Provide more memory to the s390-ccw.img firmware, jrossi, 2024/10/16
- [PATCH v4 03/19] pc-bios/s390-ccw: Link the netboot code into the main s390-ccw.img binary, jrossi, 2024/10/16
- [PATCH v4 04/19] hw/s390x: Remove the possibility to load the s390-netboot.img binary, jrossi, 2024/10/16
- [PATCH v4 02/19] pc-bios/s390-ccw: Use the libc from SLOF and remove sclp prints, jrossi, 2024/10/16
- [PATCH v4 05/19] pc-bios/s390-ccw: Merge netboot.mak into the main Makefile, jrossi, 2024/10/16
- [PATCH v4 07/19] pc-bios/s390-ccw: Remove panics from ISO IPL path, jrossi, 2024/10/16
- [PATCH v4 09/19] pc-bios/s390-ccw: Remove panics from SCSI IPL path, jrossi, 2024/10/16
- Re: [PATCH v4 09/19] pc-bios/s390-ccw: Remove panics from SCSI IPL path,
Thomas Huth <=
- [PATCH v4 10/19] pc-bios/s390-ccw: Remove panics from DASD IPL path, jrossi, 2024/10/16
- [PATCH v4 06/19] docs/system/s390x/bootdevices: Update the documentation about network booting, jrossi, 2024/10/16
- [PATCH v4 08/19] pc-bios/s390-ccw: Remove panics from ECKD IPL path, jrossi, 2024/10/16
- [PATCH v4 13/19] include/hw/s390x: Add include files for common IPL structs, jrossi, 2024/10/16
- [PATCH v4 12/19] pc-bios/s390-ccw: Enable failed IPL to return after error, jrossi, 2024/10/16
- [PATCH v4 14/19] s390x: Add individual loadparm assignment to CCW device, jrossi, 2024/10/16
- [PATCH v4 16/19] s390x: Rebuild IPLB for SCSI device directly from DIAG308, jrossi, 2024/10/16
- [PATCH v4 15/19] hw/s390x: Build an IPLB for each boot device, jrossi, 2024/10/16