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

[Christian Borntraeger <borntraeger@linux.ibm.com>]

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)