Re: [PATCH 1/2] pc-bios/s390-ccw/virtio-blkdev: Simplify/fix virtio_ipl_disk_is_valid()
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 --- 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
Re: [PATCH 1/2] pc-bios/s390-ccw/virtio-blkdev: Simplify/fix virtio_ipl_disk_is_valid()
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()
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
---
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)
[PATCH 1/2] pc-bios/s390-ccw/virtio-blkdev: Simplify/fix virtio_ipl_disk_is_valid()
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
---
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
