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

Add little-endian virt-queue configuration and support for virtio-blk-pci IPL
devices.

Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
 pc-bios/s390-ccw/main.c          |  61 ++++++-
 pc-bios/s390-ccw/pci.h           |   3 +
 pc-bios/s390-ccw/virtio-blkdev.c |  18 +++
 pc-bios/s390-ccw/virtio-pci.c    | 265 +++++++++++++++++++++++++++++++
 pc-bios/s390-ccw/virtio-pci.h    |   2 +
 pc-bios/s390-ccw/virtio.c        |  54 ++++++-
 pc-bios/s390-ccw/virtio.h        |   1 +
 7 files changed, 399 insertions(+), 5 deletions(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index c5ee575385..0e59ee3ea1 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -18,6 +18,8 @@
 #include "virtio.h"
 #include "virtio-scsi.h"
 #include "dasd-ipl.h"
+#include "clp.h"
+#include "virtio-pci.h"
 
 static SubChannelId blk_schid = { .one = 1 };
 static char loadparm_str[LOADPARM_LEN + 1];
@@ -151,6 +153,21 @@ static bool find_subch(int dev_no)
     return false;
 }
 
+static bool find_fid(uint32_t fid)
+{
+    ClpFhListEntry entry;
+    VDev *vdev = virtio_get_device();
+
+    if (find_pci_function(fid, &entry)) {
+        return false;
+    }
+
+    vdev->pci_fh = entry.fh;
+    virtio_pci_id2type(vdev, entry.device_id);
+
+    return vdev->dev_type != 0;
+}
+
 static void menu_setup(void)
 {
     if (memcmp(loadparm_str, LOADPARM_PROMPT, LOADPARM_LEN) == 0) {
@@ -239,6 +256,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.pci.fid);
+        break;
     default:
         puts("Unsupported IPLB");
     }
@@ -275,7 +295,7 @@ static int virtio_setup(void)
     return ret;
 }
 
-static void ipl_boot_device(void)
+static void ipl_ccw_device(void)
 {
     switch (cutype) {
     case CU_TYPE_DASD_3990:
@@ -289,7 +309,44 @@ static void ipl_boot_device(void)
         }
         break;
     default:
-        printf("Attempting to boot from unexpected device type 0x%X\n", cutype);
+        printf("Cannot boot CCW device with cu type 0x%X\n", cutype);
+    }
+}
+
+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_BLOCK:
+        if (virtio_setup() == 0) {
+            zipl_load(); /* only return on error */
+            virtio_reset(virtio_get_device());
+        }
+        break;
+    default:
+        printf("Cannot boot PCI device type 0x%X\n", vdev->dev_type);
+    }
+}
+
+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;
+    case S390_IPL_TYPE_PCI:
+        ipl_pci_device();
+        break;
+    default:
+        puts("Unrecognized IPL type!");
     }
 }
 
diff --git a/pc-bios/s390-ccw/pci.h b/pc-bios/s390-ccw/pci.h
index 40a0bf9dcb..63825dd21c 100644
--- a/pc-bios/s390-ccw/pci.h
+++ b/pc-bios/s390-ccw/pci.h
@@ -29,8 +29,11 @@ union register_pair {
 #define PCIST_ENABLED          0x1
 
 #define PCI_CFGBAR             0xF  /* Base Address Register for config space */
+#define PCI_CMD_REG            0x4  /* Offset of command register */
 #define PCI_CAPABILITY_LIST    0x34 /* Offset of first capability list entry */
 
+#define PCI_BUS_MASTER_MASK    0x0020 /* LE bit 3 of 16 bit register */
+
 int pci_write(uint32_t fhandle, uint64_t offset, uint8_t pcias, uint64_t data,
               uint8_t len);
 int pci_read(uint32_t fhandle, uint64_t offset, uint8_t pcias, void *buf,
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 9722b6970f..98b6cec3a0 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -13,10 +13,22 @@
 #include "virtio.h"
 #include "virtio-scsi.h"
 #include "virtio-ccw.h"
+#include "virtio-pci.h"
+#include "bswap.h"
 
 #define VIRTIO_BLK_F_GEOMETRY   (1 << 4)
 #define VIRTIO_BLK_F_BLK_SIZE   (1 << 6)
 
+/*
+ * Format header for little endian IPL
+ */
+static void fmt_blk_hdr_le(VirtioBlkOuthdr *hdr)
+{
+    hdr->type = bswap32(hdr->type);
+    hdr->ioprio = bswap32(hdr->ioprio);
+    hdr->sector = bswap64(hdr->sector);
+}
+
 static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_addr,
                                 int sec_num)
 {
@@ -29,6 +41,10 @@ static int virtio_blk_read_many(VDev *vdev, unsigned long sector, void *load_add
     out_hdr.ioprio = 99;
     out_hdr.sector = virtio_sector_adjust(sector);
 
+    if (!be_ipl()) {
+        fmt_blk_hdr_le(&out_hdr);
+    }
+
     vring_send_buf(vr, &out_hdr, sizeof(out_hdr), VRING_DESC_F_NEXT);
 
     /* This is where we want to receive data */
@@ -240,6 +256,8 @@ int virtio_blk_setup_device(VDev *vdev)
     case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         return virtio_ccw_setup(vdev);
+    case S390_IPL_TYPE_PCI:
+        return virtio_pci_setup(vdev);
     default:
         return 1;
     }
diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
index 2cfb84bf66..beb97961d0 100644
--- a/pc-bios/s390-ccw/virtio-pci.c
+++ b/pc-bios/s390-ccw/virtio-pci.c
@@ -165,3 +165,268 @@ int vpci_read_flex(uint64_t offset, uint8_t pcias, void *buf, int len)
 
     return 0;
 }
+
+static int vpci_set_selected_vq(uint16_t queue_num)
+{
+    return vpci_bswap16_write(c_cap.off + VPCI_C_OFFSET_Q_SELECT, c_cap.bar, queue_num);
+}
+
+static int vpci_set_queue_enable(uint16_t enabled)
+{
+    return vpci_bswap16_write(c_cap.off + VPCI_C_OFFSET_Q_ENABLE, c_cap.bar, enabled);
+}
+
+static int set_pci_vq_addr(uint64_t config_off, void *addr)
+{
+    return vpci_bswap64_write(c_cap.off + config_off, c_cap.bar, (uint64_t) addr);
+}
+
+static int virtio_pci_get_blk_config(void)
+{
+    VirtioBlkConfig *cfg = &virtio_get_device()->config.blk;
+    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, sizeof(VirtioBlkConfig));
+
+    /* single byte fields are not touched */
+    cfg->capacity = bswap64(cfg->capacity);
+    cfg->size_max = bswap32(cfg->size_max);
+    cfg->seg_max = bswap32(cfg->seg_max);
+
+    cfg->geometry.cylinders = bswap16(cfg->geometry.cylinders);
+
+    cfg->blk_size = bswap32(cfg->blk_size);
+    cfg->min_io_size = bswap16(cfg->min_io_size);
+    cfg->opt_io_size = bswap32(cfg->opt_io_size);
+
+    return rc;
+}
+
+static int virtio_pci_negotiate(void)
+{
+    int i, rc;
+    VDev *vdev = virtio_get_device();
+    struct VirtioFeatureDesc {
+        uint32_t features;
+        uint8_t index;
+    } __attribute__((packed)) feats;
+
+    for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) {
+        feats.features = 0;
+        feats.index = i;
+
+        rc = vpci_bswap32_write(c_cap.off + VPCI_C_OFFSET_DFSELECT, c_cap.bar,
+                                feats.index);
+        rc |= vpci_read_flex(c_cap.off + VPCI_C_OFFSET_DF, c_cap.bar, &feats, 4);
+
+        vdev->guest_features[i] &= bswap32(feats.features);
+        feats.features = vdev->guest_features[i];
+
+
+        rc |= vpci_bswap32_write(c_cap.off + VPCI_C_OFFSET_GFSELECT, c_cap.bar,
+                                 feats.index);
+        rc |= vpci_bswap32_write(c_cap.off + VPCI_C_OFFSET_GF, c_cap.bar,
+                                 feats.features);
+    }
+
+    return rc;
+}
+
+/*
+ * Find the position of the capability config within PCI configuration
+ * space for a given cfg type.  Return the position if found, otherwise 0.
+ */
+static uint8_t virtio_pci_find_cap_pos(uint8_t cfg_type)
+{
+    uint8_t next, cfg;
+    int rc;
+
+    rc = vpci_read_byte(PCI_CAPABILITY_LIST, PCI_CFGBAR, &next);
+    rc |= vpci_read_byte(next + 3, PCI_CFGBAR, &cfg);
+
+    while (!rc && (cfg != cfg_type) && next) {
+        rc = vpci_read_byte(next + 1, PCI_CFGBAR, &next);
+        rc |= vpci_read_byte(next + 3, PCI_CFGBAR, &cfg);
+    }
+
+    return rc ? 0 : next;
+}
+
+/*
+ * Read PCI configuration space to find the offset of the Common, Device, and
+ * Notification memory regions within the modern memory space.
+ * Returns 0 if success, 1 if a capability could not be located, or a
+ * negative RC if the configuration read failed.
+ */
+static int virtio_pci_read_pci_cap_config(void)
+{
+    uint8_t pos;
+    int rc;
+
+    /* Common capabilities */
+    pos = virtio_pci_find_cap_pos(VPCI_CAP_COMMON_CFG);
+    if (!pos) {
+        puts("Failed to locate PCI common configuration");
+        return 1;
+    }
+
+    rc = vpci_read_byte(pos + VPCI_CAP_BAR, PCI_CFGBAR, &c_cap.bar);
+    if (rc || vpci_read_bswap32(pos + VPCI_CAP_OFFSET, PCI_CFGBAR, &c_cap.off)) {
+        puts("Failed to read PCI common configuration");
+        return -EIO;
+    }
+
+    /* Device capabilities */
+    pos = virtio_pci_find_cap_pos(VPCI_CAP_DEVICE_CFG);
+    if (!pos) {
+        puts("Failed to locate PCI device configuration");
+        return 1;
+    }
+
+    rc = vpci_read_byte(pos + VPCI_CAP_BAR, PCI_CFGBAR, &d_cap.bar);
+    if (rc || vpci_read_bswap32(pos + VPCI_CAP_OFFSET, PCI_CFGBAR, &d_cap.off)) {
+        puts("Failed to read PCI device configuration");
+        return -EIO;
+    }
+
+    /* Notification capabilities */
+    pos = virtio_pci_find_cap_pos(VPCI_CAP_NOTIFY_CFG);
+    if (!pos) {
+        puts("Failed to locate PCI notification configuration");
+        return 1;
+    }
+
+    rc = vpci_read_byte(pos + VPCI_CAP_BAR, PCI_CFGBAR, &n_cap.bar);
+    if (rc || vpci_read_bswap32(pos + VPCI_CAP_OFFSET, PCI_CFGBAR, &n_cap.off)) {
+        puts("Failed to read PCI notification configuration");
+        return -EIO;
+    }
+
+    rc = vpci_read_bswap32(pos + VPCI_N_CAP_MULT, PCI_CFGBAR, &notify_mult);
+    if (rc || vpci_read_bswap16(c_cap.off + VPCI_C_OFFSET_Q_NOFF, c_cap.bar,
+                                &q_notify_offset)) {
+        puts("Failed to read notification queue configuration");
+        return -EIO;
+    }
+
+    return 0;
+}
+
+static int enable_pci_bus_master(void) {
+    uint16_t cmd_reg;
+
+    if (vpci_read_bswap16(PCI_CMD_REG, PCI_CFGBAR, &cmd_reg)) {
+        puts("Failed to read PCI command register");
+        return -EIO;
+    }
+
+    if (vpci_bswap16_write(PCI_CMD_REG, PCI_CFGBAR, cmd_reg | PCI_BUS_MASTER_MASK)) {
+        puts("Failed to enable PCI bus mastering");
+        return -EIO;
+    }
+
+    return 0;
+}
+
+int virtio_pci_setup(VDev *vdev)
+{
+    VRing *vr;
+    int rc;
+    uint8_t status;
+    uint16_t vq_size;
+    int i = 0;
+
+    vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
+    vdev->cmd_vr_idx = 0;
+
+    if (virtio_pci_read_pci_cap_config()) {
+        puts("Invalid virtio PCI capabilities");
+        return -EIO;
+    }
+
+    if (enable_pci_bus_master()) {
+        return -EIO;
+    }
+
+    if (virtio_reset(vdev)) {
+        return -EIO;
+    }
+
+    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
+    if (virtio_pci_set_status(status)) {
+        puts("Virtio-pci device Failed to ACKNOWLEDGE");
+        return -EIO;
+    }
+
+    vdev->guest_features[1] = VIRTIO_F_VERSION_1;
+    if (virtio_pci_negotiate()) {
+        panic("Virtio feature negotation failed!");
+    }
+
+    switch (vdev->dev_type) {
+    case VIRTIO_ID_BLOCK:
+        vdev->nr_vqs = 1;
+        vdev->cmd_vr_idx = 0;
+        virtio_pci_get_blk_config();
+        break;
+    default:
+        puts("Unsupported virtio device");
+        return -ENODEV;
+    }
+
+    status |= VIRTIO_CONFIG_S_DRIVER;
+    rc = virtio_pci_set_status(status);
+    if (rc) {
+        puts("Set status failed");
+        return -EIO;
+    }
+
+    if (vpci_read_bswap16(VPCI_C_OFFSET_Q_SIZE, c_cap.bar, &vq_size)) {
+        puts("Failed to read virt-queue configuration");
+        return -EIO;
+    }
+
+    /* Configure virt-queues for pci */
+    for (i = 0; i < vdev->nr_vqs; i++) {
+        VqInfo info = {
+            .queue = (unsigned long long) virtio_get_ring_area(i),
+            .align = KVM_S390_VIRTIO_RING_ALIGN,
+            .index = i,
+            .num = vq_size,
+        };
+
+        vr = &vdev->vrings[i];
+        vring_init(vr, &info);
+
+        if (vpci_set_selected_vq(vr->id)) {
+            puts("Failed to set selected virt-queue");
+            return -EIO;
+        }
+
+        rc = set_pci_vq_addr(VPCI_C_OFFSET_Q_DESCLO, vr->desc);
+        rc |= set_pci_vq_addr(VPCI_C_OFFSET_Q_AVAILLO, vr->avail);
+        rc |= set_pci_vq_addr(VPCI_C_OFFSET_Q_USEDLO, vr->used);
+        if (rc) {
+            puts("Failed to configure virt-queue address");
+            return -EIO;
+        }
+
+        if (vpci_set_queue_enable(true)) {
+            puts("Failed to set virt-queue enabled");
+            return -EIO;
+        }
+    }
+
+    status |= VIRTIO_CONFIG_S_FEATURES_OK | VIRTIO_CONFIG_S_DRIVER_OK;
+    return virtio_pci_set_status(status);
+}
+
+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;
+    }
+
+    return 0;
+}
diff --git a/pc-bios/s390-ccw/virtio-pci.h b/pc-bios/s390-ccw/virtio-pci.h
index 54c524f698..90d07cb9a7 100644
--- a/pc-bios/s390-ccw/virtio-pci.h
+++ b/pc-bios/s390-ccw/virtio-pci.h
@@ -65,6 +65,8 @@ typedef struct VirtioPciCap  VirtioPciCap;
 void virtio_pci_id2type(VDev *vdev, uint16_t device_id);
 int virtio_pci_reset(VDev *vdev);
 long virtio_pci_notify(int vq_id);
+int virtio_pci_setup(VDev *vdev);
+int virtio_pci_setup_device(void);
 
 int vpci_read_flex(uint64_t offset, uint8_t pcias, void *buf, int len);
 int vpci_read_bswap64(uint64_t offset, uint8_t pcias, uint64_t *buf);
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 956b34ff33..390b55c7b9 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -17,6 +17,7 @@
 #include "virtio.h"
 #include "virtio-scsi.h"
 #include "virtio-ccw.h"
+#include "virtio-pci.h"
 #include "bswap.h"
 #include "helper.h"
 #include "s390-time.h"
@@ -96,7 +97,7 @@ void vring_init(VRing *vr, VqInfo *info)
     vr->avail->idx = 0;
 
     /* We're running with interrupts off anyways, so don't bother */
-    vr->used->flags = VRING_USED_F_NO_NOTIFY;
+    vr->used->flags = be_ipl() ? VRING_USED_F_NO_NOTIFY : bswap16(VRING_USED_F_NO_NOTIFY);
     vr->used->idx = 0;
     vr->used_idx = 0;
     vr->next_idx = 0;
@@ -112,6 +113,8 @@ bool vring_notify(VRing *vr)
     case S390_IPL_TYPE_CCW:
         vr->cookie = virtio_ccw_notify(vdev.schid, vr->id, vr->cookie);
         break;
+    case S390_IPL_TYPE_PCI:
+        vr->cookie = virtio_pci_notify(vr->id);
     default:
         return 1;
     }
@@ -119,11 +122,45 @@ bool vring_notify(VRing *vr)
     return vr->cookie >= 0;
 }
 
+/*
+ * Get endienness of the IPL type
+ * Return true for s390x native big-endian
+ */
+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:
+        return false;
+    default:
+        return true;
+    }
+}
+
+/*
+ * Format the virtio ring descriptor endianness
+ * Return the available index increment in the appropriate endianness
+ */
+static void vr_bswap_descriptor(VRingDesc *desc)
+{
+    desc->addr = bswap64(desc->addr);
+    desc->len = bswap32(desc->len);
+    desc->flags = bswap16(desc->flags);
+    desc->next = bswap16(desc->next);
+}
+
 void vring_send_buf(VRing *vr, void *p, int len, int flags)
 {
+    if (!be_ipl()) {
+        vr->avail->idx = bswap16(vr->avail->idx);
+    }
+
     /* For follow-up chains we need to keep the first entry point */
     if (!(flags & VRING_HIDDEN_IS_CHAIN)) {
-        vr->avail->ring[vr->avail->idx % vr->num] = vr->next_idx;
+        vr->avail->ring[vr->avail->idx % vr->num] = be_ipl() ? vr->next_idx :
+                                                               bswap16(vr->next_idx);
     }
 
     vr->desc[vr->next_idx].addr = (unsigned long)p;
@@ -131,12 +168,21 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags)
     vr->desc[vr->next_idx].flags = flags & ~VRING_HIDDEN_IS_CHAIN;
     vr->desc[vr->next_idx].next = vr->next_idx;
     vr->desc[vr->next_idx].next++;
+
+    if (!be_ipl()) {
+        vr_bswap_descriptor(&vr->desc[vr->next_idx]);
+    }
+
     vr->next_idx++;
 
     /* Chains only have a single ID */
     if (!(flags & VRING_DESC_F_NEXT)) {
         vr->avail->idx++;
     }
+
+    if (!be_ipl()) {
+        vr->avail->idx = bswap16(vr->avail->idx);
+    }
 }
 
 int vr_poll(VRing *vr)
@@ -147,7 +193,7 @@ int vr_poll(VRing *vr)
         return 0;
     }
 
-    vr->used_idx = vr->used->idx;
+    vr->used_idx = vr->used->idx; /* Endianness is preserved */
     vr->next_idx = 0;
     vr->desc[0].len = 0;
     vr->desc[0].flags = 0;
@@ -187,6 +233,8 @@ int virtio_reset(VDev *vdev)
     case S390_IPL_TYPE_QEMU_SCSI:
     case S390_IPL_TYPE_CCW:
         return virtio_ccw_reset(vdev);
+    case S390_IPL_TYPE_PCI:
+        return virtio_pci_reset(vdev);
     default:
         return -1;
     }
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 0e7dbdb64c..18083b64cb 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -275,6 +275,7 @@ struct VirtioCmd {
 };
 typedef struct VirtioCmd VirtioCmd;
 
+bool be_ipl(void);
 void vring_init(VRing *vr, VqInfo *info);
 bool virtio_is_supported(VDev *vdev);
 bool vring_notify(VRing *vr);
-- 
2.52.0

On Tue, 2026-03-03 at 21:59 -0500, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Add little-endian virt-queue configuration and support for virtio-blk-pci IPL
> devices.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/main.c          |  61 ++++++-
>  pc-bios/s390-ccw/pci.h           |   3 +
>  pc-bios/s390-ccw/virtio-blkdev.c |  18 +++
>  pc-bios/s390-ccw/virtio-pci.c    | 265 +++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/virtio-pci.h    |   2 +
>  pc-bios/s390-ccw/virtio.c        |  54 ++++++-
>  pc-bios/s390-ccw/virtio.h        |   1 +
>  7 files changed, 399 insertions(+), 5 deletions(-)

I realize this makes use of vpci_bswapXX_write routines that I made a comment on earlier. Feel free
to disregard that.

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

On 04/03/2026 03.59, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
> 
> Add little-endian virt-queue configuration and support for virtio-blk-pci IPL
> devices.
> 
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
...
> +static int virtio_pci_get_blk_config(void)
> +{
> +    VirtioBlkConfig *cfg = &virtio_get_device()->config.blk;
> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, sizeof(VirtioBlkConfig));
> +
> +    /* single byte fields are not touched */
> +    cfg->capacity = bswap64(cfg->capacity);
> +    cfg->size_max = bswap32(cfg->size_max);
> +    cfg->seg_max = bswap32(cfg->seg_max);
> +
> +    cfg->geometry.cylinders = bswap16(cfg->geometry.cylinders);
> +
> +    cfg->blk_size = bswap32(cfg->blk_size);
> +    cfg->min_io_size = bswap16(cfg->min_io_size);
> +    cfg->opt_io_size = bswap32(cfg->opt_io_size);

So it looks like you read a bunch of optional config fields here ...

> +    return rc;
> +}
...
> +int virtio_pci_setup(VDev *vdev)
> +{
> +    VRing *vr;
> +    int rc;
> +    uint8_t status;
> +    uint16_t vq_size;
> +    int i = 0;
> +
> +    vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
> +    vdev->cmd_vr_idx = 0;
> +
> +    if (virtio_pci_read_pci_cap_config()) {
> +        puts("Invalid virtio PCI capabilities");
> +        return -EIO;
> +    }
> +
> +    if (enable_pci_bus_master()) {
> +        return -EIO;
> +    }
> +
> +    if (virtio_reset(vdev)) {
> +        return -EIO;
> +    }
> +
> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
> +    if (virtio_pci_set_status(status)) {
> +        puts("Virtio-pci device Failed to ACKNOWLEDGE");
> +        return -EIO;
> +    }

... so I think you should enable the corresponding feature bits in 
vdev->guest_features[0] here? QEMU might be very forgiving and provide you 
with the fields anyway, but let's better play safe. Something like:

     vdev->guest_features[0] = VIRTIO_BLK_F_SIZE_MAX |
                               VIRTIO_BLK_F_SEG_MAX |
                               VIRTIO_BLK_F_GEOMETRY |
                               VIRTIO_BLK_F_BLK_SIZE;

?

> +    vdev->guest_features[1] = VIRTIO_F_VERSION_1;
> +    if (virtio_pci_negotiate()) {
> +        panic("Virtio feature negotation failed!");
> +    }

Maybe double-check whether VIRTIO_F_VERSION_1 has really been negotiated? 
Otherwise, what happens if a user runs QEMU with "-device 
virtio-blk-pci,disable-modern=on" ?

  Thomas

On 3/4/26 4:53 AM, Thomas Huth wrote:
> On 04/03/2026 03.59, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> Add little-endian virt-queue configuration and support for 
>> virtio-blk-pci IPL
>> devices.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
> ...
>> +static int virtio_pci_get_blk_config(void)
>> +{
>> +    VirtioBlkConfig *cfg = &virtio_get_device()->config.blk;
>> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, 
>> sizeof(VirtioBlkConfig));
>> +
>> +    /* single byte fields are not touched */
>> +    cfg->capacity = bswap64(cfg->capacity);
>> +    cfg->size_max = bswap32(cfg->size_max);
>> +    cfg->seg_max = bswap32(cfg->seg_max);
>> +
>> +    cfg->geometry.cylinders = bswap16(cfg->geometry.cylinders);
>> +
>> +    cfg->blk_size = bswap32(cfg->blk_size);
>> +    cfg->min_io_size = bswap16(cfg->min_io_size);
>> +    cfg->opt_io_size = bswap32(cfg->opt_io_size);
>
> So it looks like you read a bunch of optional config fields here ...
>
>> +    return rc;
>> +}
> ...
>> +int virtio_pci_setup(VDev *vdev)
>> +{
>> +    VRing *vr;
>> +    int rc;
>> +    uint8_t status;
>> +    uint16_t vq_size;
>> +    int i = 0;
>> +
>> +    vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
>> +    vdev->cmd_vr_idx = 0;
>> +
>> +    if (virtio_pci_read_pci_cap_config()) {
>> +        puts("Invalid virtio PCI capabilities");
>> +        return -EIO;
>> +    }
>> +
>> +    if (enable_pci_bus_master()) {
>> +        return -EIO;
>> +    }
>> +
>> +    if (virtio_reset(vdev)) {
>> +        return -EIO;
>> +    }
>> +
>> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
>> +    if (virtio_pci_set_status(status)) {
>> +        puts("Virtio-pci device Failed to ACKNOWLEDGE");
>> +        return -EIO;
>> +    }
>
> ... so I think you should enable the corresponding feature bits in 
> vdev->guest_features[0] here? QEMU might be very forgiving and provide 
> you with the fields anyway, but let's better play safe. Something like:
>
>     vdev->guest_features[0] = VIRTIO_BLK_F_SIZE_MAX |
>                               VIRTIO_BLK_F_SEG_MAX |
>                               VIRTIO_BLK_F_GEOMETRY |
>                               VIRTIO_BLK_F_BLK_SIZE;
>
> ?

VIRTIO_BLK_F_GEOMETRY and VIRTIO_BLK_F_BLK_SIZE are already set during 
the virtio-blk setup.  I actually don't think the other two fields are 
used, I jut swapped any fields larger than 1 byte.  I suppose the 
feature bits should be enabled though... otherwise we could just just 
not touch the unused fields at all?

>
>> +    vdev->guest_features[1] = VIRTIO_F_VERSION_1;
>> +    if (virtio_pci_negotiate()) {
>> +        panic("Virtio feature negotation failed!");
>> +    }
>
> Maybe double-check whether VIRTIO_F_VERSION_1 has really been 
> negotiated? Otherwise, what happens if a user runs QEMU with "-device 
> virtio-blk-pci,disable-modern=on" ?

I haven't tried running it with "disable-modern=on" (I will test that 
next) but the config is big endian if I don't negotiate that feature 
bit, and little endian if I do, which I think is the expected behavior 
when VIRTIO_F_VERSION_1 is set.

Just for my understanding, do you see something that makes you suspect 
the negotiation isn't actually happening?  I will try running with 
"disable-modern=on" and let you know the results.

Thanks for the reviews,
Jared Rossi

On 04/03/2026 15.29, Jared Rossi wrote:
> 
> 
> On 3/4/26 4:53 AM, Thomas Huth wrote:
>> On 04/03/2026 03.59, jrossi@linux.ibm.com wrote:
>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>
>>> Add little-endian virt-queue configuration and support for virtio-blk-pci 
>>> IPL
>>> devices.
>>>
>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>> ---
>> ...
>>> +static int virtio_pci_get_blk_config(void)
>>> +{
>>> +    VirtioBlkConfig *cfg = &virtio_get_device()->config.blk;
>>> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, 
>>> sizeof(VirtioBlkConfig));
>>> +
>>> +    /* single byte fields are not touched */
>>> +    cfg->capacity = bswap64(cfg->capacity);
>>> +    cfg->size_max = bswap32(cfg->size_max);
>>> +    cfg->seg_max = bswap32(cfg->seg_max);
>>> +
>>> +    cfg->geometry.cylinders = bswap16(cfg->geometry.cylinders);
>>> +
>>> +    cfg->blk_size = bswap32(cfg->blk_size);
>>> +    cfg->min_io_size = bswap16(cfg->min_io_size);
>>> +    cfg->opt_io_size = bswap32(cfg->opt_io_size);
>>
>> So it looks like you read a bunch of optional config fields here ...
>>
>>> +    return rc;
>>> +}
>> ...
>>> +int virtio_pci_setup(VDev *vdev)
>>> +{
>>> +    VRing *vr;
>>> +    int rc;
>>> +    uint8_t status;
>>> +    uint16_t vq_size;
>>> +    int i = 0;
>>> +
>>> +    vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
>>> +    vdev->cmd_vr_idx = 0;
>>> +
>>> +    if (virtio_pci_read_pci_cap_config()) {
>>> +        puts("Invalid virtio PCI capabilities");
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    if (enable_pci_bus_master()) {
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    if (virtio_reset(vdev)) {
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
>>> +    if (virtio_pci_set_status(status)) {
>>> +        puts("Virtio-pci device Failed to ACKNOWLEDGE");
>>> +        return -EIO;
>>> +    }
>>
>> ... so I think you should enable the corresponding feature bits in vdev- 
>> >guest_features[0] here? QEMU might be very forgiving and provide you with 
>> the fields anyway, but let's better play safe. Something like:
>>
>>     vdev->guest_features[0] = VIRTIO_BLK_F_SIZE_MAX |
>>                               VIRTIO_BLK_F_SEG_MAX |
>>                               VIRTIO_BLK_F_GEOMETRY |
>>                               VIRTIO_BLK_F_BLK_SIZE;
>>
>> ?
> 
> VIRTIO_BLK_F_GEOMETRY and VIRTIO_BLK_F_BLK_SIZE are already set during the 
> virtio-blk setup.  I actually don't think the other two fields are used, I 
> jut swapped any fields larger than 1 byte.  I suppose the feature bits 
> should be enabled though... otherwise we could just just not touch the 
> unused fields at all?

Ah, right, I missed the initialization in virtio_blk_setup_device(), so we 
should be fine here, indeed!

>>
>>> +    vdev->guest_features[1] = VIRTIO_F_VERSION_1;
>>> +    if (virtio_pci_negotiate()) {
>>> +        panic("Virtio feature negotation failed!");
>>> +    }
>>
>> Maybe double-check whether VIRTIO_F_VERSION_1 has really been negotiated? 
>> Otherwise, what happens if a user runs QEMU with "-device virtio-blk- 
>> pci,disable-modern=on" ?
> 
> I haven't tried running it with "disable-modern=on" (I will test that next) 
> but the config is big endian if I don't negotiate that feature bit, and 
> little endian if I do, which I think is the expected behavior when 
> VIRTIO_F_VERSION_1 is set.
> 
> Just for my understanding, do you see something that makes you suspect the 
> negotiation isn't actually happening?  I will try running with "disable- 
> modern=on" and let you know the results.

No, I think it's fine for the default case. I'm just wondering what happens 
when someone uses "disable-modern=on" ... I guess the code will currently 
behave in weird ways since the endianness is wrong ... thus I thought it 
might be better to check VIRTIO_F_VERSION_1 again and emit a proper error 
message in this case?

  Thomas

On 3/4/26 9:39 AM, Thomas Huth wrote:
> On 04/03/2026 15.29, Jared Rossi wrote:
>>
>>
>> On 3/4/26 4:53 AM, Thomas Huth wrote:
>>> On 04/03/2026 03.59, jrossi@linux.ibm.com wrote:
>>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>>
>>>> Add little-endian virt-queue configuration and support for 
>>>> virtio-blk-pci IPL
>>>> devices.
>>>>
>>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>>> ---
>>> ...
>>>> +static int virtio_pci_get_blk_config(void)
>>>> +{
>>>> +    VirtioBlkConfig *cfg = &virtio_get_device()->config.blk;
>>>> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, 
>>>> sizeof(VirtioBlkConfig));
>>>> +
>>>> +    /* single byte fields are not touched */
>>>> +    cfg->capacity = bswap64(cfg->capacity);
>>>> +    cfg->size_max = bswap32(cfg->size_max);
>>>> +    cfg->seg_max = bswap32(cfg->seg_max);
>>>> +
>>>> +    cfg->geometry.cylinders = bswap16(cfg->geometry.cylinders);
>>>> +
>>>> +    cfg->blk_size = bswap32(cfg->blk_size);
>>>> +    cfg->min_io_size = bswap16(cfg->min_io_size);
>>>> +    cfg->opt_io_size = bswap32(cfg->opt_io_size);
>>>
>>> So it looks like you read a bunch of optional config fields here ...
>>>
>>>> +    return rc;
>>>> +}
>>> ...
>>>> +int virtio_pci_setup(VDev *vdev)
>>>> +{
>>>> +    VRing *vr;
>>>> +    int rc;
>>>> +    uint8_t status;
>>>> +    uint16_t vq_size;
>>>> +    int i = 0;
>>>> +
>>>> +    vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
>>>> +    vdev->cmd_vr_idx = 0;
>>>> +
>>>> +    if (virtio_pci_read_pci_cap_config()) {
>>>> +        puts("Invalid virtio PCI capabilities");
>>>> +        return -EIO;
>>>> +    }
>>>> +
>>>> +    if (enable_pci_bus_master()) {
>>>> +        return -EIO;
>>>> +    }
>>>> +
>>>> +    if (virtio_reset(vdev)) {
>>>> +        return -EIO;
>>>> +    }
>>>> +
>>>> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
>>>> +    if (virtio_pci_set_status(status)) {
>>>> +        puts("Virtio-pci device Failed to ACKNOWLEDGE");
>>>> +        return -EIO;
>>>> +    }
>>>
>>> ... so I think you should enable the corresponding feature bits in 
>>> vdev- >guest_features[0] here? QEMU might be very forgiving and 
>>> provide you with the fields anyway, but let's better play safe. 
>>> Something like:
>>>
>>>     vdev->guest_features[0] = VIRTIO_BLK_F_SIZE_MAX |
>>>                               VIRTIO_BLK_F_SEG_MAX |
>>>                               VIRTIO_BLK_F_GEOMETRY |
>>>                               VIRTIO_BLK_F_BLK_SIZE;
>>>
>>> ?
>>
>> VIRTIO_BLK_F_GEOMETRY and VIRTIO_BLK_F_BLK_SIZE are already set 
>> during the virtio-blk setup.  I actually don't think the other two 
>> fields are used, I jut swapped any fields larger than 1 byte.  I 
>> suppose the feature bits should be enabled though... otherwise we 
>> could just just not touch the unused fields at all?
>
> Ah, right, I missed the initialization in virtio_blk_setup_device(), 
> so we should be fine here, indeed!
>
>>>
>>>> +    vdev->guest_features[1] = VIRTIO_F_VERSION_1;
>>>> +    if (virtio_pci_negotiate()) {
>>>> +        panic("Virtio feature negotation failed!");
>>>> +    }
>>>
>>> Maybe double-check whether VIRTIO_F_VERSION_1 has really been 
>>> negotiated? Otherwise, what happens if a user runs QEMU with 
>>> "-device virtio-blk- pci,disable-modern=on" ?
>>
>> I haven't tried running it with "disable-modern=on" (I will test that 
>> next) but the config is big endian if I don't negotiate that feature 
>> bit, and little endian if I do, which I think is the expected 
>> behavior when VIRTIO_F_VERSION_1 is set.
>>
>> Just for my understanding, do you see something that makes you 
>> suspect the negotiation isn't actually happening?  I will try running 
>> with "disable- modern=on" and let you know the results.
>
> No, I think it's fine for the default case. I'm just wondering what 
> happens when someone uses "disable-modern=on" ... I guess the code 
> will currently behave in weird ways since the endianness is wrong ... 
> thus I thought it might be better to check VIRTIO_F_VERSION_1 again 
> and emit a proper error message in this case?
>

I tried running with "disable-moden=on" and it failed very early in the 
virtio-pci setup when trying to read the PCI configuration space.

    Failed to locate PCI common configuration
    Invalid virtio PCI capabilities
    ERROR: No suitable device for IPL. Halting...


Actually that happens before we even try to negotiate 
VIRTIO_F_VERSION_1.  From the virtio spec, it looks like the legacy 
interface requires the common configuration to be in BAR0 (4.1.4.10), 
while we normally expect BAR15 to specify the layout. Typically we need 
to read the capabilities config in BAR15 to determine which BAR the 
common config is in, then that location is used when negotiating the 
features, etc.  My guess is BAR15 isn't populated when 
"disable-modern=on" so it bails out when there is no capabilities 
configuration.

But as far as I can tell it isn't an endianness issue since we are 
trying to read single byte fields at this point anyway.  What are your 
thoughts?

Regards,
Jared Rossi

On 04/03/2026 21.17, Jared Rossi wrote:
> 
> 
> On 3/4/26 9:39 AM, Thomas Huth wrote:
>> On 04/03/2026 15.29, Jared Rossi wrote:
>>>
>>>
>>> On 3/4/26 4:53 AM, Thomas Huth wrote:
>>>> On 04/03/2026 03.59, jrossi@linux.ibm.com wrote:
>>>>> From: Jared Rossi <jrossi@linux.ibm.com>
>>>>>
>>>>> Add little-endian virt-queue configuration and support for virtio-blk- 
>>>>> pci IPL
>>>>> devices.
>>>>>
>>>>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>>>>> ---
>>>> ...
>>>>> +static int virtio_pci_get_blk_config(void)
>>>>> +{
>>>>> +    VirtioBlkConfig *cfg = &virtio_get_device()->config.blk;
>>>>> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, 
>>>>> sizeof(VirtioBlkConfig));
>>>>> +
>>>>> +    /* single byte fields are not touched */
>>>>> +    cfg->capacity = bswap64(cfg->capacity);
>>>>> +    cfg->size_max = bswap32(cfg->size_max);
>>>>> +    cfg->seg_max = bswap32(cfg->seg_max);
>>>>> +
>>>>> +    cfg->geometry.cylinders = bswap16(cfg->geometry.cylinders);
>>>>> +
>>>>> +    cfg->blk_size = bswap32(cfg->blk_size);
>>>>> +    cfg->min_io_size = bswap16(cfg->min_io_size);
>>>>> +    cfg->opt_io_size = bswap32(cfg->opt_io_size);
>>>>
>>>> So it looks like you read a bunch of optional config fields here ...
>>>>
>>>>> +    return rc;
>>>>> +}
>>>> ...
>>>>> +int virtio_pci_setup(VDev *vdev)
>>>>> +{
>>>>> +    VRing *vr;
>>>>> +    int rc;
>>>>> +    uint8_t status;
>>>>> +    uint16_t vq_size;
>>>>> +    int i = 0;
>>>>> +
>>>>> +    vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
>>>>> +    vdev->cmd_vr_idx = 0;
>>>>> +
>>>>> +    if (virtio_pci_read_pci_cap_config()) {
>>>>> +        puts("Invalid virtio PCI capabilities");
>>>>> +        return -EIO;
>>>>> +    }
>>>>> +
>>>>> +    if (enable_pci_bus_master()) {
>>>>> +        return -EIO;
>>>>> +    }
>>>>> +
>>>>> +    if (virtio_reset(vdev)) {
>>>>> +        return -EIO;
>>>>> +    }
>>>>> +
>>>>> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
>>>>> +    if (virtio_pci_set_status(status)) {
>>>>> +        puts("Virtio-pci device Failed to ACKNOWLEDGE");
>>>>> +        return -EIO;
>>>>> +    }
>>>>
>>>> ... so I think you should enable the corresponding feature bits in vdev- 
>>>> >guest_features[0] here? QEMU might be very forgiving and provide you 
>>>> with the fields anyway, but let's better play safe. Something like:
>>>>
>>>>     vdev->guest_features[0] = VIRTIO_BLK_F_SIZE_MAX |
>>>>                               VIRTIO_BLK_F_SEG_MAX |
>>>>                               VIRTIO_BLK_F_GEOMETRY |
>>>>                               VIRTIO_BLK_F_BLK_SIZE;
>>>>
>>>> ?
>>>
>>> VIRTIO_BLK_F_GEOMETRY and VIRTIO_BLK_F_BLK_SIZE are already set during 
>>> the virtio-blk setup.  I actually don't think the other two fields are 
>>> used, I jut swapped any fields larger than 1 byte.  I suppose the feature 
>>> bits should be enabled though... otherwise we could just just not touch 
>>> the unused fields at all?
>>
>> Ah, right, I missed the initialization in virtio_blk_setup_device(), so we 
>> should be fine here, indeed!
>>
>>>>
>>>>> +    vdev->guest_features[1] = VIRTIO_F_VERSION_1;
>>>>> +    if (virtio_pci_negotiate()) {
>>>>> +        panic("Virtio feature negotation failed!");
>>>>> +    }
>>>>
>>>> Maybe double-check whether VIRTIO_F_VERSION_1 has really been 
>>>> negotiated? Otherwise, what happens if a user runs QEMU with "-device 
>>>> virtio-blk- pci,disable-modern=on" ?
>>>
>>> I haven't tried running it with "disable-modern=on" (I will test that 
>>> next) but the config is big endian if I don't negotiate that feature bit, 
>>> and little endian if I do, which I think is the expected behavior when 
>>> VIRTIO_F_VERSION_1 is set.
>>>
>>> Just for my understanding, do you see something that makes you suspect 
>>> the negotiation isn't actually happening?  I will try running with 
>>> "disable- modern=on" and let you know the results.
>>
>> No, I think it's fine for the default case. I'm just wondering what 
>> happens when someone uses "disable-modern=on" ... I guess the code will 
>> currently behave in weird ways since the endianness is wrong ... thus I 
>> thought it might be better to check VIRTIO_F_VERSION_1 again and emit a 
>> proper error message in this case?
>>
> 
> I tried running with "disable-moden=on" and it failed very early in the 
> virtio-pci setup when trying to read the PCI configuration space.
> 
>     Failed to locate PCI common configuration
>     Invalid virtio PCI capabilities
>     ERROR: No suitable device for IPL. Halting...
> 
> 
> Actually that happens before we even try to negotiate VIRTIO_F_VERSION_1.  
>  From the virtio spec, it looks like the legacy interface requires the 
> common configuration to be in BAR0 (4.1.4.10), while we normally expect 
> BAR15 to specify the layout. Typically we need to read the capabilities 
> config in BAR15 to determine which BAR the common config is in, then that 
> location is used when negotiating the features, etc.  My guess is BAR15 
> isn't populated when "disable-modern=on" so it bails out when there is no 
> capabilities configuration.
> 
> But as far as I can tell it isn't an endianness issue since we are trying to 
> read single byte fields at this point anyway.  What are your thoughts?

Ok, having the error message "Failed to locate ..." sounds good to me, so 
I'm fine if you keep this patch as it is. I was just worried that we end up 
with the bios crashing in weird ways due to the endianness issues, and that 
you could not tell by the output on the serial console what was going on. 
But since we have a clear error message instead of a crash, I think we're fine.

Thanks for checking!

  Thomas