Hi Jared!
On 20/10/2025 18.20, [email protected] wrote:
From: Jared Rossi <[email protected]>
Separate the CCW specific virtio routines and create generic wrappers for easier
reuse of existing virtio functions with non-CCW devices.
Signed-off-by: Jared Rossi <[email protected]>
---
hw/s390x/ipl.h | 5 -
include/hw/s390x/ipl/qipl.h | 6 +
pc-bios/s390-ccw/iplb.h | 4 -
pc-bios/s390-ccw/virtio-ccw.h | 25 ++++
pc-bios/s390-ccw/virtio-scsi.h | 2 +-
pc-bios/s390-ccw/virtio.h | 11 +-
pc-bios/s390-ccw/main.c | 13 +-
pc-bios/s390-ccw/virtio-blkdev.c | 57 +++++---
pc-bios/s390-ccw/virtio-ccw.c | 240 +++++++++++++++++++++++++++++++
pc-bios/s390-ccw/virtio-net.c | 5 +-
pc-bios/s390-ccw/virtio-scsi.c | 7 +-
pc-bios/s390-ccw/virtio.c | 209 +++++----------------------
pc-bios/s390-ccw/Makefile | 3 +-
13 files changed, 367 insertions(+), 220 deletions(-)
create mode 100644 pc-bios/s390-ccw/virtio-ccw.h
create mode 100644 pc-bios/s390-ccw/virtio-ccw.c
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index 505cded490..aec2244321 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -103,11 +103,6 @@ QEMU_BUILD_BUG_MSG(offsetof(S390IPLState, iplb) & 3, "alignment
of iplb wrong");
#define DIAG308_PV_STORE 9
#define DIAG308_PV_START 10
-#define S390_IPL_TYPE_FCP 0x00
-#define S390_IPL_TYPE_CCW 0x02
-#define S390_IPL_TYPE_PV 0x05
-#define S390_IPL_TYPE_QEMU_SCSI 0xff
-
#define S390_IPLB_HEADER_LEN 8
#define S390_IPLB_MIN_PV_LEN 148
#define S390_IPLB_MIN_CCW_LEN 200
diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
index 6824391111..aadab87c2e 100644
--- a/include/hw/s390x/ipl/qipl.h
+++ b/include/hw/s390x/ipl/qipl.h
@@ -20,6 +20,12 @@
#define LOADPARM_LEN 8
#define NO_LOADPARM "\0\0\0\0\0\0\0\0"
+#define S390_IPL_TYPE_FCP 0x00
+#define S390_IPL_TYPE_CCW 0x02
+#define S390_IPL_TYPE_PCI 0x04
+#define S390_IPL_TYPE_PV 0x05
+#define S390_IPL_TYPE_QEMU_SCSI 0xff
+
/*
* The QEMU IPL Parameters will be stored at absolute address
* 204 (0xcc) which means it is 32-bit word aligned but not
diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index 08f259ff31..926e8eed5d 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -23,10 +23,6 @@ extern QemuIplParameters qipl;
extern IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
extern bool have_iplb;
-#define S390_IPL_TYPE_FCP 0x00
-#define S390_IPL_TYPE_CCW 0x02
-#define S390_IPL_TYPE_QEMU_SCSI 0xff
This patch is doing quite a bit of different changes at once, making it hard
to review ... It would be nice if you could at least move the
"S390_IPL_TYPE_*" movement into a separate patch.
diff --git a/pc-bios/s390-ccw/virtio-ccw.h b/pc-bios/s390-ccw/virtio-ccw.h
new file mode 100644
index 0000000000..366c4812af
--- /dev/null
+++ b/pc-bios/s390-ccw/virtio-ccw.h
@@ -0,0 +1,25 @@
+/*
+ * Virtio definitions for CCW devices
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <[email protected]>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef VIRTIO_CCW_H
+#define VIRTIO_CCW_H
+
+/* main.c */
+extern SubChannelId blk_schid;
+
+/* virtio-ccw.c */
+int run_ccw(VDev *vdev, int cmd, void *ptr, int len, bool sli);
+int drain_irqs_ccw(SubChannelId schid);
+bool virtio_ccw_is_supported(SubChannelId schid);
+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);
+
+#endif
diff --git a/pc-bios/s390-ccw/virtio-scsi.h b/pc-bios/s390-ccw/virtio-scsi.h
index c5612e16a2..7a37f8b45a 100644
--- a/pc-bios/s390-ccw/virtio-scsi.h
+++ b/pc-bios/s390-ccw/virtio-scsi.h
@@ -69,6 +69,6 @@ static inline bool virtio_scsi_response_ok(const
VirtioScsiCmdResp *r)
int virtio_scsi_read_many(VDev *vdev,
unsigned long sector, void *load_addr, int sec_num);
-int virtio_scsi_setup_device(SubChannelId schid);
+int virtio_scsi_setup_device(void);
#endif /* VIRTIO_SCSI_H */
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 597bd42358..1c1017a0db 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -109,6 +109,8 @@ struct VRing {
};
typedef struct VRing VRing;
+char *virtio_get_ring_area(void);
+
/***********************************************
* Virtio block *
@@ -138,6 +140,9 @@ typedef struct VRing VRing;
/* Barrier before this op. */
#define VIRTIO_BLK_T_BARRIER 0x80000000
+/* For device bus switches */
+extern int ipl_type;
+
/* This is the first element of the read scatter-gather list. */
struct VirtioBlkOuthdr {
/* VIRTIO_BLK_T* */
@@ -236,6 +241,7 @@ struct VDev {
int cmd_vr_idx;
void *ring_area;
long wait_reply_timeout;
+ VirtioDevType type;
VirtioGDN guessed_disk_nature;
SubChannelId schid;
SenseId senseid;
@@ -268,8 +274,9 @@ struct VirtioCmd {
};
typedef struct VirtioCmd VirtioCmd;
+void vring_init(VRing *vr, VqInfo *info);
bool vring_notify(VRing *vr);
-int drain_irqs(SubChannelId schid);
+int drain_irqs(VRing *vr);
void vring_send_buf(VRing *vr, void *p, int len, int flags);
int vr_poll(VRing *vr);
int vring_wait_reply(void);
@@ -282,7 +289,7 @@ int virtio_net_init(void *mac_addr);
void virtio_net_deinit(void);
/* virtio-blkdev.c */
-int virtio_blk_setup_device(SubChannelId schid);
+int virtio_blk_setup_device(void);
int virtio_read(unsigned long sector, void *load_addr);
unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long
rec_list2,
void *load_addr);
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 76bf743900..7299b8911f 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -19,11 +19,12 @@
#include "virtio-scsi.h"
#include "dasd-ipl.h"
-static SubChannelId blk_schid = { .one = 1 };
+SubChannelId blk_schid = { .one = 1 };
static char loadparm_str[LOADPARM_LEN + 1];
QemuIplParameters qipl;
IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
bool have_iplb;
+int ipl_type;
Having two new global variables to pass around information is somewhat ugly...
static uint16_t cutype;
LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
@@ -216,7 +217,7 @@ static bool find_boot_device(void)
VDev *vdev = virtio_get_device();
bool found = false;
- switch (iplb.pbt) {
+ switch (ipl_type) {
case S390_IPL_TYPE_CCW:
vdev->scsi_device_selected = false;
... could we maybe set up vdev->schid here already (or in virtio_setup
below) ...
debug_print_int("device no. ", iplb.ccw.devno);
@@ -245,15 +246,15 @@ static int virtio_setup(void)
vdev->is_cdrom = false;
int ret;
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_NET:
puts("Network boot device detected");
return 0;
case VIRTIO_ID_BLOCK:
- ret = virtio_blk_setup_device(blk_schid);
+ ret = virtio_blk_setup_device();
... and then simply pass vdev as parameter to virtio_blk_setup_device() here ...
break;
case VIRTIO_ID_SCSI:
- ret = virtio_scsi_setup_device(blk_schid);
+ ret = virtio_scsi_setup_device();
... and here.
Then we don't have to make blk_schid global.
(Please also make this as a separate patch first, since this one here is
already quite complicated).
Would it also be possible to store ipl_type in the VDev, so we don't have to
make this variable global?
break;
default:
puts("\n! No IPL device available !\n");
@@ -316,11 +317,13 @@ void main(void)
css_setup();
have_iplb = store_iplb(&iplb);
if (!have_iplb) {
+ ipl_type = S390_IPL_TYPE_CCW; /* Assume CCW for probing */
boot_setup();
probe_boot_device();
}
while (have_iplb) {
+ ipl_type = iplb.pbt;
boot_setup();
if (have_iplb && find_boot_device()) {
ipl_boot_device();
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 4b819dd80f..df6a6d5b70 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -11,6 +11,7 @@
#include <stdio.h>
#include "s390-ccw.h"
#include "virtio.h"
+#include "virtio-ccw.h"
#include "virtio-scsi.h"
#define VIRTIO_BLK_F_GEOMETRY (1 << 4)
@@ -40,9 +41,10 @@ static int virtio_blk_read_many(VDev *vdev, unsigned long
sector, void *load_add
VRING_DESC_F_WRITE | VRING_HIDDEN_IS_CHAIN);
/* Now we can tell the host to read */
+ vring_notify(vr);
Why is there suddenly a new call to vring_notify() popping up here? ... if
it's necessary now, this deserves a separate patch, I think.
vring_wait_reply();
- if (drain_irqs(vr->schid)) {
+ if (drain_irqs(vr)) {
/* Well, whatever status is supposed to contain... */
status = 1;
}
@@ -53,14 +55,14 @@ int virtio_read_many(unsigned long sector, void *load_addr,
int sec_num)
{
VDev *vdev = virtio_get_device();
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_BLOCK:
return virtio_blk_read_many(vdev, sector, load_addr, sec_num);
case VIRTIO_ID_SCSI:
return virtio_scsi_read_many(vdev, sector, load_addr, sec_num);
+ default:
+ return -1;
}
-
- return -1;
}
unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
@@ -119,7 +121,7 @@ void virtio_assume_iso9660(void)
{
VDev *vdev = virtio_get_device();
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_BLOCK:
vdev->guessed_disk_nature = VIRTIO_GDN_SCSI;
vdev->config.blk.blk_size = VIRTIO_ISO_BLOCK_SIZE;
@@ -129,6 +131,8 @@ void virtio_assume_iso9660(void)
case VIRTIO_ID_SCSI:
vdev->scsi_block_size = VIRTIO_ISO_BLOCK_SIZE;
break;
+ default:
+ panic("Virtio device type mismatch for iso9660 IPL");
}
}
@@ -139,13 +143,15 @@ void virtio_assume_eckd(void)
vdev->guessed_disk_nature = VIRTIO_GDN_DASD;
vdev->blk_factor = 1;
vdev->config.blk.physical_block_exp = 0;
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_BLOCK:
vdev->config.blk.blk_size = VIRTIO_DASD_DEFAULT_BLOCK_SIZE;
break;
case VIRTIO_ID_SCSI:
vdev->config.blk.blk_size = vdev->scsi_block_size;
break;
+ default:
+ panic("Virtio device type mismatch for ECKD IPL");
}
vdev->config.blk.geometry.heads = 15;
vdev->config.blk.geometry.sectors =
@@ -162,8 +168,7 @@ bool virtio_ipl_disk_is_valid(void)
return true;
}
- return (vdev->senseid.cu_model == VIRTIO_ID_BLOCK ||
- vdev->senseid.cu_model == VIRTIO_ID_SCSI) &&
+ return (vdev->type == VIRTIO_ID_BLOCK || vdev->type == VIRTIO_ID_SCSI) &&
blksize >= 512 && blksize <= 4096;
}
@@ -171,41 +176,44 @@ int virtio_get_block_size(void)
{
VDev *vdev = virtio_get_device();
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_BLOCK:
return vdev->config.blk.blk_size;
case VIRTIO_ID_SCSI:
return vdev->scsi_block_size;
+ default:
+ return 0;
}
- return 0;
}
uint8_t virtio_get_heads(void)
{
VDev *vdev = virtio_get_device();
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_BLOCK:
return vdev->config.blk.geometry.heads;
case VIRTIO_ID_SCSI:
return vdev->guessed_disk_nature == VIRTIO_GDN_DASD
? vdev->config.blk.geometry.heads : 255;
+ default:
+ return 0;
}
- return 0;
}
uint8_t virtio_get_sectors(void)
{
VDev *vdev = virtio_get_device();
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_BLOCK:
return vdev->config.blk.geometry.sectors;
case VIRTIO_ID_SCSI:
return vdev->guessed_disk_nature == VIRTIO_GDN_DASD
? vdev->config.blk.geometry.sectors : 63;
+ default:
+ return 0;
}
- return 0;
}
uint64_t virtio_get_blocks(void)
@@ -213,24 +221,29 @@ uint64_t virtio_get_blocks(void)
VDev *vdev = virtio_get_device();
const uint64_t factor = virtio_get_block_size() / VIRTIO_SECTOR_SIZE;
- switch (vdev->senseid.cu_model) {
+ switch (vdev->type) {
case VIRTIO_ID_BLOCK:
return vdev->config.blk.capacity / factor;
case VIRTIO_ID_SCSI:
return vdev->scsi_last_block / factor;
+ default:
+ return 0;
}
- return 0;
}
-int virtio_blk_setup_device(SubChannelId schid)
+int virtio_blk_setup_device()
{
VDev *vdev = virtio_get_device();
- vdev->guest_features[0] = VIRTIO_BLK_F_GEOMETRY | VIRTIO_BLK_F_BLK_SIZE;
- vdev->schid = schid;
- virtio_setup_ccw(vdev);
-
puts("Using virtio-blk.");
- return 0;
+ vdev->guest_features[0] = VIRTIO_BLK_F_GEOMETRY | VIRTIO_BLK_F_BLK_SIZE;
+ switch (ipl_type) {
+ case S390_IPL_TYPE_QEMU_SCSI:
+ case S390_IPL_TYPE_CCW:
+ vdev->schid = blk_schid;
+ return virtio_ccw_setup(vdev);
+ }
+
+ return 1;
}
diff --git a/pc-bios/s390-ccw/virtio-ccw.c b/pc-bios/s390-ccw/virtio-ccw.c
new file mode 100644
index 0000000000..1d6e8532f6
--- /dev/null
+++ b/pc-bios/s390-ccw/virtio-ccw.c
@@ -0,0 +1,240 @@
+/*
+ * Virtio functionality for CCW devices
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <[email protected]>
Since you're copying over functions from another file, I think it would make
sense to add the copyright statement from the other file here, too.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <string.h>
+#include "s390-ccw.h"
+#include "cio.h"
+#include "virtio.h"
+#include "virtio-ccw.h"
+#include "virtio-scsi.h"
+#include "bswap.h"
+#include "helper.h"
+#include "s390-time.h"
+
+/* virtio spec v1.0 para 4.3.3.2 */
+static long kvm_hypercall(unsigned long nr, unsigned long param1,
+ unsigned long param2, unsigned long param3)
+{
+ register unsigned long r_nr asm("1") = nr;
+ register unsigned long r_param1 asm("2") = param1;
+ register unsigned long r_param2 asm("3") = param2;
+ register unsigned long r_param3 asm("4") = param3;
+ register long retval asm("2");
+
+ asm volatile ("diag %%r2,%%r4,0x500"
+ : "=d" (retval)
+ : "d" (r_nr), "0" (r_param1), "r"(r_param2), "d"(r_param3)
+ : "memory", "cc");
+
+ return retval;
+}
+
+bool virtio_ccw_is_supported(SubChannelId schid)
+{
+ VDev *vdev = virtio_get_device();
+ vdev->schid = schid;
+ memset(&vdev->senseid, 0, sizeof(vdev->senseid));
+
+ /*
+ * Run sense id command.
+ * The size of the senseid data differs between devices (notably,
+ * between virtio devices and dasds), so specify the largest possible
+ * size and suppress the incorrect length indication for smaller sizes.
+ */
+ if (run_ccw(vdev, CCW_CMD_SENSE_ID, &vdev->senseid, sizeof(vdev->senseid),
+ true)) {
+ return false;
+ }
+
+ vdev->type = vdev->senseid.cu_model;
+
+ if (vdev->senseid.cu_type == 0x3832) {
+ switch (vdev->type) {
+ case VIRTIO_ID_BLOCK:
+ case VIRTIO_ID_SCSI:
+ case VIRTIO_ID_NET:
+ return true;
+ default:
+ return false;
+ }
+ }
+ return false;
+}
You copied the is_supported() function to the new file, but also kept the
original. As far as I can see, virtio_ccw_is_supported() is not used at all
in your new code, so you could remove that here again? Or remove the
original function and only use this one here?
+int drain_irqs_ccw(SubChannelId schid)
+{
+ Irb irb = {};
+ int r = 0;
+
+ while (1) {
+ /* FIXME: make use of TPI, for that enable subchannel and isc */
+ if (tsch(schid, &irb)) {
+ /* Might want to differentiate error codes later on. */
+ if (irb.scsw.cstat) {
+ r = -EIO;
+ } else if (irb.scsw.dstat != 0xc) {
+ r = -EIO;
+ }
+ return r;
+ }
+ }
+}
+
+long virtio_ccw_notify(SubChannelId schid, int vq_idx, long cookie)
+{
+ return kvm_hypercall(KVM_S390_VIRTIO_CCW_NOTIFY, *(u32 *)&schid,
+ 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(vr)) {
+ return -1;
+ }
+ return 0;
+}
+
+int run_ccw(VDev *vdev, int cmd, void *ptr, int len, bool sli)
+{
+ Ccw1 ccw = {};
+
+ ccw.cmd_code = cmd;
+ ccw.cda = (long)ptr;
+ ccw.count = len;
+
+ if (sli) {
+ ccw.flags |= CCW_FLAG_SLI;
+ }
+
+ return do_cio(vdev->schid, vdev->senseid.cu_type, ptr2u32(&ccw), CCW_FMT1);
+}
As far as I can see, run_ccw() is now only used in the new file, so you
could make this function "static".
Thomas
