On 20/10/2025 18.20, [email protected] wrote:
From: Jared Rossi <[email protected]>
Enable virt-queue PCI configuration and add routines for virtio-blk-pci devices.
Signed-off-by: Jared Rossi <[email protected]>
---
...
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 1c1017a0db..4e4a7280b6 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -259,6 +259,7 @@ struct VDev {
uint8_t scsi_dev_heads;
bool scsi_device_selected;
ScsiDevice selected_scsi_device;
+ uint32_t pci_fh;
uint32_t max_transfer;
uint32_t guest_features[2];
};
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 7299b8911f..69e7d39862 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -15,8 +15,10 @@
#include "s390-arch.h"
#include "s390-ccw.h"
#include "cio.h"
+#include "clp.h"
#include "virtio.h"
#include "virtio-scsi.h"
+#include "virtio-pci.h"
#include "dasd-ipl.h"
SubChannelId blk_schid = { .one = 1 };
@@ -150,6 +152,20 @@ static bool find_subch(int dev_no)
return false;
}
+static bool find_fid(uint32_t fid) {
Put the curly bracket into the next line, please.
+ 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->type != 0);
You could drop the braces here.
+}
+
...
diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
new file mode 100644
index 0000000000..b6cb4a661a
--- /dev/null
+++ b/pc-bios/s390-ccw/virtio-pci.c
@@ -0,0 +1,357 @@
+/*
+ * Functionality for virtio-pci
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "clp.h"
+#include "pci.h"
+#include "helper.h"
+#include "s390-ccw.h"
+#include "virtio.h"
+#include "bswap.h"
+#include "virtio-pci.h"
+#include "s390-time.h"
+#include <stdio.h>
+
+/* Variable offsets used for reads/writes to modern memory region BAR 4 */
+uint32_t common_offset;
+uint32_t device_offset;
+uint32_t notify_offset;
+uint32_t notify_mult;
+uint16_t q_notify_offset;
+
+static int virtio_pci_set_status(VDev *vdev, uint8_t status)
+{
+ uint64_t status64 = status;
+
+ return pci_write(vdev->pci_fh, VPCI_C_OFFSET_STATUS, status64, 1);
+}
+
+static int virtio_pci_get_status(VDev *vdev, uint8_t *status)
+{
+ uint64_t status64;
+ int rc;
+
+ rc = pci_read(vdev->pci_fh, VPCI_C_OFFSET_STATUS, 4, &status64, 1);
+ if (rc) {
+ puts("Failed to read virtio-pci status");
+ return rc;
+ }
+
+ *status = (uint8_t) status64;
Ok, so after reading this code, I realized that pci_read is definitely only
supposed to work on 64-bit values.
Could you please change the "buf" parameter of pci_read() from "void *" to
"uint64_t *", otherwise this is super-confusing.
+ return 0;
+}
+
+static int virtio_pci_get_hfeatures(VDev *vdev, uint64_t *features)
+{
+ uint64_t feat0, feat1;
+ uint32_t selector;
+ int rc;
+
+ selector = bswap32(0);
+ rc = pci_write(vdev->pci_fh, VPCI_C_OFFSET_DFSELECT, selector, 4);
+ rc |= pci_read(vdev->pci_fh, VPCI_C_OFFSET_DF, 4, &feat0, 4);
+ feat0 = bswap32(feat0);
> +> + selector = bswap32(1);
+ rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_DFSELECT, selector, 4);
+ rc |= pci_read(vdev->pci_fh, VPCI_C_OFFSET_DF, 4, &feat1, 4);
+ feat1 = bswap32(feat1);
+> + *features = feat1 << 32;
+ *features |= feat0;
+
+ return rc;
+}
+
+static int virtio_pci_set_gfeatures(VDev *vdev)
+{
+ uint64_t feats;
+ uint32_t selector;
+ int rc;
+
+ selector = bswap32(0);
+ rc = pci_write(vdev->pci_fh, VPCI_C_OFFSET_GFSELECT, selector, 4);
+
+ feats = bswap32((uint64_t)vdev->guest_features[1]);
The cast to 64-bit does not make sense here.
+ rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_GF, feats, 4);
+
+ selector = bswap32(1);
+ rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_GFSELECT, selector, 4);
+
+ feats = bswap32((uint64_t)vdev->guest_features[0]);
dito
+ rc |= pci_write(vdev->pci_fh, VPCI_C_OFFSET_GF, feats, 4);
+
+ return rc;
+}
+
+static int virtio_pci_get_blk_config(VDev *vdev)
+{
+ return pci_read(vdev->pci_fh, device_offset, 4, (uint64_t
*)&vdev->config.blk,
+ sizeof(VirtioBlkConfig));
+
Please remove the empty line.
+}
+
+int virtio_pci_set_selected_vq(VDev *vdev, uint16_t queue_num)
+{
+ uint16_t le_queue_num;
+
+ le_queue_num = bswap16(queue_num);
+ return pci_write(vdev->pci_fh, VPCI_C_OFFSET_Q_SELECT,
(uint64_t)le_queue_num, 2);
The "(uint64_t)" is not required here, I think.
+}
+
+int virtio_pci_set_queue_size(VDev *vdev, uint16_t queue_size)
+{
+ uint16_t le_queue_size;
+
+ le_queue_size = bswap16(queue_size);
+ return pci_write(vdev->pci_fh, VPCI_C_OFFSET_Q_SIZE,
(uint64_t)le_queue_size, 2);
dito
+}
+
+static int virtio_pci_set_queue_enable(VDev *vdev, uint16_t enabled)
+{
+ uint16_t le_enabled;
+
+ le_enabled = bswap16(enabled);
+ return pci_write(vdev->pci_fh, VPCI_C_OFFSET_Q_ENABLE,
(uint64_t)le_enabled, 2);
dito
+}
+
+static int set_pci_vq_addr(VDev *vdev, void* addr, uint64_t config_offset_lo)
+{
+ uint32_t le_lo, le_hi;
+ uint32_t tmp;
+ int rc;
+
+ tmp = (uint32_t)(((uint64_t)addr) >> 32);
+ le_hi = bswap32(tmp);
+
+ tmp = (uint32_t)((uint64_t)addr & 0xFFFFFFFF);
+ le_lo = bswap32(tmp);
+
+ rc = pci_write(vdev->pci_fh, config_offset_lo, (uint64_t)le_lo, 4);
+ rc |= pci_write(vdev->pci_fh, config_offset_lo + 4, (uint64_t)le_hi, 4);
Wouldn't it be possible bswap64() the value and write all 8 bytes at once?
+ return rc;
+}
+
+/* virtio spec v1.1 para 4.1.2.1 */
+void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
+{
+ switch(device_id) {
+ case 0x1001:
+ vdev->type = VIRTIO_ID_BLOCK;
+ break;
+ case 0x1000: /* Other valid but currently unsupported virtio device types
*/
+ case 0x1004:
+ default:
+ vdev->type = 0;
+ }
+}
+
+/*
+ * 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(VDev *vdev)
+{
+ uint8_t pos;
+ uint64_t data;
+
+ /* Common cabilities */
+ pos = find_cap_pos(vdev->pci_fh, VIRTIO_PCI_CAP_COMMON_CFG);
+ if (!pos) {
+ puts("Failed to locate PCI common configuration");
+ return 1;
+ }
+ if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_CAP_OFFSET, 15, &data, 4)) {
+ return -EIO;
+ }
+ common_offset = bswap32(data);
+
+ /* Device cabilities */
+ pos = find_cap_pos(vdev->pci_fh, VIRTIO_PCI_CAP_DEVICE_CFG);
+ if (!pos) {
+ puts("Failed to locate PCI device configuration");
+ return 1;
+ }
+ if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_CAP_OFFSET, 15, &data, 4)) {
+ return -EIO;
+ }
+ device_offset = bswap32(data);
+
+ /* Notification cabilities */
+ pos = find_cap_pos(vdev->pci_fh, VIRTIO_PCI_CAP_NOTIFY_CFG);
+ if (!pos) {
+ puts("Failed to locate PCI notification configuration");
+ return 1;
+ }
+ if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_CAP_OFFSET, 15, &data, 4)) {
+ return -EIO;
+ }
+ notify_offset = bswap32(data);
+
+ if (pci_read(vdev->pci_fh, pos + VIRTIO_PCI_NOTIFY_CAP_MULT, 15, &data,
4)) {
+ return -EIO;
+ }
+ notify_mult = bswap32(data);
+
+ if (pci_read(vdev->pci_fh, device_offset + VPCI_C_OFFSET_Q_NOFF, 4, &data,
2)) {
+ return -EIO;
+ }
+ q_notify_offset = bswap16(data);
After reading all this code, I wonder whether it would make sense to have
proper pci_read16(), pci_read32() and pci_read64() functions that do also
the byte-swapping for you and operate on the right size of variable size?
+ return 0;
+}
+
+int virtio_pci_reset(VDev *vdev)
+{
+ int rc;
+ uint8_t status = VPCI_S_RESET;
+
+ rc = virtio_pci_set_status(vdev, status);
+ rc |= virtio_pci_get_status(vdev, &status);
+
+ if (rc || status) {
+ puts("Failed to reset virtio-pci device");
+ return 1;
Maybe nicer to return a negative error code (-1 if nothing else fits)?
+ }
+
+ return 0;
+}
Thomas
