* [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable
@ 2020-08-06 10:53 Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 1/9] pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and -fno-common Thomas Huth
` (8 more replies)
0 siblings, 9 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
The traditional / architected way of booting on s390x is to always
specify the device where the guest should be booted from - that means
s390x guests should be always started with a device that has the
"bootindex" property set.
For the users that are used to a firmware from a different CPU archi-
tecture (or the lazy s390x folks like myself), the s390-ccw bios also
tries to find a bootable device on its own in case the user did not
specify a "bootindex" property. Unfortunately, it always stops at the
very first device that it can find, no matter whether it's bootable or
not. That causes some weird behavior, for example while
qemu-system-s390x -hda bootable.qcow2
boots perfectly fine, the bios refuses to work if you just specify
a virtio-scsi controller in front of it:
qemu-system-s390x -device virtio-scsi -hda bootable.qcow2
While this is perfectly fine from the Z architecture point of view, it
still could be a little bit uncomfortable and confusing for the lazy
or ignorant users who did not specify a "bootindex". And since all major
firmwares on other architectures correctly boot in such cases, too,
let's also try to teach the s390-ccw bios how to boot in such cases.
For this, we have to get rid of the various panic()s and IPL_assert()
statements at the "low-level" function and let the main code handle
the decision instead whether a boot from a device should fail or not,
so that the main code can continue searching in case it wants to.
Thomas
v2:
- Add patch to remove superfluous call to enable_subchannel()
- Add patch to test the new behavior in the tests/qtest/cdrom-test
- Added Reviewed-bys from v1
- Renamed check_sch_no() to is_dev_possibly_bootable()
- Reworked the return codes to use 0/-ENODEV instead of true/false
Thomas Huth (9):
pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and
-fno-common
pc-bios/s390-ccw: Move ipl-related code from main() into a separate
function
pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others
pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate
function
pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk
pc-bios/s390-ccw: Scan through all devices if no boot device specified
pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is
bad
pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()
tests/qtest/cdrom: Add more s390x-related boot tests
pc-bios/s390-ccw/Makefile | 7 +-
pc-bios/s390-ccw/bootmap.c | 34 ++++--
pc-bios/s390-ccw/main.c | 172 +++++++++++++++++++------------
pc-bios/s390-ccw/s390-ccw.h | 8 +-
pc-bios/s390-ccw/virtio-blkdev.c | 7 +-
pc-bios/s390-ccw/virtio-scsi.c | 28 +++--
pc-bios/s390-ccw/virtio-scsi.h | 2 +-
tests/qtest/cdrom-test.c | 12 +++
8 files changed, 174 insertions(+), 96 deletions(-)
--
2.18.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 1/9] pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and -fno-common
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 2/9] pc-bios/s390-ccw: Move ipl-related code from main() into a separate function Thomas Huth
` (7 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
The main QEMU code is compiled with -std=gnu99, -fwrapv and -fno-common.
We should use the same flags for the s390-ccw bios, too, to avoid that
we get different behavior with different compiler versions that changed
their default settings in the course of time (it happened at least with
-std=... and -fno-common in the past already).
While we're at it, also group the other flags here in a little bit nicer
fashion: Move the two "-m" flags out of the "-f" area and specify them on
a separate line.
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Acked-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/Makefile | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 50bc880272..9abb0ea4c0 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -13,10 +13,11 @@ OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o \
virtio.o virtio-scsi.o virtio-blkdev.o libc.o cio.o dasd-ipl.o
QEMU_CFLAGS := $(filter -W%, $(QEMU_CFLAGS))
-QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -msoft-float
-QEMU_CFLAGS += -march=z900 -fPIE -fno-strict-aliasing
-QEMU_CFLAGS += -fno-asynchronous-unwind-tables
+QEMU_CFLAGS += -ffreestanding -fno-delete-null-pointer-checks -fno-common -fPIE
+QEMU_CFLAGS += -fwrapv -fno-strict-aliasing -fno-asynchronous-unwind-tables
QEMU_CFLAGS += $(call cc-option, $(QEMU_CFLAGS), -fno-stack-protector)
+QEMU_CFLAGS += -msoft-float -march=z900
+QEMU_CFLAGS += -std=gnu99
LDFLAGS += -Wl,-pie -nostdlib
build-all: s390-ccw.img s390-netboot.img
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 2/9] pc-bios/s390-ccw: Move ipl-related code from main() into a separate function
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 1/9] pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and -fno-common Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others Thomas Huth
` (6 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
Let's move this part of the code into a separate function to be able
to use it from multiple spots later.
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/main.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 146a50760b..9b64eb0c24 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -223,14 +223,8 @@ static void virtio_setup(void)
}
}
-int main(void)
+static void ipl_boot_device(void)
{
- sclp_setup();
- css_setup();
- boot_setup();
- find_boot_device();
- enable_subchannel(blk_schid);
-
switch (cutype) {
case CU_TYPE_DASD_3990:
case CU_TYPE_DASD_2107:
@@ -242,8 +236,18 @@ int main(void)
break;
default:
print_int("Attempting to boot from unexpected device type", cutype);
- panic("");
+ panic("\nBoot failed.\n");
}
+}
+
+int main(void)
+{
+ sclp_setup();
+ css_setup();
+ boot_setup();
+ find_boot_device();
+ enable_subchannel(blk_schid);
+ ipl_boot_device();
panic("Failed to load OS from hard disk\n");
return 0; /* make compiler happy */
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 1/9] pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and -fno-common Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 2/9] pc-bios/s390-ccw: Move ipl-related code from main() into a separate function Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 11:06 ` Cornelia Huck
2020-08-06 13:27 ` Janosch Frank
2020-08-06 10:53 ` [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function Thomas Huth
` (5 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
Remove the "#ifndef E..." guards from the defines here - the header
guard S390_CCW_H at the top of the file should avoid double definition,
and if the error code is defined in a different file already, we're in
trouble anyway, then it's better to see the error at compile time instead
of hunting weird behavior during runtime later.
Also define ENODEV - we will use this in a later patch.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/s390-ccw.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 36b884cced..dbc4c64851 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -27,12 +27,10 @@ typedef unsigned long long __u64;
#define false 0
#define PAGE_SIZE 4096
-#ifndef EIO
#define EIO 1
-#endif
-#ifndef EBUSY
#define EBUSY 2
-#endif
+#define ENODEV 3
+
#ifndef NULL
#define NULL 0
#endif
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
` (2 preceding siblings ...)
2020-08-06 10:53 ` [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 11:14 ` Cornelia Huck
2020-08-06 13:28 ` Janosch Frank
2020-08-06 10:53 ` [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk Thomas Huth
` (4 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
Move the code to a separate function to be able to re-use it from a
different spot later.
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/main.c | 99 ++++++++++++++++++++++++-----------------
1 file changed, 57 insertions(+), 42 deletions(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 9b64eb0c24..0d2aabbc58 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -51,6 +51,60 @@ unsigned int get_loadparm_index(void)
return atoui(loadparm_str);
}
+static int is_dev_possibly_bootable(int dev_no, int sch_no)
+{
+ bool is_virtio;
+ Schib schib;
+ int r;
+
+ blk_schid.sch_no = sch_no;
+ r = stsch_err(blk_schid, &schib);
+ if (r == 3 || r == -EIO) {
+ return -ENODEV;
+ }
+ if (!schib.pmcw.dnv) {
+ return false;
+ }
+
+ enable_subchannel(blk_schid);
+ cutype = cu_type(blk_schid);
+
+ /*
+ * Note: we always have to run virtio_is_supported() here to make
+ * sure that the vdev.senseid data gets pre-initialized correctly
+ */
+ is_virtio = virtio_is_supported(blk_schid);
+
+ /* No specific devno given, just return whether the device is possibly bootable */
+ if (dev_no < 0) {
+ switch (cutype) {
+ case CU_TYPE_VIRTIO:
+ if (is_virtio) {
+ /*
+ * Skip net devices since no IPLB is created and therefore
+ * no network bootloader has been loaded
+ */
+ if (virtio_get_device_type() != VIRTIO_ID_NET) {
+ return true;
+ }
+ }
+ return false;
+ case CU_TYPE_DASD_3990:
+ case CU_TYPE_DASD_2107:
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ /* Caller asked for a specific devno */
+ if (schib.pmcw.dev == dev_no) {
+ return true;
+ }
+
+ return false;
+}
+
/*
* Find the subchannel connected to the given device (dev_no) and fill in the
* subchannel information block (schib) with the connected subchannel's info.
@@ -62,53 +116,14 @@ unsigned int get_loadparm_index(void)
*/
static bool find_subch(int dev_no)
{
- Schib schib;
int i, r;
- bool is_virtio;
for (i = 0; i < 0x10000; i++) {
- blk_schid.sch_no = i;
- r = stsch_err(blk_schid, &schib);
- if ((r == 3) || (r == -EIO)) {
+ r = is_dev_possibly_bootable(dev_no, i);
+ if (r < 0) {
break;
}
- if (!schib.pmcw.dnv) {
- continue;
- }
-
- enable_subchannel(blk_schid);
- cutype = cu_type(blk_schid);
-
- /*
- * Note: we always have to run virtio_is_supported() here to make
- * sure that the vdev.senseid data gets pre-initialized correctly
- */
- is_virtio = virtio_is_supported(blk_schid);
-
- /* No specific devno given, just return 1st possibly bootable device */
- if (dev_no < 0) {
- switch (cutype) {
- case CU_TYPE_VIRTIO:
- if (is_virtio) {
- /*
- * Skip net devices since no IPLB is created and therefore
- * no network bootloader has been loaded
- */
- if (virtio_get_device_type() != VIRTIO_ID_NET) {
- return true;
- }
- }
- continue;
- case CU_TYPE_DASD_3990:
- case CU_TYPE_DASD_2107:
- return true;
- default:
- continue;
- }
- }
-
- /* Caller asked for a specific devno */
- if (schib.pmcw.dev == dev_no) {
+ if (r == true) {
return true;
}
}
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
` (3 preceding siblings ...)
2020-08-06 10:53 ` [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 11:20 ` Cornelia Huck
2020-08-06 10:53 ` [PATCH for-5.2 v2 6/9] pc-bios/s390-ccw: Scan through all devices if no boot device specified Thomas Huth
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
In case the user did not specify a boot device, we want to continue
looking for other devices if there are no valid SCSI disks on a virtio-
scsi controller. As a first step, do not panic in this case and let
the control flow carry the error to the upper functions instead.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/main.c | 14 ++++++++++----
pc-bios/s390-ccw/s390-ccw.h | 2 +-
pc-bios/s390-ccw/virtio-blkdev.c | 7 +++++--
pc-bios/s390-ccw/virtio-scsi.c | 28 ++++++++++++++++++++--------
pc-bios/s390-ccw/virtio-scsi.h | 2 +-
5 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 0d2aabbc58..7bdd12ab2e 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -218,7 +218,7 @@ static void find_boot_device(void)
IPL_assert(found, "Boot device not found\n");
}
-static void virtio_setup(void)
+static int virtio_setup(void)
{
VDev *vdev = virtio_get_device();
QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
@@ -233,9 +233,14 @@ static void virtio_setup(void)
sclp_print("Network boot device detected\n");
vdev->netboot_start_addr = qipl.netboot_start_addr;
} else {
- virtio_blk_setup_device(blk_schid);
+ int ret = virtio_blk_setup_device(blk_schid);
+ if (ret) {
+ return ret;
+ }
IPL_assert(virtio_ipl_disk_is_valid(), "No valid IPL device detected");
}
+
+ return 0;
}
static void ipl_boot_device(void)
@@ -246,8 +251,9 @@ static void ipl_boot_device(void)
dasd_ipl(blk_schid, cutype); /* no return */
break;
case CU_TYPE_VIRTIO:
- virtio_setup();
- zipl_load(); /* no return */
+ if (virtio_setup() == 0) {
+ zipl_load(); /* no return */
+ }
break;
default:
print_int("Attempting to boot from unexpected device type", cutype);
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index dbc4c64851..9b86c120b4 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -69,7 +69,7 @@ int sclp_read(char *str, size_t count);
unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
ulong subchan_id, void *load_addr);
bool virtio_is_supported(SubChannelId schid);
-void virtio_blk_setup_device(SubChannelId schid);
+int virtio_blk_setup_device(SubChannelId schid);
int virtio_read(ulong sector, void *load_addr);
/* bootmap.c */
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 11c56261ca..7d35050292 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -263,9 +263,10 @@ uint64_t virtio_get_blocks(void)
return 0;
}
-void virtio_blk_setup_device(SubChannelId schid)
+int virtio_blk_setup_device(SubChannelId schid)
{
VDev *vdev = virtio_get_device();
+ int ret = 0;
vdev->schid = schid;
virtio_setup_ccw(vdev);
@@ -288,9 +289,11 @@ void virtio_blk_setup_device(SubChannelId schid)
"Config: CDB size mismatch");
sclp_print("Using virtio-scsi.\n");
- virtio_scsi_setup(vdev);
+ ret = virtio_scsi_setup(vdev);
break;
default:
panic("\n! No IPL device available !\n");
}
+
+ return ret;
}
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index eddfb8a7ad..2c8d0f3097 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -194,7 +194,12 @@ static bool scsi_read_capacity(VDev *vdev,
/* virtio-scsi routines */
-static void virtio_scsi_locate_device(VDev *vdev)
+/*
+ * Tries to locate a SCSI device and and adds the information for the found
+ * device to the vdev->scsi_device structure.
+ * Returns 0 if SCSI device could be located, or a error code < 0 otherwise
+ */
+static int virtio_scsi_locate_device(VDev *vdev)
{
const uint16_t channel = 0; /* again, it's what QEMU does */
uint16_t target;
@@ -220,7 +225,7 @@ static void virtio_scsi_locate_device(VDev *vdev)
IPL_check(sdev->channel == 0, "non-zero channel requested");
IPL_check(sdev->target <= vdev->config.scsi.max_target, "target# high");
IPL_check(sdev->lun <= vdev->config.scsi.max_lun, "LUN# high");
- return;
+ return 0;
}
for (target = 0; target <= vdev->config.scsi.max_target; target++) {
@@ -247,18 +252,20 @@ static void virtio_scsi_locate_device(VDev *vdev)
*/
sdev->lun = r->lun[0].v16[0]; /* it's returned this way */
debug_print_int("Have to use LUN", sdev->lun);
- return; /* we have to use this device */
+ return 0; /* we have to use this device */
}
for (i = 0; i < luns; i++) {
if (r->lun[i].v64) {
/* Look for non-zero LUN - we have where to choose from */
sdev->lun = r->lun[i].v16[0];
debug_print_int("Will use LUN", sdev->lun);
- return; /* we have found a device */
+ return 0; /* we have found a device */
}
}
}
- panic("\n! Cannot locate virtio-scsi device !\n");
+
+ sclp_print("Warning: Could not locate a usable virtio-scsi device\n");
+ return -ENODEV;
}
int virtio_scsi_read_many(VDev *vdev,
@@ -322,17 +329,20 @@ static void scsi_parse_capacity_report(void *data,
}
}
-void virtio_scsi_setup(VDev *vdev)
+int virtio_scsi_setup(VDev *vdev)
{
int retry_test_unit_ready = 3;
uint8_t data[256];
uint32_t data_size = sizeof(data);
ScsiInquiryEvpdPages *evpd = &scsi_inquiry_evpd_pages_response;
ScsiInquiryEvpdBl *evpd_bl = &scsi_inquiry_evpd_bl_response;
- int i;
+ int i, ret;
vdev->scsi_device = &default_scsi_device;
- virtio_scsi_locate_device(vdev);
+ ret = virtio_scsi_locate_device(vdev);
+ if (ret < 0) {
+ return ret;
+ }
/* We have to "ping" the device before it becomes readable */
while (!scsi_test_unit_ready(vdev)) {
@@ -417,4 +427,6 @@ void virtio_scsi_setup(VDev *vdev)
}
scsi_parse_capacity_report(data, &vdev->scsi_last_block,
(uint32_t *) &vdev->scsi_block_size);
+
+ return 0;
}
diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-ccw/virtio-scsi.h
index 4c4f4bbc31..4b14c2c2f9 100644
--- a/pc-bios/s390-ccw/virtio-scsi.h
+++ b/pc-bios/s390-ccw/virtio-scsi.h
@@ -67,7 +67,7 @@ static inline bool virtio_scsi_response_ok(const VirtioScsiCmdResp *r)
return r->response == VIRTIO_SCSI_S_OK && r->status == CDB_STATUS_GOOD;
}
-void virtio_scsi_setup(VDev *vdev);
+int virtio_scsi_setup(VDev *vdev);
int virtio_scsi_read_many(VDev *vdev,
ulong sector, void *load_addr, int sec_num);
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 6/9] pc-bios/s390-ccw: Scan through all devices if no boot device specified
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
` (4 preceding siblings ...)
2020-08-06 10:53 ` [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 7/9] pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is bad Thomas Huth
` (2 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
If no boot device has been specified (via "bootindex=..."), the s390-ccw
bios scans through all devices to find a bootable device. But so far, it
stops at the very first block device (including virtio-scsi controllers
without attached devices) that it finds, no matter whether it is bootable
or not. That leads to some weird situatation where it is e.g. possible
to boot via:
qemu-system-s390x -hda /path/to/disk.qcow2
but not if there is e.g. a virtio-scsi controller specified before:
qemu-system-s390x -device virtio-scsi -hda /path/to/disk.qcow2
While using "bootindex=..." is clearly the preferred way of booting
on s390x, we still can make the life for the users at least a little
bit easier if we look at all available devices to find a bootable one.
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1846975
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/main.c | 46 +++++++++++++++++++++++++++--------------
1 file changed, 31 insertions(+), 15 deletions(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 7bdd12ab2e..9b581074a1 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -182,20 +182,8 @@ static void boot_setup(void)
static void find_boot_device(void)
{
VDev *vdev = virtio_get_device();
- int ssid;
bool found;
- if (!have_iplb) {
- for (ssid = 0; ssid < 0x3; ssid++) {
- blk_schid.ssid = ssid;
- found = find_subch(-1);
- if (found) {
- return;
- }
- }
- panic("Could not find a suitable boot device (none specified)\n");
- }
-
switch (iplb.pbt) {
case S390_IPL_TYPE_CCW:
debug_print_int("device no. ", iplb.ccw.devno);
@@ -261,14 +249,42 @@ static void ipl_boot_device(void)
}
}
+/*
+ * No boot device has been specified, so we have to scan through the
+ * channels to find one.
+ */
+static void probe_boot_device(void)
+{
+ int ssid, sch_no, ret;
+
+ for (ssid = 0; ssid < 0x3; ssid++) {
+ blk_schid.ssid = ssid;
+ for (sch_no = 0; sch_no < 0x10000; sch_no++) {
+ ret = is_dev_possibly_bootable(-1, sch_no);
+ if (ret < 0) {
+ break;
+ }
+ if (ret == true) {
+ ipl_boot_device(); /* Only returns if unsuccessful */
+ }
+ }
+ }
+
+ sclp_print("Could not find a suitable boot device (none specified)\n");
+}
+
int main(void)
{
sclp_setup();
css_setup();
boot_setup();
- find_boot_device();
- enable_subchannel(blk_schid);
- ipl_boot_device();
+ if (have_iplb) {
+ find_boot_device();
+ enable_subchannel(blk_schid);
+ ipl_boot_device();
+ } else {
+ probe_boot_device();
+ }
panic("Failed to load OS from hard disk\n");
return 0; /* make compiler happy */
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 7/9] pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is bad
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
` (5 preceding siblings ...)
2020-08-06 10:53 ` [PATCH for-5.2 v2 6/9] pc-bios/s390-ccw: Scan through all devices if no boot device specified Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel() Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests Thomas Huth
8 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
If you try to boot with two virtio-blk disks (without bootindex), and
only the second one is bootable, the s390-ccw bios currently stops at
the first disk and does not continue booting from the second one. This
is annoying - and all other major QEMU firmwares succeed to boot from
the second disk in this case, so we should do the same in the s390-ccw
bios, too.
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/bootmap.c | 34 +++++++++++++++++++++++-----------
pc-bios/s390-ccw/main.c | 2 +-
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 97205674e5..0ef6b851f3 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -289,11 +289,18 @@ static void ipl_eckd_cdl(void)
read_block(1, ipl2, "Cannot read IPL2 record at block 1");
mbr = &ipl2->mbr;
- IPL_assert(magic_match(mbr, ZIPL_MAGIC), "No zIPL section in IPL2 record.");
- IPL_assert(block_size_ok(mbr->blockptr.xeckd.bptr.size),
- "Bad block size in zIPL section of IPL2 record.");
- IPL_assert(mbr->dev_type == DEV_TYPE_ECKD,
- "Non-ECKD device type in zIPL section of IPL2 record.");
+ if (!magic_match(mbr, ZIPL_MAGIC)) {
+ sclp_print("No zIPL section in IPL2 record.\n");
+ return;
+ }
+ if (!block_size_ok(mbr->blockptr.xeckd.bptr.size)) {
+ sclp_print("Bad block size in zIPL section of IPL2 record.\n");
+ return;
+ }
+ if (!mbr->dev_type == DEV_TYPE_ECKD) {
+ sclp_print("Non-ECKD device type in zIPL section of IPL2 record.\n");
+ return;
+ }
/* save pointer to Boot Map Table */
bmt_block_nr = eckd_block_num(&mbr->blockptr.xeckd.bptr.chs);
@@ -303,10 +310,14 @@ static void ipl_eckd_cdl(void)
memset(sec, FREE_SPACE_FILLER, sizeof(sec));
read_block(2, vlbl, "Cannot read Volume Label at block 2");
- IPL_assert(magic_match(vlbl->key, VOL1_MAGIC),
- "Invalid magic of volume label block");
- IPL_assert(magic_match(vlbl->f.key, VOL1_MAGIC),
- "Invalid magic of volser block");
+ if (!magic_match(vlbl->key, VOL1_MAGIC)) {
+ sclp_print("Invalid magic of volume label block.\n");
+ return;
+ }
+ if (!magic_match(vlbl->f.key, VOL1_MAGIC)) {
+ sclp_print("Invalid magic of volser block.\n");
+ return;
+ }
print_volser(vlbl->f.volser);
run_eckd_boot_script(bmt_block_nr, s1b_block_nr);
@@ -398,7 +409,8 @@ static void ipl_eckd(void)
read_block(0, mbr, "Cannot read block 0 on DASD");
if (magic_match(mbr->magic, IPL1_MAGIC)) {
- ipl_eckd_cdl(); /* no return */
+ ipl_eckd_cdl(); /* only returns in case of error */
+ return;
}
/* LDL/CMS? */
@@ -825,5 +837,5 @@ void zipl_load(void)
panic("\n! Unknown IPL device type !\n");
}
- panic("\n* this can never happen *\n");
+ sclp_print("zIPL load failed.\n");
}
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 9b581074a1..fc17e6ab83 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -240,7 +240,7 @@ static void ipl_boot_device(void)
break;
case CU_TYPE_VIRTIO:
if (virtio_setup() == 0) {
- zipl_load(); /* no return */
+ zipl_load(); /* Only returns in case of errors */
}
break;
default:
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
` (6 preceding siblings ...)
2020-08-06 10:53 ` [PATCH for-5.2 v2 7/9] pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is bad Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 11:21 ` Cornelia Huck
2020-08-06 10:53 ` [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests Thomas Huth
8 siblings, 1 reply; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
enable_subchannel() is already done during is_dev_possibly_bootable()
(which is called from find_boot_device() -> find_subch()), so there
is no need to do this again in the main() function.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/main.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index fc17e6ab83..43c792cf95 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -280,7 +280,6 @@ int main(void)
boot_setup();
if (have_iplb) {
find_boot_device();
- enable_subchannel(blk_schid);
ipl_boot_device();
} else {
probe_boot_device();
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
` (7 preceding siblings ...)
2020-08-06 10:53 ` [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel() Thomas Huth
@ 2020-08-06 10:53 ` Thomas Huth
2020-08-06 11:23 ` Cornelia Huck
2020-08-06 13:34 ` Janosch Frank
8 siblings, 2 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 10:53 UTC (permalink / raw)
To: qemu-s390x, qemu-devel
Cc: Jason J . Herne, Thomas Huth, Janosch Frank, qemu-block,
Cornelia Huck, Collin Walling, Christian Borntraeger,
Claudio Imbrenda
Let's add two new tests:
1) Booting with "bootindex" is the architected default behavior on the
s390x target, so we should have at least one test that is using the
"bootindex" property.
2) The s390-ccw bios used to fail when other unbootable devices have
been specified before the bootable device (without "bootindex"). Now
that the s390-ccw bios is a little bit smarter here, we should test
this scenario, too, to avoid regressions.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
tests/qtest/cdrom-test.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 833a0508a1..13e22f57c1 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -163,6 +163,18 @@ static void add_s390x_tests(void)
qtest_add_data_func("cdrom/boot/virtio-scsi",
"-device virtio-scsi -device scsi-cd,drive=cdr "
"-blockdev file,node-name=cdr,filename=", test_cdboot);
+ qtest_add_data_func("cdrom/boot/with-bootindex",
+ "-device virtio-serial -device virtio-scsi "
+ "-device virtio-blk,drive=d1 "
+ "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
+ "-device virtio-blk,drive=d2,bootindex=1 "
+ "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
+ qtest_add_data_func("cdrom/boot/without-bootindex",
+ "-device virtio-scsi -device virtio-serial "
+ "-device x-terminal3270 -device virtio-blk,drive=d1 "
+ "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
+ "-device virtio-blk,drive=d2 "
+ "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
}
int main(int argc, char **argv)
--
2.18.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others
2020-08-06 10:53 ` [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others Thomas Huth
@ 2020-08-06 11:06 ` Cornelia Huck
2020-08-06 13:27 ` Janosch Frank
1 sibling, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-08-06 11:06 UTC (permalink / raw)
To: Thomas Huth
Cc: Jason J . Herne, Collin Walling, Janosch Frank, qemu-block,
qemu-devel, Christian Borntraeger, qemu-s390x, Claudio Imbrenda
On Thu, 6 Aug 2020 12:53:43 +0200
Thomas Huth <thuth@redhat.com> wrote:
> Remove the "#ifndef E..." guards from the defines here - the header
> guard S390_CCW_H at the top of the file should avoid double definition,
> and if the error code is defined in a different file already, we're in
> trouble anyway, then it's better to see the error at compile time instead
> of hunting weird behavior during runtime later.
> Also define ENODEV - we will use this in a later patch.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> pc-bios/s390-ccw/s390-ccw.h | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function
2020-08-06 10:53 ` [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function Thomas Huth
@ 2020-08-06 11:14 ` Cornelia Huck
2020-08-06 13:28 ` Janosch Frank
1 sibling, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-08-06 11:14 UTC (permalink / raw)
To: Thomas Huth
Cc: Jason J . Herne, Collin Walling, Janosch Frank, qemu-block,
qemu-devel, Christian Borntraeger, qemu-s390x, Claudio Imbrenda
On Thu, 6 Aug 2020 12:53:44 +0200
Thomas Huth <thuth@redhat.com> wrote:
> Move the code to a separate function to be able to re-use it from a
> different spot later.
>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> pc-bios/s390-ccw/main.c | 99 ++++++++++++++++++++++++-----------------
> 1 file changed, 57 insertions(+), 42 deletions(-)
(...)
> @@ -62,53 +116,14 @@ unsigned int get_loadparm_index(void)
> */
> static bool find_subch(int dev_no)
> {
> - Schib schib;
> int i, r;
> - bool is_virtio;
>
> for (i = 0; i < 0x10000; i++) {
> - blk_schid.sch_no = i;
> - r = stsch_err(blk_schid, &schib);
> - if ((r == 3) || (r == -EIO)) {
> + r = is_dev_possibly_bootable(dev_no, i);
Maybe explicitly check for -ENODEV here? But no strong opinion.
> + if (r < 0) {
> break;
> }
(...)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk
2020-08-06 10:53 ` [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk Thomas Huth
@ 2020-08-06 11:20 ` Cornelia Huck
0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-08-06 11:20 UTC (permalink / raw)
To: Thomas Huth
Cc: Jason J . Herne, Collin Walling, Janosch Frank, qemu-block,
qemu-devel, Christian Borntraeger, qemu-s390x, Claudio Imbrenda
On Thu, 6 Aug 2020 12:53:45 +0200
Thomas Huth <thuth@redhat.com> wrote:
> In case the user did not specify a boot device, we want to continue
> looking for other devices if there are no valid SCSI disks on a virtio-
> scsi controller. As a first step, do not panic in this case and let
> the control flow carry the error to the upper functions instead.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> pc-bios/s390-ccw/main.c | 14 ++++++++++----
> pc-bios/s390-ccw/s390-ccw.h | 2 +-
> pc-bios/s390-ccw/virtio-blkdev.c | 7 +++++--
> pc-bios/s390-ccw/virtio-scsi.c | 28 ++++++++++++++++++++--------
> pc-bios/s390-ccw/virtio-scsi.h | 2 +-
> 5 files changed, 37 insertions(+), 16 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()
2020-08-06 10:53 ` [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel() Thomas Huth
@ 2020-08-06 11:21 ` Cornelia Huck
0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-08-06 11:21 UTC (permalink / raw)
To: Thomas Huth
Cc: Jason J . Herne, Collin Walling, Janosch Frank, qemu-block,
qemu-devel, Christian Borntraeger, qemu-s390x, Claudio Imbrenda
On Thu, 6 Aug 2020 12:53:48 +0200
Thomas Huth <thuth@redhat.com> wrote:
> enable_subchannel() is already done during is_dev_possibly_bootable()
> (which is called from find_boot_device() -> find_subch()), so there
> is no need to do this again in the main() function.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> pc-bios/s390-ccw/main.c | 1 -
> 1 file changed, 1 deletion(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests
2020-08-06 10:53 ` [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests Thomas Huth
@ 2020-08-06 11:23 ` Cornelia Huck
2020-08-06 11:58 ` Thomas Huth
2020-08-06 13:34 ` Janosch Frank
1 sibling, 1 reply; 21+ messages in thread
From: Cornelia Huck @ 2020-08-06 11:23 UTC (permalink / raw)
To: Thomas Huth
Cc: Jason J . Herne, Collin Walling, Janosch Frank, qemu-block,
qemu-devel, Christian Borntraeger, qemu-s390x, Claudio Imbrenda
On Thu, 6 Aug 2020 12:53:49 +0200
Thomas Huth <thuth@redhat.com> wrote:
> Let's add two new tests:
>
> 1) Booting with "bootindex" is the architected default behavior on the
> s390x target, so we should have at least one test that is using the
> "bootindex" property.
>
> 2) The s390-ccw bios used to fail when other unbootable devices have
> been specified before the bootable device (without "bootindex"). Now
> that the s390-ccw bios is a little bit smarter here, we should test
> this scenario, too, to avoid regressions.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> tests/qtest/cdrom-test.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> index 833a0508a1..13e22f57c1 100644
> --- a/tests/qtest/cdrom-test.c
> +++ b/tests/qtest/cdrom-test.c
> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
> qtest_add_data_func("cdrom/boot/virtio-scsi",
> "-device virtio-scsi -device scsi-cd,drive=cdr "
> "-blockdev file,node-name=cdr,filename=", test_cdboot);
> + qtest_add_data_func("cdrom/boot/with-bootindex",
> + "-device virtio-serial -device virtio-scsi "
> + "-device virtio-blk,drive=d1 "
> + "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> + "-device virtio-blk,drive=d2,bootindex=1 "
> + "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
> + qtest_add_data_func("cdrom/boot/without-bootindex",
> + "-device virtio-scsi -device virtio-serial "
> + "-device x-terminal3270 -device virtio-blk,drive=d1 "
Any special reason for that 3270 device here? Or just to add more
variety? :)
> + "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> + "-device virtio-blk,drive=d2 "
> + "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
> }
>
> int main(int argc, char **argv)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests
2020-08-06 11:23 ` Cornelia Huck
@ 2020-08-06 11:58 ` Thomas Huth
2020-08-06 13:11 ` Cornelia Huck
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Huth @ 2020-08-06 11:58 UTC (permalink / raw)
To: Cornelia Huck
Cc: Jason J . Herne, Collin Walling, Janosch Frank, qemu-block,
qemu-devel, Christian Borntraeger, qemu-s390x, Claudio Imbrenda
On 06/08/2020 13.23, Cornelia Huck wrote:
> On Thu, 6 Aug 2020 12:53:49 +0200
> Thomas Huth <thuth@redhat.com> wrote:
>
>> Let's add two new tests:
>>
>> 1) Booting with "bootindex" is the architected default behavior on the
>> s390x target, so we should have at least one test that is using the
>> "bootindex" property.
>>
>> 2) The s390-ccw bios used to fail when other unbootable devices have
>> been specified before the bootable device (without "bootindex"). Now
>> that the s390-ccw bios is a little bit smarter here, we should test
>> this scenario, too, to avoid regressions.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> tests/qtest/cdrom-test.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
>> index 833a0508a1..13e22f57c1 100644
>> --- a/tests/qtest/cdrom-test.c
>> +++ b/tests/qtest/cdrom-test.c
>> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
>> qtest_add_data_func("cdrom/boot/virtio-scsi",
>> "-device virtio-scsi -device scsi-cd,drive=cdr "
>> "-blockdev file,node-name=cdr,filename=", test_cdboot);
>> + qtest_add_data_func("cdrom/boot/with-bootindex",
>> + "-device virtio-serial -device virtio-scsi "
>> + "-device virtio-blk,drive=d1 "
>> + "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
>> + "-device virtio-blk,drive=d2,bootindex=1 "
>> + "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
>> + qtest_add_data_func("cdrom/boot/without-bootindex",
>> + "-device virtio-scsi -device virtio-serial "
>> + "-device x-terminal3270 -device virtio-blk,drive=d1 "
>
> Any special reason for that 3270 device here? Or just to add more
> variety? :)
Yes, there is a reason:
https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07231.html
... so this is a check that this does not happen again.
Thomas
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests
2020-08-06 11:58 ` Thomas Huth
@ 2020-08-06 13:11 ` Cornelia Huck
0 siblings, 0 replies; 21+ messages in thread
From: Cornelia Huck @ 2020-08-06 13:11 UTC (permalink / raw)
To: Thomas Huth
Cc: Jason J . Herne, Collin Walling, Janosch Frank, qemu-block,
qemu-devel, Christian Borntraeger, qemu-s390x, Claudio Imbrenda
On Thu, 6 Aug 2020 13:58:47 +0200
Thomas Huth <thuth@redhat.com> wrote:
> On 06/08/2020 13.23, Cornelia Huck wrote:
> > On Thu, 6 Aug 2020 12:53:49 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >
> >> Let's add two new tests:
> >>
> >> 1) Booting with "bootindex" is the architected default behavior on the
> >> s390x target, so we should have at least one test that is using the
> >> "bootindex" property.
> >>
> >> 2) The s390-ccw bios used to fail when other unbootable devices have
> >> been specified before the bootable device (without "bootindex"). Now
> >> that the s390-ccw bios is a little bit smarter here, we should test
> >> this scenario, too, to avoid regressions.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >> tests/qtest/cdrom-test.c | 12 ++++++++++++
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> >> index 833a0508a1..13e22f57c1 100644
> >> --- a/tests/qtest/cdrom-test.c
> >> +++ b/tests/qtest/cdrom-test.c
> >> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
> >> qtest_add_data_func("cdrom/boot/virtio-scsi",
> >> "-device virtio-scsi -device scsi-cd,drive=cdr "
> >> "-blockdev file,node-name=cdr,filename=", test_cdboot);
> >> + qtest_add_data_func("cdrom/boot/with-bootindex",
> >> + "-device virtio-serial -device virtio-scsi "
> >> + "-device virtio-blk,drive=d1 "
> >> + "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> >> + "-device virtio-blk,drive=d2,bootindex=1 "
> >> + "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
> >> + qtest_add_data_func("cdrom/boot/without-bootindex",
> >> + "-device virtio-scsi -device virtio-serial "
> >> + "-device x-terminal3270 -device virtio-blk,drive=d1 "
> >
> > Any special reason for that 3270 device here? Or just to add more
> > variety? :)
>
> Yes, there is a reason:
>
> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07231.html
>
> ... so this is a check that this does not happen again.
Ah, thanks for reminding me. Adding 3270 is a good idea.
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others
2020-08-06 10:53 ` [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others Thomas Huth
2020-08-06 11:06 ` Cornelia Huck
@ 2020-08-06 13:27 ` Janosch Frank
2020-08-27 9:10 ` Thomas Huth
1 sibling, 1 reply; 21+ messages in thread
From: Janosch Frank @ 2020-08-06 13:27 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, qemu-devel
Cc: Jason J . Herne, Collin Walling, qemu-block, Cornelia Huck,
Christian Borntraeger, Claudio Imbrenda
[-- Attachment #1.1: Type: text/plain, Size: 1210 bytes --]
On 8/6/20 12:53 PM, Thomas Huth wrote:
> Remove the "#ifndef E..." guards from the defines here - the header
> guard S390_CCW_H at the top of the file should avoid double definition,
> and if the error code is defined in a different file already, we're in
> trouble anyway, then it's better to see the error at compile time instead
> of hunting weird behavior during runtime later.
> Also define ENODEV - we will use this in a later patch.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Would it make sense to use the errno.h numbers for the defines?
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/s390-ccw.h | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 36b884cced..dbc4c64851 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -27,12 +27,10 @@ typedef unsigned long long __u64;
> #define false 0
> #define PAGE_SIZE 4096
>
> -#ifndef EIO
> #define EIO 1
> -#endif
> -#ifndef EBUSY
> #define EBUSY 2
> -#endif
> +#define ENODEV 3
> +
> #ifndef NULL
> #define NULL 0
> #endif
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function
2020-08-06 10:53 ` [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function Thomas Huth
2020-08-06 11:14 ` Cornelia Huck
@ 2020-08-06 13:28 ` Janosch Frank
1 sibling, 0 replies; 21+ messages in thread
From: Janosch Frank @ 2020-08-06 13:28 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, qemu-devel
Cc: Jason J . Herne, Collin Walling, qemu-block, Cornelia Huck,
Christian Borntraeger, Claudio Imbrenda
[-- Attachment #1.1: Type: text/plain, Size: 4400 bytes --]
On 8/6/20 12:53 PM, Thomas Huth wrote:
> Move the code to a separate function to be able to re-use it from a
> different spot later.
>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
The new function name helps a lot :)
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/main.c | 99 ++++++++++++++++++++++++-----------------
> 1 file changed, 57 insertions(+), 42 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 9b64eb0c24..0d2aabbc58 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -51,6 +51,60 @@ unsigned int get_loadparm_index(void)
> return atoui(loadparm_str);
> }
>
> +static int is_dev_possibly_bootable(int dev_no, int sch_no)
> +{
> + bool is_virtio;
> + Schib schib;
> + int r;
> +
> + blk_schid.sch_no = sch_no;
> + r = stsch_err(blk_schid, &schib);
> + if (r == 3 || r == -EIO) {
> + return -ENODEV;
> + }
> + if (!schib.pmcw.dnv) {
> + return false;
> + }
> +
> + enable_subchannel(blk_schid);
> + cutype = cu_type(blk_schid);
> +
> + /*
> + * Note: we always have to run virtio_is_supported() here to make
> + * sure that the vdev.senseid data gets pre-initialized correctly
> + */
> + is_virtio = virtio_is_supported(blk_schid);
> +
> + /* No specific devno given, just return whether the device is possibly bootable */
> + if (dev_no < 0) {
> + switch (cutype) {
> + case CU_TYPE_VIRTIO:
> + if (is_virtio) {
> + /*
> + * Skip net devices since no IPLB is created and therefore
> + * no network bootloader has been loaded
> + */
> + if (virtio_get_device_type() != VIRTIO_ID_NET) {
> + return true;
> + }
> + }
> + return false;
> + case CU_TYPE_DASD_3990:
> + case CU_TYPE_DASD_2107:
> + return true;
> + default:
> + return false;
> + }
> + }
> +
> + /* Caller asked for a specific devno */
> + if (schib.pmcw.dev == dev_no) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> /*
> * Find the subchannel connected to the given device (dev_no) and fill in the
> * subchannel information block (schib) with the connected subchannel's info.
> @@ -62,53 +116,14 @@ unsigned int get_loadparm_index(void)
> */
> static bool find_subch(int dev_no)
> {
> - Schib schib;
> int i, r;
> - bool is_virtio;
>
> for (i = 0; i < 0x10000; i++) {
> - blk_schid.sch_no = i;
> - r = stsch_err(blk_schid, &schib);
> - if ((r == 3) || (r == -EIO)) {
> + r = is_dev_possibly_bootable(dev_no, i);
> + if (r < 0) {
> break;
> }
> - if (!schib.pmcw.dnv) {
> - continue;
> - }
> -
> - enable_subchannel(blk_schid);
> - cutype = cu_type(blk_schid);
> -
> - /*
> - * Note: we always have to run virtio_is_supported() here to make
> - * sure that the vdev.senseid data gets pre-initialized correctly
> - */
> - is_virtio = virtio_is_supported(blk_schid);
> -
> - /* No specific devno given, just return 1st possibly bootable device */
> - if (dev_no < 0) {
> - switch (cutype) {
> - case CU_TYPE_VIRTIO:
> - if (is_virtio) {
> - /*
> - * Skip net devices since no IPLB is created and therefore
> - * no network bootloader has been loaded
> - */
> - if (virtio_get_device_type() != VIRTIO_ID_NET) {
> - return true;
> - }
> - }
> - continue;
> - case CU_TYPE_DASD_3990:
> - case CU_TYPE_DASD_2107:
> - return true;
> - default:
> - continue;
> - }
> - }
> -
> - /* Caller asked for a specific devno */
> - if (schib.pmcw.dev == dev_no) {
> + if (r == true) {
> return true;
> }
> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests
2020-08-06 10:53 ` [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests Thomas Huth
2020-08-06 11:23 ` Cornelia Huck
@ 2020-08-06 13:34 ` Janosch Frank
1 sibling, 0 replies; 21+ messages in thread
From: Janosch Frank @ 2020-08-06 13:34 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, qemu-devel
Cc: Jason J . Herne, Collin Walling, qemu-block, Cornelia Huck,
Christian Borntraeger, Claudio Imbrenda
[-- Attachment #1.1: Type: text/plain, Size: 2085 bytes --]
On 8/6/20 12:53 PM, Thomas Huth wrote:
> Let's add two new tests:
>
> 1) Booting with "bootindex" is the architected default behavior on the
> s390x target, so we should have at least one test that is using the
> "bootindex" property.
>
> 2) The s390-ccw bios used to fail when other unbootable devices have
> been specified before the bootable device (without "bootindex"). Now
> that the s390-ccw bios is a little bit smarter here, we should test
> this scenario, too, to avoid regressions.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Nice!
Acked-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> tests/qtest/cdrom-test.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> index 833a0508a1..13e22f57c1 100644
> --- a/tests/qtest/cdrom-test.c
> +++ b/tests/qtest/cdrom-test.c
> @@ -163,6 +163,18 @@ static void add_s390x_tests(void)
> qtest_add_data_func("cdrom/boot/virtio-scsi",
> "-device virtio-scsi -device scsi-cd,drive=cdr "
> "-blockdev file,node-name=cdr,filename=", test_cdboot);
> + qtest_add_data_func("cdrom/boot/with-bootindex",
> + "-device virtio-serial -device virtio-scsi "
> + "-device virtio-blk,drive=d1 "
> + "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> + "-device virtio-blk,drive=d2,bootindex=1 "
> + "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
> + qtest_add_data_func("cdrom/boot/without-bootindex",
> + "-device virtio-scsi -device virtio-serial "
> + "-device x-terminal3270 -device virtio-blk,drive=d1 "
> + "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
> + "-device virtio-blk,drive=d2 "
> + "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
> }
>
> int main(int argc, char **argv)
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others
2020-08-06 13:27 ` Janosch Frank
@ 2020-08-27 9:10 ` Thomas Huth
0 siblings, 0 replies; 21+ messages in thread
From: Thomas Huth @ 2020-08-27 9:10 UTC (permalink / raw)
To: Janosch Frank, qemu-s390x, qemu-devel
Cc: Jason J . Herne, Collin Walling, qemu-block, Cornelia Huck,
Christian Borntraeger, Claudio Imbrenda
On 06/08/2020 15.27, Janosch Frank wrote:
> On 8/6/20 12:53 PM, Thomas Huth wrote:
>> Remove the "#ifndef E..." guards from the defines here - the header
>> guard S390_CCW_H at the top of the file should avoid double definition,
>> and if the error code is defined in a different file already, we're in
>> trouble anyway, then it's better to see the error at compile time instead
>> of hunting weird behavior during runtime later.
>> Also define ENODEV - we will use this in a later patch.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>
> Would it make sense to use the errno.h numbers for the defines?
Which one? From Linux? From Windows? From BSD? ... I think it's likely
best if we keep them separate to avoid confusion.
Thomas
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
>
>> ---
>> pc-bios/s390-ccw/s390-ccw.h | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 36b884cced..dbc4c64851 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -27,12 +27,10 @@ typedef unsigned long long __u64;
>> #define false 0
>> #define PAGE_SIZE 4096
>>
>> -#ifndef EIO
>> #define EIO 1
>> -#endif
>> -#ifndef EBUSY
>> #define EBUSY 2
>> -#endif
>> +#define ENODEV 3
>> +
>> #ifndef NULL
>> #define NULL 0
>> #endif
>>
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2020-08-27 9:12 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-06 10:53 [PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 1/9] pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and -fno-common Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 2/9] pc-bios/s390-ccw: Move ipl-related code from main() into a separate function Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others Thomas Huth
2020-08-06 11:06 ` Cornelia Huck
2020-08-06 13:27 ` Janosch Frank
2020-08-27 9:10 ` Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function Thomas Huth
2020-08-06 11:14 ` Cornelia Huck
2020-08-06 13:28 ` Janosch Frank
2020-08-06 10:53 ` [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk Thomas Huth
2020-08-06 11:20 ` Cornelia Huck
2020-08-06 10:53 ` [PATCH for-5.2 v2 6/9] pc-bios/s390-ccw: Scan through all devices if no boot device specified Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 7/9] pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is bad Thomas Huth
2020-08-06 10:53 ` [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel() Thomas Huth
2020-08-06 11:21 ` Cornelia Huck
2020-08-06 10:53 ` [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests Thomas Huth
2020-08-06 11:23 ` Cornelia Huck
2020-08-06 11:58 ` Thomas Huth
2020-08-06 13:11 ` Cornelia Huck
2020-08-06 13:34 ` Janosch Frank
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.