[PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable

[PATCH for-5.2 v2 0/9] Continue booting in case the first device is not bootable

[Thomas Huth]

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

[PATCH for-5.2 v2 1/9] pc-bios/s390-ccw/Makefile: Compile with -std=gnu99, -fwrapv and -fno-common

[Thomas Huth]

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

[PATCH for-5.2 v2 2/9] pc-bios/s390-ccw: Move ipl-related code from main() into a separate function

[Thomas Huth]

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

[PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others

[Thomas Huth]

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

Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others

[Cornelia Huck]

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>

Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others

[Janosch Frank]

[-- 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 --]

Re: [PATCH for-5.2 v2 3/9] pc-bios/s390-ccw: Introduce ENODEV define and remove guards of others

[Thomas Huth]

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
>>
> 
> 

[PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function

[Thomas Huth]

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

Re: [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function

[Cornelia Huck]

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>

Re: [PATCH for-5.2 v2 4/9] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function

[Janosch Frank]

[-- 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 --]

[PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk

[Thomas Huth]

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

Re: [PATCH for-5.2 v2 5/9] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk

[Cornelia Huck]

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>

[PATCH for-5.2 v2 6/9] pc-bios/s390-ccw: Scan through all devices if no boot device specified

[Thomas Huth]

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

[PATCH for-5.2 v2 7/9] pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is bad

[Thomas Huth]

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

[PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()

[Thomas Huth]

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

Re: [PATCH for-5.2 v2 8/9] pc-bios/s390-ccw/main: Remove superfluous call to enable_subchannel()

[Cornelia Huck]

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>

[PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Thomas Huth]

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

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Cornelia Huck]

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)

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Thomas Huth]

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

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Cornelia Huck]

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>

Re: [PATCH for-5.2 v2 9/9] tests/qtest/cdrom: Add more s390x-related boot tests

[Janosch Frank]

[-- 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 --]