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

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

[Thomas Huth]

If the user did not specify a "bootindex" property, the s390-ccw bios
tries to find a bootable device on its own. Unfortunately, it alwasy
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

Since this is quite uncomfortable and confusing for the users, and
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

Thomas Huth (6):
  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: 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 boot devices if none has been
    specified
  pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is
    bad

 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      |   2 +-
 pc-bios/s390-ccw/virtio-blkdev.c |   7 +-
 pc-bios/s390-ccw/virtio-scsi.c   |  25 +++--
 pc-bios/s390-ccw/virtio-scsi.h   |   2 +-
 7 files changed, 157 insertions(+), 92 deletions(-)

-- 
2.18.1

[PATCH for-5.2 1/6] 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.

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

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

[Claudio Imbrenda]

On Tue, 28 Jul 2020 20:37:29 +0200
Thomas Huth <thuth@redhat.com> wrote:

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

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:29 +0200
Thomas Huth <thuth@redhat.com> wrote:

> 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/Makefile | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Commonality is good :)

Acked-by: Cornelia Huck <cohuck@redhat.com>

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1848 bytes --]

On 7/28/20 8:37 PM, Thomas Huth wrote:
> 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.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

What was the argument for z900 again? TCG?
Maybe we can also move to z10?

Anyway:
Acked-by: Janosch Frank <frankja@linux.ibm.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
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thomas Huth]

On 31/07/2020 09.46, Janosch Frank wrote:
> On 7/28/20 8:37 PM, Thomas Huth wrote:
>> 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.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> What was the argument for z900 again? TCG?

Yes. As long as you can still select "-cpu z900" as parameter, the bios
should of course support that CPU level, too.

 Thomas

[PATCH for-5.2 2/6] 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.

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

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

[Claudio Imbrenda]

On Tue, 28 Jul 2020 20:37:30 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Let's move this part of the code into a separate function to be able
> to use it from multiple spots later.
> 
> 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 */

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:30 +0200
Thomas Huth <thuth@redhat.com> wrote:

> Let's move this part of the code into a separate function to be able
> to use it from multiple spots later.
> 
> 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");

Maybe "Boot failed: no supported device type"?

>      }
> +}
> +
> +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 */

Anyway,
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Thomas Huth]

On 29/07/2020 10.47, Cornelia Huck wrote:
> On Tue, 28 Jul 2020 20:37:30 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> Let's move this part of the code into a separate function to be able
>> to use it from multiple spots later.
>>
>> 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");
> 
> Maybe "Boot failed: no supported device type"?

The print_int right before already talks about "unexpected device type",
so I think the simple "Boot failed" should be enough?

>>      }
>> +}
>> +
>> +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 */
> 
> Anyway,
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Thanks!

 Thomas

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

[Cornelia Huck]

On Wed, 29 Jul 2020 13:05:08 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 29/07/2020 10.47, Cornelia Huck wrote:
> > On Tue, 28 Jul 2020 20:37:30 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> Let's move this part of the code into a separate function to be able
> >> to use it from multiple spots later.
> >>
> >> 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");  
> > 
> > Maybe "Boot failed: no supported device type"?  
> 
> The print_int right before already talks about "unexpected device type",
> so I think the simple "Boot failed" should be enough?

Yes, as long as we don't end up with other boot failures printing the
same messages.

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 1447 bytes --]

On 7/28/20 8:37 PM, Thomas Huth wrote:
> Let's move this part of the code into a separate function to be able
> to use it from multiple spots later.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Janosch Frank <frankja@linux.ibm.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 */
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH for-5.2 3/6] 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.

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..9477313188 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 check_sch_no(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 -EIO;
+    }
+    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 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;
+                }
+            }
+            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 = check_sch_no(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 3/6] pc-bios/s390-ccw: Move the inner logic of find_subch() to a separate function

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:31 +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.
> 
> 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..9477313188 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 check_sch_no(int dev_no, int sch_no)

check_subchannel()? You verify dev_no as well, if supplied.

> +{
> +    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 -EIO;

-ENODEV? It means that you either have no devices, or an invalid
subchannel set.

> +    }
> +    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 1st possibly bootable device */

"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.
 (...)

Otherwise, LGTM.

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

[Thomas Huth]

On 29/07/2020 10.54, Cornelia Huck wrote:
> On Tue, 28 Jul 2020 20:37:31 +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.
>>
>> 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..9477313188 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 check_sch_no(int dev_no, int sch_no)
> 
> check_subchannel()? You verify dev_no as well, if supplied.

Ok.

>> +{
>> +    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 -EIO;
> 
> -ENODEV? It means that you either have no devices, or an invalid
> subchannel set.

We don't have ENODEV in the s390-ccw bios... but I could introduce it, I
guess :-)

>> +    }
>> +    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 1st possibly bootable device */
> 
> "just return whether the device is possibly bootable" ?

That makes more sense now, indeed.

>> +    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.
>  (...)
> 
> Otherwise, LGTM.

Thanks!

 Thomas

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

[Cornelia Huck]

On Wed, 29 Jul 2020 13:13:17 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 29/07/2020 10.54, Cornelia Huck wrote:
> > On Tue, 28 Jul 2020 20:37:31 +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.
> >>
> >> 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..9477313188 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 check_sch_no(int dev_no, int sch_no)  
> > 
> > check_subchannel()? You verify dev_no as well, if supplied.  
> 
> Ok.
> 
> >> +{
> >> +    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 -EIO;  
> > 
> > -ENODEV? It means that you either have no devices, or an invalid
> > subchannel set.  
> 
> We don't have ENODEV in the s390-ccw bios... but I could introduce it, I
> guess :-)

I always forget that we have only a subset of error codes here :)

ENODEV looks like a reasonable value to have, though.

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

[Claudio Imbrenda]

On Tue, 28 Jul 2020 20:37:31 +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.
> 
> 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..9477313188 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 check_sch_no(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 -EIO;
> +    }
> +    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 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;
> +                }

here it seems you are returning true for any non-network virtio device,
is this the intended behaviour? (I know it was like this in the old
code)

like, non-block devices?

> +            }
> +            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 = check_sch_no(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;
>          }
>      }

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

[Thomas Huth]

On 03/08/2020 10.46, Claudio Imbrenda wrote:
> On Tue, 28 Jul 2020 20:37:31 +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.
>>
>> 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..9477313188 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 check_sch_no(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 -EIO;
>> +    }
>> +    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 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;
>> +                }
> 
> here it seems you are returning true for any non-network virtio device,
> is this the intended behaviour? (I know it was like this in the old
> code) like, non-block devices?

Yes. Other devices are already ignored by the virtio_is_supported() call
some lines earlier in this function.

 Thomas

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

[Claudio Imbrenda]

On Tue, 4 Aug 2020 15:24:09 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 03/08/2020 10.46, Claudio Imbrenda wrote:
> > On Tue, 28 Jul 2020 20:37:31 +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.
> >>
> >> 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..9477313188 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 check_sch_no(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 -EIO;
> >> +    }
> >> +    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 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;
> >> +                }  
> > 
> > here it seems you are returning true for any non-network virtio
> > device, is this the intended behaviour? (I know it was like this in
> > the old code) like, non-block devices?  
> 
> Yes. Other devices are already ignored by the virtio_is_supported()
> call some lines earlier in this function.

ah, that makes sense


Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

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

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 4366 bytes --]

On 7/28/20 8:37 PM, Thomas Huth wrote:
> Move the code to a separate function to be able to re-use it from a
> different spot later.
> 
> 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..9477313188 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 check_sch_no(int dev_no, int sch_no)

How about something like is_sch_no_bootable() or check_sch_no_bootable()?

check_sch_no doesn't tell me what you actually check for.

Otherwise LGTM

> +{
> +    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 -EIO;
> +    }
> +    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 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;
> +                }
> +            }
> +            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 = check_sch_no(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 4/6] 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          | 13 +++++++++----
 pc-bios/s390-ccw/s390-ccw.h      |  2 +-
 pc-bios/s390-ccw/virtio-blkdev.c |  7 ++++---
 pc-bios/s390-ccw/virtio-scsi.c   | 25 ++++++++++++++++++-------
 pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
 5 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 9477313188..3cd01cd80f 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 bool virtio_setup(void)
 {
     VDev *vdev = virtio_get_device();
     QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
@@ -233,9 +233,13 @@ 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);
+        if (!virtio_blk_setup_device(blk_schid)) {
+            return false;
+        }
         IPL_assert(virtio_ipl_disk_is_valid(), "No valid IPL device detected");
     }
+
+    return true;
 }
 
 static void ipl_boot_device(void)
@@ -246,8 +250,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()) {
+            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 36b884cced..a46d15431e 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -71,7 +71,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);
+bool 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..0746627b1e 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -263,7 +263,7 @@ uint64_t virtio_get_blocks(void)
     return 0;
 }
 
-void virtio_blk_setup_device(SubChannelId schid)
+bool virtio_blk_setup_device(SubChannelId schid)
 {
     VDev *vdev = virtio_get_device();
 
@@ -288,9 +288,10 @@ void virtio_blk_setup_device(SubChannelId schid)
             "Config: CDB size mismatch");
 
         sclp_print("Using virtio-scsi.\n");
-        virtio_scsi_setup(vdev);
-        break;
+        return virtio_scsi_setup(vdev);
     default:
         panic("\n! No IPL device available !\n");
     }
+
+    return true;
 }
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index eddfb8a7ad..4d05b02ed0 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 adds that information to the
+ * vdev->scsi_device structure.
+ * Returns true if SCSI device could be located, false otherwise
+ */
+static bool 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 true;
     }
 
     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 true; /* 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 true; /* 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 false;
 }
 
 int virtio_scsi_read_many(VDev *vdev,
@@ -322,7 +329,7 @@ static void scsi_parse_capacity_report(void *data,
     }
 }
 
-void virtio_scsi_setup(VDev *vdev)
+bool virtio_scsi_setup(VDev *vdev)
 {
     int retry_test_unit_ready = 3;
     uint8_t data[256];
@@ -332,7 +339,9 @@ void virtio_scsi_setup(VDev *vdev)
     int i;
 
     vdev->scsi_device = &default_scsi_device;
-    virtio_scsi_locate_device(vdev);
+    if (!virtio_scsi_locate_device(vdev)) {
+        return false;
+    }
 
     /* We have to "ping" the device before it becomes readable */
     while (!scsi_test_unit_ready(vdev)) {
@@ -417,4 +426,6 @@ void virtio_scsi_setup(VDev *vdev)
     }
     scsi_parse_capacity_report(data, &vdev->scsi_last_block,
                                (uint32_t *) &vdev->scsi_block_size);
+
+    return true;
 }
diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-ccw/virtio-scsi.h
index 4c4f4bbc31..2e405d0436 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);
+bool 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 4/6] pc-bios/s390-ccw: Do not bail out early if not finding a SCSI disk

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:32 +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          | 13 +++++++++----
>  pc-bios/s390-ccw/s390-ccw.h      |  2 +-
>  pc-bios/s390-ccw/virtio-blkdev.c |  7 ++++---
>  pc-bios/s390-ccw/virtio-scsi.c   | 25 ++++++++++++++++++-------
>  pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
>  5 files changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 9477313188..3cd01cd80f 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 bool virtio_setup(void)

Hm... I'm always wondering what to make of a function returning bool if
it is not of the "check something" variety. For a function called
virtio_setup(), I'd expect it to setup something, but would be unsure
what it meant if it returned true or false. Maybe better make it return
0 or a negative error?

(also applies to the other setup functions in this patch)

>  {
>      VDev *vdev = virtio_get_device();
>      QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;

(...)

> @@ -288,9 +288,10 @@ void virtio_blk_setup_device(SubChannelId schid)
>              "Config: CDB size mismatch");
>  
>          sclp_print("Using virtio-scsi.\n");
> -        virtio_scsi_setup(vdev);
> -        break;
> +        return virtio_scsi_setup(vdev);

You now have one case with a direct return, one that does not return,
and one that just continues. Can we make that a bit more consistent?

>      default:
>          panic("\n! No IPL device available !\n");
>      }
> +
> +    return true;
>  }
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
> index eddfb8a7ad..4d05b02ed0 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 adds that information to the
> + * vdev->scsi_device structure.

"and adds the information for the found device" ?

> + * Returns true if SCSI device could be located, false otherwise
> + */
> +static bool virtio_scsi_locate_device(VDev *vdev)

Here, I think a bool is fine. 0/-ENODEV would also make sense.

>  {
>      const uint16_t channel = 0; /* again, it's what QEMU does */
>      uint16_t target;

[PATCH for-5.2 5/6] pc-bios/s390-ccw: Scan through all boot devices if none has been 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
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 3cd01cd80f..0af872f9e3 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);
@@ -260,14 +248,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 = check_sch_no(-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

Re: [PATCH for-5.2 5/6] pc-bios/s390-ccw: Scan through all boot devices if none has been specified

[Claudio Imbrenda]

On Tue, 28 Jul 2020 20:37:33 +0200
Thomas Huth <thuth@redhat.com> wrote:

> If no boot device has been specified (via "bootindex=..."), the
> s390-ccw bios scans through all devices to find a bootable device.

maybe a better title for the patch is "scan through all devices if no
boot device specified" then, since it seems we will scan all
devices, not just "boot" devices?

> 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
> 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 3cd01cd80f..0af872f9e3 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);
> @@ -260,14 +248,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 = check_sch_no(-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 */

Re: [PATCH for-5.2 5/6] pc-bios/s390-ccw: Scan through all boot devices if none has been specified

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:33 +0200
Thomas Huth <thuth@redhat.com> wrote:

> 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
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/main.c | 46 +++++++++++++++++++++++++++--------------
>  1 file changed, 31 insertions(+), 15 deletions(-)

(...)

>  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();
> +    }

The one thing that's a bit surprising with the code is that
enable_subchannel() sticking out now. The code looking for a boot
device does that for all subchannels it looks at... but I think
find_boot_device() did that for specified devices already as well, so
it seems redundant?

Anyway, that's something that can be looked at later.

>  
>      panic("Failed to load OS from hard disk\n");
>      return 0; /* make compiler happy */

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH for-5.2 5/6] pc-bios/s390-ccw: Scan through all boot devices if none has been specified

[Thomas Huth]

On 05/08/2020 11.36, Cornelia Huck wrote:
> On Tue, 28 Jul 2020 20:37:33 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> 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
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  pc-bios/s390-ccw/main.c | 46 +++++++++++++++++++++++++++--------------
>>  1 file changed, 31 insertions(+), 15 deletions(-)
> 
> (...)
> 
>>  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();
>> +    }
> 
> The one thing that's a bit surprising with the code is that
> enable_subchannel() sticking out now. The code looking for a boot
> device does that for all subchannels it looks at... but I think
> find_boot_device() did that for specified devices already as well, so
> it seems redundant?
> 
> Anyway, that's something that can be looked at later.

Yes, I noticed that, too ... but yes, one clean-up step at a time. I've
put it on my todo-list for later.

>>  
>>      panic("Failed to load OS from hard disk\n");
>>      return 0; /* make compiler happy */
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Thanks!

 Thomas

[PATCH for-5.2 6/6] 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.

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 0af872f9e3..4026e0ef20 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -239,7 +239,7 @@ static void ipl_boot_device(void)
         break;
     case CU_TYPE_VIRTIO:
         if (virtio_setup()) {
-            zipl_load(); /* no return */
+            zipl_load();             /* Only returns in case of errors */
         }
         break;
     default:
-- 
2.18.1

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

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:34 +0200
Thomas Huth <thuth@redhat.com> wrote:

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

Does it make sense to do something like that for other device types as
well?

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

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

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

[Thomas Huth]

On 05/08/2020 12.04, Cornelia Huck wrote:
> On Tue, 28 Jul 2020 20:37:34 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> 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.
> 
> Does it make sense to do something like that for other device types as
> well?

It would be nice if we could do the same for virtio-scsi disks, but the
code is written in a way here that it will need much more thinking,
cleanups and time to get this done right...

 Thomas

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

[Cornelia Huck]

On Wed, 5 Aug 2020 12:08:59 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 05/08/2020 12.04, Cornelia Huck wrote:
> > On Tue, 28 Jul 2020 20:37:34 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> 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.  
> > 
> > Does it make sense to do something like that for other device types as
> > well?  
> 
> It would be nice if we could do the same for virtio-scsi disks, but the
> code is written in a way here that it will need much more thinking,
> cleanups and time to get this done right...

Yeah, let's just go for the lower-hanging fruit first, and then see if
someone's sufficiently motivated to grab a ladder.

Re: [PATCH for-5.2 0/6] Continue booting in case the first device is not bootable

[Cornelia Huck]

On Tue, 28 Jul 2020 20:37:28 +0200
Thomas Huth <thuth@redhat.com> wrote:

> If the user did not specify a "bootindex" property, the s390-ccw bios
> tries to find a bootable device on its own. Unfortunately, it alwasy
> 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
> 
> Since this is quite uncomfortable and confusing for the users, and
> 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.

I think this makes sense: If you IPL an LPAR or a z/VM guest, you are
always required to explicitly specify a device to load from (at least
as far as I remember). For QEMU, we have decided to allow starting
without an explicitly specified boot device as well, so we have already
deviated from the other s390 procedures. Let's just make that case
behave the same as everywhere else.

> 
> 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
> 
> Thomas Huth (6):
>   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: 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 boot devices if none has been
>     specified
>   pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is
>     bad
> 
>  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      |   2 +-
>  pc-bios/s390-ccw/virtio-blkdev.c |   7 +-
>  pc-bios/s390-ccw/virtio-scsi.c   |  25 +++--
>  pc-bios/s390-ccw/virtio-scsi.h   |   2 +-
>  7 files changed, 157 insertions(+), 92 deletions(-)
> 

Re: [PATCH for-5.2 0/6] Continue booting in case the first device is not bootable

[Viktor Mihajlovski]

On 7/28/20 8:37 PM, Thomas Huth wrote:
> If the user did not specify a "bootindex" property, the s390-ccw bios
> tries to find a bootable device on its own. Unfortunately, it alwasy
> 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
> 
> Since this is quite uncomfortable and confusing for the users, and
> 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.
> 

Looking at it from an architectural perspective: If an IPL Information 
Block specifying the boot device has been set and can be retrieved using 
Diagnose 308 it has to be respected, even if the device doesn't contain 
a bootable program. The boot has to fail in this case.

I had not the bandwidth to follow all code paths, but I gather that this 
is still the case with the series. So one can argue that these changes 
are taking care of an undefined situation (real hardware will always 
have the IPIB set).

As long as the architecture is not violated, I can live with the 
proposed changes. I however would like to point out that this only 
covers a corner case (no -boot or -device ..,bootindex specified). A VM 
defined and started with libvirt will always specify the boot device. 
Please don't create the impression that this patches will lead to the 
same behavior as on other platforms. It is still not possible to have an 
order list of potential boot devices in an architecture compliant way.

[...]

-- 
Kind Regards,
    Viktor

Re: [PATCH for-5.2 0/6] Continue booting in case the first device is not bootable

[Cornelia Huck]

[restored cc:s]

On Wed, 29 Jul 2020 13:42:05 +0200
Viktor Mihajlovski <mihajlov@linux.ibm.com> wrote:

> On 7/28/20 8:37 PM, Thomas Huth wrote:
> > If the user did not specify a "bootindex" property, the s390-ccw bios
> > tries to find a bootable device on its own. Unfortunately, it alwasy
> > 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
> > 
> > Since this is quite uncomfortable and confusing for the users, and
> > 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.
> >   
> 
> Looking at it from an architectural perspective: If an IPL Information 
> Block specifying the boot device has been set and can be retrieved using 
> Diagnose 308 it has to be respected, even if the device doesn't contain 
> a bootable program. The boot has to fail in this case.
> 
> I had not the bandwidth to follow all code paths, but I gather that this 
> is still the case with the series. So one can argue that these changes 
> are taking care of an undefined situation (real hardware will always 
> have the IPIB set).
> 
> As long as the architecture is not violated, I can live with the 
> proposed changes. I however would like to point out that this only 
> covers a corner case (no -boot or -device ..,bootindex specified). A VM 
> defined and started with libvirt will always specify the boot device. 
> Please don't create the impression that this patches will lead to the 
> same behavior as on other platforms. It is still not possible to have an 
> order list of potential boot devices in an architecture compliant way.

Yes, libvirt will always add this parameter. Still, I've seen confusion
generated by this behaviour, so this change sounds like a good idea to
me.

(Is there any possibility to enhance the architecture to provide a list
of devices in the future?)

Re: [PATCH for-5.2 0/6] Continue booting in case the first device is not bootable

[Thomas Huth]

On 29/07/2020 13.42, Viktor Mihajlovski wrote:
> 
> 
> On 7/28/20 8:37 PM, Thomas Huth wrote:
>> If the user did not specify a "bootindex" property, the s390-ccw bios
>> tries to find a bootable device on its own. Unfortunately, it alwasy
>> 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
>>
>> Since this is quite uncomfortable and confusing for the users, and
>> 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.
>>
> 
> Looking at it from an architectural perspective: If an IPL Information
> Block specifying the boot device has been set and can be retrieved using
> Diagnose 308 it has to be respected, even if the device doesn't contain
> a bootable program. The boot has to fail in this case.
> 
> I had not the bandwidth to follow all code paths, but I gather that this
> is still the case with the series.

Right. Just to be sure, I just double-checked with:

... -device virtio-blk,drive=baddrive,bootindex=1 \
    -device virtio-blk,drive=gooddrive

and indeed, the s390-ccw bios only checks the "baddrive" here and
refuses to boot.

> So one can argue that these changes
> are taking care of an undefined situation (real hardware will always
> have the IPIB set).
> 
> As long as the architecture is not violated, I can live with the
> proposed changes.

Thanks!

> I however would like to point out that this only
> covers a corner case (no -boot or -device ..,bootindex specified).

Sure. We™ should/could maybe also add some more documentation to

 https://www.qemu.org/docs/master/system/target-s390x.html

to make it more clear to the "unexperienced" qemu-system-s390x users
that "bootindex" is the preferred / architected way of booting there.

> Please don't create the impression that this patches will lead to the
> same behavior as on other platforms.
Ok, I'll try to state that more clearly in the cover letter of v2.

 Thomas

Re: [PATCH for-5.2 0/6] Continue booting in case the first device is not bootable

[Janosch Frank]

[-- Attachment #1.1: Type: text/plain, Size: 2216 bytes --]

On 7/28/20 8:37 PM, Thomas Huth wrote:
> If the user did not specify a "bootindex" property, the s390-ccw bios
> tries to find a bootable device on its own. Unfortunately, it alwasy
> 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
> 
> Since this is quite uncomfortable and confusing for the users, and
> 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.

Are you planning to add a/re-use an existing test for this?
The PXE test already helped me quite a lot to find my mistakes for the
bios cleanup series.

> 
>  Thomas
> 
> Thomas Huth (6):
>   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: 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 boot devices if none has been
>     specified
>   pc-bios/s390-ccw: Allow booting in case the first virtio-blk disk is
>     bad
> 
>  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      |   2 +-
>  pc-bios/s390-ccw/virtio-blkdev.c |   7 +-
>  pc-bios/s390-ccw/virtio-scsi.c   |  25 +++--
>  pc-bios/s390-ccw/virtio-scsi.h   |   2 +-
>  7 files changed, 157 insertions(+), 92 deletions(-)
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-5.2 0/6] Continue booting in case the first device is not bootable

[Thomas Huth]

[-- Attachment #1.1: Type: text/plain, Size: 1322 bytes --]

On 04/08/2020 16.49, Janosch Frank wrote:
> On 7/28/20 8:37 PM, Thomas Huth wrote:
>> If the user did not specify a "bootindex" property, the s390-ccw bios
>> tries to find a bootable device on its own. Unfortunately, it alwasy
>> 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
>>
>> Since this is quite uncomfortable and confusing for the users, and
>> 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.
> 
> Are you planning to add a/re-use an existing test for this?

Not yet, but maybe the cdrom-test could be used for testing some
scenarios. I'll have a look...

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]