[PATCH v2 0/8] s390x: Add support for virtio-scsi-pci boot device

[PATCH v2 0/8] s390x: Add support for virtio-scsi-pci boot device

[jrossi]

Add support for booting from SCSI devices with a virtio-scsi-pci controller.

Some minor clean-up is done with naming in order to clarify if the function
or value used with CCW, PCI, or generic IPL and to check if a PCI function
is already enabled.

Changes v1 -> v2:
    - Add fix for PCI IPLB length
    - Use a universal scsi IPLB rather than creating a new one for PCI
    - When enabling a PCI function, first check if it is already online
    - Handle multiple boot targets assigned to a single scsi controller

Jared Rossi (8):
  pc-bios/s390-ccw: Check if a PCI function is already enabled before
    trying to enable it
  hw/s390x/ipl: Fix incorrect PCI IPL block length constant
  s390x/ipl: Add PCI and bus fields to iplBlockQemuScsi
  pc-bios/s390-ccw: Use bus specific IPL_TYPE directly for scsi IPL
  pc-bios/s390-ccw: Abstract virtio_run() for generic use
  pc-bios/s390-ccw: Add support for virtio-scsi-pci IPL
  s390x: Find scsi-pci boot device and build IPLB
  tests/qtest: Add s390x PCI SCSI fallback test to cdrom-test.c

 hw/s390x/ipl.c                   | 40 ++++++++++++++++++++++++++++++++
 hw/s390x/ipl.h                   |  4 +++-
 include/hw/s390x/ipl/qipl.h      | 13 +++++++----
 pc-bios/s390-ccw/clp.c           |  4 ++++
 pc-bios/s390-ccw/main.c          | 19 +++++++++++----
 pc-bios/s390-ccw/virtio-blkdev.c |  1 -
 pc-bios/s390-ccw/virtio-ccw.c    | 17 --------------
 pc-bios/s390-ccw/virtio-ccw.h    |  1 -
 pc-bios/s390-ccw/virtio-pci.c    | 31 ++++++++++++++++++++++++-
 pc-bios/s390-ccw/virtio-scsi.c   | 12 +++++++++-
 pc-bios/s390-ccw/virtio.c        | 21 +++++++++--------
 tests/qtest/cdrom-test.c         |  8 +++++++
 12 files changed, 131 insertions(+), 40 deletions(-)

-- 
2.54.0

[PATCH v2 1/8] pc-bios/s390-ccw: Check if a PCI function is already enabled before trying to enable it

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

Enabling a PCI function that is already configured will result in an error.
Simply check if the device has already been enabled, and if so, return
without doing anything further; otherwise, enable it as usual.

Also, because enable_pci_function() prints its own message, let's remove
the duplicate message emitted during the virtio PCI setup steps.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 pc-bios/s390-ccw/clp.c        | 4 ++++
 pc-bios/s390-ccw/virtio-pci.c | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/clp.c b/pc-bios/s390-ccw/clp.c
index ca9565d8de..7917f13345 100644
--- a/pc-bios/s390-ccw/clp.c
+++ b/pc-bios/s390-ccw/clp.c
@@ -82,6 +82,10 @@ int enable_pci_function(uint32_t *fhandle)
 {
     ClpReqRspSetPci rrb;
 
+    if (*fhandle & CLP_FHLIST_MASK_CONFIG) {
+        return 0;   /* already enabled */
+    }
+
     rrb.request.hdr.len = sizeof(ClpReqSetPci);
     rrb.request.hdr.cmd = 0x05;
     rrb.request.fh = *fhandle;
diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
index f501252c81..231ab6e04d 100644
--- a/pc-bios/s390-ccw/virtio-pci.c
+++ b/pc-bios/s390-ccw/virtio-pci.c
@@ -452,7 +452,6 @@ int virtio_pci_setup_device(void)
     VDev *vdev = virtio_get_device();
 
     if (enable_pci_function(&vdev->pci_fh)) {
-        puts("Failed to enable PCI function");
         return -ENODEV;
     }
 
-- 
2.54.0

[PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

The IplBlockPci struct is 336 bytes in length.

Fix the S390_IPLB_MIN_PCI_LEN to reflect this, and assign a valid blk0_len
while we are at it.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 hw/s390x/ipl.c | 2 ++
 hw/s390x/ipl.h | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index d5fdc3ea0f..fd6e9100f9 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -526,6 +526,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
         switch (devtype) {
         case PCI_DEVTYPE_VIRTIO:
             iplb->len = cpu_to_be32(S390_IPLB_MIN_PCI_LEN);
+            iplb->blk0_len =
+                cpu_to_be32(S390_IPLB_MIN_PCI_LEN - S390_IPLB_HEADER_LEN);
             iplb->pbt = S390_IPL_TYPE_PCI;
             iplb->pci.fid = cpu_to_be32(pbdev->fid);
             break;
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index fac30763df..b4d93054a7 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
 #define S390_IPLB_MIN_PV_LEN 148
 #define S390_IPLB_MIN_CCW_LEN 200
 #define S390_IPLB_MIN_FCP_LEN 384
-#define S390_IPLB_MIN_PCI_LEN 376
+#define S390_IPLB_MIN_PCI_LEN 336
 #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
 
 static inline bool iplb_valid_len(IplParameterBlock *iplb)
-- 
2.54.0

[PATCH v2 3/8] s390x/ipl: Add PCI and bus fields to iplBlockQemuScsi

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

Rather than add a second non-architected IPLB type for scsi-pci devices,
let's add the relevant fields to the existing struct so that it can handle
both PCI and CCW scsi controllers.

The PCI and CCW fields of the IplBlockQemuScsi are aligned with same
offsets as the corresponding fields in IplBlockPci and IplBlockCcw for
potential union access.

Add comments for PCI/CCW specifics and also denote that the S390IplType
enum values must fit in uint8_t.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 hw/s390x/ipl.c              |  1 +
 hw/s390x/ipl.h              |  2 ++
 include/hw/s390x/ipl/qipl.h | 13 +++++++++----
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index fd6e9100f9..7e68be4c20 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -482,6 +482,7 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
             iplb->scsi.lun = cpu_to_be32(sd->lun);
             iplb->scsi.target = cpu_to_be16(sd->id);
             iplb->scsi.channel = cpu_to_be16(sd->channel);
+            iplb->scsi.bus = S390_IPL_TYPE_CCW;
             iplb->scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
             iplb->scsi.ssid = ccw_dev->sch->ssid & 3;
             break;
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index b4d93054a7..0734d72117 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -86,6 +86,8 @@ struct S390IPLState {
     uint16_t devno;
 };
 QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
+QEMU_BUILD_BUG_MSG(offsetof(IplBlockQemuScsi, fid) != offsetof(IplBlockPci, fid),
+                   "iplb fields misaligned across bus types");
 
 #define DIAG_308_RC_OK              0x0001
 #define DIAG_308_RC_NO_CONF         0x0102
diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
index 8d3c83a80b..8d3937376b 100644
--- a/include/hw/s390x/ipl/qipl.h
+++ b/include/hw/s390x/ipl/qipl.h
@@ -25,7 +25,7 @@ enum S390IplType {
     S390_IPL_TYPE_CCW = 0x02,
     S390_IPL_TYPE_PCI = 0x04,
     S390_IPL_TYPE_PV = 0x05,
-    S390_IPL_TYPE_QEMU_SCSI = 0xff
+    S390_IPL_TYPE_QEMU_SCSI = 0xff /* Max value, must fit in uint8_t */
 };
 typedef enum S390IplType S390IplType;
 
@@ -103,9 +103,14 @@ struct IplBlockQemuScsi {
     uint32_t lun;
     uint16_t target;
     uint16_t channel;
-    uint8_t  reserved0[77];
-    uint8_t  ssid;
-    uint16_t devno;
+    uint8_t  bus;
+    uint8_t  reserved0[76];
+    uint8_t  ssid;              /* CCW only */
+    uint16_t devno;             /* CCW only */
+    uint8_t  reserved1[216];
+    uint8_t  opt;               /* PCI only */
+    uint8_t  reserved2[3];
+    uint32_t fid;               /* PCI only */
 } QEMU_PACKED;
 typedef struct IplBlockQemuScsi IplBlockQemuScsi;
 
-- 
2.54.0

[PATCH v2 4/8] pc-bios/s390-ccw: Use bus specific IPL_TYPE directly for scsi IPL

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

All existing switch cases that handle S390_IPL_TYPE_QEMU_SCSI directly fall
through to S390_IPL_TYPE_CCW.  Rather than always fall through, reassign
vdev->ipl_type to the appropriate bus for the scsi controller and proceed
as usual, where the bus is designated by the "bus" field and set during
the IPLB build.

Because S390_IPL_TYPE_QEMU_SCSI should not appear as an ipl_type once the
device is found, remove that specific case from other existing switches
and allow it to land in the default case, which will generally result
in an immediate error.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 pc-bios/s390-ccw/main.c          | 13 ++++++++++---
 pc-bios/s390-ccw/virtio-blkdev.c |  1 -
 pc-bios/s390-ccw/virtio.c        |  5 -----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 8bc6e8eaa3..63692260a4 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -257,8 +257,16 @@ static bool find_boot_device(void)
         vdev->selected_scsi_device.channel = iplb.scsi.channel;
         vdev->selected_scsi_device.target = iplb.scsi.target;
         vdev->selected_scsi_device.lun = iplb.scsi.lun;
-        blk_schid.ssid = iplb.scsi.ssid & 0x3;
-        found = find_subch(iplb.scsi.devno);
+        vdev->ipl_type = iplb.scsi.bus;
+        switch (vdev->ipl_type) {
+        case S390_IPL_TYPE_CCW:
+            blk_schid.ssid = iplb.scsi.ssid & 0x3;
+            found = find_subch(iplb.scsi.devno);
+            break;
+        default:
+            puts("Unrecognized SCSI controller");
+            break;
+        }
         break;
      case S390_IPL_TYPE_PCI:
         found = find_fid(iplb.pci.fid);
@@ -342,7 +350,6 @@ static void ipl_pci_device(void)
 static void ipl_boot_device(void)
 {
     switch (virtio_get_device()->ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         ipl_ccw_device();
         break;
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 98b6cec3a0..83a1185dd5 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -253,7 +253,6 @@ int virtio_blk_setup_device(VDev *vdev)
     puts("Using virtio-blk.");
 
     switch (vdev->ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         return virtio_ccw_setup(vdev);
     case S390_IPL_TYPE_PCI:
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index a0d249db24..0b33544144 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -61,7 +61,6 @@ char *virtio_get_ring_area(int ring_num)
 int drain_irqs(void)
 {
     switch (vdev.ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         return drain_irqs_ccw(vdev.schid);
     default:
@@ -109,7 +108,6 @@ void vring_init(VRing *vr, VqInfo *info)
 bool vring_notify(VRing *vr)
 {
     switch (vdev.ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         vr->cookie = virtio_ccw_notify(vdev.schid, vr->id, vr->cookie);
         break;
@@ -130,7 +128,6 @@ bool vring_notify(VRing *vr)
 bool be_ipl(void)
 {
     switch (virtio_get_device()->ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         return true;
     case S390_IPL_TYPE_PCI:
@@ -231,7 +228,6 @@ int vring_wait_reply(void)
 int virtio_reset(VDev *vdev)
 {
     switch (vdev->ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         return virtio_ccw_reset(vdev);
     case S390_IPL_TYPE_PCI:
@@ -244,7 +240,6 @@ int virtio_reset(VDev *vdev)
 bool virtio_is_supported(VDev *vdev)
 {
     switch (vdev->ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         return virtio_ccw_is_supported(vdev);
     case S390_IPL_TYPE_PCI:
-- 
2.54.0

[PATCH v2 5/8] pc-bios/s390-ccw: Abstract virtio_run() for generic use

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

The existing virtio_ccw_run() actually has no dependency on CCW.  Rather
than add a case for non-ccw devices, we can just put main logic right in
the virtio_run() call so that it can be used for other bus types directly.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 pc-bios/s390-ccw/virtio-ccw.c | 17 -----------------
 pc-bios/s390-ccw/virtio-ccw.h |  1 -
 pc-bios/s390-ccw/virtio.c     | 16 +++++++++++-----
 3 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/pc-bios/s390-ccw/virtio-ccw.c b/pc-bios/s390-ccw/virtio-ccw.c
index 5cb2158ed2..ea5dbc2857 100644
--- a/pc-bios/s390-ccw/virtio-ccw.c
+++ b/pc-bios/s390-ccw/virtio-ccw.c
@@ -107,23 +107,6 @@ long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie)
                          vq_idx, cookie);
 }
 
-int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd)
-{
-    VRing *vr = &vdev->vrings[vqid];
-    int i = 0;
-
-    do {
-        vring_send_buf(vr, cmd[i].data, cmd[i].size,
-                       cmd[i].flags | (i ? VRING_HIDDEN_IS_CHAIN : 0));
-    } while (cmd[i++].flags & VRING_DESC_F_NEXT);
-
-    vring_wait_reply();
-    if (drain_irqs()) {
-        return -1;
-    }
-    return 0;
-}
-
 int virtio_ccw_reset(VDev *vdev)
 {
     return run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
diff --git a/pc-bios/s390-ccw/virtio-ccw.h b/pc-bios/s390-ccw/virtio-ccw.h
index a506767eaa..43d21906e5 100644
--- a/pc-bios/s390-ccw/virtio-ccw.h
+++ b/pc-bios/s390-ccw/virtio-ccw.h
@@ -16,7 +16,6 @@ extern SubChannelId blk_schid;
 /* virtio-ccw.c */
 int drain_irqs_ccw(SubChannelId schid);
 bool virtio_ccw_is_supported(VDev *vdev);
-int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd);
 long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie);
 int virtio_ccw_setup(VDev *vdev);
 int virtio_ccw_reset(VDev *vdev);
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 0b33544144..34c5e6ee22 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -70,13 +70,19 @@ int drain_irqs(void)
 
 int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
 {
-    switch (vdev->ipl_type) {
-    case S390_IPL_TYPE_QEMU_SCSI:
-    case S390_IPL_TYPE_CCW:
-        return virtio_ccw_run(vdev, vqid, cmd);
-    default:
+    VRing *vr = &vdev->vrings[vqid];
+    int i = 0;
+
+    do {
+        vring_send_buf(vr, cmd[i].data, cmd[i].size,
+                       cmd[i].flags | (i ? VRING_HIDDEN_IS_CHAIN : 0));
+    } while (cmd[i++].flags & VRING_DESC_F_NEXT);
+
+    vring_wait_reply();
+    if (drain_irqs()) {
         return -1;
     }
+    return 0;
 }
 
 void vring_init(VRing *vr, VqInfo *info)
-- 
2.54.0

[PATCH v2 6/8] pc-bios/s390-ccw: Add support for virtio-scsi-pci IPL

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

Read virtio-pci configuration and configure virt-queue for scsi devices.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 pc-bios/s390-ccw/main.c        |  3 ++-
 pc-bios/s390-ccw/virtio-pci.c  | 30 ++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/virtio-scsi.c | 12 +++++++++++-
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 63692260a4..50f455324e 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -269,6 +269,7 @@ static bool find_boot_device(void)
         }
         break;
      case S390_IPL_TYPE_PCI:
+        vdev->scsi_device_selected = false;
         found = find_fid(iplb.pci.fid);
         break;
     default:
@@ -329,13 +330,13 @@ static void ipl_pci_device(void)
 {
     VDev *vdev = virtio_get_device();
     vdev->is_cdrom = false;
-    vdev->scsi_device_selected = false;
 
     if (virtio_pci_setup_device()) {
         return;
     }
 
     switch (vdev->dev_type) {
+    case VIRTIO_ID_SCSI:
     case VIRTIO_ID_BLOCK:
         if (virtio_setup() == 0) {
             zipl_load(); /* only return on error */
diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
index 231ab6e04d..277c3ea68d 100644
--- a/pc-bios/s390-ccw/virtio-pci.c
+++ b/pc-bios/s390-ccw/virtio-pci.c
@@ -52,6 +52,10 @@ void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
     case 0x1001:
         vdev->dev_type = VIRTIO_ID_BLOCK;
         break;
+    case 0x1048:
+    case 0x1004:
+        vdev->dev_type = VIRTIO_ID_SCSI;
+        break;
     default:
         vdev->dev_type = 0;
     }
@@ -199,6 +203,26 @@ static int virtio_pci_get_blk_config(void)
     return rc;
 }
 
+static int virtio_pci_get_scsi_config(void)
+{
+    VirtioScsiConfig *cfg = &virtio_get_device()->config.scsi;
+    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, sizeof(VirtioScsiConfig));
+
+    /* all fields of scsi config must be byte swapped */
+    cfg->num_queues = bswap32(cfg->num_queues);
+    cfg->seg_max = bswap32(cfg->seg_max);
+    cfg->max_sectors = bswap32(cfg->max_sectors);
+    cfg->cmd_per_lun = bswap32(cfg->cmd_per_lun);
+    cfg->event_info_size = bswap32(cfg->event_info_size);
+    cfg->sense_size = bswap32(cfg->sense_size);
+    cfg->cdb_size = bswap32(cfg->cdb_size);
+    cfg->max_channel = bswap16(cfg->max_channel);
+    cfg->max_target = bswap16(cfg->max_target);
+    cfg->max_lun = bswap32(cfg->max_lun);
+
+    return rc;
+}
+
 static int virtio_pci_negotiate(void)
 {
     int i, rc;
@@ -330,6 +354,7 @@ bool virtio_pci_is_supported(VDev *vdev)
     if (vdev->vendor_id == PCI_VENDOR_VIRTIO) {
         switch (vdev->dev_type) {
         case VIRTIO_ID_BLOCK:
+        case VIRTIO_ID_SCSI:
             return true;
         default:
             return false;
@@ -384,6 +409,11 @@ int virtio_pci_setup(VDev *vdev)
         vdev->cmd_vr_idx = 0;
         virtio_pci_get_blk_config();
         break;
+    case VIRTIO_ID_SCSI:
+        vdev->nr_vqs = 3;
+        vdev->cmd_vr_idx = 2;
+        virtio_pci_get_scsi_config();
+        break;
     default:
         puts("Unsupported virtio device");
         return -ENODEV;
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 9ea00c6fe6..095d7ac430 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -16,6 +16,7 @@
 #include "scsi.h"
 #include "virtio-scsi.h"
 #include "virtio-ccw.h"
+#include "virtio-pci.h"
 #include "s390-time.h"
 #include "helper.h"
 
@@ -479,7 +480,16 @@ static int virtio_scsi_setup(VDev *vdev)
 
 int virtio_scsi_setup_device(VDev *vdev)
 {
-    virtio_ccw_setup(vdev);
+    switch (vdev->ipl_type) {
+    case S390_IPL_TYPE_CCW:
+        virtio_ccw_setup(vdev);
+        break;
+    case S390_IPL_TYPE_PCI:
+        virtio_pci_setup(vdev);
+        break;
+    default:
+        return 1;
+    }
 
     if (vdev->config.scsi.sense_size != VIRTIO_SCSI_SENSE_SIZE) {
         puts("Config: sense size mismatch");
-- 
2.54.0

[PATCH v2 7/8] s390x: Find scsi-pci boot device and build IPLB

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

Add logic to identify a virtio-pci scsi controller and target lun with
assigned bootindex, then build IPLB for it.

Unfence PCI case for scsi devices in BIOS.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 hw/s390x/ipl.c          | 37 +++++++++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/main.c |  3 +++
 2 files changed, 40 insertions(+)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 7e68be4c20..3ab48e1224 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -401,6 +401,7 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
 }
 
 #define PCI_DEVTYPE_VIRTIO       0x05
+#define PCI_DEVTYPE_SCSI         0x06
 
 static S390PCIBusDevice *s390_get_pci_device(DeviceState *dev_st, int *devtype)
 {
@@ -416,6 +417,26 @@ static S390PCIBusDevice *s390_get_pci_device(DeviceState *dev_st, int *devtype)
             if (pbdev) {
                 tmp_dt = PCI_DEVTYPE_VIRTIO;
             }
+        } else {
+            SCSIDevice *sd = (SCSIDevice *)
+                object_dynamic_cast(OBJECT(dev_st), TYPE_SCSI_DEVICE);
+            if (sd) {
+                SCSIBus *sbus = scsi_bus_from_device(sd);
+                VirtIODevice *vdev = (VirtIODevice *)
+                    object_dynamic_cast(OBJECT(sbus->qbus.parent),
+                                                TYPE_VIRTIO_DEVICE);
+                if (vdev) {
+                    pci_dev = (PCIDevice *)
+                        object_dynamic_cast(OBJECT(qdev_get_parent_bus(DEVICE(vdev))->parent),
+                                            TYPE_PCI_DEVICE);
+                    if (pci_dev) {
+                        pbdev = s390_pci_find_dev_by_pci(s390_get_phb(), pci_dev);
+                        if (pbdev) {
+                            tmp_dt = PCI_DEVTYPE_SCSI;
+                        }
+                    }
+                }
+            }
         }
     }
     if (devtype) {
@@ -532,6 +553,22 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
             iplb->pbt = S390_IPL_TYPE_PCI;
             iplb->pci.fid = cpu_to_be32(pbdev->fid);
             break;
+        case PCI_DEVTYPE_SCSI:
+            sd = SCSI_DEVICE(dev_st);
+            scsi_lp = object_property_get_str(OBJECT(sd), "loadparm", NULL);
+            if (scsi_lp && strlen(scsi_lp) > 0) {
+                lp = scsi_lp;
+            }
+            iplb->len = cpu_to_be32(S390_IPLB_MIN_PCI_LEN);
+            iplb->blk0_len =
+                cpu_to_be32(S390_IPLB_MIN_PCI_LEN - S390_IPLB_HEADER_LEN);
+            iplb->pbt = S390_IPL_TYPE_QEMU_SCSI;
+            iplb->scsi.lun = cpu_to_be32(sd->lun);
+            iplb->scsi.target = cpu_to_be16(sd->id);
+            iplb->scsi.channel = cpu_to_be16(sd->channel);
+            iplb->scsi.bus = S390_IPL_TYPE_PCI;
+            iplb->scsi.fid = cpu_to_be32(pbdev->fid);
+            break;
         default:
             return false;
         }
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 50f455324e..40b21f5f87 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -263,6 +263,9 @@ static bool find_boot_device(void)
             blk_schid.ssid = iplb.scsi.ssid & 0x3;
             found = find_subch(iplb.scsi.devno);
             break;
+        case S390_IPL_TYPE_PCI:
+            found = find_fid(iplb.scsi.fid);
+            break;
         default:
             puts("Unrecognized SCSI controller");
             break;
-- 
2.54.0

[PATCH v2 8/8] tests/qtest: Add s390x PCI SCSI fallback test to cdrom-test.c

[jrossi]

From: Jared Rossi <jrossi@linux.ibm.com>

Add a test to verify that a guest can use a PCI controlled CD as a fallback boot
device after a failed CCW IPL.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 tests/qtest/cdrom-test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index a65854d2bc..ab34930404 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -253,6 +253,14 @@ static void add_s390x_tests(void)
                             "-drive if=none,id=d1,media=cdrom,file=",
                             test_cdboot);
     }
+    if (qtest_has_device("virtio-scsi-pci")) {
+        qtest_add_data_func("cdrom/boot/pci-scsi-fallback-from-blk-ccw",
+                            "-device virtio-scsi-pci -device virtio-serial "
+                            "-device virtio-blk-ccw,drive=d1,bootindex=1 "
+                            "-drive driver=null-co,read-zeroes=on,if=none,id=d1 "
+                            "-device scsi-cd,drive=d2,bootindex=2 "
+                            "-drive if=none,id=d2,media=cdrom,file=", test_cdboot);
+    }
 }
 
 int main(int argc, char **argv)
-- 
2.54.0

Re: [PATCH v2 1/8] pc-bios/s390-ccw: Check if a PCI function is already enabled before trying to enable it

[Zhuoying Cai]

Reviewed-by: Zhuoying Cai <zycai@linux.ibm.com>

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Enabling a PCI function that is already configured will result in an error.
> Simply check if the device has already been enabled, and if so, return
> without doing anything further; otherwise, enable it as usual.
> 
> Also, because enable_pci_function() prints its own message, let's remove
> the duplicate message emitted during the virtio PCI setup steps.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/clp.c        | 4 ++++
>  pc-bios/s390-ccw/virtio-pci.c | 1 -
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/clp.c b/pc-bios/s390-ccw/clp.c
> index ca9565d8de..7917f13345 100644
> --- a/pc-bios/s390-ccw/clp.c
> +++ b/pc-bios/s390-ccw/clp.c
> @@ -82,6 +82,10 @@ int enable_pci_function(uint32_t *fhandle)
>  {
>      ClpReqRspSetPci rrb;
>  
> +    if (*fhandle & CLP_FHLIST_MASK_CONFIG) {
> +        return 0;   /* already enabled */
> +    }
> +
>      rrb.request.hdr.len = sizeof(ClpReqSetPci);
>      rrb.request.hdr.cmd = 0x05;
>      rrb.request.fh = *fhandle;
> diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
> index f501252c81..231ab6e04d 100644
> --- a/pc-bios/s390-ccw/virtio-pci.c
> +++ b/pc-bios/s390-ccw/virtio-pci.c
> @@ -452,7 +452,6 @@ int virtio_pci_setup_device(void)
>      VDev *vdev = virtio_get_device();
>  
>      if (enable_pci_function(&vdev->pci_fh)) {
> -        puts("Failed to enable PCI function");
>          return -ENODEV;
>      }
>  

Re: [PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[Zhuoying Cai]

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> The IplBlockPci struct is 336 bytes in length.
> 

The commit message is slightly misleading because IplBlockPci itself is
312 bytes in length.

Maybe reword it to something like:
"IplBlockPci is 312 bytes, so the minimum PCI IPLB length is 336 bytes."

Otherwise:
Reviewed-by: Zhuoying Cai <zycai@linux.ibm.com>

> Fix the S390_IPLB_MIN_PCI_LEN to reflect this, and assign a valid blk0_len
> while we are at it.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  hw/s390x/ipl.c | 2 ++
>  hw/s390x/ipl.h | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index d5fdc3ea0f..fd6e9100f9 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -526,6 +526,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>          switch (devtype) {
>          case PCI_DEVTYPE_VIRTIO:
>              iplb->len = cpu_to_be32(S390_IPLB_MIN_PCI_LEN);
> +            iplb->blk0_len =
> +                cpu_to_be32(S390_IPLB_MIN_PCI_LEN - S390_IPLB_HEADER_LEN);
>              iplb->pbt = S390_IPL_TYPE_PCI;
>              iplb->pci.fid = cpu_to_be32(pbdev->fid);
>              break;
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index fac30763df..b4d93054a7 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
>  #define S390_IPLB_MIN_PV_LEN 148
>  #define S390_IPLB_MIN_CCW_LEN 200
>  #define S390_IPLB_MIN_FCP_LEN 384
> -#define S390_IPLB_MIN_PCI_LEN 376
> +#define S390_IPLB_MIN_PCI_LEN 336
>  #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
>  
>  static inline bool iplb_valid_len(IplParameterBlock *iplb)

Re: [PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[Eric Farman]

On 8/24/26 1:53 PM, Zhuoying Cai wrote:
> On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> The IplBlockPci struct is 336 bytes in length.
>>
> 
> The commit message is slightly misleading because IplBlockPci itself is
> 312 bytes in length.
> 
> Maybe reword it to something like:
> "IplBlockPci is 312 bytes, so the minimum PCI IPLB length is 336 bytes."
> 

Given that, should the #define be sizeof(IplBlockPci) + 24?

And does the patch itself need to be cc'd to stable?

> Otherwise:
> Reviewed-by: Zhuoying Cai <zycai@linux.ibm.com>
> 
>> Fix the S390_IPLB_MIN_PCI_LEN to reflect this, and assign a valid blk0_len
>> while we are at it.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>>   hw/s390x/ipl.c | 2 ++
>>   hw/s390x/ipl.h | 2 +-
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index d5fdc3ea0f..fd6e9100f9 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -526,6 +526,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>>           switch (devtype) {
>>           case PCI_DEVTYPE_VIRTIO:
>>               iplb->len = cpu_to_be32(S390_IPLB_MIN_PCI_LEN);
>> +            iplb->blk0_len =
>> +                cpu_to_be32(S390_IPLB_MIN_PCI_LEN - S390_IPLB_HEADER_LEN);
>>               iplb->pbt = S390_IPL_TYPE_PCI;
>>               iplb->pci.fid = cpu_to_be32(pbdev->fid);
>>               break;
>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>> index fac30763df..b4d93054a7 100644
>> --- a/hw/s390x/ipl.h
>> +++ b/hw/s390x/ipl.h
>> @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
>>   #define S390_IPLB_MIN_PV_LEN 148
>>   #define S390_IPLB_MIN_CCW_LEN 200
>>   #define S390_IPLB_MIN_FCP_LEN 384
>> -#define S390_IPLB_MIN_PCI_LEN 376
>> +#define S390_IPLB_MIN_PCI_LEN 336
>>   #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
>>   
>>   static inline bool iplb_valid_len(IplParameterBlock *iplb)
> 

Re: [PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[Zhuoying Cai]

On 8/25/26 6:29 AM, Eric Farman wrote:
> 
> 
> On 8/24/26 1:53 PM, Zhuoying Cai wrote:
>> On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>
>>> The IplBlockPci struct is 336 bytes in length.
>>>
>>
>> The commit message is slightly misleading because IplBlockPci itself is
>> 312 bytes in length.
>>
>> Maybe reword it to something like:
>> "IplBlockPci is 312 bytes, so the minimum PCI IPLB length is 336 bytes."
>>
> 
> Given that, should the #define be sizeof(IplBlockPci) + 24?
> 

If IplBlockPci is likely to expand in the future, it might make sense to
derive this value from the structure layout. For example,
offsetof(IplParameterBlock, pci) + sizeof(IplBlockPci) could be more
accurate. Otherwise, keeping it as a constant like the other definitions
seems reasonable as well.

> And does the patch itself need to be cc'd to stable?
> 
>> Otherwise:
>> Reviewed-by: Zhuoying Cai <zycai@linux.ibm.com>
>>
>>> Fix the S390_IPLB_MIN_PCI_LEN to reflect this, and assign a valid blk0_len
>>> while we are at it.
>>>
>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>> ---
>>>   hw/s390x/ipl.c | 2 ++
>>>   hw/s390x/ipl.h | 2 +-
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>> index d5fdc3ea0f..fd6e9100f9 100644
>>> --- a/hw/s390x/ipl.c
>>> +++ b/hw/s390x/ipl.c
>>> @@ -526,6 +526,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>>>           switch (devtype) {
>>>           case PCI_DEVTYPE_VIRTIO:
>>>               iplb->len = cpu_to_be32(S390_IPLB_MIN_PCI_LEN);
>>> +            iplb->blk0_len =
>>> +                cpu_to_be32(S390_IPLB_MIN_PCI_LEN - S390_IPLB_HEADER_LEN);
>>>               iplb->pbt = S390_IPL_TYPE_PCI;
>>>               iplb->pci.fid = cpu_to_be32(pbdev->fid);
>>>               break;
>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>>> index fac30763df..b4d93054a7 100644
>>> --- a/hw/s390x/ipl.h
>>> +++ b/hw/s390x/ipl.h
>>> @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
>>>   #define S390_IPLB_MIN_PV_LEN 148
>>>   #define S390_IPLB_MIN_CCW_LEN 200
>>>   #define S390_IPLB_MIN_FCP_LEN 384
>>> -#define S390_IPLB_MIN_PCI_LEN 376
>>> +#define S390_IPLB_MIN_PCI_LEN 336
>>>   #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
>>>   
>>>   static inline bool iplb_valid_len(IplParameterBlock *iplb)
>>
> 

Re: [PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[Jared Rossi]

On 8/25/26 12:44 PM, Zhuoying Cai wrote:
> On 8/25/26 6:29 AM, Eric Farman wrote:
>>
>> On 8/24/26 1:53 PM, Zhuoying Cai wrote:
>>> On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
>>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>>
>>>> The IplBlockPci struct is 336 bytes in length.
>>>>
>>> The commit message is slightly misleading because IplBlockPci itself is
>>> 312 bytes in length.
>>>
>>> Maybe reword it to something like:
>>> "IplBlockPci is 312 bytes, so the minimum PCI IPLB length is 336 bytes."
>>>
>> Given that, should the #define be sizeof(IplBlockPci) + 24?
>>
> If IplBlockPci is likely to expand in the future, it might make sense to
> derive this value from the structure layout. For example,
> offsetof(IplParameterBlock, pci) + sizeof(IplBlockPci) could be more
> accurate. Otherwise, keeping it as a constant like the other definitions
> seems reasonable as well.

All of those suggestions would be valid, but I'm leaning toward keeping it
defined without using sizeof() for the sake of consistency if nothing
else.  The other IPLB types are already defined as fixed numbers and for
the IplBlockQemuScsi being used later in this patch series, it actually has
a minimum value that is less than the sizeof() itself due to the same struct
servicing both PCI and CCW controllers, where the minimum size for CCW is
less than PCI.

I can envision several ways to improve the definitions and/or naming
conventions, but I think it is outside the scope of this series because the
changes should be uniformly applied to all definitions, not just PCI.
Possibly something to consider in the future.

As far as the commit message, I will update it to state more specifically
that the minimum size of the IPLB used for PCI devices is 336 (referring
to the IplBlockPci struct directly was not accurate).

>> And does the patch itself need to be cc'd to stable?

I think it could be a candidate, but I don't believe it really needs to be
applied to stable.  There is no exploitable bug with the incorrect length
definition as it is; I only noticed it because I was checking the field
offsets against the other IPLBs. The length is still within the designated
IPLB boundaries.  I'll cc stable in the next version and they can pick it
up if they feel it warrants the change, but it has no impact in practice.

>>
>>> Otherwise:
>>> Reviewed-by: Zhuoying Cai <zycai@linux.ibm.com>
>>>
>>>> Fix the S390_IPLB_MIN_PCI_LEN to reflect this, and assign a valid blk0_len
>>>> while we are at it.
>>>>
>>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>>> ---
>>>>    hw/s390x/ipl.c | 2 ++
>>>>    hw/s390x/ipl.h | 2 +-
>>>>    2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>>>> index d5fdc3ea0f..fd6e9100f9 100644
>>>> --- a/hw/s390x/ipl.c
>>>> +++ b/hw/s390x/ipl.c
>>>> @@ -526,6 +526,8 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>>>>            switch (devtype) {
>>>>            case PCI_DEVTYPE_VIRTIO:
>>>>                iplb->len = cpu_to_be32(S390_IPLB_MIN_PCI_LEN);
>>>> +            iplb->blk0_len =
>>>> +                cpu_to_be32(S390_IPLB_MIN_PCI_LEN - S390_IPLB_HEADER_LEN);
>>>>                iplb->pbt = S390_IPL_TYPE_PCI;
>>>>                iplb->pci.fid = cpu_to_be32(pbdev->fid);
>>>>                break;
>>>> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
>>>> index fac30763df..b4d93054a7 100644
>>>> --- a/hw/s390x/ipl.h
>>>> +++ b/hw/s390x/ipl.h
>>>> @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
>>>>    #define S390_IPLB_MIN_PV_LEN 148
>>>>    #define S390_IPLB_MIN_CCW_LEN 200
>>>>    #define S390_IPLB_MIN_FCP_LEN 384
>>>> -#define S390_IPLB_MIN_PCI_LEN 376
>>>> +#define S390_IPLB_MIN_PCI_LEN 336
>>>>    #define S390_IPLB_MIN_QEMU_SCSI_LEN 200
>>>>    
>>>>    static inline bool iplb_valid_len(IplParameterBlock *iplb)

Re: [PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[Matthew Rosato]

>>>>
>>> Given that, should the #define be sizeof(IplBlockPci) + 24?
>>>
>> If IplBlockPci is likely to expand in the future, it might make sense to
>> derive this value from the structure layout. For example,
>> offsetof(IplParameterBlock, pci) + sizeof(IplBlockPci) could be more
>> accurate. Otherwise, keeping it as a constant like the other definitions
>> seems reasonable as well.
> 
> All of those suggestions would be valid, but I'm leaning toward keeping it
> defined without using sizeof() for the sake of consistency if nothing
> else.  The other IPLB types are already defined as fixed numbers and for
> the IplBlockQemuScsi being used later in this patch series, it actually has
> a minimum value that is less than the sizeof() itself due to the same
> struct
> servicing both PCI and CCW controllers, where the minimum size for CCW is
> less than PCI.
> 
> I can envision several ways to improve the definitions and/or naming
> conventions, but I think it is outside the scope of this series because the
> changes should be uniformly applied to all definitions, not just PCI.

Ehh...  If you already think the other definitions should be fixed then
convention is not a good enough reason to propagate the bad practice to
new code.  Doing the new definition the right way now in this patch is
certainly within the scope of this series.

If we have structures that define the entire 336 bytes, I personally
would much prefer to see that written out with sizeof()s vs a magic
number that coincidentally must line up with the size of one or more
well-defined structures.

As for existing definitions, I agree that is out of scope: you could
follow-up later with one or more patches that cleanup the existing
definitions.

Thanks,
Matt

Re: [PATCH v2 2/8] hw/s390x/ipl: Fix incorrect PCI IPL block length constant

[Jared Rossi]

On 8/25/26 3:26 PM, Matthew Rosato wrote:
>>>> Given that, should the #define be sizeof(IplBlockPci) + 24?
>>>>
>>> If IplBlockPci is likely to expand in the future, it might make sense to
>>> derive this value from the structure layout. For example,
>>> offsetof(IplParameterBlock, pci) + sizeof(IplBlockPci) could be more
>>> accurate. Otherwise, keeping it as a constant like the other definitions
>>> seems reasonable as well.
>> All of those suggestions would be valid, but I'm leaning toward keeping it
>> defined without using sizeof() for the sake of consistency if nothing
>> else.  The other IPLB types are already defined as fixed numbers and for
>> the IplBlockQemuScsi being used later in this patch series, it actually has
>> a minimum value that is less than the sizeof() itself due to the same
>> struct
>> servicing both PCI and CCW controllers, where the minimum size for CCW is
>> less than PCI.
>>
>> I can envision several ways to improve the definitions and/or naming
>> conventions, but I think it is outside the scope of this series because the
>> changes should be uniformly applied to all definitions, not just PCI.
> Ehh...  If you already think the other definitions should be fixed then
> convention is not a good enough reason to propagate the bad practice to
> new code.  Doing the new definition the right way now in this patch is
> certainly within the scope of this series.
>
> If we have structures that define the entire 336 bytes, I personally
> would much prefer to see that written out with sizeof()s vs a magic
> number that coincidentally must line up with the size of one or more
> well-defined structures.
>
> As for existing definitions, I agree that is out of scope: you could
> follow-up later with one or more patches that cleanup the existing
> definitions.
>
> Thanks,
> Matt

OK.  I will use offsetof(IplParameterBlock, pci) + sizeof(IplBlockPci) for
the new definition, with the intention of cleanly splitting the IPLB header
from Block0 and uniformly updating the definitions in a follow-up series
later.

Regards,
Jared Rossi

Re: [PATCH v2 5/8] pc-bios/s390-ccw: Abstract virtio_run() for generic use

[Zhuoying Cai]

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> The existing virtio_ccw_run() actually has no dependency on CCW.  Rather
> than add a case for non-ccw devices, we can just put main logic right in

Small nit: "we can just put the main logic..."

> the virtio_run() call so that it can be used for other bus types directly.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/virtio-ccw.c | 17 -----------------
>  pc-bios/s390-ccw/virtio-ccw.h |  1 -
>  pc-bios/s390-ccw/virtio.c     | 16 +++++++++++-----
>  3 files changed, 11 insertions(+), 23 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/virtio-ccw.c b/pc-bios/s390-ccw/virtio-ccw.c
> index 5cb2158ed2..ea5dbc2857 100644
> --- a/pc-bios/s390-ccw/virtio-ccw.c
> +++ b/pc-bios/s390-ccw/virtio-ccw.c
> @@ -107,23 +107,6 @@ long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie)
>                           vq_idx, cookie);
>  }
>  
> -int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd)
> -{
> -    VRing *vr = &vdev->vrings[vqid];
> -    int i = 0;
> -
> -    do {
> -        vring_send_buf(vr, cmd[i].data, cmd[i].size,
> -                       cmd[i].flags | (i ? VRING_HIDDEN_IS_CHAIN : 0));
> -    } while (cmd[i++].flags & VRING_DESC_F_NEXT);
> -
> -    vring_wait_reply();
> -    if (drain_irqs()) {
> -        return -1;
> -    }
> -    return 0;
> -}
> -
>  int virtio_ccw_reset(VDev *vdev)
>  {
>      return run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
> diff --git a/pc-bios/s390-ccw/virtio-ccw.h b/pc-bios/s390-ccw/virtio-ccw.h
> index a506767eaa..43d21906e5 100644
> --- a/pc-bios/s390-ccw/virtio-ccw.h
> +++ b/pc-bios/s390-ccw/virtio-ccw.h
> @@ -16,7 +16,6 @@ extern SubChannelId blk_schid;
>  /* virtio-ccw.c */
>  int drain_irqs_ccw(SubChannelId schid);
>  bool virtio_ccw_is_supported(VDev *vdev);
> -int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd);
>  long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie);
>  int virtio_ccw_setup(VDev *vdev);
>  int virtio_ccw_reset(VDev *vdev);
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 0b33544144..34c5e6ee22 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -70,13 +70,19 @@ int drain_irqs(void)
>  
>  int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
>  {
> -    switch (vdev->ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
> -    case S390_IPL_TYPE_CCW:
> -        return virtio_ccw_run(vdev, vqid, cmd);
> -    default:
> +    VRing *vr = &vdev->vrings[vqid];
> +    int i = 0;
> +
> +    do {
> +        vring_send_buf(vr, cmd[i].data, cmd[i].size,
> +                       cmd[i].flags | (i ? VRING_HIDDEN_IS_CHAIN : 0));
> +    } while (cmd[i++].flags & VRING_DESC_F_NEXT);
> +
> +    vring_wait_reply();
> +    if (drain_irqs()) {
>          return -1;
>      }
> +    return 0;
>  }
>  
>  void vring_init(VRing *vr, VqInfo *info)

Re: [PATCH v2 3/8] s390x/ipl: Add PCI and bus fields to iplBlockQemuScsi

[Eric Farman]

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Rather than add a second non-architected IPLB type for scsi-pci devices,
> let's add the relevant fields to the existing struct so that it can handle
> both PCI and CCW scsi controllers.
> 
> The PCI and CCW fields of the IplBlockQemuScsi are aligned with same
> offsets as the corresponding fields in IplBlockPci and IplBlockCcw for
> potential union access.
> 
> Add comments for PCI/CCW specifics and also denote that the S390IplType
> enum values must fit in uint8_t.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   hw/s390x/ipl.c              |  1 +
>   hw/s390x/ipl.h              |  2 ++
>   include/hw/s390x/ipl/qipl.h | 13 +++++++++----
>   3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index fd6e9100f9..7e68be4c20 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -482,6 +482,7 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
>               iplb->scsi.lun = cpu_to_be32(sd->lun);
>               iplb->scsi.target = cpu_to_be16(sd->id);
>               iplb->scsi.channel = cpu_to_be16(sd->channel);
> +            iplb->scsi.bus = S390_IPL_TYPE_CCW;

Ah, so you're just initializing this here. Patch 4 will actually rely on 
this. That is fine.

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

>               iplb->scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
>               iplb->scsi.ssid = ccw_dev->sch->ssid & 3;
>               break;
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> index b4d93054a7..0734d72117 100644
> --- a/hw/s390x/ipl.h
> +++ b/hw/s390x/ipl.h
> @@ -86,6 +86,8 @@ struct S390IPLState {
>       uint16_t devno;
>   };
>   QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment of iplb wrong");
> +QEMU_BUILD_BUG_MSG(offsetof(IplBlockQemuScsi, fid) != offsetof(IplBlockPci, fid),
> +                   "iplb fields misaligned across bus types");
>   
>   #define DIAG_308_RC_OK              0x0001
>   #define DIAG_308_RC_NO_CONF         0x0102
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index 8d3c83a80b..8d3937376b 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -25,7 +25,7 @@ enum S390IplType {
>       S390_IPL_TYPE_CCW = 0x02,
>       S390_IPL_TYPE_PCI = 0x04,
>       S390_IPL_TYPE_PV = 0x05,
> -    S390_IPL_TYPE_QEMU_SCSI = 0xff
> +    S390_IPL_TYPE_QEMU_SCSI = 0xff /* Max value, must fit in uint8_t */
>   };
>   typedef enum S390IplType S390IplType;
>   
> @@ -103,9 +103,14 @@ struct IplBlockQemuScsi {
>       uint32_t lun;
>       uint16_t target;
>       uint16_t channel;
> -    uint8_t  reserved0[77];
> -    uint8_t  ssid;
> -    uint16_t devno;
> +    uint8_t  bus;
> +    uint8_t  reserved0[76];
> +    uint8_t  ssid;              /* CCW only */
> +    uint16_t devno;             /* CCW only */
> +    uint8_t  reserved1[216];
> +    uint8_t  opt;               /* PCI only */
> +    uint8_t  reserved2[3];
> +    uint32_t fid;               /* PCI only */
>   } QEMU_PACKED;
>   typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>   

Re: [PATCH v2 4/8] pc-bios/s390-ccw: Use bus specific IPL_TYPE directly for scsi IPL

[Eric Farman]

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> All existing switch cases that handle S390_IPL_TYPE_QEMU_SCSI directly fall
> through to S390_IPL_TYPE_CCW.  Rather than always fall through, reassign
> vdev->ipl_type to the appropriate bus for the scsi controller and proceed
> as usual, where the bus is designated by the "bus" field and set during
> the IPLB build.
> 
> Because S390_IPL_TYPE_QEMU_SCSI should not appear as an ipl_type once the
> device is found, remove that specific case from other existing switches
> and allow it to land in the default case, which will generally result
> in an immediate error.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/main.c          | 13 ++++++++++---
>   pc-bios/s390-ccw/virtio-blkdev.c |  1 -
>   pc-bios/s390-ccw/virtio.c        |  5 -----
>   3 files changed, 10 insertions(+), 9 deletions(-)

The expected followup described in patch 2 apply to 
S390_IPLB_MIN_QEMU_SCSI_LEN, but this is good...

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

> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 8bc6e8eaa3..63692260a4 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -257,8 +257,16 @@ static bool find_boot_device(void)
>           vdev->selected_scsi_device.channel = iplb.scsi.channel;
>           vdev->selected_scsi_device.target = iplb.scsi.target;
>           vdev->selected_scsi_device.lun = iplb.scsi.lun;
> -        blk_schid.ssid = iplb.scsi.ssid & 0x3;
> -        found = find_subch(iplb.scsi.devno);
> +        vdev->ipl_type = iplb.scsi.bus;
> +        switch (vdev->ipl_type) {
> +        case S390_IPL_TYPE_CCW:
> +            blk_schid.ssid = iplb.scsi.ssid & 0x3;
> +            found = find_subch(iplb.scsi.devno);
> +            break;
> +        default:
> +            puts("Unrecognized SCSI controller");
> +            break;
> +        }
>           break;
>        case S390_IPL_TYPE_PCI:
>           found = find_fid(iplb.pci.fid);
> @@ -342,7 +350,6 @@ static void ipl_pci_device(void)
>   static void ipl_boot_device(void)
>   {
>       switch (virtio_get_device()->ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
>       case S390_IPL_TYPE_CCW:
>           ipl_ccw_device();
>           break;
> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> index 98b6cec3a0..83a1185dd5 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -253,7 +253,6 @@ int virtio_blk_setup_device(VDev *vdev)
>       puts("Using virtio-blk.");
>   
>       switch (vdev->ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
>       case S390_IPL_TYPE_CCW:
>           return virtio_ccw_setup(vdev);
>       case S390_IPL_TYPE_PCI:
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index a0d249db24..0b33544144 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -61,7 +61,6 @@ char *virtio_get_ring_area(int ring_num)
>   int drain_irqs(void)
>   {
>       switch (vdev.ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
>       case S390_IPL_TYPE_CCW:
>           return drain_irqs_ccw(vdev.schid);
>       default:
> @@ -109,7 +108,6 @@ void vring_init(VRing *vr, VqInfo *info)
>   bool vring_notify(VRing *vr)
>   {
>       switch (vdev.ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
>       case S390_IPL_TYPE_CCW:
>           vr->cookie = virtio_ccw_notify(vdev.schid, vr->id, vr->cookie);
>           break;
> @@ -130,7 +128,6 @@ bool vring_notify(VRing *vr)
>   bool be_ipl(void)
>   {
>       switch (virtio_get_device()->ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
>       case S390_IPL_TYPE_CCW:
>           return true;
>       case S390_IPL_TYPE_PCI:
> @@ -231,7 +228,6 @@ int vring_wait_reply(void)
>   int virtio_reset(VDev *vdev)
>   {
>       switch (vdev->ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
>       case S390_IPL_TYPE_CCW:
>           return virtio_ccw_reset(vdev);
>       case S390_IPL_TYPE_PCI:
> @@ -244,7 +240,6 @@ int virtio_reset(VDev *vdev)
>   bool virtio_is_supported(VDev *vdev)
>   {
>       switch (vdev->ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
>       case S390_IPL_TYPE_CCW:
>           return virtio_ccw_is_supported(vdev);
>       case S390_IPL_TYPE_PCI:

Re: [PATCH v2 5/8] pc-bios/s390-ccw: Abstract virtio_run() for generic use

[Eric Farman]

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> The existing virtio_ccw_run() actually has no dependency on CCW.  Rather
> than add a case for non-ccw devices, we can just put main logic right in
> the virtio_run() call so that it can be used for other bus types directly.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/virtio-ccw.c | 17 -----------------
>   pc-bios/s390-ccw/virtio-ccw.h |  1 -
>   pc-bios/s390-ccw/virtio.c     | 16 +++++++++++-----
>   3 files changed, 11 insertions(+), 23 deletions(-)

I thought for sure I reviewed this in v1, and indeed I did. But you 
abstracted it further and left my tag off (thanks!), and this is even 
better.

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

> 
> diff --git a/pc-bios/s390-ccw/virtio-ccw.c b/pc-bios/s390-ccw/virtio-ccw.c
> index 5cb2158ed2..ea5dbc2857 100644
> --- a/pc-bios/s390-ccw/virtio-ccw.c
> +++ b/pc-bios/s390-ccw/virtio-ccw.c
> @@ -107,23 +107,6 @@ long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie)
>                            vq_idx, cookie);
>   }
>   
> -int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd)
> -{
> -    VRing *vr = &vdev->vrings[vqid];
> -    int i = 0;
> -
> -    do {
> -        vring_send_buf(vr, cmd[i].data, cmd[i].size,
> -                       cmd[i].flags | (i ? VRING_HIDDEN_IS_CHAIN : 0));
> -    } while (cmd[i++].flags & VRING_DESC_F_NEXT);
> -
> -    vring_wait_reply();
> -    if (drain_irqs()) {
> -        return -1;
> -    }
> -    return 0;
> -}
> -
>   int virtio_ccw_reset(VDev *vdev)
>   {
>       return run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
> diff --git a/pc-bios/s390-ccw/virtio-ccw.h b/pc-bios/s390-ccw/virtio-ccw.h
> index a506767eaa..43d21906e5 100644
> --- a/pc-bios/s390-ccw/virtio-ccw.h
> +++ b/pc-bios/s390-ccw/virtio-ccw.h
> @@ -16,7 +16,6 @@ extern SubChannelId blk_schid;
>   /* virtio-ccw.c */
>   int drain_irqs_ccw(SubChannelId schid);
>   bool virtio_ccw_is_supported(VDev *vdev);
> -int virtio_ccw_run(VDev *vdev, int vqid, VirtioCmd *cmd);
>   long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie);
>   int virtio_ccw_setup(VDev *vdev);
>   int virtio_ccw_reset(VDev *vdev);
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 0b33544144..34c5e6ee22 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -70,13 +70,19 @@ int drain_irqs(void)
>   
>   int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd)
>   {
> -    switch (vdev->ipl_type) {
> -    case S390_IPL_TYPE_QEMU_SCSI:
> -    case S390_IPL_TYPE_CCW:
> -        return virtio_ccw_run(vdev, vqid, cmd);
> -    default:
> +    VRing *vr = &vdev->vrings[vqid];
> +    int i = 0;
> +
> +    do {
> +        vring_send_buf(vr, cmd[i].data, cmd[i].size,
> +                       cmd[i].flags | (i ? VRING_HIDDEN_IS_CHAIN : 0));
> +    } while (cmd[i++].flags & VRING_DESC_F_NEXT);
> +
> +    vring_wait_reply();
> +    if (drain_irqs()) {
>           return -1;
>       }
> +    return 0;
>   }
>   
>   void vring_init(VRing *vr, VqInfo *info)

Re: [PATCH v2 1/8] pc-bios/s390-ccw: Check if a PCI function is already enabled before trying to enable it

[Eric Farman]

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Enabling a PCI function that is already configured will result in an error.
> Simply check if the device has already been enabled, and if so, return
> without doing anything further; otherwise, enable it as usual.
> 
> Also, because enable_pci_function() prints its own message, let's remove
> the duplicate message emitted during the virtio PCI setup steps.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/clp.c        | 4 ++++
>   pc-bios/s390-ccw/virtio-pci.c | 1 -
>   2 files changed, 4 insertions(+), 1 deletion(-)

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

Re: [PATCH v2 6/8] pc-bios/s390-ccw: Add support for virtio-scsi-pci IPL

[Eric Farman]

On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Read virtio-pci configuration and configure virt-queue for scsi devices.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>   pc-bios/s390-ccw/main.c        |  3 ++-
>   pc-bios/s390-ccw/virtio-pci.c  | 30 ++++++++++++++++++++++++++++++
>   pc-bios/s390-ccw/virtio-scsi.c | 12 +++++++++++-
>   3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 63692260a4..50f455324e 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -269,6 +269,7 @@ static bool find_boot_device(void)
>           }
>           break;
>        case S390_IPL_TYPE_PCI:
> +        vdev->scsi_device_selected = false;
>           found = find_fid(iplb.pci.fid);
>           break;
>       default:
> @@ -329,13 +330,13 @@ static void ipl_pci_device(void)
>   {
>       VDev *vdev = virtio_get_device();
>       vdev->is_cdrom = false;
> -    vdev->scsi_device_selected = false;
>   
>       if (virtio_pci_setup_device()) {
>           return;
>       }
>   
>       switch (vdev->dev_type) {
> +    case VIRTIO_ID_SCSI:
>       case VIRTIO_ID_BLOCK:
>           if (virtio_setup() == 0) {
>               zipl_load(); /* only return on error */
> diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
> index 231ab6e04d..277c3ea68d 100644
> --- a/pc-bios/s390-ccw/virtio-pci.c
> +++ b/pc-bios/s390-ccw/virtio-pci.c
> @@ -52,6 +52,10 @@ void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
>       case 0x1001:
>           vdev->dev_type = VIRTIO_ID_BLOCK;
>           break;
> +    case 0x1048:
> +    case 0x1004:
> +        vdev->dev_type = VIRTIO_ID_SCSI;
> +        break;
>       default:
>           vdev->dev_type = 0;
>       }
> @@ -199,6 +203,26 @@ static int virtio_pci_get_blk_config(void)
>       return rc;
>   }
>   
> +static int virtio_pci_get_scsi_config(void)
> +{
> +    VirtioScsiConfig *cfg = &virtio_get_device()->config.scsi;
> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, sizeof(VirtioScsiConfig));
> +
> +    /* all fields of scsi config must be byte swapped */
> +    cfg->num_queues = bswap32(cfg->num_queues);
> +    cfg->seg_max = bswap32(cfg->seg_max);
> +    cfg->max_sectors = bswap32(cfg->max_sectors);
> +    cfg->cmd_per_lun = bswap32(cfg->cmd_per_lun);
> +    cfg->event_info_size = bswap32(cfg->event_info_size);
> +    cfg->sense_size = bswap32(cfg->sense_size);
> +    cfg->cdb_size = bswap32(cfg->cdb_size);
> +    cfg->max_channel = bswap16(cfg->max_channel);
> +    cfg->max_target = bswap16(cfg->max_target);
> +    cfg->max_lun = bswap32(cfg->max_lun);
> +
> +    return rc;
> +}
> +
>   static int virtio_pci_negotiate(void)
>   {
>       int i, rc;
> @@ -330,6 +354,7 @@ bool virtio_pci_is_supported(VDev *vdev)
>       if (vdev->vendor_id == PCI_VENDOR_VIRTIO) {
>           switch (vdev->dev_type) {
>           case VIRTIO_ID_BLOCK:
> +        case VIRTIO_ID_SCSI:
>               return true;
>           default:
>               return false;
> @@ -384,6 +409,11 @@ int virtio_pci_setup(VDev *vdev)
>           vdev->cmd_vr_idx = 0;
>           virtio_pci_get_blk_config();
>           break;
> +    case VIRTIO_ID_SCSI:
> +        vdev->nr_vqs = 3;
> +        vdev->cmd_vr_idx = 2;
> +        virtio_pci_get_scsi_config();
> +        break;
>       default:
>           puts("Unsupported virtio device");
>           return -ENODEV;
> diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
> index 9ea00c6fe6..095d7ac430 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -16,6 +16,7 @@
>   #include "scsi.h"
>   #include "virtio-scsi.h"
>   #include "virtio-ccw.h"
> +#include "virtio-pci.h"
>   #include "s390-time.h"
>   #include "helper.h"
>   
> @@ -479,7 +480,16 @@ static int virtio_scsi_setup(VDev *vdev)
>   
>   int virtio_scsi_setup_device(VDev *vdev)
>   {
> -    virtio_ccw_setup(vdev);
> +    switch (vdev->ipl_type) {
> +    case S390_IPL_TYPE_CCW:
> +        virtio_ccw_setup(vdev);
> +        break;
> +    case S390_IPL_TYPE_PCI:
> +        virtio_pci_setup(vdev);
> +        break;

Why was/is the return code from virtio_*_setup discarded? Shouldn't we 
do -something- if it's nonzero instead of continuing on?

Besides that, this looks good to me.

> +    default:
> +        return 1;
> +    }
>   
>       if (vdev->config.scsi.sense_size != VIRTIO_SCSI_SENSE_SIZE) {
>           puts("Config: sense size mismatch");

Re: [PATCH v2 6/8] pc-bios/s390-ccw: Add support for virtio-scsi-pci IPL

[Jared Rossi]

On 8/26/26 5:11 PM, Eric Farman wrote:
>
>
> On 8/11/26 10:46 AM, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
[...]
>> diff --git a/pc-bios/s390-ccw/virtio-scsi.c 
>> b/pc-bios/s390-ccw/virtio-scsi.c
>> index 9ea00c6fe6..095d7ac430 100644
>> --- a/pc-bios/s390-ccw/virtio-scsi.c
>> +++ b/pc-bios/s390-ccw/virtio-scsi.c
>> @@ -16,6 +16,7 @@
>>   #include "scsi.h"
>>   #include "virtio-scsi.h"
>>   #include "virtio-ccw.h"
>> +#include "virtio-pci.h"
>>   #include "s390-time.h"
>>   #include "helper.h"
>>   @@ -479,7 +480,16 @@ static int virtio_scsi_setup(VDev *vdev)
>>     int virtio_scsi_setup_device(VDev *vdev)
>>   {
>> -    virtio_ccw_setup(vdev);
>> +    switch (vdev->ipl_type) {
>> +    case S390_IPL_TYPE_CCW:
>> +        virtio_ccw_setup(vdev);
>> +        break;
>> +    case S390_IPL_TYPE_PCI:
>> +        virtio_pci_setup(vdev);
>> +        break;
>
> Why was/is the return code from virtio_*_setup discarded? Shouldn't we 
> do -something- if it's nonzero instead of continuing on?
>
> Besides that, this looks good to me.
>

Unfortunately I don’t have a good answer for that.  That is just how it has
always been and so I didn’t change it; however, in the same vein as the
discussion with Matt about clarifying the IPLB length definitions from the
other day, now would be a good time to update it rather than continuing
existing bad practices.

I’ll propagate the return code on error in the next version.

Regards,
Jared Rossi