[PATCH 00/12] s390-ccw bios fixes and clean-ups

[PATCH 00/12] s390-ccw bios fixes and clean-ups

[Thomas Huth]

The s390-ccw bios currently fails to boot a guest that has been
installed into a partition on a DASD drive with 4k sectors.
While trying to fix this, I noticed a couple of other problems
and possibilites for clean-ups that this series is trying to
address now.

The first patch is an old one which I already sent out a year
ago, but it got never included since there were no other bios-related
patches pending and this patch alone never justified a bios rebuild.

The main problem (with the bios not getting the geometry right)
comes from the virtio_disk_is_scsi() [sic] function in the file
pc-bios/s390-ccw/virtio-blkdev.c - this more or less always
currently reports that the geometry is wrong for virtio-block
devices, which causes the code to call virtio_assume_scsi() that
sets a "guessed" geometry with 512-byte sectors. To get this fixed
and cleaned up, a couple of other preparing patches were necessary,
which you can find in patches 2 - 5.

While working on the above problem, I also noticed that the virtio
init sequence is not done according to the virtio specification.
It's currently not a problem since QEMU is very forgiving, but we
should clean this up anyway to be sure to avoid future problems. This
is done in patches 6 - 10.

Patch 11 simply beautifies up an oddity in a header, and patch 12
silences a compiler warning with Clang.

I know, it's more than one topic in this series now, but I wanted to
send them out as one series since some of the patches depend on each
other and I've got to rebuild the bios images at the end anyway, so
it's easier to keep everything together.

Thomas Huth (12):
  pc-bios/s390-ccw: Add a proper prototype for main()
  pc-bios/s390-ccw/virtio: Introduce a macro for the DASD block size
  pc-bios/s390-ccw/bootmap: Improve the guessing logic in
    zipl_load_vblk()
  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: Set missing status bits while initializing
  pc-bios/s390-ccw/virtio: Read device config after feature negotiation
  pc-bios/s390-ccw/virtio: Beautify the code for reading virtqueue
    configuration
  pc-bios/s390-ccw: Split virtio-scsi code from
    virtio_blk_setup_device()
  pc-bios/s390-ccw/virtio-blkdev: Request the right feature bits
  pc-bios/s390-ccw/virtio: Remove "extern" keyword from prototypes
  pc-bios/s390-ccw/netboot.mak: Ignore Clang's warnings about GNU
    extensions

 pc-bios/s390-ccw/netboot.mak     |  7 ++-
 pc-bios/s390-ccw/s390-ccw.h      |  1 +
 pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
 pc-bios/s390-ccw/virtio.h        | 16 +++---
 pc-bios/s390-ccw/bootmap.c       | 25 +++++++--
 pc-bios/s390-ccw/main.c          | 27 ++++++----
 pc-bios/s390-ccw/virtio-blkdev.c | 91 +++++---------------------------
 pc-bios/s390-ccw/virtio-scsi.c   | 19 ++++++-
 pc-bios/s390-ccw/virtio.c        | 28 ++++++----
 9 files changed, 103 insertions(+), 113 deletions(-)

-- 
2.31.1

[PATCH 01/12] pc-bios/s390-ccw: Add a proper prototype for main()

[Thomas Huth]

Older versions of Clang complain if there is no prototype for main().
Add one, and while we're at it, make sure that we use the same type
for main.c and netmain.c - since the return value does not matter,
declare the return type of main() as "void".

Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/s390-ccw.h | 1 +
 pc-bios/s390-ccw/main.c     | 3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 79db69ff54..b88e0550ab 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -57,6 +57,7 @@ void write_subsystem_identification(void);
 void write_iplb_location(void);
 extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
 unsigned int get_loadparm_index(void);
+void main(void);
 
 /* sclp.c */
 void sclp_print(const char *string);
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 5d2b7ba94d..835341457d 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -281,7 +281,7 @@ static void probe_boot_device(void)
     sclp_print("Could not find a suitable boot device (none specified)\n");
 }
 
-int main(void)
+void main(void)
 {
     sclp_setup();
     css_setup();
@@ -294,5 +294,4 @@ int main(void)
     }
 
     panic("Failed to load OS from hard disk\n");
-    return 0; /* make compiler happy */
 }
-- 
2.31.1

[PATCH 02/12] pc-bios/s390-ccw/virtio: Introduce a macro for the DASD block size

[Thomas Huth]

Use VIRTIO_DASD_BLOCK_SIZE instead of the magic value 4096.

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

diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 19fceb6495..c2c17c29ca 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -198,6 +198,7 @@ extern int virtio_read_many(ulong sector, void *load_addr, int sec_num);
 #define VIRTIO_SECTOR_SIZE 512
 #define VIRTIO_ISO_BLOCK_SIZE 2048
 #define VIRTIO_SCSI_BLOCK_SIZE 512
+#define VIRTIO_DASD_BLOCK_SIZE 4096
 
 static inline ulong virtio_sector_adjust(ulong sector)
 {
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 7d35050292..49ed2b4bee 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -155,7 +155,7 @@ void virtio_assume_eckd(void)
     vdev->config.blk.physical_block_exp = 0;
     switch (vdev->senseid.cu_model) {
     case VIRTIO_ID_BLOCK:
-        vdev->config.blk.blk_size = 4096;
+        vdev->config.blk.blk_size = VIRTIO_DASD_BLOCK_SIZE;
         break;
     case VIRTIO_ID_SCSI:
         vdev->config.blk.blk_size = vdev->scsi_block_size;
-- 
2.31.1

[PATCH 03/12] pc-bios/s390-ccw/bootmap: Improve the guessing logic in zipl_load_vblk()

[Thomas Huth]

The logic of trying an final ISO or ECKD boot on virtio-block devices is
very weird: Since the geometry hardly ever matches in virtio_disk_is_scsi(),
virtio_blk_setup_device() always sets a "guessed" disk geometry via
virtio_assume_scsi() (which is certainly also wrong in a lot of cases).

zipl_load_vblk() then sees that there's been a "virtio_guessed_disk_nature"
and tries to fix up the geometry again via virtio_assume_iso9660() before
always trying to do ipl_iso_el_torito(). That's a very brain-twisting
way of attempting to boot from ISO images, which won't work anymore after
the following patches that will clean up the virtio_assume_scsi() mess
(and thus get rid of the "virtio_guessed_disk_nature" here).

Let's try a better approach instead: ISO files always have a magic
string "CD001" at offset 0x8001 (see e.g. the ECMA-119 specification)
which we can use to decide whether we should try to boot in ISO 9660
mode (which we should also try if we see a sector size of 2048).

And if we were not able to boot in ISO mode here, the final boot attempt
before panicking is to boot in ECKD mode. Since this is our last boot
attempt anyway, simply always assume the ECKD geometry here (if the sector
size was not 4096 yet), so that we also do not depend on the guessed disk
geometry from virtio_blk_setup_device() here anymore.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/bootmap.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 56411ab3b6..3181b05382 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -780,18 +780,35 @@ static void ipl_iso_el_torito(void)
     }
 }
 
+/**
+ * Detect whether we're trying to boot from an .ISO image.
+ * These always have a signature string "CD001" at offset 0x8001.
+ */
+static bool has_iso_signature(void)
+{
+    if (virtio_read(0x8000 / virtio_get_block_size(), sec)) {
+        return false;
+    }
+
+    return !memcmp("CD001", &sec[1], 5);
+}
+
 /***********************************************************************
  * Bus specific IPL sequences
  */
 
 static void zipl_load_vblk(void)
 {
-    if (virtio_guessed_disk_nature()) {
-        virtio_assume_iso9660();
+    int blksize = virtio_get_block_size();
+
+    if (blksize == VIRTIO_ISO_BLOCK_SIZE || has_iso_signature()) {
+        if (blksize != VIRTIO_ISO_BLOCK_SIZE) {
+            virtio_assume_iso9660();
+        }
+        ipl_iso_el_torito();
     }
-    ipl_iso_el_torito();
 
-    if (virtio_guessed_disk_nature()) {
+    if (blksize != VIRTIO_DASD_BLOCK_SIZE) {
         sclp_print("Using guessed DASD geometry.\n");
         virtio_assume_eckd();
     }
-- 
2.31.1

[PATCH 04/12] 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
our 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 c2c17c29ca..8f917d47a9 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 49ed2b4bee..b14cbc3d9e 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

[PATCH 05/12] 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.h        |  1 -
 pc-bios/s390-ccw/virtio-blkdev.c | 24 ------------------------
 2 files changed, 25 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 8f917d47a9..303438f159 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -182,7 +182,6 @@ enum guessed_disk_nature_type {
 typedef enum guessed_disk_nature_type VirtioGDN;
 
 VirtioGDN virtio_guessed_disk_nature(void);
-void virtio_assume_scsi(void);
 void virtio_assume_eckd(void);
 void virtio_assume_iso9660(void);
 
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index b14cbc3d9e..11820754f3 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

[PATCH 06/12] pc-bios/s390-ccw/virtio: Set missing status bits while initializing

[Thomas Huth]

According chapter "3.1.1 Driver Requirements: Device Initialization"
of the Virtio specification (v1.1), a driver for a device has to set
the ACKNOWLEDGE and DRIVER bits in the status field after resetting
the device. The s390-ccw bios skipped these steps so far and seems
like QEMU never cared. Anyway, it's better to follow the spec, so
let's set these bits now in the right spots, too.

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/virtio.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 5d2c6e3381..4e85a2eb82 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -220,7 +220,7 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
 void virtio_setup_ccw(VDev *vdev)
 {
     int i, rc, cfg_size = 0;
-    unsigned char status = VIRTIO_CONFIG_S_DRIVER_OK;
+    uint8_t status;
     struct VirtioFeatureDesc {
         uint32_t features;
         uint8_t index;
@@ -234,6 +234,10 @@ void virtio_setup_ccw(VDev *vdev)
 
     run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
 
+    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
+    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
+    IPL_assert(rc == 0, "Could not write ACKNOWLEDGE status to host");
+
     switch (vdev->senseid.cu_model) {
     case VIRTIO_ID_NET:
         vdev->nr_vqs = 2;
@@ -253,6 +257,11 @@ void virtio_setup_ccw(VDev *vdev)
     default:
         panic("Unsupported virtio device\n");
     }
+
+    status |= VIRTIO_CONFIG_S_DRIVER;
+    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
+    IPL_assert(rc == 0, "Could not write DRIVER status to host");
+
     IPL_assert(
         run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false) == 0,
        "Could not get block device configuration");
@@ -291,9 +300,10 @@ void virtio_setup_ccw(VDev *vdev)
             run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info), false) == 0,
             "Cannot set VQ info");
     }
-    IPL_assert(
-        run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false) == 0,
-        "Could not write status to host");
+
+    status |= VIRTIO_CONFIG_S_DRIVER_OK;
+    rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
+    IPL_assert(rc == 0, "Could not write DRIVER_OK status to host");
 }
 
 bool virtio_is_supported(SubChannelId schid)
-- 
2.31.1

[PATCH 07/12] pc-bios/s390-ccw/virtio: Read device config after feature negotiation

[Thomas Huth]

Feature negotiation should be done first, since some fields in the
config area can depend on the negotiated features and thus should
rather be read afterwards.

While we're at it, also adjust the error message here a little bit
(the code is nowadays used for non-block virtio devices, too).

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

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 4e85a2eb82..d8c2b52710 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -262,10 +262,6 @@ void virtio_setup_ccw(VDev *vdev)
     rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false);
     IPL_assert(rc == 0, "Could not write DRIVER status to host");
 
-    IPL_assert(
-        run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false) == 0,
-       "Could not get block device configuration");
-
     /* Feature negotiation */
     for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) {
         feats.features = 0;
@@ -278,6 +274,9 @@ void virtio_setup_ccw(VDev *vdev)
         IPL_assert(rc == 0, "Could not set features bits");
     }
 
+    rc = run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false);
+    IPL_assert(rc == 0, "Could not get virtio device configuration");
+
     for (i = 0; i < vdev->nr_vqs; i++) {
         VqInfo info = {
             .queue = (unsigned long long) ring_area + (i * VIRTIO_RING_SIZE),
-- 
2.31.1

[PATCH 08/12] pc-bios/s390-ccw/virtio: Beautify the code for reading virtqueue configuration

[Thomas Huth]

It looks nicer if we separate the run_ccw() from the IPL_assert()
statement, and the error message should talk about "virtio device"
instead of "block device", since this code is nowadays used for
non-block (i.e. network) devices, too.

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

diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index d8c2b52710..f37510f312 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -289,9 +289,8 @@ void virtio_setup_ccw(VDev *vdev)
             .num = 0,
         };
 
-        IPL_assert(
-            run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config), false) == 0,
-            "Could not get block device VQ configuration");
+        rc = run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config), false);
+        IPL_assert(rc == 0, "Could not get virtio device VQ configuration");
         info.num = config.num;
         vring_init(&vdev->vrings[i], &info);
         vdev->vrings[i].schid = vdev->schid;
-- 
2.31.1

[PATCH 09/12] pc-bios/s390-ccw: Split virtio-scsi code from virtio_blk_setup_device()

[Thomas Huth]

The next patch is going to add more virtio-block specific code to
virtio_blk_setup_device(), and if the virtio-scsi code is also in
there, this is more cumbersome. And the calling function virtio_setup()
in main.c looks at the device type already anyway, so it's more
logical to separate the virtio-scsi stuff into a new function in
virtio-scsi.c instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
 pc-bios/s390-ccw/main.c          | 24 +++++++++++++++++-------
 pc-bios/s390-ccw/virtio-blkdev.c | 20 ++------------------
 pc-bios/s390-ccw/virtio-scsi.c   | 19 ++++++++++++++++++-
 4 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-ccw/virtio-scsi.h
index 4b14c2c2f9..e6b6cd4815 100644
--- a/pc-bios/s390-ccw/virtio-scsi.h
+++ b/pc-bios/s390-ccw/virtio-scsi.h
@@ -67,8 +67,8 @@ static inline bool virtio_scsi_response_ok(const VirtioScsiCmdResp *r)
         return r->response == VIRTIO_SCSI_S_OK && r->status == CDB_STATUS_GOOD;
 }
 
-int virtio_scsi_setup(VDev *vdev);
 int virtio_scsi_read_many(VDev *vdev,
                           ulong sector, void *load_addr, int sec_num);
+int virtio_scsi_setup_device(SubChannelId schid);
 
 #endif /* VIRTIO_SCSI_H */
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 835341457d..a2def83e82 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -14,6 +14,7 @@
 #include "s390-ccw.h"
 #include "cio.h"
 #include "virtio.h"
+#include "virtio-scsi.h"
 #include "dasd-ipl.h"
 
 char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
@@ -218,6 +219,7 @@ static int virtio_setup(void)
 {
     VDev *vdev = virtio_get_device();
     QemuIplParameters *early_qipl = (QemuIplParameters *)QIPL_ADDRESS;
+    int ret;
 
     memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
 
@@ -225,18 +227,26 @@ static int virtio_setup(void)
         menu_setup();
     }
 
-    if (virtio_get_device_type() == VIRTIO_ID_NET) {
+    switch (vdev->senseid.cu_model) {
+    case VIRTIO_ID_NET:
         sclp_print("Network boot device detected\n");
         vdev->netboot_start_addr = qipl.netboot_start_addr;
-    } else {
-        int ret = virtio_blk_setup_device(blk_schid);
-        if (ret) {
-            return ret;
-        }
+        return 0;
+    case VIRTIO_ID_BLOCK:
+        ret = virtio_blk_setup_device(blk_schid);
+        break;
+    case VIRTIO_ID_SCSI:
+        ret = virtio_scsi_setup_device(blk_schid);
+        break;
+    default:
+        panic("\n! No IPL device available !\n");
+    }
+
+    if (!ret) {
         IPL_assert(virtio_ipl_disk_is_valid(), "No valid IPL device detected");
     }
 
-    return 0;
+    return ret;
 }
 
 static void ipl_boot_device(void)
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 11820754f3..a2b157b2c0 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -222,27 +222,11 @@ uint64_t virtio_get_blocks(void)
 int virtio_blk_setup_device(SubChannelId schid)
 {
     VDev *vdev = virtio_get_device();
-    int ret = 0;
 
     vdev->schid = schid;
     virtio_setup_ccw(vdev);
 
-    switch (vdev->senseid.cu_model) {
-    case VIRTIO_ID_BLOCK:
-        sclp_print("Using virtio-blk.\n");
-        break;
-    case VIRTIO_ID_SCSI:
-        IPL_assert(vdev->config.scsi.sense_size == VIRTIO_SCSI_SENSE_SIZE,
-            "Config: sense size mismatch");
-        IPL_assert(vdev->config.scsi.cdb_size == VIRTIO_SCSI_CDB_SIZE,
-            "Config: CDB size mismatch");
+    sclp_print("Using virtio-blk.\n");
 
-        sclp_print("Using virtio-scsi.\n");
-        ret = virtio_scsi_setup(vdev);
-        break;
-    default:
-        panic("\n! No IPL device available !\n");
-    }
-
-    return ret;
+    return 0;
 }
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 2c8d0f3097..3b7069270c 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -329,7 +329,7 @@ static void scsi_parse_capacity_report(void *data,
     }
 }
 
-int virtio_scsi_setup(VDev *vdev)
+static int virtio_scsi_setup(VDev *vdev)
 {
     int retry_test_unit_ready = 3;
     uint8_t data[256];
@@ -430,3 +430,20 @@ int virtio_scsi_setup(VDev *vdev)
 
     return 0;
 }
+
+int virtio_scsi_setup_device(SubChannelId schid)
+{
+    VDev *vdev = virtio_get_device();
+
+    vdev->schid = schid;
+    virtio_setup_ccw(vdev);
+
+    IPL_assert(vdev->config.scsi.sense_size == VIRTIO_SCSI_SENSE_SIZE,
+               "Config: sense size mismatch");
+    IPL_assert(vdev->config.scsi.cdb_size == VIRTIO_SCSI_CDB_SIZE,
+               "Config: CDB size mismatch");
+
+    sclp_print("Using virtio-scsi.\n");
+
+    return virtio_scsi_setup(vdev);
+}
-- 
2.31.1

[PATCH 10/12] pc-bios/s390-ccw/virtio-blkdev: Request the right feature bits

[Thomas Huth]

The virtio-blk code uses the block size and geometry fields in the
config area. According to the virtio-spec, these have to be negotiated
with the right feature bits during initialization, otherwise they
might not be available. QEMU is so far very forgiving and always
provides them, but we should not rely on this behavior, so let's
better request them properly via the VIRTIO_BLK_F_GEOMETRY and
VIRTIO_BLK_F_BLK_SIZE feature bits.

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

diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index a2b157b2c0..96cb18c72c 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -13,6 +13,9 @@
 #include "virtio.h"
 #include "virtio-scsi.h"
 
+#define VIRTIO_BLK_F_GEOMETRY   (1 << 4)
+#define VIRTIO_BLK_F_BLK_SIZE   (1 << 6)
+
 static int virtio_blk_read_many(VDev *vdev, ulong sector, void *load_addr,
                                 int sec_num)
 {
@@ -223,6 +226,7 @@ int virtio_blk_setup_device(SubChannelId schid)
 {
     VDev *vdev = virtio_get_device();
 
+    vdev->guest_features[0] = VIRTIO_BLK_F_GEOMETRY | VIRTIO_BLK_F_BLK_SIZE;
     vdev->schid = schid;
     virtio_setup_ccw(vdev);
 
-- 
2.31.1

[PATCH 11/12] pc-bios/s390-ccw/virtio: Remove "extern" keyword from prototypes

[Thomas Huth]

All the other protytpes in the headers here do not use the "extern"
keyword, so let's unify this by removing the "extern" from the misfits,
too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/virtio.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 303438f159..1fa0a8e41e 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -185,12 +185,12 @@ VirtioGDN virtio_guessed_disk_nature(void);
 void virtio_assume_eckd(void);
 void virtio_assume_iso9660(void);
 
-extern bool virtio_ipl_disk_is_valid(void);
-extern int virtio_get_block_size(void);
-extern uint8_t virtio_get_heads(void);
-extern uint8_t virtio_get_sectors(void);
-extern uint64_t virtio_get_blocks(void);
-extern int virtio_read_many(ulong sector, void *load_addr, int sec_num);
+bool virtio_ipl_disk_is_valid(void);
+int virtio_get_block_size(void);
+uint8_t virtio_get_heads(void);
+uint8_t virtio_get_sectors(void);
+uint64_t virtio_get_blocks(void);
+int virtio_read_many(ulong sector, void *load_addr, int sec_num);
 
 #define VIRTIO_SECTOR_SIZE 512
 #define VIRTIO_ISO_BLOCK_SIZE 2048
-- 
2.31.1

[PATCH 12/12] pc-bios/s390-ccw/netboot.mak: Ignore Clang's warnings about GNU extensions

[Thomas Huth]

When compiling the s390-ccw bios with Clang (v14.0), there is currently
an unuseful warning like this:

  CC      pc-bios/s390-ccw/ipv6.o
 ../../roms/SLOF/lib/libnet/ipv6.c:447:18: warning: variable length array
  folded to constant array as an extension [-Wgnu-folding-constant]
                unsigned short raw[ip6size];
                               ^

SLOF is currently GCC-only and cannot be compiled with Clang yet, so
it is expected that such extensions sneak in there - and as long as
we don't want to compile the code with a compiler that is neither GCC
or Clang, it is also not necessary to avoid such extensions.

Thus these GNU-extension related warnings are completely useless in
the s390-ccw bios, especially in the code that is coming from SLOF,
so we should simply disable the related warnings here now.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 pc-bios/s390-ccw/netboot.mak | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/netboot.mak b/pc-bios/s390-ccw/netboot.mak
index 1a06befa4b..057f13bdb4 100644
--- a/pc-bios/s390-ccw/netboot.mak
+++ b/pc-bios/s390-ccw/netboot.mak
@@ -16,9 +16,12 @@ s390-netboot.elf: $(NETOBJS) libnet.a libc.a
 s390-netboot.img: s390-netboot.elf
 	$(call quiet-command,$(STRIP) --strip-unneeded $< -o $@,"STRIP","$(TARGET_DIR)$@")
 
+# SLOF is GCC-only, so ignore warnings about GNU extensions with Clang here
+NO_GNU_WARN := $(call cc-option,-Werror $(QEMU_CFLAGS),-Wno-gnu)
+
 # libc files:
 
-LIBC_CFLAGS = $(QEMU_CFLAGS) $(CFLAGS) $(LIBC_INC) $(LIBNET_INC) \
+LIBC_CFLAGS = $(QEMU_CFLAGS) $(CFLAGS) $(NO_GNU_WARN) $(LIBC_INC) $(LIBNET_INC) \
 	      -MMD -MP -MT $@ -MF $(@:%.o=%.d)
 
 CTYPE_OBJS = isdigit.o isxdigit.o toupper.o
@@ -52,7 +55,7 @@ libc.a: $(LIBCOBJS)
 
 LIBNETOBJS := args.o dhcp.o dns.o icmpv6.o ipv6.o tcp.o udp.o bootp.o \
 	      dhcpv6.o ethernet.o ipv4.o ndp.o tftp.o pxelinux.o
-LIBNETCFLAGS = $(QEMU_CFLAGS) $(CFLAGS) $(LIBC_INC) $(LIBNET_INC) \
+LIBNETCFLAGS = $(QEMU_CFLAGS) $(CFLAGS) $(NO_GNU_WARN) $(LIBC_INC) $(LIBNET_INC) \
 	       -DDHCPARCH=0x1F -MMD -MP -MT $@ -MF $(@:%.o=%.d)
 
 %.o : $(SLOF_DIR)/lib/libnet/%.c
-- 
2.31.1

Re: [PATCH 02/12] pc-bios/s390-ccw/virtio: Introduce a macro for the DASD block size

[Cornelia Huck]

On Tue, Jun 28 2022, Thomas Huth <thuth@redhat.com> wrote:

> Use VIRTIO_DASD_BLOCK_SIZE instead of the magic value 4096.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/virtio.h        | 1 +
>  pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index 19fceb6495..c2c17c29ca 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -198,6 +198,7 @@ extern int virtio_read_many(ulong sector, void *load_addr, int sec_num);
>  #define VIRTIO_SECTOR_SIZE 512
>  #define VIRTIO_ISO_BLOCK_SIZE 2048
>  #define VIRTIO_SCSI_BLOCK_SIZE 512
> +#define VIRTIO_DASD_BLOCK_SIZE 4096
>  
>  static inline ulong virtio_sector_adjust(ulong sector)
>  {
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> index 7d35050292..49ed2b4bee 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -155,7 +155,7 @@ void virtio_assume_eckd(void)
>      vdev->config.blk.physical_block_exp = 0;
>      switch (vdev->senseid.cu_model) {
>      case VIRTIO_ID_BLOCK:
> -        vdev->config.blk.blk_size = 4096;
> +        vdev->config.blk.blk_size = VIRTIO_DASD_BLOCK_SIZE;
>          break;
>      case VIRTIO_ID_SCSI:
>          vdev->config.blk.blk_size = vdev->scsi_block_size;

Unrelated to this change, but can't dasd be formatted with other block
sizes as well?

Re: [PATCH 07/12] pc-bios/s390-ccw/virtio: Read device config after feature negotiation

[Cornelia Huck]

On Tue, Jun 28 2022, Thomas Huth <thuth@redhat.com> wrote:

> Feature negotiation should be done first, since some fields in the
> config area can depend on the negotiated features and thus should
> rather be read afterwards.
>
> While we're at it, also adjust the error message here a little bit
> (the code is nowadays used for non-block virtio devices, too).
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/virtio.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

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

Re: [PATCH 08/12] pc-bios/s390-ccw/virtio: Beautify the code for reading virtqueue configuration

[Cornelia Huck]

On Tue, Jun 28 2022, Thomas Huth <thuth@redhat.com> wrote:

> It looks nicer if we separate the run_ccw() from the IPL_assert()
> statement, and the error message should talk about "virtio device"
> instead of "block device", since this code is nowadays used for
> non-block (i.e. network) devices, too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/virtio.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

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

Re: [PATCH 11/12] pc-bios/s390-ccw/virtio: Remove "extern" keyword from prototypes

[Cornelia Huck]

On Tue, Jun 28 2022, Thomas Huth <thuth@redhat.com> wrote:

> All the other protytpes in the headers here do not use the "extern"
> keyword, so let's unify this by removing the "extern" from the misfits,
> too.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/virtio.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>

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

Re: [PATCH 01/12] pc-bios/s390-ccw: Add a proper prototype for main()

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> Older versions of Clang complain if there is no prototype for main().
> Add one, and while we're at it, make sure that we use the same type
> for main.c and netmain.c - since the return value does not matter,
> declare the return type of main() as "void".
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/s390-ccw.h | 1 +
>  pc-bios/s390-ccw/main.c     | 3 +--
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-
> ccw.h
> index 79db69ff54..b88e0550ab 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -57,6 +57,7 @@ void write_subsystem_identification(void);
>  void write_iplb_location(void);
>  extern char stack[PAGE_SIZE * 8]
> __attribute__((__aligned__(PAGE_SIZE)));
>  unsigned int get_loadparm_index(void);
> +void main(void);
>  
>  /* sclp.c */
>  void sclp_print(const char *string);
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 5d2b7ba94d..835341457d 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -281,7 +281,7 @@ static void probe_boot_device(void)
>      sclp_print("Could not find a suitable boot device (none
> specified)\n");
>  }
>  
> -int main(void)
> +void main(void)
>  {
>      sclp_setup();
>      css_setup();
> @@ -294,5 +294,4 @@ int main(void)
>      }
>  
>      panic("Failed to load OS from hard disk\n");
> -    return 0; /* make compiler happy */
>  }

Re: [PATCH 02/12] pc-bios/s390-ccw/virtio: Introduce a macro for the DASD block size

[Eric Farman]

On Tue, 2022-06-28 at 15:21 +0200, Cornelia Huck wrote:
> On Tue, Jun 28 2022, Thomas Huth <thuth@redhat.com> wrote:
> 
> > Use VIRTIO_DASD_BLOCK_SIZE instead of the magic value 4096.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  pc-bios/s390-ccw/virtio.h        | 1 +
> >  pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> > index 19fceb6495..c2c17c29ca 100644
> > --- a/pc-bios/s390-ccw/virtio.h
> > +++ b/pc-bios/s390-ccw/virtio.h
> > @@ -198,6 +198,7 @@ extern int virtio_read_many(ulong sector, void
> > *load_addr, int sec_num);
> >  #define VIRTIO_SECTOR_SIZE 512
> >  #define VIRTIO_ISO_BLOCK_SIZE 2048
> >  #define VIRTIO_SCSI_BLOCK_SIZE 512
> > +#define VIRTIO_DASD_BLOCK_SIZE 4096
> >  
> >  static inline ulong virtio_sector_adjust(ulong sector)
> >  {
> > diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-
> > ccw/virtio-blkdev.c
> > index 7d35050292..49ed2b4bee 100644
> > --- a/pc-bios/s390-ccw/virtio-blkdev.c
> > +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> > @@ -155,7 +155,7 @@ void virtio_assume_eckd(void)
> >      vdev->config.blk.physical_block_exp = 0;
> >      switch (vdev->senseid.cu_model) {
> >      case VIRTIO_ID_BLOCK:
> > -        vdev->config.blk.blk_size = 4096;
> > +        vdev->config.blk.blk_size = VIRTIO_DASD_BLOCK_SIZE;
> >          break;
> >      case VIRTIO_ID_SCSI:
> >          vdev->config.blk.blk_size = vdev->scsi_block_size;
> 
> Unrelated to this change, but can't dasd be formatted with other
> block
> sizes as well?

True. I'd guess it's unlikely that anyone is jumping through those
hoops, though.

This is fine.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> 

Re: [PATCH 03/12] pc-bios/s390-ccw/bootmap: Improve the guessing logic in zipl_load_vblk()

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> The logic of trying an final ISO or ECKD boot on virtio-block devices
> is
> very weird: Since the geometry hardly ever matches in
> virtio_disk_is_scsi(),
> virtio_blk_setup_device() always sets a "guessed" disk geometry via
> virtio_assume_scsi() (which is certainly also wrong in a lot of
> cases).
> 
> zipl_load_vblk() then sees that there's been a
> "virtio_guessed_disk_nature"
> and tries to fix up the geometry again via virtio_assume_iso9660()
> before
> always trying to do ipl_iso_el_torito(). That's a very brain-twisting
> way of attempting to boot from ISO images, which won't work anymore
> after
> the following patches that will clean up the virtio_assume_scsi()
> mess
> (and thus get rid of the "virtio_guessed_disk_nature" here).
> 
> Let's try a better approach instead: ISO files always have a magic
> string "CD001" at offset 0x8001 (see e.g. the ECMA-119 specification)
> which we can use to decide whether we should try to boot in ISO 9660
> mode (which we should also try if we see a sector size of 2048).
> 
> And if we were not able to boot in ISO mode here, the final boot
> attempt
> before panicking is to boot in ECKD mode. Since this is our last boot
> attempt anyway, simply always assume the ECKD geometry here (if the
> sector
> size was not 4096 yet), so that we also do not depend on the guessed
> disk
> geometry from virtio_blk_setup_device() here anymore.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  pc-bios/s390-ccw/bootmap.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 56411ab3b6..3181b05382 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -780,18 +780,35 @@ static void ipl_iso_el_torito(void)
>      }
>  }
>  
> +/**
> + * Detect whether we're trying to boot from an .ISO image.
> + * These always have a signature string "CD001" at offset 0x8001.
> + */
> +static bool has_iso_signature(void)
> +{
> +    if (virtio_read(0x8000 / virtio_get_block_size(), sec)) {

Certainly unlikely, but virtio_get_block_size is able to return zero.

> +        return false;
> +    }
> +
> +    return !memcmp("CD001", &sec[1], 5);
> +}
> +
>  /*******************************************************************
> ****
>   * Bus specific IPL sequences
>   */
>  
>  static void zipl_load_vblk(void)
>  {
> -    if (virtio_guessed_disk_nature()) {
> -        virtio_assume_iso9660();
> +    int blksize = virtio_get_block_size();
> +
> +    if (blksize == VIRTIO_ISO_BLOCK_SIZE || has_iso_signature()) {
> +        if (blksize != VIRTIO_ISO_BLOCK_SIZE) {
> +            virtio_assume_iso9660();
> +        }
> +        ipl_iso_el_torito();
>      }
> -    ipl_iso_el_torito();
>  
> -    if (virtio_guessed_disk_nature()) {
> +    if (blksize != VIRTIO_DASD_BLOCK_SIZE) {
>          sclp_print("Using guessed DASD geometry.\n");
>          virtio_assume_eckd();
>      }

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

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, 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
> our boot disks. Let's relax the check and allow everything that
> remotely looks like a sane disk.

This indeed corrects that problem (thank you!), and I really don't
understand why we'd want to limit the geometry anyhow. I think this is
good.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> 
> 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 c2c17c29ca..8f917d47a9 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 49ed2b4bee..b14cbc3d9e 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 05/12] pc-bios/s390-ccw/virtio-blkdev: Remove virtio_assume_scsi()

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> 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>

Agreed.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/virtio.h        |  1 -
>  pc-bios/s390-ccw/virtio-blkdev.c | 24 ------------------------
>  2 files changed, 25 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index 8f917d47a9..303438f159 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -182,7 +182,6 @@ enum guessed_disk_nature_type {
>  typedef enum guessed_disk_nature_type VirtioGDN;
>  
>  VirtioGDN virtio_guessed_disk_nature(void);
> -void virtio_assume_scsi(void);
>  void virtio_assume_eckd(void);
>  void virtio_assume_iso9660(void);
>  
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-
> ccw/virtio-blkdev.c
> index b14cbc3d9e..11820754f3 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,

Re: [PATCH 07/12] pc-bios/s390-ccw/virtio: Read device config after feature negotiation

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> Feature negotiation should be done first, since some fields in the
> config area can depend on the negotiated features and thus should
> rather be read afterwards.
> 
> While we're at it, also adjust the error message here a little bit
> (the code is nowadays used for non-block virtio devices, too).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/virtio.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 4e85a2eb82..d8c2b52710 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -262,10 +262,6 @@ void virtio_setup_ccw(VDev *vdev)
>      rc = run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status,
> sizeof(status), false);
>      IPL_assert(rc == 0, "Could not write DRIVER status to host");
>  
> -    IPL_assert(
> -        run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size,
> false) == 0,
> -       "Could not get block device configuration");
> -
>      /* Feature negotiation */
>      for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) {
>          feats.features = 0;
> @@ -278,6 +274,9 @@ void virtio_setup_ccw(VDev *vdev)
>          IPL_assert(rc == 0, "Could not set features bits");
>      }
>  
> +    rc = run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size,
> false);
> +    IPL_assert(rc == 0, "Could not get virtio device
> configuration");
> +
>      for (i = 0; i < vdev->nr_vqs; i++) {
>          VqInfo info = {
>              .queue = (unsigned long long) ring_area + (i *
> VIRTIO_RING_SIZE),

Re: [PATCH 08/12] pc-bios/s390-ccw/virtio: Beautify the code for reading virtqueue configuration

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> It looks nicer if we separate the run_ccw() from the IPL_assert()
> statement, and the error message should talk about "virtio device"
> instead of "block device", since this code is nowadays used for
> non-block (i.e. network) devices, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/virtio.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index d8c2b52710..f37510f312 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -289,9 +289,8 @@ void virtio_setup_ccw(VDev *vdev)
>              .num = 0,
>          };
>  
> -        IPL_assert(
> -            run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config,
> sizeof(config), false) == 0,
> -            "Could not get block device VQ configuration");
> +        rc = run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config,
> sizeof(config), false);
> +        IPL_assert(rc == 0, "Could not get virtio device VQ
> configuration");
>          info.num = config.num;
>          vring_init(&vdev->vrings[i], &info);
>          vdev->vrings[i].schid = vdev->schid;

Re: [PATCH 09/12] pc-bios/s390-ccw: Split virtio-scsi code from virtio_blk_setup_device()

[Eric Farman]

On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> The next patch is going to add more virtio-block specific code to
> virtio_blk_setup_device(), and if the virtio-scsi code is also in
> there, this is more cumbersome. And the calling function
> virtio_setup()
> in main.c looks at the device type already anyway, so it's more
> logical to separate the virtio-scsi stuff into a new function in
> virtio-scsi.c instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

I think this is a good untangling.

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  pc-bios/s390-ccw/virtio-scsi.h   |  2 +-
>  pc-bios/s390-ccw/main.c          | 24 +++++++++++++++++-------
>  pc-bios/s390-ccw/virtio-blkdev.c | 20 ++------------------
>  pc-bios/s390-ccw/virtio-scsi.c   | 19 ++++++++++++++++++-
>  4 files changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-
> ccw/virtio-scsi.h
> index 4b14c2c2f9..e6b6cd4815 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.h
> +++ b/pc-bios/s390-ccw/virtio-scsi.h
> @@ -67,8 +67,8 @@ static inline bool virtio_scsi_response_ok(const
> VirtioScsiCmdResp *r)
>          return r->response == VIRTIO_SCSI_S_OK && r->status ==
> CDB_STATUS_GOOD;
>  }
>  
> -int virtio_scsi_setup(VDev *vdev);
>  int virtio_scsi_read_many(VDev *vdev,
>                            ulong sector, void *load_addr, int
> sec_num);
> +int virtio_scsi_setup_device(SubChannelId schid);
>  
>  #endif /* VIRTIO_SCSI_H */
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 835341457d..a2def83e82 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -14,6 +14,7 @@
>  #include "s390-ccw.h"
>  #include "cio.h"
>  #include "virtio.h"
> +#include "virtio-scsi.h"
>  #include "dasd-ipl.h"
>  
>  char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
> @@ -218,6 +219,7 @@ static int virtio_setup(void)
>  {
>      VDev *vdev = virtio_get_device();
>      QemuIplParameters *early_qipl = (QemuIplParameters
> *)QIPL_ADDRESS;
> +    int ret;
>  
>      memcpy(&qipl, early_qipl, sizeof(QemuIplParameters));
>  
> @@ -225,18 +227,26 @@ static int virtio_setup(void)
>          menu_setup();
>      }
>  
> -    if (virtio_get_device_type() == VIRTIO_ID_NET) {
> +    switch (vdev->senseid.cu_model) {
> +    case VIRTIO_ID_NET:
>          sclp_print("Network boot device detected\n");
>          vdev->netboot_start_addr = qipl.netboot_start_addr;
> -    } else {
> -        int ret = virtio_blk_setup_device(blk_schid);
> -        if (ret) {
> -            return ret;
> -        }
> +        return 0;
> +    case VIRTIO_ID_BLOCK:
> +        ret = virtio_blk_setup_device(blk_schid);
> +        break;
> +    case VIRTIO_ID_SCSI:
> +        ret = virtio_scsi_setup_device(blk_schid);
> +        break;
> +    default:
> +        panic("\n! No IPL device available !\n");
> +    }
> +
> +    if (!ret) {
>          IPL_assert(virtio_ipl_disk_is_valid(), "No valid IPL device
> detected");
>      }
>  
> -    return 0;
> +    return ret;
>  }
>  
>  static void ipl_boot_device(void)
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-
> ccw/virtio-blkdev.c
> index 11820754f3..a2b157b2c0 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -222,27 +222,11 @@ uint64_t virtio_get_blocks(void)
>  int virtio_blk_setup_device(SubChannelId schid)
>  {
>      VDev *vdev = virtio_get_device();
> -    int ret = 0;
>  
>      vdev->schid = schid;
>      virtio_setup_ccw(vdev);
>  
> -    switch (vdev->senseid.cu_model) {
> -    case VIRTIO_ID_BLOCK:
> -        sclp_print("Using virtio-blk.\n");
> -        break;
> -    case VIRTIO_ID_SCSI:
> -        IPL_assert(vdev->config.scsi.sense_size ==
> VIRTIO_SCSI_SENSE_SIZE,
> -            "Config: sense size mismatch");
> -        IPL_assert(vdev->config.scsi.cdb_size ==
> VIRTIO_SCSI_CDB_SIZE,
> -            "Config: CDB size mismatch");
> +    sclp_print("Using virtio-blk.\n");
>  
> -        sclp_print("Using virtio-scsi.\n");
> -        ret = virtio_scsi_setup(vdev);
> -        break;
> -    default:
> -        panic("\n! No IPL device available !\n");
> -    }
> -
> -    return ret;
> +    return 0;
>  }
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-
> ccw/virtio-scsi.c
> index 2c8d0f3097..3b7069270c 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -329,7 +329,7 @@ static void scsi_parse_capacity_report(void
> *data,
>      }
>  }
>  
> -int virtio_scsi_setup(VDev *vdev)
> +static int virtio_scsi_setup(VDev *vdev)
>  {
>      int retry_test_unit_ready = 3;
>      uint8_t data[256];
> @@ -430,3 +430,20 @@ int virtio_scsi_setup(VDev *vdev)
>  
>      return 0;
>  }
> +
> +int virtio_scsi_setup_device(SubChannelId schid)
> +{
> +    VDev *vdev = virtio_get_device();
> +
> +    vdev->schid = schid;
> +    virtio_setup_ccw(vdev);
> +
> +    IPL_assert(vdev->config.scsi.sense_size ==
> VIRTIO_SCSI_SENSE_SIZE,
> +               "Config: sense size mismatch");
> +    IPL_assert(vdev->config.scsi.cdb_size == VIRTIO_SCSI_CDB_SIZE,
> +               "Config: CDB size mismatch");
> +
> +    sclp_print("Using virtio-scsi.\n");
> +
> +    return virtio_scsi_setup(vdev);
> +}

Re: [PATCH 02/12] pc-bios/s390-ccw/virtio: Introduce a macro for the DASD block size

[Thomas Huth]

On 28/06/2022 15.21, Cornelia Huck wrote:
> On Tue, Jun 28 2022, Thomas Huth <thuth@redhat.com> wrote:
> 
>> Use VIRTIO_DASD_BLOCK_SIZE instead of the magic value 4096.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   pc-bios/s390-ccw/virtio.h        | 1 +
>>   pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
>> index 19fceb6495..c2c17c29ca 100644
>> --- a/pc-bios/s390-ccw/virtio.h
>> +++ b/pc-bios/s390-ccw/virtio.h
>> @@ -198,6 +198,7 @@ extern int virtio_read_many(ulong sector, void *load_addr, int sec_num);
>>   #define VIRTIO_SECTOR_SIZE 512
>>   #define VIRTIO_ISO_BLOCK_SIZE 2048
>>   #define VIRTIO_SCSI_BLOCK_SIZE 512
>> +#define VIRTIO_DASD_BLOCK_SIZE 4096
>>   
>>   static inline ulong virtio_sector_adjust(ulong sector)
>>   {
>> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
>> index 7d35050292..49ed2b4bee 100644
>> --- a/pc-bios/s390-ccw/virtio-blkdev.c
>> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
>> @@ -155,7 +155,7 @@ void virtio_assume_eckd(void)
>>       vdev->config.blk.physical_block_exp = 0;
>>       switch (vdev->senseid.cu_model) {
>>       case VIRTIO_ID_BLOCK:
>> -        vdev->config.blk.blk_size = 4096;
>> +        vdev->config.blk.blk_size = VIRTIO_DASD_BLOCK_SIZE;
>>           break;
>>       case VIRTIO_ID_SCSI:
>>           vdev->config.blk.blk_size = vdev->scsi_block_size;
> 
> Unrelated to this change, but can't dasd be formatted with other block
> sizes as well?

You're right, "dasdfmt" has a parameter for this. Shall I rename the macro 
to VIRTIO_DASD_DEFAULT_BLOCK_SIZE ?

  Thomas

Re: [PATCH 03/12] pc-bios/s390-ccw/bootmap: Improve the guessing logic in zipl_load_vblk()

[Thomas Huth]

On 01/07/2022 17.36, Eric Farman wrote:
> On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
>> The logic of trying an final ISO or ECKD boot on virtio-block devices
>> is
>> very weird: Since the geometry hardly ever matches in
>> virtio_disk_is_scsi(),
>> virtio_blk_setup_device() always sets a "guessed" disk geometry via
>> virtio_assume_scsi() (which is certainly also wrong in a lot of
>> cases).
>>
>> zipl_load_vblk() then sees that there's been a
>> "virtio_guessed_disk_nature"
>> and tries to fix up the geometry again via virtio_assume_iso9660()
>> before
>> always trying to do ipl_iso_el_torito(). That's a very brain-twisting
>> way of attempting to boot from ISO images, which won't work anymore
>> after
>> the following patches that will clean up the virtio_assume_scsi()
>> mess
>> (and thus get rid of the "virtio_guessed_disk_nature" here).
>>
>> Let's try a better approach instead: ISO files always have a magic
>> string "CD001" at offset 0x8001 (see e.g. the ECMA-119 specification)
>> which we can use to decide whether we should try to boot in ISO 9660
>> mode (which we should also try if we see a sector size of 2048).
>>
>> And if we were not able to boot in ISO mode here, the final boot
>> attempt
>> before panicking is to boot in ECKD mode. Since this is our last boot
>> attempt anyway, simply always assume the ECKD geometry here (if the
>> sector
>> size was not 4096 yet), so that we also do not depend on the guessed
>> disk
>> geometry from virtio_blk_setup_device() here anymore.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c | 25 +++++++++++++++++++++----
>>   1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 56411ab3b6..3181b05382 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -780,18 +780,35 @@ static void ipl_iso_el_torito(void)
>>       }
>>   }
>>   
>> +/**
>> + * Detect whether we're trying to boot from an .ISO image.
>> + * These always have a signature string "CD001" at offset 0x8001.
>> + */
>> +static bool has_iso_signature(void)
>> +{
>> +    if (virtio_read(0x8000 / virtio_get_block_size(), sec)) {
> 
> Certainly unlikely, but virtio_get_block_size is able to return zero.

Right ... I guess I'd better add a check for this here.

  Thanks,
   Thomas

Re: [PATCH 02/12] pc-bios/s390-ccw/virtio: Introduce a macro for the DASD block size

[Cornelia Huck]

On Sat, Jul 02 2022, Thomas Huth <thuth@redhat.com> wrote:

> On 28/06/2022 15.21, Cornelia Huck wrote:
>> On Tue, Jun 28 2022, Thomas Huth <thuth@redhat.com> wrote:
>> 
>>> Use VIRTIO_DASD_BLOCK_SIZE instead of the magic value 4096.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   pc-bios/s390-ccw/virtio.h        | 1 +
>>>   pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
>>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
>>> index 19fceb6495..c2c17c29ca 100644
>>> --- a/pc-bios/s390-ccw/virtio.h
>>> +++ b/pc-bios/s390-ccw/virtio.h
>>> @@ -198,6 +198,7 @@ extern int virtio_read_many(ulong sector, void *load_addr, int sec_num);
>>>   #define VIRTIO_SECTOR_SIZE 512
>>>   #define VIRTIO_ISO_BLOCK_SIZE 2048
>>>   #define VIRTIO_SCSI_BLOCK_SIZE 512
>>> +#define VIRTIO_DASD_BLOCK_SIZE 4096
>>>   
>>>   static inline ulong virtio_sector_adjust(ulong sector)
>>>   {
>>> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
>>> index 7d35050292..49ed2b4bee 100644
>>> --- a/pc-bios/s390-ccw/virtio-blkdev.c
>>> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
>>> @@ -155,7 +155,7 @@ void virtio_assume_eckd(void)
>>>       vdev->config.blk.physical_block_exp = 0;
>>>       switch (vdev->senseid.cu_model) {
>>>       case VIRTIO_ID_BLOCK:
>>> -        vdev->config.blk.blk_size = 4096;
>>> +        vdev->config.blk.blk_size = VIRTIO_DASD_BLOCK_SIZE;
>>>           break;
>>>       case VIRTIO_ID_SCSI:
>>>           vdev->config.blk.blk_size = vdev->scsi_block_size;
>> 
>> Unrelated to this change, but can't dasd be formatted with other block
>> sizes as well?
>
> You're right, "dasdfmt" has a parameter for this. Shall I rename the macro 
> to VIRTIO_DASD_DEFAULT_BLOCK_SIZE ?

Sounds good to me.