[PATCH 0/2] Fix booting from non-DASD disks with sector size of 4096

[PATCH 0/2] Fix booting from non-DASD disks with sector size of 4096

[Thomas Huth]

The s390-ccw bios fails to boot from non-DASD disks that have a
sector size of 4096. Fix it by relaxing the check for the disk
geometries.

Thomas Huth (2):
  pc-bios/s390-ccw/virtio-blkdev: Simplify/fix
    virtio_ipl_disk_is_valid()
  pc-bios/s390-ccw/virtio-blkdev: Remove virtio_assume_scsi()

 pc-bios/s390-ccw/virtio.h        |  2 -
 pc-bios/s390-ccw/virtio-blkdev.c | 65 ++++----------------------------
 2 files changed, 7 insertions(+), 60 deletions(-)

-- 
2.31.1

[PATCH 1/2] pc-bios/s390-ccw/virtio-blkdev: Simplify/fix virtio_ipl_disk_is_valid()

[Thomas Huth]

The s390-ccw bios fails to boot if the boot disk is a virtio-blk
disk with a sector size of 4096. For example:

 dasdfmt -b 4096 -d cdl -y -p -M quick /dev/dasdX
 fdasd -a /dev/dasdX
 install a guest onto /dev/dasdX1 using virtio-blk
 qemu-system-s390x -nographic -hda /dev/dasdX1

The bios then bails out with:

 ! Cannot read block 0 !

Looking at virtio_ipl_disk_is_valid() and especially the function
virtio_disk_is_scsi(), it does not really make sense that we expect
only such a limited disk geometry (like a block size of 512) for
out boot disks. Let's relax the check and allow everything that
remotely looks like a sane disk.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/virtio.h        |  2 --
 pc-bios/s390-ccw/virtio-blkdev.c | 41 ++++++--------------------------
 2 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 19fceb6495..07fdcabd9f 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -186,8 +186,6 @@ void virtio_assume_scsi(void);
 void virtio_assume_eckd(void);
 void virtio_assume_iso9660(void);
 
-extern bool virtio_disk_is_scsi(void);
-extern bool virtio_disk_is_eckd(void);
 extern bool virtio_ipl_disk_is_valid(void);
 extern int virtio_get_block_size(void);
 extern uint8_t virtio_get_heads(void);
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 7d35050292..b5506bb29d 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -166,46 +166,19 @@ void virtio_assume_eckd(void)
         virtio_eckd_sectors_for_block_size(vdev->config.blk.blk_size);
 }
 
-bool virtio_disk_is_scsi(void)
-{
-    VDev *vdev = virtio_get_device();
-
-    if (vdev->guessed_disk_nature == VIRTIO_GDN_SCSI) {
-        return true;
-    }
-    switch (vdev->senseid.cu_model) {
-    case VIRTIO_ID_BLOCK:
-        return (vdev->config.blk.geometry.heads == 255)
-            && (vdev->config.blk.geometry.sectors == 63)
-            && (virtio_get_block_size()  == VIRTIO_SCSI_BLOCK_SIZE);
-    case VIRTIO_ID_SCSI:
-        return true;
-    }
-    return false;
-}
-
-bool virtio_disk_is_eckd(void)
+bool virtio_ipl_disk_is_valid(void)
 {
+    int blksize = virtio_get_block_size();
     VDev *vdev = virtio_get_device();
-    const int block_size = virtio_get_block_size();
 
-    if (vdev->guessed_disk_nature == VIRTIO_GDN_DASD) {
+    if (vdev->guessed_disk_nature == VIRTIO_GDN_SCSI ||
+        vdev->guessed_disk_nature == VIRTIO_GDN_DASD) {
         return true;
     }
-    switch (vdev->senseid.cu_model) {
-    case VIRTIO_ID_BLOCK:
-        return (vdev->config.blk.geometry.heads == 15)
-            && (vdev->config.blk.geometry.sectors ==
-                virtio_eckd_sectors_for_block_size(block_size));
-    case VIRTIO_ID_SCSI:
-        return false;
-    }
-    return false;
-}
 
-bool virtio_ipl_disk_is_valid(void)
-{
-    return virtio_disk_is_scsi() || virtio_disk_is_eckd();
+    return (vdev->senseid.cu_model == VIRTIO_ID_BLOCK ||
+            vdev->senseid.cu_model == VIRTIO_ID_SCSI) &&
+           blksize >= 512 && blksize <= 4096;
 }
 
 int virtio_get_block_size(void)
-- 
2.31.1

Re: [PATCH 1/2] pc-bios/s390-ccw/virtio-blkdev: Simplify/fix virtio_ipl_disk_is_valid()

[Christian Borntraeger]

Am 24.06.22 um 10:50 schrieb Thomas Huth:
> The s390-ccw bios fails to boot if the boot disk is a virtio-blk
> disk with a sector size of 4096. For example:
> 
>   dasdfmt -b 4096 -d cdl -y -p -M quick /dev/dasdX
>   fdasd -a /dev/dasdX
>   install a guest onto /dev/dasdX1 using virtio-blk
>   qemu-system-s390x -nographic -hda /dev/dasdX1

Interestingly enough a real DASD (dasdX and not dasdX1) did work in the
past and I also successfully uses an NVMe disk. So I guess the NVMe
was 512 byte sector size then?

> 
> The bios then bails out with:
> 
>   ! Cannot read block 0 !
> 
> Looking at virtio_ipl_disk_is_valid() and especially the function
> virtio_disk_is_scsi(), it does not really make sense that we expect
> only such a limited disk geometry (like a block size of 512) for
> out boot disks. Let's relax the check and allow everything that
> remotely looks like a sane disk.

I agree that we should accept as much list-directed IPL formats as possible.
I have not fully looked into your changes though.

> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   pc-bios/s390-ccw/virtio.h        |  2 --
>   pc-bios/s390-ccw/virtio-blkdev.c | 41 ++++++--------------------------
>   2 files changed, 7 insertions(+), 36 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index 19fceb6495..07fdcabd9f 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -186,8 +186,6 @@ void virtio_assume_scsi(void);
>   void virtio_assume_eckd(void);
>   void virtio_assume_iso9660(void);
>   
> -extern bool virtio_disk_is_scsi(void);
> -extern bool virtio_disk_is_eckd(void);
>   extern bool virtio_ipl_disk_is_valid(void);
>   extern int virtio_get_block_size(void);
>   extern uint8_t virtio_get_heads(void);
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> index 7d35050292..b5506bb29d 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -166,46 +166,19 @@ void virtio_assume_eckd(void)
>           virtio_eckd_sectors_for_block_size(vdev->config.blk.blk_size);
>   }
>   
> -bool virtio_disk_is_scsi(void)
> -{
> -    VDev *vdev = virtio_get_device();
> -
> -    if (vdev->guessed_disk_nature == VIRTIO_GDN_SCSI) {
> -        return true;
> -    }
> -    switch (vdev->senseid.cu_model) {
> -    case VIRTIO_ID_BLOCK:
> -        return (vdev->config.blk.geometry.heads == 255)
> -            && (vdev->config.blk.geometry.sectors == 63)
> -            && (virtio_get_block_size()  == VIRTIO_SCSI_BLOCK_SIZE);
> -    case VIRTIO_ID_SCSI:
> -        return true;
> -    }
> -    return false;
> -}
> -
> -bool virtio_disk_is_eckd(void)
> +bool virtio_ipl_disk_is_valid(void)
>   {
> +    int blksize = virtio_get_block_size();
>       VDev *vdev = virtio_get_device();
> -    const int block_size = virtio_get_block_size();
>   
> -    if (vdev->guessed_disk_nature == VIRTIO_GDN_DASD) {
> +    if (vdev->guessed_disk_nature == VIRTIO_GDN_SCSI ||
> +        vdev->guessed_disk_nature == VIRTIO_GDN_DASD) {
>           return true;
>       }
> -    switch (vdev->senseid.cu_model) {
> -    case VIRTIO_ID_BLOCK:
> -        return (vdev->config.blk.geometry.heads == 15)
> -            && (vdev->config.blk.geometry.sectors ==
> -                virtio_eckd_sectors_for_block_size(block_size));
> -    case VIRTIO_ID_SCSI:
> -        return false;
> -    }
> -    return false;
> -}
>   
> -bool virtio_ipl_disk_is_valid(void)
> -{
> -    return virtio_disk_is_scsi() || virtio_disk_is_eckd();
> +    return (vdev->senseid.cu_model == VIRTIO_ID_BLOCK ||
> +            vdev->senseid.cu_model == VIRTIO_ID_SCSI) &&
> +           blksize >= 512 && blksize <= 4096;
>   }
>   
>   int virtio_get_block_size(void)

Re: [PATCH 1/2] pc-bios/s390-ccw/virtio-blkdev: Simplify/fix virtio_ipl_disk_is_valid()

[Thomas Huth]

On 24/06/2022 11.20, Christian Borntraeger wrote:
> 
> 
> Am 24.06.22 um 10:50 schrieb Thomas Huth:
>> The s390-ccw bios fails to boot if the boot disk is a virtio-blk
>> disk with a sector size of 4096. For example:
>>
>>   dasdfmt -b 4096 -d cdl -y -p -M quick /dev/dasdX
>>   fdasd -a /dev/dasdX
>>   install a guest onto /dev/dasdX1 using virtio-blk
>>   qemu-system-s390x -nographic -hda /dev/dasdX1
> 
> Interestingly enough a real DASD (dasdX and not dasdX1) did work in the
> past and I also successfully uses an NVMe disk. So I guess the NVMe
> was 512 byte sector size then?

If you're using a full DASD, I think this was working thanks to the 
virtio_disk_is_eckd() function recognizing the geometry.

For NVMe disk - no clue. It either used 512 sectors, or it was at least 
accidentally still able to deal with the 512-byte sector requests after 
virtio_assume_scsi() "fixed" up the geometry. If you've got some spare 
minutes and still have access to those disks, it would be interesting to 
know if the boot still works there with my patches applied.

  Thomas

Re: [PATCH 1/2] pc-bios/s390-ccw/virtio-blkdev: Simplify/fix virtio_ipl_disk_is_valid()

[Thomas Huth]

On 24/06/2022 10.50, Thomas Huth wrote:
> The s390-ccw bios fails to boot if the boot disk is a virtio-blk
> disk with a sector size of 4096. For example:
> 
>   dasdfmt -b 4096 -d cdl -y -p -M quick /dev/dasdX
>   fdasd -a /dev/dasdX
>   install a guest onto /dev/dasdX1 using virtio-blk
>   qemu-system-s390x -nographic -hda /dev/dasdX1
> 
> The bios then bails out with:
> 
>   ! Cannot read block 0 !
> 
> Looking at virtio_ipl_disk_is_valid() and especially the function
> virtio_disk_is_scsi(), it does not really make sense that we expect
> only such a limited disk geometry (like a block size of 512) for
> out boot disks. Let's relax the check and allow everything that
> remotely looks like a sane disk.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   pc-bios/s390-ccw/virtio.h        |  2 --
>   pc-bios/s390-ccw/virtio-blkdev.c | 41 ++++++--------------------------
>   2 files changed, 7 insertions(+), 36 deletions(-)

I just noticed that this breaks booting ISO images via the "-cdrom" option 
... looks like this needs some additional fixes on top.

  Thomas

[PATCH 2/2] pc-bios/s390-ccw/virtio-blkdev: Remove virtio_assume_scsi()

[Thomas Huth]

The virtio_assume_scsi() function is very questionable: First, it
is only called for virtio-blk, and not for virtio-scsi, so the naming
is already quite confusing. Second, it is called if we detected a
"invalid" IPL disk, trying to fix it by blindly setting a sector
size of 512. This of course won't work in most cases since disks
might have a different sector size for a reason.

Thus let's remove this strange function now. The calling code can
also be removed completely, since there is another spot in main.c
that does "IPL_assert(virtio_ipl_disk_is_valid(), ...)" to make
sure that we do not try to IPL from an invalid device.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/virtio-blkdev.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index b5506bb29d..9ba28c6f54 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -112,23 +112,6 @@ VirtioGDN virtio_guessed_disk_nature(void)
     return virtio_get_device()->guessed_disk_nature;
 }
 
-void virtio_assume_scsi(void)
-{
-    VDev *vdev = virtio_get_device();
-
-    switch (vdev->senseid.cu_model) {
-    case VIRTIO_ID_BLOCK:
-        vdev->guessed_disk_nature = VIRTIO_GDN_SCSI;
-        vdev->config.blk.blk_size = VIRTIO_SCSI_BLOCK_SIZE;
-        vdev->config.blk.physical_block_exp = 0;
-        vdev->blk_factor = 1;
-        break;
-    case VIRTIO_ID_SCSI:
-        vdev->scsi_block_size = VIRTIO_SCSI_BLOCK_SIZE;
-        break;
-    }
-}
-
 void virtio_assume_iso9660(void)
 {
     VDev *vdev = virtio_get_device();
@@ -247,13 +230,6 @@ int virtio_blk_setup_device(SubChannelId schid)
     switch (vdev->senseid.cu_model) {
     case VIRTIO_ID_BLOCK:
         sclp_print("Using virtio-blk.\n");
-        if (!virtio_ipl_disk_is_valid()) {
-            /* make sure all getters but blocksize return 0 for
-             * invalid IPL disk
-             */
-            memset(&vdev->config.blk, 0, sizeof(vdev->config.blk));
-            virtio_assume_scsi();
-        }
         break;
     case VIRTIO_ID_SCSI:
         IPL_assert(vdev->config.scsi.sense_size == VIRTIO_SCSI_SENSE_SIZE,
-- 
2.31.1