* [PATCH 1/7] pc-bios/s390-ccw: Fix Misattributed Function Prototypes
2025-10-20 16:20 [PATCH 0/7] s390x: Add support for virtio-blk-pci IPL device jrossi
@ 2025-10-20 16:20 ` jrossi
2025-10-20 16:50 ` Thomas Huth
2025-10-20 16:20 ` [PATCH 2/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio jrossi
` (5 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: jrossi @ 2025-10-20 16:20 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato, jrossi, zycai
From: Jared Rossi <jrossi@linux.ibm.com>
The virtio-blkdev functions are incorrectly listed in s390-ccw.h as belonging to
virtio.c. Additionally, virtio_load_direct() has an unused subchan_id argument.
Remove the unused argument and move the prototypes to virtio.h so that they are
independent from the CCW bus.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
pc-bios/s390-ccw/s390-ccw.h | 4 ----
pc-bios/s390-ccw/virtio.h | 7 +++++++
pc-bios/s390-ccw/bootmap.c | 2 +-
pc-bios/s390-ccw/virtio-blkdev.c | 2 +-
4 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index b1dc35cded..47ea66bd4d 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -67,11 +67,7 @@ void sclp_get_loadparm_ascii(char *loadparm);
int sclp_read(char *str, size_t count);
/* virtio.c */
-unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
- unsigned long subchan_id, void *load_addr);
bool virtio_is_supported(SubChannelId schid);
-int virtio_blk_setup_device(SubChannelId schid);
-int virtio_read(unsigned long sector, void *load_addr);
/* bootmap.c */
void zipl_load(void);
diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
index 5c5e808a50..597bd42358 100644
--- a/pc-bios/s390-ccw/virtio.h
+++ b/pc-bios/s390-ccw/virtio.h
@@ -277,7 +277,14 @@ int virtio_run(VDev *vdev, int vqid, VirtioCmd *cmd);
int virtio_reset(VDev *vdev);
int virtio_setup_ccw(VDev *vdev);
+/* virtio-net.c */
int virtio_net_init(void *mac_addr);
void virtio_net_deinit(void);
+/* virtio-blkdev.c */
+int virtio_blk_setup_device(SubChannelId schid);
+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);
+
#endif /* VIRTIO_H */
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 0f8baa0198..420ee32eff 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -662,7 +662,7 @@ static int zipl_load_segment(ComponentEntry *entry)
*/
break;
}
- address = virtio_load_direct(cur_desc[0], cur_desc[1], 0,
+ address = virtio_load_direct(cur_desc[0], cur_desc[1],
(void *)address);
if (!address) {
puts("zIPL load segment failed");
diff --git a/pc-bios/s390-ccw/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index 7b2d1e20f4..4b819dd80f 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -64,7 +64,7 @@ int virtio_read_many(unsigned long sector, void *load_addr, int sec_num)
}
unsigned long virtio_load_direct(unsigned long rec_list1, unsigned long rec_list2,
- unsigned long subchan_id, void *load_addr)
+ void *load_addr)
{
u8 status;
int sec = rec_list1;
--
2.49.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 1/7] pc-bios/s390-ccw: Fix Misattributed Function Prototypes
2025-10-20 16:20 ` [PATCH 1/7] pc-bios/s390-ccw: Fix Misattributed Function Prototypes jrossi
@ 2025-10-20 16:50 ` Thomas Huth
2025-10-20 18:57 ` Jared Rossi
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Huth @ 2025-10-20 16:50 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> The virtio-blkdev functions are incorrectly listed in s390-ccw.h as belonging to
> virtio.c. Additionally, virtio_load_direct() has an unused subchan_id argument.
>
> Remove the unused argument and move the prototypes to virtio.h so that they are
> independent from the CCW bus.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
I'd maybe spell "Misattributed Function Prototypes" with small letters in
the title, but apart from that, patch looks good to me:
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/7] pc-bios/s390-ccw: Fix Misattributed Function Prototypes
2025-10-20 16:50 ` Thomas Huth
@ 2025-10-20 18:57 ` Jared Rossi
0 siblings, 0 replies; 25+ messages in thread
From: Jared Rossi @ 2025-10-20 18:57 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x
Cc: jjherne, alifm, farman, mjrosato, zycai
On 10/20/25 12:50 PM, Thomas Huth wrote:
> On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> The virtio-blkdev functions are incorrectly listed in s390-ccw.h as
>> belonging to
>> virtio.c. Additionally, virtio_load_direct() has an unused
>> subchan_id argument.
>>
>> Remove the unused argument and move the prototypes to virtio.h so
>> that they are
>> independent from the CCW bus.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
>
> I'd maybe spell "Misattributed Function Prototypes" with small letters
> in the title, but apart from that, patch looks good to me:
>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>
Thanks Thomas,
I'm not sure why I wrote it like that. I notice now I did the same
thing with "Architecture" in patch 3. I'll fix the capitalization in
both patches for the next version.
Regards,
Jared Rossi
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio
2025-10-20 16:20 [PATCH 0/7] s390x: Add support for virtio-blk-pci IPL device jrossi
2025-10-20 16:20 ` [PATCH 1/7] pc-bios/s390-ccw: Fix Misattributed Function Prototypes jrossi
@ 2025-10-20 16:20 ` jrossi
2025-10-21 9:23 ` Thomas Huth
2025-10-20 16:20 ` [PATCH 3/7] pc-bios/s390-ccw: Introduce CLP Architecture jrossi
` (4 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: jrossi @ 2025-10-20 16:20 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato, jrossi, zycai
From: Jared Rossi <jrossi@linux.ibm.com>
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 <jrossi@linux.ibm.com>
---
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
-
static inline bool manage_iplb(IplParameterBlock *iplb, bool store)
{
register unsigned long addr asm("0") = (unsigned long) iplb;
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 <jrossi@linux.ibm.com>
+ *
+ * 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;
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;
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();
break;
case VIRTIO_ID_SCSI:
- ret = virtio_scsi_setup_device(blk_schid);
+ ret = virtio_scsi_setup_device();
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);
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 <jrossi@linux.ibm.com>
+ *
+ * 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;
+}
+
+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);
+}
+
+int virtio_ccw_reset(VDev *vdev)
+{
+ return run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
+}
+
+int virtio_ccw_setup(VDev *vdev)
+{
+ int i, cfg_size = 0;
+ uint8_t status;
+ struct VirtioFeatureDesc {
+ uint32_t features;
+ uint8_t index;
+ } __attribute__((packed)) feats;
+
+ if (!virtio_is_supported(vdev->schid)) {
+ puts("Virtio unsupported for this device ID");
+ return -ENODEV;
+ }
+ /* device ID has been established now */
+
+ vdev->config.blk.blk_size = 0; /* mark "illegal" - setup started... */
+ vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
+
+ virtio_reset(vdev);
+
+ status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
+ if (run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false)) {
+ puts("Could not write ACKNOWLEDGE status to host");
+ return -EIO;
+ }
+
+ switch (vdev->senseid.cu_model) {
+ case VIRTIO_ID_NET:
+ vdev->nr_vqs = 2;
+ vdev->cmd_vr_idx = 0;
+ cfg_size = sizeof(vdev->config.net);
+ break;
+ case VIRTIO_ID_BLOCK:
+ vdev->nr_vqs = 1;
+ vdev->cmd_vr_idx = 0;
+ cfg_size = sizeof(vdev->config.blk);
+ break;
+ case VIRTIO_ID_SCSI:
+ vdev->nr_vqs = 3;
+ vdev->cmd_vr_idx = VR_REQUEST;
+ cfg_size = sizeof(vdev->config.scsi);
+ break;
+ default:
+ puts("Unsupported virtio device");
+ return -ENODEV;
+ }
+
+ status |= VIRTIO_CONFIG_S_DRIVER;
+ if (run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false)) {
+ puts("Could not write DRIVER status to host");
+ return -EIO;
+ }
+
+ /* Feature negotiation */
+ for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) {
+ feats.features = 0;
+ feats.index = i;
+ if (run_ccw(vdev, CCW_CMD_READ_FEAT, &feats, sizeof(feats), false)) {
+ puts("Could not get features bits");
+ return -EIO;
+ }
+
+ vdev->guest_features[i] &= bswap32(feats.features);
+ feats.features = bswap32(vdev->guest_features[i]);
+ if (run_ccw(vdev, CCW_CMD_WRITE_FEAT, &feats, sizeof(feats), false)) {
+ puts("Could not set features bits");
+ return -EIO;
+ }
+ }
+
+ if (run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false)) {
+ puts("Could not get virtio device configuration");
+ return -EIO;
+ }
+
+ for (i = 0; i < vdev->nr_vqs; i++) {
+ VqInfo info = {
+ .queue = (unsigned long long) virtio_get_ring_area() + (i * VIRTIO_RING_SIZE),
+ .align = KVM_S390_VIRTIO_RING_ALIGN,
+ .index = i,
+ .num = 0,
+ };
+ VqConfig config = {
+ .index = i,
+ .num = 0,
+ };
+
+ if (run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config),
+ false)) {
+ puts("Could not get virtio device VQ config");
+ return -EIO;
+ }
+ info.num = config.num;
+ vring_init(&vdev->vrings[i], &info);
+ vdev->vrings[i].schid = vdev->schid;
+ if (run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info), false)) {
+ puts("Cannot set VQ info");
+ return -EIO;
+ }
+ }
+
+ status |= VIRTIO_CONFIG_S_DRIVER_OK;
+ if (run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false)) {
+ puts("Could not write DRIVER_OK status to host");
+ return -EIO;
+ }
+
+ return 0;
+}
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index 301445bf97..604f1cf003 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -19,6 +19,7 @@
#include <ethernet.h>
#include "s390-ccw.h"
#include "virtio.h"
+#include "virtio-ccw.h"
#include "s390-time.h"
#include "helper.h"
@@ -54,7 +55,7 @@ int virtio_net_init(void *mac_addr)
rx_last_idx = 0;
vdev->guest_features[0] = VIRTIO_NET_F_MAC_BIT;
- virtio_setup_ccw(vdev);
+ virtio_ccw_setup(vdev);
if (!(vdev->guest_features[0] & VIRTIO_NET_F_MAC_BIT)) {
puts("virtio-net device does not support the MAC address feature");
@@ -88,7 +89,7 @@ int send(int fd, const void *buf, int len, int flags)
while (!vr_poll(txvq)) {
yield();
}
- if (drain_irqs(txvq->schid)) {
+ if (drain_irqs(txvq)) {
puts("send: drain irqs failed");
return -1;
}
diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c
index 71db75ce7b..4a615599fd 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -13,6 +13,7 @@
#include <stdio.h>
#include "s390-ccw.h"
#include "virtio.h"
+#include "virtio-ccw.h"
#include "scsi.h"
#include "virtio-scsi.h"
#include "s390-time.h"
@@ -476,12 +477,12 @@ static int virtio_scsi_setup(VDev *vdev)
return 0;
}
-int virtio_scsi_setup_device(SubChannelId schid)
+int virtio_scsi_setup_device()
{
VDev *vdev = virtio_get_device();
- vdev->schid = schid;
- virtio_setup_ccw(vdev);
+ vdev->schid = blk_schid;
+ virtio_ccw_setup(vdev);
if (vdev->config.scsi.sense_size != VIRTIO_SCSI_SENSE_SIZE) {
puts("Config: sense size mismatch");
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index cd6c99c7e3..05cfca4dad 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -13,6 +13,7 @@
#include "cio.h"
#include "virtio.h"
#include "virtio-scsi.h"
+#include "virtio-ccw.h"
#include "bswap.h"
#include "helper.h"
#include "s390-time.h"
@@ -41,72 +42,30 @@ VDev *virtio_get_device(void)
VirtioDevType virtio_get_device_type(void)
{
- return vdev.senseid.cu_model;
+ return vdev.type;
}
-/* 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)
+char *virtio_get_ring_area(void)
{
- 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;
-}
-
-static long virtio_notify(SubChannelId schid, int vq_idx, long cookie)
-{
- return kvm_hypercall(KVM_S390_VIRTIO_CCW_NOTIFY, *(u32 *)&schid,
- vq_idx, cookie);
+ return ring_area;
}
/***********************************************
* Virtio functions *
***********************************************/
-int drain_irqs(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;
- }
- }
-}
-
-static int run_ccw(VDev *vdev, int cmd, void *ptr, int len, bool sli)
+int drain_irqs(VRing *vr)
{
- Ccw1 ccw = {};
-
- ccw.cmd_code = cmd;
- ccw.cda = (long)ptr;
- ccw.count = len;
-
- if (sli) {
- ccw.flags |= CCW_FLAG_SLI;
+ switch (ipl_type) {
+ case S390_IPL_TYPE_QEMU_SCSI:
+ case S390_IPL_TYPE_CCW:
+ return drain_irqs_ccw(vr->schid);
}
- return do_cio(vdev->schid, vdev->senseid.cu_type, ptr2u32(&ccw), CCW_FMT1);
+ return 0;
}
-static void vring_init(VRing *vr, VqInfo *info)
+void vring_init(VRing *vr, VqInfo *info)
{
void *p = (void *) info->queue;
@@ -134,7 +93,12 @@ static void vring_init(VRing *vr, VqInfo *info)
bool vring_notify(VRing *vr)
{
- vr->cookie = virtio_notify(vr->schid, vr->id, vr->cookie);
+ switch (ipl_type) {
+ case S390_IPL_TYPE_QEMU_SCSI:
+ case S390_IPL_TYPE_CCW:
+ vr->cookie = virtio_ccw_notify(vr->schid, vr->id, vr->cookie);
+ }
+
return vr->cookie >= 0;
}
@@ -202,134 +166,24 @@ int vring_wait_reply(void)
int virtio_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->schid)) {
- return -1;
+ switch (ipl_type) {
+ case S390_IPL_TYPE_QEMU_SCSI:
+ case S390_IPL_TYPE_CCW:
+ return virtio_ccw_run(vdev, vqid, cmd);
}
- return 0;
-}
-int virtio_reset(VDev *vdev)
-{
- return run_ccw(vdev, CCW_CMD_VDEV_RESET, NULL, 0, false);
+ return -1;
}
-int virtio_setup_ccw(VDev *vdev)
+int virtio_reset(VDev *vdev)
{
- int i, cfg_size = 0;
- uint8_t status;
- struct VirtioFeatureDesc {
- uint32_t features;
- uint8_t index;
- } __attribute__((packed)) feats;
-
- if (!virtio_is_supported(vdev->schid)) {
- puts("Virtio unsupported for this device ID");
- return -ENODEV;
+ switch (ipl_type) {
+ case S390_IPL_TYPE_QEMU_SCSI:
+ case S390_IPL_TYPE_CCW:
+ return virtio_ccw_reset(vdev);
}
- /* device ID has been established now */
-
- vdev->config.blk.blk_size = 0; /* mark "illegal" - setup started... */
- vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
-
- virtio_reset(vdev);
- status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
- if (run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false)) {
- puts("Could not write ACKNOWLEDGE status to host");
- return -EIO;
- }
-
- switch (vdev->senseid.cu_model) {
- case VIRTIO_ID_NET:
- vdev->nr_vqs = 2;
- vdev->cmd_vr_idx = 0;
- cfg_size = sizeof(vdev->config.net);
- break;
- case VIRTIO_ID_BLOCK:
- vdev->nr_vqs = 1;
- vdev->cmd_vr_idx = 0;
- cfg_size = sizeof(vdev->config.blk);
- break;
- case VIRTIO_ID_SCSI:
- vdev->nr_vqs = 3;
- vdev->cmd_vr_idx = VR_REQUEST;
- cfg_size = sizeof(vdev->config.scsi);
- break;
- default:
- puts("Unsupported virtio device");
- return -ENODEV;
- }
-
- status |= VIRTIO_CONFIG_S_DRIVER;
- if (run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false)) {
- puts("Could not write DRIVER status to host");
- return -EIO;
- }
-
- /* Feature negotiation */
- for (i = 0; i < ARRAY_SIZE(vdev->guest_features); i++) {
- feats.features = 0;
- feats.index = i;
- if (run_ccw(vdev, CCW_CMD_READ_FEAT, &feats, sizeof(feats), false)) {
- puts("Could not get features bits");
- return -EIO;
- }
-
- vdev->guest_features[i] &= bswap32(feats.features);
- feats.features = bswap32(vdev->guest_features[i]);
- if (run_ccw(vdev, CCW_CMD_WRITE_FEAT, &feats, sizeof(feats), false)) {
- puts("Could not set features bits");
- return -EIO;
- }
- }
-
- if (run_ccw(vdev, CCW_CMD_READ_CONF, &vdev->config, cfg_size, false)) {
- puts("Could not get virtio device configuration");
- return -EIO;
- }
-
- for (i = 0; i < vdev->nr_vqs; i++) {
- VqInfo info = {
- .queue = (unsigned long long) ring_area + (i * VIRTIO_RING_SIZE),
- .align = KVM_S390_VIRTIO_RING_ALIGN,
- .index = i,
- .num = 0,
- };
- VqConfig config = {
- .index = i,
- .num = 0,
- };
-
- if (run_ccw(vdev, CCW_CMD_READ_VQ_CONF, &config, sizeof(config),
- false)) {
- puts("Could not get virtio device VQ config");
- return -EIO;
- }
- info.num = config.num;
- vring_init(&vdev->vrings[i], &info);
- vdev->vrings[i].schid = vdev->schid;
- if (run_ccw(vdev, CCW_CMD_SET_VQ, &info, sizeof(info), false)) {
- puts("Cannot set VQ info");
- return -EIO;
- }
- }
-
- status |= VIRTIO_CONFIG_S_DRIVER_OK;
- if (run_ccw(vdev, CCW_CMD_WRITE_STATUS, &status, sizeof(status), false)) {
- puts("Could not write DRIVER_OK status to host");
- return -EIO;
- }
-
- return 0;
+ return -1;
}
bool virtio_is_supported(SubChannelId schid)
@@ -347,12 +201,17 @@ bool virtio_is_supported(SubChannelId schid)
true)) {
return false;
}
+
+ vdev.type = vdev.senseid.cu_model;
+
if (vdev.senseid.cu_type == 0x3832) {
- switch (vdev.senseid.cu_model) {
+ switch (vdev.type) {
case VIRTIO_ID_BLOCK:
case VIRTIO_ID_SCSI:
case VIRTIO_ID_NET:
return true;
+ default:
+ return false;
}
}
return false;
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index a0f24c94a8..3577ac381a 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -34,7 +34,8 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
.PHONY : all clean build-all distclean
OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o netmain.o \
- virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o
+ virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o \
+ virtio-ccw.o
SLOF_DIR := $(SRC_PATH)/../../roms/SLOF
--
2.49.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio
2025-10-20 16:20 ` [PATCH 2/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio jrossi
@ 2025-10-21 9:23 ` Thomas Huth
2025-10-22 19:44 ` Jared Rossi
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Huth @ 2025-10-21 9:23 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
Hi Jared!
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> 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 <jrossi@linux.ibm.com>
> ---
> 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 <jrossi@linux.ibm.com>
> + *
> + * 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 <jrossi@linux.ibm.com>
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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio
2025-10-21 9:23 ` Thomas Huth
@ 2025-10-22 19:44 ` Jared Rossi
0 siblings, 0 replies; 25+ messages in thread
From: Jared Rossi @ 2025-10-22 19:44 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x
Cc: jjherne, alifm, farman, mjrosato, zycai
Hi Thomas,
Thanks for your comments...
On 10/21/25 5:23 AM, Thomas Huth wrote:
> Hi Jared!
>
> On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> 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 <jrossi@linux.ibm.com>
>> ---
>> 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
>>
> 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.
Sure, I can break this up. I think your suggestions about removing the
global variables by storing them in the VDev struct make sense. I'll
split it into smaller patches and clean up the things you mentioned.
Thanks,
Jared Rossi
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/7] pc-bios/s390-ccw: Introduce CLP Architecture
2025-10-20 16:20 [PATCH 0/7] s390x: Add support for virtio-blk-pci IPL device jrossi
2025-10-20 16:20 ` [PATCH 1/7] pc-bios/s390-ccw: Fix Misattributed Function Prototypes jrossi
2025-10-20 16:20 ` [PATCH 2/7] pc-bios/s390-ccw: Split virtio-ccw and generic virtio jrossi
@ 2025-10-20 16:20 ` jrossi
2025-10-21 5:24 ` Thomas Huth
2025-10-21 9:30 ` Thomas Huth
2025-10-20 16:20 ` [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format jrossi
` (3 subsequent siblings)
6 siblings, 2 replies; 25+ messages in thread
From: jrossi @ 2025-10-20 16:20 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato, jrossi, zycai
From: Jared Rossi <jrossi@linux.ibm.com>
Call Logical Processor (CLP) Architecture is used for managing PCI functions on
s390x. Define and include the structures and routines needed to interact with
PCI devices during IPL.
Headers in ~/qemu/include/hw are not normally visible and must be included
using a relative path. Due to this, the QEMU_PACKED macro must also be defined
here.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
pc-bios/s390-ccw/clp.h | 24 +++++++++
pc-bios/s390-ccw/clp.c | 106 ++++++++++++++++++++++++++++++++++++++
pc-bios/s390-ccw/Makefile | 2 +-
3 files changed, 131 insertions(+), 1 deletion(-)
create mode 100644 pc-bios/s390-ccw/clp.h
create mode 100644 pc-bios/s390-ccw/clp.c
diff --git a/pc-bios/s390-ccw/clp.h b/pc-bios/s390-ccw/clp.h
new file mode 100644
index 0000000000..cb130e5e90
--- /dev/null
+++ b/pc-bios/s390-ccw/clp.h
@@ -0,0 +1,24 @@
+/*
+ * Call Logical Processor (CLP) architecture definitions
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <jrossi@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef CLP_H
+#define CLP_H
+
+#ifndef QEMU_PACKED
+#define QEMU_PACKED __attribute__((packed))
+#endif
+
+#include <stdint.h>
+#include "../../include/hw/s390x/s390-pci-clp.h"
+
+int clp_pci(void *data);
+int enable_pci_function(uint32_t *fhandle);
+int enumerate_pci_functions(void);
+
+#endif
diff --git a/pc-bios/s390-ccw/clp.c b/pc-bios/s390-ccw/clp.c
new file mode 100644
index 0000000000..45d496fc18
--- /dev/null
+++ b/pc-bios/s390-ccw/clp.c
@@ -0,0 +1,106 @@
+/*
+ * Call Logical Processor (CLP) architecture
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <jrossi@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "clp.h"
+#include <stdio.h>
+#include <string.h>
+
+int clp_pci(void *data)
+{
+ struct { uint8_t _[2048]; } *req = data;
+ int cc = 3;
+
+ asm volatile (
+ " .insn rrf,0xb9a00000,0,%[req],0,2\n"
+ " ipm %[cc]\n"
+ " srl %[cc],28\n"
+ : [cc] "+d" (cc), "+m" (*req)
+ : [req] "a" (req)
+ : "cc");
+ return cc;
+}
+
+/*
+ * Get the PCI function entry for a given function ID
+ * Return 0 on success, 1 if the FID is not found, or a negative RC on error
+ */
+int find_pci_function(uint32_t fid, ClpFhListEntry *entry)
+{
+ int rc;
+ int count = 0;
+ int limit = PCI_MAX_FUNCTIONS;
+ ClpReqRspListPci rrb;
+
+ rrb.request.hdr.len = 32;
+ rrb.request.hdr.cmd = 0x02;
+ rrb.request.resume_token = 0;
+ rrb.response.hdr.len = sizeof(ClpRspListPci);
+
+ do {
+ rc = clp_pci(&rrb);
+ if (rc) {
+ return -rc;
+ }
+
+ if (rrb.response.hdr.rsp != 0x0010) {
+ printf("Failed to list PCI functions: %x", rrb.response.hdr.rsp);
+ return -1;
+ }
+
+ /* Resume token set when max enteries are returned */
+ if (rrb.response.resume_token) {
+ count = CLP_FH_LIST_NR_ENTRIES;
+ rrb.request.resume_token = rrb.response.resume_token;
+ } else {
+ count = (rrb.response.hdr.len - 32) / sizeof(ClpFhListEntry);
+ }
+
+ limit -= count;
+
+ for (int i = 0; i < count; i++) {
+ if (rrb.response.fh_list[i].fid == fid) {
+ memcpy(entry, &rrb.response.fh_list[i], sizeof(ClpFhListEntry));
+ return 0;
+ }
+ }
+
+ } while (rrb.request.resume_token && limit);
+
+ return 1;
+}
+
+/*
+ * Enable the PCI function associated with a given handle
+ * Return 0 on success or a negative RC on error
+ */
+int enable_pci_function(uint32_t *fhandle)
+{
+ ClpReqRspSetPci rrb;
+ int rc;
+
+ rrb.request.hdr.len = 32;
+ rrb.request.hdr.cmd = 0x05;
+ rrb.request.fh = *fhandle;
+ rrb.request.oc = 0;
+ rrb.request.ndas = 1;
+ rrb.response.hdr.len = 32;
+
+ rc = clp_pci(&rrb);
+ if (rc) {
+ return -rc;
+ }
+
+ if (rrb.response.hdr.rsp != 0x0010) {
+ printf("Failed to enable PCI function: %x", rrb.response.hdr.rsp);
+ return -1;
+ }
+
+ *fhandle = rrb.response.fh;
+ return 0;
+}
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 3577ac381a..89a42ae506 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -35,7 +35,7 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o netmain.o \
virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o \
- virtio-ccw.o
+ virtio-ccw.o clp.o
SLOF_DIR := $(SRC_PATH)/../../roms/SLOF
--
2.49.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 3/7] pc-bios/s390-ccw: Introduce CLP Architecture
2025-10-20 16:20 ` [PATCH 3/7] pc-bios/s390-ccw: Introduce CLP Architecture jrossi
@ 2025-10-21 5:24 ` Thomas Huth
2025-10-21 9:30 ` Thomas Huth
1 sibling, 0 replies; 25+ messages in thread
From: Thomas Huth @ 2025-10-21 5:24 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Call Logical Processor (CLP) Architecture is used for managing PCI functions on
> s390x. Define and include the structures and routines needed to interact with
> PCI devices during IPL.
>
> Headers in ~/qemu/include/hw are not normally visible and must be included
> using a relative path.
That's kind of ugly. Could we move the header (or at least the structs that
you need) into include/hw/s390x/ipl/ instead? IMHO it should be ok to
include "include/hw/s390x/ipl/s390-pci-bus.h" from hw/s390x/s390-pci-vfio.c,
but if you mind the "ipl" here, we could also keep a stub header in
include/hw/s390x/s390-pci-bus.h that itself simply includes
"ipl/s390-pci-bus.h" ? Or something similar?
Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/7] pc-bios/s390-ccw: Introduce CLP Architecture
2025-10-20 16:20 ` [PATCH 3/7] pc-bios/s390-ccw: Introduce CLP Architecture jrossi
2025-10-21 5:24 ` Thomas Huth
@ 2025-10-21 9:30 ` Thomas Huth
1 sibling, 0 replies; 25+ messages in thread
From: Thomas Huth @ 2025-10-21 9:30 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Call Logical Processor (CLP) Architecture is used for managing PCI functions on
> s390x. Define and include the structures and routines needed to interact with
> PCI devices during IPL.
>
> Headers in ~/qemu/include/hw are not normally visible and must be included
> using a relative path. Due to this, the QEMU_PACKED macro must also be defined
> here.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
> pc-bios/s390-ccw/clp.h | 24 +++++++++
> pc-bios/s390-ccw/clp.c | 106 ++++++++++++++++++++++++++++++++++++++
> pc-bios/s390-ccw/Makefile | 2 +-
> 3 files changed, 131 insertions(+), 1 deletion(-)
> create mode 100644 pc-bios/s390-ccw/clp.h
> create mode 100644 pc-bios/s390-ccw/clp.c
>
> diff --git a/pc-bios/s390-ccw/clp.h b/pc-bios/s390-ccw/clp.h
> new file mode 100644
> index 0000000000..cb130e5e90
> --- /dev/null
> +++ b/pc-bios/s390-ccw/clp.h
> @@ -0,0 +1,24 @@
> +/*
> + * Call Logical Processor (CLP) architecture definitions
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi <jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef CLP_H
> +#define CLP_H
> +
> +#ifndef QEMU_PACKED
> +#define QEMU_PACKED __attribute__((packed))
> +#endif
> +
> +#include <stdint.h>
> +#include "../../include/hw/s390x/s390-pci-clp.h"
> +
> +int clp_pci(void *data);
> +int enable_pci_function(uint32_t *fhandle);
> +int enumerate_pci_functions(void);
I just noticed that you later remove this unused prototype for
enumerate_pci_functions() in a later patch again. Please remove it from this
patch here in the next respin instead.
Thanks,
Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format
2025-10-20 16:20 [PATCH 0/7] s390x: Add support for virtio-blk-pci IPL device jrossi
` (2 preceding siblings ...)
2025-10-20 16:20 ` [PATCH 3/7] pc-bios/s390-ccw: Introduce CLP Architecture jrossi
@ 2025-10-20 16:20 ` jrossi
2025-10-21 6:42 ` Thomas Huth
2025-10-23 17:31 ` Farhan Ali
2025-10-20 16:20 ` [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL jrossi
` (2 subsequent siblings)
6 siblings, 2 replies; 25+ messages in thread
From: jrossi @ 2025-10-20 16:20 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato, jrossi, zycai
From: Jared Rossi <jrossi@linux.ibm.com>
Define selected s390x PCI instructions and extend IPLB to allow PCI devices.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
include/hw/s390x/ipl/qipl.h | 9 ++
pc-bios/s390-ccw/pci.h | 77 +++++++++++++++
pc-bios/s390-ccw/pci.c | 191 ++++++++++++++++++++++++++++++++++++
pc-bios/s390-ccw/Makefile | 2 +-
4 files changed, 278 insertions(+), 1 deletion(-)
create mode 100644 pc-bios/s390-ccw/pci.h
create mode 100644 pc-bios/s390-ccw/pci.c
diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
index aadab87c2e..efd7b3797c 100644
--- a/include/hw/s390x/ipl/qipl.h
+++ b/include/hw/s390x/ipl/qipl.h
@@ -104,6 +104,14 @@ struct IplBlockQemuScsi {
} QEMU_PACKED;
typedef struct IplBlockQemuScsi IplBlockQemuScsi;
+struct IplBlockPci {
+ uint32_t reserved0[80];
+ uint8_t opt;
+ uint8_t reserved1[3];
+ uint32_t fid;
+} QEMU_PACKED;
+typedef struct IplBlockPci IplBlockPci;
+
union IplParameterBlock {
struct {
uint32_t len;
@@ -119,6 +127,7 @@ union IplParameterBlock {
IplBlockFcp fcp;
IPLBlockPV pv;
IplBlockQemuScsi scsi;
+ IplBlockPci pci;
};
} QEMU_PACKED;
struct {
diff --git a/pc-bios/s390-ccw/pci.h b/pc-bios/s390-ccw/pci.h
new file mode 100644
index 0000000000..b5dc5bff35
--- /dev/null
+++ b/pc-bios/s390-ccw/pci.h
@@ -0,0 +1,77 @@
+/*
+ * s390x PCI definitions
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <jrossi@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef PCI_H
+#define PCI_H
+
+#include <stdint.h>
+#include "clp.h"
+
+#define ZPCI_CREATE_REQ(handle, space, len) \
+ ((uint64_t) handle << 32 | space << 16 | len)
+
+union register_pair {
+ unsigned __int128 pair;
+ struct {
+ unsigned long even;
+ unsigned long odd;
+ };
+};
+
+#define PCIFIB_FC_ENABLED 0x80
+#define PCIFIB_FC_ERROR 0x40
+#define PCIFIB_FC_BLOCKED 0x20
+#define PCIFIB_FC_DMAREG 0x10
+
+#define PCIST_DISABLED 0x0
+#define PCIST_ENABLED 0x1
+
+#define PCI_CAPABILITY_LIST 0x34 /* Offset of first capability list entry */
+
+struct PciFib {
+ uint32_t reserved0[2];
+ uint8_t fcflags;
+ uint8_t reserved1[3];
+ uint32_t reserved2;
+ uint64_t pba;
+ uint64_t pal;
+ uint64_t iota;
+ uint16_t isc:4;
+ uint16_t noi:12;
+ uint8_t reserved3:2;
+ uint8_t aibvo:6;
+ uint8_t s:1;
+ uint8_t reserved4:1;
+ uint8_t aisbo:6;
+ uint32_t reserved5;
+ uint64_t aibv;
+ uint64_t aisb;
+ uint64_t fmba;
+ uint32_t reserved6[2];
+};
+typedef struct PciFib PciFib;
+
+struct PciDevice {
+ uint16_t device_id;
+ uint16_t vendor_id;
+ uint32_t fid;
+ uint32_t fhandle;
+ uint8_t status;
+ PciFib fib;
+};
+typedef struct PciDevice PciDevice;
+
+int pci_write(uint32_t fhandle, uint64_t offset, uint64_t data, uint8_t len);
+int pci_read(uint32_t fhandle, uint64_t offset, uint8_t picas, void *buf, uint8_t len);
+uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type);
+int pci_dev_enable(PciDevice *pcidev);
+int get_fib(PciFib *fib, uint32_t fhandle);
+int set_fib(PciFib *fib, uint32_t fhandle, uint8_t dma_as, uint8_t opcontrol);
+
+#endif
diff --git a/pc-bios/s390-ccw/pci.c b/pc-bios/s390-ccw/pci.c
new file mode 100644
index 0000000000..f776bc064c
--- /dev/null
+++ b/pc-bios/s390-ccw/pci.c
@@ -0,0 +1,191 @@
+/*
+ * s390x PCI funcionality
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <jrossi@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "clp.h"
+#include "pci.h"
+#include <stdio.h>
+
+/* PCI load */
+static inline int pcilg(uint64_t *data, uint64_t req, uint64_t offset, uint8_t *status)
+{
+ union register_pair req_off = {.even = req, .odd = offset};
+ int cc = -1;
+ uint64_t __data = 0x92;
+
+ asm volatile (
+ " .insn rre,0xb9d20000,%[data],%[req_off]\n"
+ " ipm %[cc]\n"
+ " srl %[cc],28\n"
+ : [cc] "+d" (cc), [data] "=d" (__data),
+ [req_off] "+&d" (req_off.pair) :: "cc");
+ *status = req_off.even >> 24 & 0xff;
+ *data = __data;
+ return cc;
+}
+
+/* PCI store */
+int pcistg(uint64_t data, uint64_t req, uint64_t offset, uint8_t *status)
+{
+ union register_pair req_off = {.even = req, .odd = offset};
+ int cc = -1;
+
+ asm volatile (
+ " .insn rre,0xb9d00000,%[data],%[req_off]\n"
+ " ipm %[cc]\n"
+ " srl %[cc],28\n"
+ : [cc] "+d" (cc), [req_off] "+&d" (req_off.pair)
+ : [data] "d" (data)
+ : "cc");
+ *status = req_off.even >> 24 & 0xff;
+ return cc;
+}
+
+/* store PCI function controls */
+int stpcifc(uint64_t req, PciFib *fib, uint8_t *status)
+{
+ uint8_t cc;
+
+ asm volatile (
+ " .insn rxy,0xe300000000d4,%[req],%[fib]\n"
+ " ipm %[cc]\n"
+ " srl %[cc],28\n"
+ : [cc] "=d" (cc), [req] "+d" (req), [fib] "+Q" (*fib)
+ : : "cc");
+ *status = req >> 24 & 0xff;
+ return cc;
+}
+
+/* modify PCI function controls */
+int mpcifc(uint64_t req, PciFib *fib, uint8_t *status)
+{
+ uint8_t cc;
+
+ asm volatile (
+ " .insn rxy,0xe300000000d0,%[req],%[fib]\n"
+ " ipm %[cc]\n"
+ " srl %[cc],28\n"
+ : [cc] "=d" (cc), [req] "+d" (req), [fib] "+Q" (*fib)
+ : : "cc");
+ *status = req >> 24 & 0xff;
+ return cc;
+}
+
+int pci_write(uint32_t fhandle, uint64_t offset, uint64_t data, uint8_t len)
+{
+
+ uint64_t req = ZPCI_CREATE_REQ(fhandle, 4, len);
+ uint8_t status;
+ int rc;
+
+ rc = pcistg(data, req, offset, &status);
+ if (rc == 1) {
+ return status;
+ } else if (rc) {
+ return rc;
+ }
+
+ return 0;
+}
+
+int pci_read(uint32_t fhandle, uint64_t offset, uint8_t picas, void *buf, uint8_t len)
+{
+ uint64_t req;
+ uint64_t data;
+ uint8_t status;
+ int readlen;
+ int i = 0;
+ int rc = 0;
+
+ while (len > 0 && !rc) {
+ data = 0;
+ readlen = len > 8 ? 8 : len;
+ req = ZPCI_CREATE_REQ(fhandle, picas, readlen);
+ rc = pcilg(&data, req, offset + (i * 8), &status);
+ ((uint64_t *)buf)[i] = data;
+ len -= readlen;
+ i++;
+ }
+
+ if (rc == 1) {
+ return status;
+ } else if (rc) {
+ return rc;
+ }
+
+ return 0;
+}
+
+/*
+ * Find the position of the capability config within PCI configuration
+ * space for a given cfg type. Return the position if found, otherwise 0.
+ */
+uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type) {
+ uint64_t req, next, cfg;
+ uint8_t status;
+ int rc;
+
+ req = ZPCI_CREATE_REQ(fhandle, 0xf, 1);
+ rc = pcilg(&next, req, PCI_CAPABILITY_LIST, &status);
+ rc = pcilg(&cfg, req, next + 3, &status);
+
+ while (!rc && (cfg != cfg_type) && next) {
+ rc = pcilg(&next, req, next + 1, &status);
+ rc = pcilg(&cfg, req, next + 3, &status);
+ }
+
+ return rc ? 0 : next;
+}
+
+int pci_dev_enable(PciDevice *pcidev)
+{
+ int rc;
+
+ rc = enable_pci_function(&pcidev->fhandle);
+ if (rc) {
+ return rc;
+ }
+
+ pcidev->status = PCIST_ENABLED;
+
+ return get_fib(&pcidev->fib, pcidev->fhandle);
+}
+
+int get_fib(PciFib *fib, uint32_t fhandle)
+{
+ uint64_t req = ZPCI_CREATE_REQ(fhandle, 0, 0);
+ uint8_t status;
+ int rc;
+
+ rc = stpcifc(req, fib, &status);
+
+ if (rc == 1) {
+ return status;
+ } else if (rc) {
+ return rc;
+ }
+
+ return 0;
+}
+
+int set_fib(PciFib *fib, uint32_t fhandle, uint8_t dma_as, uint8_t opcontrol)
+{
+ uint64_t req = ZPCI_CREATE_REQ(fhandle, dma_as, opcontrol);
+ uint8_t status;
+ int rc;
+
+ rc = mpcifc(req, fib, &status);
+
+ if (rc == 1) {
+ return status;
+ } else if (rc) {
+ return rc;
+ }
+
+ return 0;
+}
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 89a42ae506..1f17f98fc1 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -35,7 +35,7 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o netmain.o \
virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o \
- virtio-ccw.o clp.o
+ virtio-ccw.o clp.o pci.o
SLOF_DIR := $(SRC_PATH)/../../roms/SLOF
--
2.49.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format
2025-10-20 16:20 ` [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format jrossi
@ 2025-10-21 6:42 ` Thomas Huth
2025-10-23 17:31 ` Farhan Ali
1 sibling, 0 replies; 25+ messages in thread
From: Thomas Huth @ 2025-10-21 6:42 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Define selected s390x PCI instructions and extend IPLB to allow PCI devices.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
> include/hw/s390x/ipl/qipl.h | 9 ++
> pc-bios/s390-ccw/pci.h | 77 +++++++++++++++
> pc-bios/s390-ccw/pci.c | 191 ++++++++++++++++++++++++++++++++++++
> pc-bios/s390-ccw/Makefile | 2 +-
> 4 files changed, 278 insertions(+), 1 deletion(-)
> create mode 100644 pc-bios/s390-ccw/pci.h
> create mode 100644 pc-bios/s390-ccw/pci.c
>
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index aadab87c2e..efd7b3797c 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -104,6 +104,14 @@ struct IplBlockQemuScsi {
> } QEMU_PACKED;
> typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>
> +struct IplBlockPci {
> + uint32_t reserved0[80];
> + uint8_t opt;
> + uint8_t reserved1[3];
> + uint32_t fid;
> +} QEMU_PACKED;
Looks like all members of this struct are naturally aligned ... I think you
could likely drop the QEMU_PACKED here.
> +typedef struct IplBlockPci IplBlockPci;
> +
> union IplParameterBlock {
> struct {
> uint32_t len;
> @@ -119,6 +127,7 @@ union IplParameterBlock {
> IplBlockFcp fcp;
> IPLBlockPV pv;
> IplBlockQemuScsi scsi;
> + IplBlockPci pci;
> };
> } QEMU_PACKED;
> struct {
...
> diff --git a/pc-bios/s390-ccw/pci.c b/pc-bios/s390-ccw/pci.c
> new file mode 100644
> index 0000000000..f776bc064c
> --- /dev/null
> +++ b/pc-bios/s390-ccw/pci.c
> @@ -0,0 +1,191 @@
> +/*
> + * s390x PCI funcionality
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi <jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "clp.h"
> +#include "pci.h"
> +#include <stdio.h>
> +
> +/* PCI load */
> +static inline int pcilg(uint64_t *data, uint64_t req, uint64_t offset, uint8_t *status)
> +{
> + union register_pair req_off = {.even = req, .odd = offset};
> + int cc = -1;
> + uint64_t __data = 0x92;
> +
> + asm volatile (
> + " .insn rre,0xb9d20000,%[data],%[req_off]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "+d" (cc), [data] "=d" (__data),
> + [req_off] "+&d" (req_off.pair) :: "cc");
What's the "&" good for here?
> + *status = req_off.even >> 24 & 0xff;
> + *data = __data;
> + return cc;
> +}
> +
> +/* PCI store */
> +int pcistg(uint64_t data, uint64_t req, uint64_t offset, uint8_t *status)
> +{
> + union register_pair req_off = {.even = req, .odd = offset};
> + int cc = -1;
> +
> + asm volatile (
> + " .insn rre,0xb9d00000,%[data],%[req_off]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "+d" (cc), [req_off] "+&d" (req_off.pair)
dito
> + : [data] "d" (data)
> + : "cc");
> + *status = req_off.even >> 24 & 0xff;
> + return cc;
> +}
> +
> +/* store PCI function controls */
> +int stpcifc(uint64_t req, PciFib *fib, uint8_t *status)
> +{
> + uint8_t cc;
> +
> + asm volatile (
> + " .insn rxy,0xe300000000d4,%[req],%[fib]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "=d" (cc), [req] "+d" (req), [fib] "+Q" (*fib)
> + : : "cc");
> + *status = req >> 24 & 0xff;
> + return cc;
> +}
> +
> +/* modify PCI function controls */
> +int mpcifc(uint64_t req, PciFib *fib, uint8_t *status)
> +{
> + uint8_t cc;
> +
> + asm volatile (
> + " .insn rxy,0xe300000000d0,%[req],%[fib]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "=d" (cc), [req] "+d" (req), [fib] "+Q" (*fib)
> + : : "cc");
> + *status = req >> 24 & 0xff;
> + return cc;
> +}
> +
> +int pci_write(uint32_t fhandle, uint64_t offset, uint64_t data, uint8_t len)
> +{
> +
> + uint64_t req = ZPCI_CREATE_REQ(fhandle, 4, len);
> + uint8_t status;
> + int rc;
> +
> + rc = pcistg(data, req, offset, &status);
> + if (rc == 1) {
> + return status;
> + } else if (rc) {
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +int pci_read(uint32_t fhandle, uint64_t offset, uint8_t picas, void *buf, uint8_t len)
> +{
> + uint64_t req;
> + uint64_t data;
> + uint8_t status;
> + int readlen;
> + int i = 0;
> + int rc = 0;
> +
> + while (len > 0 && !rc) {
> + data = 0;
> + readlen = len > 8 ? 8 : len;
> + req = ZPCI_CREATE_REQ(fhandle, picas, readlen);
> + rc = pcilg(&data, req, offset + (i * 8), &status);
> + ((uint64_t *)buf)[i] = data;
This looks somewhat dangerous ... what if buf points to a buffer where its
lengths is not divisible by 8? ... you'll happily overwrite the data that is
right behind the buffer in memory.
> + len -= readlen;
> + i++;
> + }
> +
> + if (rc == 1) {
> + return status;
> + } else if (rc) {
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Find the position of the capability config within PCI configuration
> + * space for a given cfg type. Return the position if found, otherwise 0.
> + */
> +uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type) {
Curly bracket on the next line, please.
> + uint64_t req, next, cfg;
> + uint8_t status;
> + int rc;
> +
> + req = ZPCI_CREATE_REQ(fhandle, 0xf, 1);
> + rc = pcilg(&next, req, PCI_CAPABILITY_LIST, &status);
> + rc = pcilg(&cfg, req, next + 3, &status);
Assigning rc just to discard the value again in the next line does not make
sense... if you're lazy, use "rc |= ..." in the second line. Otherwise
please explicitly check the "rc" after the first call.
> + while (!rc && (cfg != cfg_type) && next) {
> + rc = pcilg(&next, req, next + 1, &status);
> + rc = pcilg(&cfg, req, next + 3, &status);
dito
> + }
> +
> + return rc ? 0 : next;
> +}
Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format
2025-10-20 16:20 ` [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format jrossi
2025-10-21 6:42 ` Thomas Huth
@ 2025-10-23 17:31 ` Farhan Ali
2025-10-23 17:56 ` Farhan Ali
2025-10-23 18:19 ` Jared Rossi
1 sibling, 2 replies; 25+ messages in thread
From: Farhan Ali @ 2025-10-23 17:31 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x, thuth; +Cc: jjherne, farman, mjrosato, zycai
Hi Jared,
On 10/20/2025 9:20 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi<jrossi@linux.ibm.com>
>
> Define selected s390x PCI instructions and extend IPLB to allow PCI devices.
>
> Signed-off-by: Jared Rossi<jrossi@linux.ibm.com>
> ---
> include/hw/s390x/ipl/qipl.h | 9 ++
> pc-bios/s390-ccw/pci.h | 77 +++++++++++++++
> pc-bios/s390-ccw/pci.c | 191 ++++++++++++++++++++++++++++++++++++
> pc-bios/s390-ccw/Makefile | 2 +-
> 4 files changed, 278 insertions(+), 1 deletion(-)
> create mode 100644 pc-bios/s390-ccw/pci.h
> create mode 100644 pc-bios/s390-ccw/pci.c
>
> diff --git a/include/hw/s390x/ipl/qipl.h b/include/hw/s390x/ipl/qipl.h
> index aadab87c2e..efd7b3797c 100644
> --- a/include/hw/s390x/ipl/qipl.h
> +++ b/include/hw/s390x/ipl/qipl.h
> @@ -104,6 +104,14 @@ struct IplBlockQemuScsi {
> } QEMU_PACKED;
> typedef struct IplBlockQemuScsi IplBlockQemuScsi;
>
> +struct IplBlockPci {
> + uint32_t reserved0[80];
> + uint8_t opt;
> + uint8_t reserved1[3];
> + uint32_t fid;
> +} QEMU_PACKED;
> +typedef struct IplBlockPci IplBlockPci;
> +
> union IplParameterBlock {
> struct {
> uint32_t len;
> @@ -119,6 +127,7 @@ union IplParameterBlock {
> IplBlockFcp fcp;
> IPLBlockPV pv;
> IplBlockQemuScsi scsi;
> + IplBlockPci pci;
> };
> } QEMU_PACKED;
> struct {
> diff --git a/pc-bios/s390-ccw/pci.h b/pc-bios/s390-ccw/pci.h
> new file mode 100644
> index 0000000000..b5dc5bff35
> --- /dev/null
> +++ b/pc-bios/s390-ccw/pci.h
> @@ -0,0 +1,77 @@
> +/*
> + * s390x PCI definitions
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi<jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef PCI_H
> +#define PCI_H
> +
> +#include <stdint.h>
> +#include "clp.h"
> +
> +#define ZPCI_CREATE_REQ(handle, space, len) \
> + ((uint64_t) handle << 32 | space << 16 | len)
> +
> +union register_pair {
> + unsigned __int128 pair;
> + struct {
> + unsigned long even;
> + unsigned long odd;
> + };
> +};
> +
> +#define PCIFIB_FC_ENABLED 0x80
> +#define PCIFIB_FC_ERROR 0x40
> +#define PCIFIB_FC_BLOCKED 0x20
> +#define PCIFIB_FC_DMAREG 0x10
> +
> +#define PCIST_DISABLED 0x0
> +#define PCIST_ENABLED 0x1
> +
> +#define PCI_CAPABILITY_LIST 0x34 /* Offset of first capability list entry */
> +
> +struct PciFib {
> + uint32_t reserved0[2];
> + uint8_t fcflags;
> + uint8_t reserved1[3];
> + uint32_t reserved2;
> + uint64_t pba;
> + uint64_t pal;
> + uint64_t iota;
> + uint16_t isc:4;
> + uint16_t noi:12;
> + uint8_t reserved3:2;
> + uint8_t aibvo:6;
> + uint8_t s:1;
> + uint8_t reserved4:1;
> + uint8_t aisbo:6;
> + uint32_t reserved5;
> + uint64_t aibv;
> + uint64_t aisb;
> + uint64_t fmba;
> + uint32_t reserved6[2];
> +};
> +typedef struct PciFib PciFib;
> +
> +struct PciDevice {
> + uint16_t device_id;
> + uint16_t vendor_id;
> + uint32_t fid;
> + uint32_t fhandle;
> + uint8_t status;
> + PciFib fib;
> +};
> +typedef struct PciDevice PciDevice;
> +
> +int pci_write(uint32_t fhandle, uint64_t offset, uint64_t data, uint8_t len);
> +int pci_read(uint32_t fhandle, uint64_t offset, uint8_t picas, void *buf, uint8_t len);
> +uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type);
> +int pci_dev_enable(PciDevice *pcidev);
> +int get_fib(PciFib *fib, uint32_t fhandle);
> +int set_fib(PciFib *fib, uint32_t fhandle, uint8_t dma_as, uint8_t opcontrol);
> +
> +#endif
> diff --git a/pc-bios/s390-ccw/pci.c b/pc-bios/s390-ccw/pci.c
> new file mode 100644
> index 0000000000..f776bc064c
> --- /dev/null
> +++ b/pc-bios/s390-ccw/pci.c
> @@ -0,0 +1,191 @@
> +/*
> + * s390x PCI funcionality
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi<jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "clp.h"
> +#include "pci.h"
> +#include <stdio.h>
> +
> +/* PCI load */
> +static inline int pcilg(uint64_t *data, uint64_t req, uint64_t offset, uint8_t *status)
> +{
> + union register_pair req_off = {.even = req, .odd = offset};
> + int cc = -1;
> + uint64_t __data = 0x92;
> +
Why is __data initialized to 0x92?
> + asm volatile (
> + " .insn rre,0xb9d20000,%[data],%[req_off]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "+d" (cc), [data] "=d" (__data),
> + [req_off] "+&d" (req_off.pair) :: "cc");
> + *status = req_off.even >> 24 & 0xff;
> + *data = __data;
> + return cc;
> +}
> +
> +/* PCI store */
> +int pcistg(uint64_t data, uint64_t req, uint64_t offset, uint8_t *status)
> +{
> + union register_pair req_off = {.even = req, .odd = offset};
> + int cc = -1;
> +
> + asm volatile (
> + " .insn rre,0xb9d00000,%[data],%[req_off]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "+d" (cc), [req_off] "+&d" (req_off.pair)
> + : [data] "d" (data)
> + : "cc");
> + *status = req_off.even >> 24 & 0xff;
> + return cc;
> +}
> +
> +/* store PCI function controls */
> +int stpcifc(uint64_t req, PciFib *fib, uint8_t *status)
> +{
> + uint8_t cc;
> +
> + asm volatile (
> + " .insn rxy,0xe300000000d4,%[req],%[fib]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "=d" (cc), [req] "+d" (req), [fib] "+Q" (*fib)
> + : : "cc");
> + *status = req >> 24 & 0xff;
> + return cc;
> +}
> +
> +/* modify PCI function controls */
> +int mpcifc(uint64_t req, PciFib *fib, uint8_t *status)
> +{
> + uint8_t cc;
> +
> + asm volatile (
> + " .insn rxy,0xe300000000d0,%[req],%[fib]\n"
> + " ipm %[cc]\n"
> + " srl %[cc],28\n"
> + : [cc] "=d" (cc), [req] "+d" (req), [fib] "+Q" (*fib)
> + : : "cc");
> + *status = req >> 24 & 0xff;
> + return cc;
> +}
> +
> +int pci_write(uint32_t fhandle, uint64_t offset, uint64_t data, uint8_t len)
> +{
> +
> + uint64_t req = ZPCI_CREATE_REQ(fhandle, 4, len);
This assumes that we will only read to BAR 4? I think we should pass the
PCIAS here as well if we want to generalize this function?
> + uint8_t status;
> + int rc;
> +
> + rc = pcistg(data, req, offset, &status);
> + if (rc == 1) {
> + return status;
> + } else if (rc) {
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +int pci_read(uint32_t fhandle, uint64_t offset, uint8_t picas, void *buf, uint8_t len)
> +{
> + uint64_t req;
> + uint64_t data;
> + uint8_t status;
> + int readlen;
> + int i = 0;
> + int rc = 0;
> +
> + while (len > 0 && !rc) {
> + data = 0;
> + readlen = len > 8 ? 8 : len;
> + req = ZPCI_CREATE_REQ(fhandle, picas, readlen);
> + rc = pcilg(&data, req, offset + (i * 8), &status);
Shouldn't this be offset + (i * readlen)? but I guess this works because
we will only increment i on reads greater than 8 bytes. Maybe we could
try to simplify this and have a single pci_read function and several
other helper functions that uses pci_read to read sizes of 1/2/4/8
bytes. For reads greater than 8 bytes we can have another function
(similar to zpci_memcpy_from_io, in the kernel). From what I can tell
most of the pci_read calls reads are 8 bytes in the rest of the patches,
except maybe for one case which reads greater than 8?
> + ((uint64_t *)buf)[i] = data;
> + len -= readlen;
> + i++;
> + }
> +
> + if (rc == 1) {
> + return status;
> + } else if (rc) {
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Find the position of the capability config within PCI configuration
> + * space for a given cfg type. Return the position if found, otherwise 0.
> + */
> +uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type) {
> + uint64_t req, next, cfg;
> + uint8_t status;
> + int rc;
> +
> + req = ZPCI_CREATE_REQ(fhandle, 0xf, 1);
> + rc = pcilg(&next, req, PCI_CAPABILITY_LIST, &status);
> + rc = pcilg(&cfg, req, next + 3, &status);
Why are we reading next + 3 into cfg? If I understand this correctly
next will be the address of the first capability in the linked list, and
so we should just read the first byte from next to get the capability
id? I think we should have helper function like qpci_find_capability to
find the capabilities?
> +
> + while (!rc && (cfg != cfg_type) && next) {
> + rc = pcilg(&next, req, next + 1, &status);
> + rc = pcilg(&cfg, req, next + 3, &status);
Same question here?
> + }
> +
> + return rc ? 0 : next;
> +}
> +
[..snip..]
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format
2025-10-23 17:31 ` Farhan Ali
@ 2025-10-23 17:56 ` Farhan Ali
2025-10-23 18:19 ` Jared Rossi
1 sibling, 0 replies; 25+ messages in thread
From: Farhan Ali @ 2025-10-23 17:56 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x, thuth; +Cc: jjherne, farman, mjrosato, zycai
On 10/23/2025 10:31 AM, Farhan Ali wrote:
>> + * Find the position of the capability config within PCI configuration
>> + * space for a given cfg type. Return the position if found,
>> otherwise 0.
>> + */
>> +uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type) {
>> + uint64_t req, next, cfg;
>> + uint8_t status;
>> + int rc;
>> +
>> + req = ZPCI_CREATE_REQ(fhandle, 0xf, 1);
>> + rc = pcilg(&next, req, PCI_CAPABILITY_LIST, &status);
>> + rc = pcilg(&cfg, req, next + 3, &status);
>
> Why are we reading next + 3 into cfg? If I understand this correctly
> next will be the address of the first capability in the linked list,
> and so we should just read the first byte from next to get the
> capability id? I think we should have helper function like
> qpci_find_capability to find the capabilities?
>
>
>> +
>> + while (!rc && (cfg != cfg_type) && next) {
>> + rc = pcilg(&next, req, next + 1, &status);
>> + rc = pcilg(&cfg, req, next + 3, &status);
>
> Same question here?
>
>
>> + }
>> +
>> + return rc ? 0 : next;
>> +}
>> +
>
> [..snip..]
>
Ah I see in patch 5 we are using this function to get virtio config
type, which is a vendor specific capability and according to spec the
virtio config type is defined in the 4th byte. Maybe we could just move
the function to virtio-pci.c? I would expect a function defined here
will try to find a specific PCI capability by searching through the PCI
capability list.
Thanks
Farhan
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format
2025-10-23 17:31 ` Farhan Ali
2025-10-23 17:56 ` Farhan Ali
@ 2025-10-23 18:19 ` Jared Rossi
1 sibling, 0 replies; 25+ messages in thread
From: Jared Rossi @ 2025-10-23 18:19 UTC (permalink / raw)
To: Farhan Ali, qemu-devel, qemu-s390x, thuth
Cc: jjherne, farman, mjrosato, zycai
Thanks Farhan,
On 10/23/25 1:31 PM, Farhan Ali wrote:
> [snip]
>> +
>> +int pci_write(uint32_t fhandle, uint64_t offset, uint64_t data,
>> uint8_t len)
>> +{
>> +
>> + uint64_t req = ZPCI_CREATE_REQ(fhandle, 4, len);
>
> This assumes that we will only read to BAR 4? I think we should pass
> the PCIAS here as well if we want to generalize this function?
I was thinking about this too and I agree that it should be
generalized. Also when reading the capabilities from the configuration
space, I did not check that the location is, actually, BAR 4. I will
generalize the functions so that we do not make any assumptions about BAR 4.
>
>> + uint8_t status;
>> + int rc;
>> +
>> + rc = pcistg(data, req, offset, &status);
>> + if (rc == 1) {
>> + return status;
>> + } else if (rc) {
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int pci_read(uint32_t fhandle, uint64_t offset, uint8_t picas, void
>> *buf, uint8_t len)
>> +{
>> + uint64_t req;
>> + uint64_t data;
>> + uint8_t status;
>> + int readlen;
>> + int i = 0;
>> + int rc = 0;
>> +
>> + while (len > 0 && !rc) {
>> + data = 0;
>> + readlen = len > 8 ? 8 : len;
>> + req = ZPCI_CREATE_REQ(fhandle, picas, readlen);
>> + rc = pcilg(&data, req, offset + (i * 8), &status);
>
> Shouldn't this be offset + (i * readlen)? but I guess this works
> because we will only increment i on reads greater than 8 bytes. Maybe
> we could try to simplify this and have a single pci_read function and
> several other helper functions that uses pci_read to read sizes of
> 1/2/4/8 bytes. For reads greater than 8 bytes we can have another
> function (similar to zpci_memcpy_from_io, in the kernel). From what I
> can tell most of the pci_read calls reads are 8 bytes in the rest of
> the patches, except maybe for one case which reads greater than 8?
Yes I agree. Thomas mentioned something similar. I think using some
helper functions will make the code easier to read also.
>
>> + ((uint64_t *)buf)[i] = data;
>> + len -= readlen;
>> + i++;
>> + }
>> +
>> + if (rc == 1) {
>> + return status;
>> + } else if (rc) {
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Find the position of the capability config within PCI configuration
>> + * space for a given cfg type. Return the position if found,
>> otherwise 0.
>> + */
>> +uint8_t find_cap_pos(uint32_t fhandle, uint64_t cfg_type) {
>> + uint64_t req, next, cfg;
>> + uint8_t status;
>> + int rc;
>> +
>> + req = ZPCI_CREATE_REQ(fhandle, 0xf, 1);
>> + rc = pcilg(&next, req, PCI_CAPABILITY_LIST, &status);
>> + rc = pcilg(&cfg, req, next + 3, &status);
>
> Why are we reading next + 3 into cfg? If I understand this correctly
> next will be the address of the first capability in the linked list,
> and so we should just read the first byte from next to get the
> capability id? I think we should have helper function like
> qpci_find_capability to find the capabilities?
>
>
>> +
>> + while (!rc && (cfg != cfg_type) && next) {
>> + rc = pcilg(&next, req, next + 1, &status);
>> + rc = pcilg(&cfg, req, next + 3, &status);
>
> Same question here?
I see you already answered this yourself in a follow-up mail, but you
are correct that this is for the vendor-specific capabilities. I will
move it to virtio-pci.c and add a comment for clarity.
>
>> + }
>> +
>> + return rc ? 0 : next;
>> +}
>> +
>
> [..snip..]
>
Regards,
Jared Rossi
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL
2025-10-20 16:20 [PATCH 0/7] s390x: Add support for virtio-blk-pci IPL device jrossi
` (3 preceding siblings ...)
2025-10-20 16:20 ` [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format jrossi
@ 2025-10-20 16:20 ` jrossi
2025-10-21 11:11 ` Thomas Huth
` (2 more replies)
2025-10-20 16:20 ` [PATCH 6/7] s390x: Build IPLB for virtio-pci devices jrossi
2025-10-20 16:20 ` [PATCH 7/7] tests/qtest: Add s390x PCI boot test to cdrom-test.c jrossi
6 siblings, 3 replies; 25+ messages in thread
From: jrossi @ 2025-10-20 16:20 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato, jrossi, zycai
From: Jared Rossi <jrossi@linux.ibm.com>
Enable virt-queue PCI configuration and add routines for virtio-blk-pci devices.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
pc-bios/s390-ccw/clp.h | 2 +-
pc-bios/s390-ccw/virtio-pci.h | 73 +++++++
pc-bios/s390-ccw/virtio.h | 1 +
pc-bios/s390-ccw/main.c | 59 ++++-
pc-bios/s390-ccw/virtio-blkdev.c | 3 +
pc-bios/s390-ccw/virtio-pci.c | 357 +++++++++++++++++++++++++++++++
pc-bios/s390-ccw/virtio.c | 5 +
pc-bios/s390-ccw/Makefile | 2 +-
8 files changed, 498 insertions(+), 4 deletions(-)
create mode 100644 pc-bios/s390-ccw/virtio-pci.h
create mode 100644 pc-bios/s390-ccw/virtio-pci.c
diff --git a/pc-bios/s390-ccw/clp.h b/pc-bios/s390-ccw/clp.h
index cb130e5e90..52232c4c48 100644
--- a/pc-bios/s390-ccw/clp.h
+++ b/pc-bios/s390-ccw/clp.h
@@ -19,6 +19,6 @@
int clp_pci(void *data);
int enable_pci_function(uint32_t *fhandle);
-int enumerate_pci_functions(void);
+int find_pci_function(uint32_t fid, ClpFhListEntry *entry);
#endif
diff --git a/pc-bios/s390-ccw/virtio-pci.h b/pc-bios/s390-ccw/virtio-pci.h
new file mode 100644
index 0000000000..09fff015cb
--- /dev/null
+++ b/pc-bios/s390-ccw/virtio-pci.h
@@ -0,0 +1,73 @@
+/*
+ * Definitions for virtio-pci
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi <jrossi@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef VIRTIO_PCI_H
+#define VIRTIO_PCI_H
+
+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG 1
+/* Notifications */
+#define VIRTIO_PCI_CAP_NOTIFY_CFG 2
+/* ISR access */
+#define VIRTIO_PCI_CAP_ISR_CFG 3
+/* Device specific configuration */
+#define VIRTIO_PCI_CAP_DEVICE_CFG 4
+/* PCI configuration access */
+#define VIRTIO_PCI_CAP_PCI_CFG 5
+/* Additional shared memory capability */
+#define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
+/* PCI vendor data configuration */
+#define VIRTIO_PCI_CAP_VENDOR_CFG 9
+
+/* Offsets within capability header */
+#define VIRTIO_PCI_CAP_VNDR 0
+#define VIRTIO_PCI_CAP_NEXT 1
+#define VIRTIO_PCI_CAP_LEN 2
+#define VIRTIO_PCI_CAP_CFG_TYPE 3
+#define VIRTIO_PCI_CAP_BAR 4
+#define VIRTIO_PCI_CAP_OFFSET 8
+#define VIRTIO_PCI_CAP_LENGTH 12
+
+#define VIRTIO_PCI_NOTIFY_CAP_MULT 16 /* VIRTIO_PCI_CAP_NOTIFY_CFG only */
+
+/* Common Area Offsets for virtio-pci queue */
+#define VPCI_C_OFFSET_DFSELECT 0
+#define VPCI_C_OFFSET_DF 4
+#define VPCI_C_OFFSET_GFSELECT 8
+#define VPCI_C_OFFSET_GF 12
+#define VPCI_C_COMMON_NUMQ 18
+#define VPCI_C_OFFSET_STATUS 20
+#define VPCI_C_OFFSET_Q_SELECT 22
+#define VPCI_C_OFFSET_Q_SIZE 24
+#define VPCI_C_OFFSET_Q_ENABLE 28
+#define VPCI_C_OFFSET_Q_NOFF 30
+#define VPCI_C_OFFSET_Q_DESCLO 32
+#define VPCI_C_OFFSET_Q_DESCHI 36
+#define VPCI_C_OFFSET_Q_AVAILLO 40
+#define VPCI_C_OFFSET_Q_AVAILHI 44
+#define VPCI_C_OFFSET_Q_USEDLO 48
+#define VPCI_C_OFFSET_Q_USEDHI 52
+
+#define VPCI_S_RESET 0
+#define VPCI_S_ACKNOWLEDGE 1
+#define VPCI_S_DRIVER 2
+#define VPCI_S_DRIVER_OK 4
+#define VPCI_S_FEATURES_OK 8
+
+#define VIRTIO_F_VERSION_1 (1 << (32 - 32)) /* Feature bit 32 */
+
+#define VIRT_Q_SIZE 16
+
+long virtio_pci_notify(uint32_t fhandle, int vq_id);
+int virtio_pci_setup(VDev *vdev);
+int virtio_pci_setup_device(void);
+int virtio_pci_reset(VDev *vdev);
+void virtio_pci_id2type(VDev *vdev, uint16_t device_id);
+
+#endif
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) {
+ 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);
+}
+
static void menu_setup(void)
{
if (memcmp(loadparm_str, LOADPARM_PROMPT, LOADPARM_LEN) == 0) {
@@ -233,6 +249,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");
}
@@ -269,7 +288,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:
@@ -282,7 +301,43 @@ 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->type) {
+ case VIRTIO_ID_BLOCK:
+ if (virtio_setup() == 0) {
+ zipl_load();
+ }
+ break;
+ default:
+ printf("Cannot boot PCI device type 0x%X\n", vdev->type);
+ }
+}
+
+static void ipl_boot_device(void)
+{
+ switch (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/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
index df6a6d5b70..c5b65d021b 100644
--- a/pc-bios/s390-ccw/virtio-blkdev.c
+++ b/pc-bios/s390-ccw/virtio-blkdev.c
@@ -13,6 +13,7 @@
#include "virtio.h"
#include "virtio-ccw.h"
#include "virtio-scsi.h"
+#include "virtio-pci.h"
#define VIRTIO_BLK_F_GEOMETRY (1 << 4)
#define VIRTIO_BLK_F_BLK_SIZE (1 << 6)
@@ -243,6 +244,8 @@ int virtio_blk_setup_device()
case S390_IPL_TYPE_CCW:
vdev->schid = blk_schid;
return virtio_ccw_setup(vdev);
+ case S390_IPL_TYPE_PCI:
+ return virtio_pci_setup(vdev);
}
return 1;
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 <jrossi@linux.ibm.com>
+ *
+ * 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;
+ 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]);
+ 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]);
+ 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));
+
+}
+
+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);
+}
+
+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);
+}
+
+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);
+}
+
+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);
+
+ 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);
+
+ 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;
+ }
+
+ return 0;
+}
+
+int virtio_pci_setup(VDev *vdev)
+{
+ VRing *vr;
+ int rc;
+ uint64_t pci_features, data;
+ uint8_t status;
+ int i = 0;
+
+ vdev->config.blk.blk_size = 0;
+ vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
+ vdev->cmd_vr_idx = 0;
+
+ if (virtio_reset(vdev)) {
+ return -EIO;
+ }
+
+ status = VPCI_S_ACKNOWLEDGE;
+ rc = virtio_pci_set_status(vdev, status);
+ if (rc) {
+ puts("Virtio-pci device Failed to ACKNOWLEDGE");
+ return -EIO;
+ }
+
+ virtio_pci_read_pci_cap_config(vdev);
+ if (rc) {
+ printf("Invalid PCI capabilities");
+ return -EIO;
+ }
+
+ switch (vdev->type) {
+ case VIRTIO_ID_BLOCK:
+ vdev->nr_vqs = 1;
+ vdev->cmd_vr_idx = 0;
+ virtio_pci_get_blk_config(vdev);
+ break;
+ default:
+ puts("Unsupported virtio device");
+ return -ENODEV;
+ }
+
+ status |= VPCI_S_DRIVER;
+ rc = virtio_pci_set_status(vdev, status);
+ if (rc) {
+ puts("Set status failed");
+ return -EIO;
+ }
+
+ /* Feature negotiation */
+ rc = virtio_pci_get_hfeatures(vdev, &pci_features);
+ if (rc) {
+ puts("Failed to get feature bits");
+ return -EIO;
+ }
+
+ rc = virtio_pci_set_gfeatures(vdev);
+ if (rc) {
+ puts("Failed to set feature bits");
+ 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 * VIRTIO_RING_SIZE),
+ .align = KVM_S390_VIRTIO_RING_ALIGN,
+ .index = i,
+ .num = 0,
+ };
+
+ vr = &vdev->vrings[i];
+ rc = pci_read(vdev->pci_fh, VPCI_C_COMMON_NUMQ, 4, &data, 2);
+ if (rc) {
+ return rc;
+ }
+
+ info.num = data;
+ vring_init(vr, &info);
+
+ rc = virtio_pci_set_selected_vq(vdev, vr->id);
+ if (rc) {
+ puts("Failed to set selected virt-queue");
+ return -EIO;
+ }
+
+ rc = virtio_pci_set_queue_size(vdev, 16);
+ if (rc) {
+ puts("Failed to set virt-queue size");
+ return -EIO;
+ }
+
+ rc = set_pci_vq_addr(vdev, vr->desc, VPCI_C_OFFSET_Q_DESCLO);
+ rc |= set_pci_vq_addr(vdev, vr->avail, VPCI_C_OFFSET_Q_AVAILLO);
+ rc |= set_pci_vq_addr(vdev, vr->used, VPCI_C_OFFSET_Q_USEDLO);
+ if (rc) {
+ puts("Failed to set virt-queue address");
+ return -EIO;
+ }
+
+ rc = virtio_pci_set_queue_enable(vdev, true);
+ if (rc) {
+ puts("Failed to set virt-queue enabled");
+ return -EIO;
+ }
+ }
+
+ status |= VPCI_S_FEATURES_OK | VPCI_S_DRIVER_OK;
+ return virtio_pci_set_status(vdev, status);
+}
+
+int virtio_pci_setup_device(void)
+{
+ int rc;
+ VDev *vdev = virtio_get_device();
+
+ rc = enable_pci_function(&vdev->pci_fh);
+ if (rc) {
+ puts("Failed to enable PCI function");
+ return rc;
+ }
+
+ return 0;
+}
+
+long virtio_pci_notify(uint32_t fhandle, int vq_id)
+{
+ uint64_t notice = 1;
+ uint32_t offset = notify_offset + vq_id * q_notify_offset;
+
+ return pci_write(fhandle, offset, notice, 4);
+}
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index 05cfca4dad..dba30335b7 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -14,6 +14,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"
@@ -97,6 +98,8 @@ bool vring_notify(VRing *vr)
case S390_IPL_TYPE_QEMU_SCSI:
case S390_IPL_TYPE_CCW:
vr->cookie = virtio_ccw_notify(vr->schid, vr->id, vr->cookie);
+ case S390_IPL_TYPE_PCI:
+ vr->cookie = virtio_pci_notify(virtio_get_device()->pci_fh, vr->id);
}
return vr->cookie >= 0;
@@ -181,6 +184,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);
}
return -1;
diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index 1f17f98fc1..589962b998 100644
--- a/pc-bios/s390-ccw/Makefile
+++ b/pc-bios/s390-ccw/Makefile
@@ -35,7 +35,7 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o netmain.o \
virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o \
- virtio-ccw.o clp.o pci.o
+ virtio-ccw.o clp.o pci.o virtio-pci.o
SLOF_DIR := $(SRC_PATH)/../../roms/SLOF
--
2.49.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL
2025-10-20 16:20 ` [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL jrossi
@ 2025-10-21 11:11 ` Thomas Huth
2025-10-22 16:40 ` Zhuoying Cai
2025-10-23 18:16 ` Farhan Ali
2 siblings, 0 replies; 25+ messages in thread
From: Thomas Huth @ 2025-10-21 11:11 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Enable virt-queue PCI configuration and add routines for virtio-blk-pci devices.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
...
> 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 <jrossi@linux.ibm.com>
> + *
> + * 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
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL
2025-10-20 16:20 ` [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL jrossi
2025-10-21 11:11 ` Thomas Huth
@ 2025-10-22 16:40 ` Zhuoying Cai
2025-10-22 18:57 ` Jared Rossi
2025-10-23 18:16 ` Farhan Ali
2 siblings, 1 reply; 25+ messages in thread
From: Zhuoying Cai @ 2025-10-22 16:40 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x, thuth; +Cc: jjherne, alifm, farman, mjrosato
On 10/20/25 12:20 PM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Enable virt-queue PCI configuration and add routines for virtio-blk-pci devices.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
> pc-bios/s390-ccw/clp.h | 2 +-
> pc-bios/s390-ccw/virtio-pci.h | 73 +++++++
> pc-bios/s390-ccw/virtio.h | 1 +
> pc-bios/s390-ccw/main.c | 59 ++++-
> pc-bios/s390-ccw/virtio-blkdev.c | 3 +
> pc-bios/s390-ccw/virtio-pci.c | 357 +++++++++++++++++++++++++++++++
> pc-bios/s390-ccw/virtio.c | 5 +
> pc-bios/s390-ccw/Makefile | 2 +-
> 8 files changed, 498 insertions(+), 4 deletions(-)
> create mode 100644 pc-bios/s390-ccw/virtio-pci.h
> create mode 100644 pc-bios/s390-ccw/virtio-pci.c
>
> diff --git a/pc-bios/s390-ccw/clp.h b/pc-bios/s390-ccw/clp.h
> index cb130e5e90..52232c4c48 100644
> --- a/pc-bios/s390-ccw/clp.h
> +++ b/pc-bios/s390-ccw/clp.h
> @@ -19,6 +19,6 @@
>
> int clp_pci(void *data);
> int enable_pci_function(uint32_t *fhandle);
> -int enumerate_pci_functions(void);
> +int find_pci_function(uint32_t fid, ClpFhListEntry *entry);
>
> #endif
> diff --git a/pc-bios/s390-ccw/virtio-pci.h b/pc-bios/s390-ccw/virtio-pci.h
> new file mode 100644
> index 0000000000..09fff015cb
> --- /dev/null
> +++ b/pc-bios/s390-ccw/virtio-pci.h
> @@ -0,0 +1,73 @@
> +/*
> + * Definitions for virtio-pci
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi <jrossi@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef VIRTIO_PCI_H
> +#define VIRTIO_PCI_H
> +
> +/* Common configuration */
> +#define VIRTIO_PCI_CAP_COMMON_CFG 1
> +/* Notifications */
> +#define VIRTIO_PCI_CAP_NOTIFY_CFG 2
> +/* ISR access */
> +#define VIRTIO_PCI_CAP_ISR_CFG 3
> +/* Device specific configuration */
> +#define VIRTIO_PCI_CAP_DEVICE_CFG 4
> +/* PCI configuration access */
> +#define VIRTIO_PCI_CAP_PCI_CFG 5
> +/* Additional shared memory capability */
> +#define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
> +/* PCI vendor data configuration */
> +#define VIRTIO_PCI_CAP_VENDOR_CFG 9
> +
> +/* Offsets within capability header */
> +#define VIRTIO_PCI_CAP_VNDR 0
> +#define VIRTIO_PCI_CAP_NEXT 1
> +#define VIRTIO_PCI_CAP_LEN 2
> +#define VIRTIO_PCI_CAP_CFG_TYPE 3
> +#define VIRTIO_PCI_CAP_BAR 4
> +#define VIRTIO_PCI_CAP_OFFSET 8
> +#define VIRTIO_PCI_CAP_LENGTH 12
> +
> +#define VIRTIO_PCI_NOTIFY_CAP_MULT 16 /* VIRTIO_PCI_CAP_NOTIFY_CFG only */
> +
> +/* Common Area Offsets for virtio-pci queue */
> +#define VPCI_C_OFFSET_DFSELECT 0
> +#define VPCI_C_OFFSET_DF 4
> +#define VPCI_C_OFFSET_GFSELECT 8
> +#define VPCI_C_OFFSET_GF 12
> +#define VPCI_C_COMMON_NUMQ 18
> +#define VPCI_C_OFFSET_STATUS 20
> +#define VPCI_C_OFFSET_Q_SELECT 22
> +#define VPCI_C_OFFSET_Q_SIZE 24
> +#define VPCI_C_OFFSET_Q_ENABLE 28
> +#define VPCI_C_OFFSET_Q_NOFF 30
> +#define VPCI_C_OFFSET_Q_DESCLO 32
> +#define VPCI_C_OFFSET_Q_DESCHI 36
> +#define VPCI_C_OFFSET_Q_AVAILLO 40
> +#define VPCI_C_OFFSET_Q_AVAILHI 44
> +#define VPCI_C_OFFSET_Q_USEDLO 48
> +#define VPCI_C_OFFSET_Q_USEDHI 52
> +
> +#define VPCI_S_RESET 0
> +#define VPCI_S_ACKNOWLEDGE 1
> +#define VPCI_S_DRIVER 2
> +#define VPCI_S_DRIVER_OK 4
> +#define VPCI_S_FEATURES_OK 8
> +
> +#define VIRTIO_F_VERSION_1 (1 << (32 - 32)) /* Feature bit 32 */
> +
> +#define VIRT_Q_SIZE 16
> +
> +long virtio_pci_notify(uint32_t fhandle, int vq_id);
> +int virtio_pci_setup(VDev *vdev);
> +int virtio_pci_setup_device(void);
> +int virtio_pci_reset(VDev *vdev);
> +void virtio_pci_id2type(VDev *vdev, uint16_t device_id);
> +
> +#endif
> 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) {
> + 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);
> +}
> +
> static void menu_setup(void)
> {
> if (memcmp(loadparm_str, LOADPARM_PROMPT, LOADPARM_LEN) == 0) {
> @@ -233,6 +249,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");
> }
> @@ -269,7 +288,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:
> @@ -282,7 +301,43 @@ 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->type) {
> + case VIRTIO_ID_BLOCK:
> + if (virtio_setup() == 0) {
> + zipl_load();
> + }
> + break;
> + default:
> + printf("Cannot boot PCI device type 0x%X\n", vdev->type);
> + }
> +}
> +
> +static void ipl_boot_device(void)
> +{
> + switch (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/virtio-blkdev.c b/pc-bios/s390-ccw/virtio-blkdev.c
> index df6a6d5b70..c5b65d021b 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -13,6 +13,7 @@
> #include "virtio.h"
> #include "virtio-ccw.h"
> #include "virtio-scsi.h"
> +#include "virtio-pci.h"
>
> #define VIRTIO_BLK_F_GEOMETRY (1 << 4)
> #define VIRTIO_BLK_F_BLK_SIZE (1 << 6)
> @@ -243,6 +244,8 @@ int virtio_blk_setup_device()
> case S390_IPL_TYPE_CCW:
> vdev->schid = blk_schid;
> return virtio_ccw_setup(vdev);
> + case S390_IPL_TYPE_PCI:
> + return virtio_pci_setup(vdev);
> }
>
> return 1;
> 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 <jrossi@linux.ibm.com>
> + *
> + * 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;
> + 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]);
> + 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]);
> + 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));
> +
> +}
> +
> +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);
> +}
> +
> +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);
> +}
> +
> +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);
> +}
> +
> +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);
> +
> + 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;
> + }
Could you please explain why we use
pci_read(vdev->pci_fh, pos + VIRTIO_PCI_NOTIFY_CAP_MULT, 15, &data, 4)
instead of
pci_read(vdev->pci_fh, notify_offset + VIRTIO_PCI_NOTIFY_CAP_MULT, 4,
&data, 4) here?
> + notify_mult = bswap32(data);
> +
> + if (pci_read(vdev->pci_fh, device_offset + VPCI_C_OFFSET_Q_NOFF, 4, &data, 2)) {
> + return -EIO;
> + }
Should queue_notify_off be read using pci_read(vdev->pci_fh,
common_offset + VPCI_C_OFFSET_Q_NOFF, 4, &data, 2)?
(Virtio specs v-1.3 section 4.1.4.3 Common configuration structure layout)
> + q_notify_offset = bswap16(data);
> +
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +int virtio_pci_setup(VDev *vdev)
> +{
> + VRing *vr;
> + int rc;
> + uint64_t pci_features, data;
> + uint8_t status;
> + int i = 0;
> +
> + vdev->config.blk.blk_size = 0;
> + vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
> + vdev->cmd_vr_idx = 0;
> +
> + if (virtio_reset(vdev)) {
> + return -EIO;
> + }
> +
> + status = VPCI_S_ACKNOWLEDGE;
> + rc = virtio_pci_set_status(vdev, status);
> + if (rc) {
> + puts("Virtio-pci device Failed to ACKNOWLEDGE");
> + return -EIO;
> + }
> +
> + virtio_pci_read_pci_cap_config(vdev);
> + if (rc) {
> + printf("Invalid PCI capabilities");
> + return -EIO;
> + }
> +
> + switch (vdev->type) {
> + case VIRTIO_ID_BLOCK:
> + vdev->nr_vqs = 1;
> + vdev->cmd_vr_idx = 0;
> + virtio_pci_get_blk_config(vdev);
> + break;
> + default:
> + puts("Unsupported virtio device");
> + return -ENODEV;
> + }
> +
> + status |= VPCI_S_DRIVER;
> + rc = virtio_pci_set_status(vdev, status);
> + if (rc) {
> + puts("Set status failed");
> + return -EIO;
> + }
> +
> + /* Feature negotiation */
> + rc = virtio_pci_get_hfeatures(vdev, &pci_features);
> + if (rc) {
> + puts("Failed to get feature bits");
> + return -EIO;
> + }
> +
> + rc = virtio_pci_set_gfeatures(vdev);
> + if (rc) {
> + puts("Failed to set feature bits");
> + 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 * VIRTIO_RING_SIZE),
> + .align = KVM_S390_VIRTIO_RING_ALIGN,
> + .index = i,
> + .num = 0,
> + };
> +
> + vr = &vdev->vrings[i];
> + rc = pci_read(vdev->pci_fh, VPCI_C_COMMON_NUMQ, 4, &data, 2);
> + if (rc) {
> + return rc;
> + }
> +
> + info.num = data;
> + vring_init(vr, &info);
> +
> + rc = virtio_pci_set_selected_vq(vdev, vr->id);
> + if (rc) {
> + puts("Failed to set selected virt-queue");
> + return -EIO;
> + }
> +
> + rc = virtio_pci_set_queue_size(vdev, 16);
> + if (rc) {
> + puts("Failed to set virt-queue size");
> + return -EIO;
> + }
> +
> + rc = set_pci_vq_addr(vdev, vr->desc, VPCI_C_OFFSET_Q_DESCLO);
> + rc |= set_pci_vq_addr(vdev, vr->avail, VPCI_C_OFFSET_Q_AVAILLO);
> + rc |= set_pci_vq_addr(vdev, vr->used, VPCI_C_OFFSET_Q_USEDLO);
> + if (rc) {
> + puts("Failed to set virt-queue address");
> + return -EIO;
> + }
> +
> + rc = virtio_pci_set_queue_enable(vdev, true);
> + if (rc) {
> + puts("Failed to set virt-queue enabled");
> + return -EIO;
> + }
> + }
> +
> + status |= VPCI_S_FEATURES_OK | VPCI_S_DRIVER_OK;
> + return virtio_pci_set_status(vdev, status);
> +}
> +
> +int virtio_pci_setup_device(void)
> +{
> + int rc;
> + VDev *vdev = virtio_get_device();
> +
> + rc = enable_pci_function(&vdev->pci_fh);
> + if (rc) {
> + puts("Failed to enable PCI function");
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +long virtio_pci_notify(uint32_t fhandle, int vq_id)
> +{
> + uint64_t notice = 1;
> + uint32_t offset = notify_offset + vq_id * q_notify_offset;
Shoud the offset be calculated as notify_offset + q_notify_offset *
notify_mult?
(Virtio specs v-1.3 section 4.1.4.4 Notification structure layout)
> +
> + return pci_write(fhandle, offset, notice, 4);
Please correct me if I'm wrong - instead of always writing notice = 1,
should we write vq_id to vq_index of the Queue Notify address.
(Virtio specs v-1.3 section 4.1.5.2 Available Buffer Notifications)
> +}
> diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
> index 05cfca4dad..dba30335b7 100644
> --- a/pc-bios/s390-ccw/virtio.c
> +++ b/pc-bios/s390-ccw/virtio.c
> @@ -14,6 +14,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"
> @@ -97,6 +98,8 @@ bool vring_notify(VRing *vr)
> case S390_IPL_TYPE_QEMU_SCSI:
> case S390_IPL_TYPE_CCW:
> vr->cookie = virtio_ccw_notify(vr->schid, vr->id, vr->cookie);
> + case S390_IPL_TYPE_PCI:
> + vr->cookie = virtio_pci_notify(virtio_get_device()->pci_fh, vr->id);
> }
>
> return vr->cookie >= 0;
> @@ -181,6 +184,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);
> }
>
> return -1;
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index 1f17f98fc1..589962b998 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -35,7 +35,7 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
>
> OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o netmain.o \
> virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o \
> - virtio-ccw.o clp.o pci.o
> + virtio-ccw.o clp.o pci.o virtio-pci.o
>
> SLOF_DIR := $(SRC_PATH)/../../roms/SLOF
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL
2025-10-22 16:40 ` Zhuoying Cai
@ 2025-10-22 18:57 ` Jared Rossi
0 siblings, 0 replies; 25+ messages in thread
From: Jared Rossi @ 2025-10-22 18:57 UTC (permalink / raw)
To: Zhuoying Cai, qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato
On 10/22/25 12:40 PM, Zhuoying Cai wrote:
> [snip...]
>
> +/*
> + * 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;
> + }
> Could you please explain why we use
> pci_read(vdev->pci_fh, pos + VIRTIO_PCI_NOTIFY_CAP_MULT, 15, &data, 4)
> instead of
> pci_read(vdev->pci_fh, notify_offset + VIRTIO_PCI_NOTIFY_CAP_MULT, 4,
> &data, 4) here?
The notification capability in particular has an extra field at the end,
which is what we are looking for. We are still in PCI configuration
space (BAR 15), we just want to read some different bytes in the
notification capability configuration.
>> + notify_mult = bswap32(data);
>> +
>> + if (pci_read(vdev->pci_fh, device_offset + VPCI_C_OFFSET_Q_NOFF, 4, &data, 2)) {
>> + return -EIO;
>> + }
> Should queue_notify_off be read using pci_read(vdev->pci_fh,
> common_offset + VPCI_C_OFFSET_Q_NOFF, 4, &data, 2)?
> (Virtio specs v-1.3 section 4.1.4.3 Common configuration structure layout)
I think you are correct. This is a mistake. The queue notification
offset is in the common configuration, not the device configuration. I
suspect it didn't cause a problem here because virtio-blk uses only one
queue.
>
>> +long virtio_pci_notify(uint32_t fhandle, int vq_id)
>> +{
>> + uint64_t notice = 1;
>> + uint32_t offset = notify_offset + vq_id * q_notify_offset;
> Shoud the offset be calculated as notify_offset + q_notify_offset *
> notify_mult?
> (Virtio specs v-1.3 section 4.1.4.4 Notification structure layout)
I'll double check this, but you are probably right. As with the
previous instance it didn't cause a problem due to there only being one
queue for virtio-blk, so the offset is always just the base
notify_offset and, luckily, the incorrect parts get multiplied by zero.
>
>> +
>> + return pci_write(fhandle, offset, notice, 4);
> Please correct me if I'm wrong - instead of always writing notice = 1,
> should we write vq_id to vq_index of the Queue Notify address.
> (Virtio specs v-1.3 section 4.1.5.2 Available Buffer Notifications)
I'll double and triple check the queue offset/notification calculations
since it seems there are multiple errors on my part and I just got lucky
that virtio-blk wasn't affected. Thanks for catching these mistakes.
Regards,
Jared Rossi
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL
2025-10-20 16:20 ` [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL jrossi
2025-10-21 11:11 ` Thomas Huth
2025-10-22 16:40 ` Zhuoying Cai
@ 2025-10-23 18:16 ` Farhan Ali
2 siblings, 0 replies; 25+ messages in thread
From: Farhan Ali @ 2025-10-23 18:16 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x, thuth; +Cc: jjherne, farman, mjrosato, zycai
On 10/20/2025 9:20 AM, jrossi@linux.ibm.com wrote:
> From: Jared Rossi<jrossi@linux.ibm.com>
>
> Enable virt-queue PCI configuration and add routines for virtio-blk-pci devices.
>
> Signed-off-by: Jared Rossi<jrossi@linux.ibm.com>
> ---
Just a general observation in this patch, the BAR we are reading and
writing to is BAR 4 for the virtio device. I think QEMU defines all the
structure in BAR 4
(https://github.com/qemu/qemu/blob/88f72048d2f5835a1b9eaba690c7861393aef283/hw/virtio/virtio-pci.c#L2169).
But after looking through the virtio spec, I couldn't tell if BAR 4 is
hardcoded in the spec and should be used by default. So maybe we should
try to read the BAR number from what the device provides to be more spec
compliant? If we think that its okay to just use BAR 4 for now, then we
should at least have a #define for it (also for magic number 15 which is
a zpci address space number for PCI config space).
Thanks
Farhan
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 6/7] s390x: Build IPLB for virtio-pci devices
2025-10-20 16:20 [PATCH 0/7] s390x: Add support for virtio-blk-pci IPL device jrossi
` (4 preceding siblings ...)
2025-10-20 16:20 ` [PATCH 5/7] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL jrossi
@ 2025-10-20 16:20 ` jrossi
2025-10-21 14:08 ` Thomas Huth
2025-10-20 16:20 ` [PATCH 7/7] tests/qtest: Add s390x PCI boot test to cdrom-test.c jrossi
6 siblings, 1 reply; 25+ messages in thread
From: jrossi @ 2025-10-20 16:20 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato, jrossi, zycai
From: Jared Rossi <jrossi@linux.ibm.com>
Search for a corresponding S390PCIBusDevice and build an IPLB if a device has
been indexed for boot but does not identify as a CCW device,
PCI devices are not yet included in boot probing (they must have a boot index).
Per-device loadparm is not yet supported for PCI devices.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
hw/s390x/ipl.h | 3 ++
include/hw/s390x/s390-pci-bus.h | 2 ++
hw/s390x/ipl.c | 56 ++++++++++++++++++++++++++++++---
hw/s390x/s390-pci-bus.c | 2 +-
4 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
index aec2244321..5396d4ed91 100644
--- a/hw/s390x/ipl.h
+++ b/hw/s390x/ipl.h
@@ -107,6 +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_QEMU_SCSI_LEN 200
static inline bool iplb_valid_len(IplParameterBlock *iplb)
@@ -179,6 +180,8 @@ static inline bool iplb_valid(IplParameterBlock *iplb)
return len >= S390_IPLB_MIN_FCP_LEN;
case S390_IPL_TYPE_CCW:
return len >= S390_IPLB_MIN_CCW_LEN;
+ case S390_IPL_TYPE_PCI:
+ return len >= S390_IPLB_MIN_PCI_LEN;
case S390_IPL_TYPE_QEMU_SCSI:
default:
return false;
diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index 04944d4fed..18c6a6d6d5 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -402,6 +402,8 @@ S390PCIBusDevice *s390_pci_find_dev_by_fh(S390pciState *s, uint32_t fh);
S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState *s, uint32_t fid);
S390PCIBusDevice *s390_pci_find_dev_by_target(S390pciState *s,
const char *target);
+S390PCIBusDevice *s390_pci_find_dev_by_pci(S390pciState *s,
+ PCIDevice *pci_dev);
S390PCIBusDevice *s390_pci_find_next_avail_dev(S390pciState *s,
S390PCIBusDevice *pbdev);
void s390_pci_ism_reset(void);
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 2f082396c7..4d0ff25816 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -22,12 +22,14 @@
#include "hw/loader.h"
#include "hw/qdev-properties.h"
#include "hw/boards.h"
+#include "hw/s390x/s390-pci-bus.h"
#include "hw/s390x/virtio-ccw.h"
#include "hw/s390x/vfio-ccw.h"
#include "hw/s390x/css.h"
#include "hw/s390x/ebcdic.h"
#include "hw/scsi/scsi.h"
#include "hw/virtio/virtio-net.h"
+#include "hw/virtio/virtio-pci.h"
#include "ipl.h"
#include "qemu/error-report.h"
#include "qemu/config-file.h"
@@ -337,7 +339,8 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
ipl->qipl.boot_menu_timeout = cpu_to_be32(splash_time);
}
-#define CCW_DEVTYPE_NONE 0x00
+#define S390_DEVTYPE_NONE 0x00
+
#define CCW_DEVTYPE_VIRTIO 0x01
#define CCW_DEVTYPE_VIRTIO_NET 0x02
#define CCW_DEVTYPE_SCSI 0x03
@@ -346,7 +349,7 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
{
CcwDevice *ccw_dev = NULL;
- int tmp_dt = CCW_DEVTYPE_NONE;
+ int tmp_dt = S390_DEVTYPE_NONE;
if (dev_st) {
VirtIONet *virtio_net_dev = (VirtIONet *)
@@ -393,6 +396,31 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
return ccw_dev;
}
+#define PCI_DEVTYPE_VIRTIO 0x05
+
+static S390PCIBusDevice *s390_get_pci_device(DeviceState *dev_st, int *devtype)
+{
+ S390PCIBusDevice *pbdev = NULL;
+ int tmp_dt = S390_DEVTYPE_NONE;
+
+ if (dev_st) {
+ PCIDevice *pci_dev = (PCIDevice *)
+ object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->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_VIRTIO;
+ }
+ }
+ }
+ if (devtype) {
+ *devtype = tmp_dt;
+ }
+
+ return pbdev;
+}
+
static uint64_t s390_ipl_map_iplb_chain(IplParameterBlock *iplb_chain)
{
S390IPLState *ipl = get_ipl_device();
@@ -425,14 +453,12 @@ void s390_ipl_convert_loadparm(char *ascii_lp, uint8_t *ebcdic_lp)
static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
{
CcwDevice *ccw_dev = NULL;
+ S390PCIBusDevice *pbdev = NULL;
SCSIDevice *sd;
int devtype;
uint8_t *lp;
g_autofree void *scsi_lp = NULL;
- /*
- * Currently allow IPL only from CCW devices.
- */
ccw_dev = s390_get_ccw_device(dev_st, &devtype);
if (ccw_dev) {
lp = ccw_dev->loadparm;
@@ -482,6 +508,26 @@ static bool s390_build_iplb(DeviceState *dev_st, IplParameterBlock *iplb)
return true;
}
+ pbdev = s390_get_pci_device(dev_st, &devtype);
+ if (pbdev) {
+ switch (devtype) {
+ case PCI_DEVTYPE_VIRTIO:
+ iplb->len = S390_IPLB_MIN_PCI_LEN;
+ iplb->pbt = S390_IPL_TYPE_PCI;
+ iplb->pci.fid = pbdev->fid;
+ break;
+ default:
+ return false;
+ }
+
+ /* Per-device loadparm not yet supported for non-ccw IPL */
+ lp = S390_CCW_MACHINE(qdev_get_machine())->loadparm;
+ s390_ipl_convert_loadparm((char *)lp, iplb->loadparm);
+ iplb->flags |= DIAG308_FLAGS_LP_VALID;
+
+ return true;
+ }
+
return false;
}
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 52820894fa..615974851b 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -249,7 +249,7 @@ S390PCIBusDevice *s390_pci_find_dev_by_target(S390pciState *s,
return NULL;
}
-static S390PCIBusDevice *s390_pci_find_dev_by_pci(S390pciState *s,
+S390PCIBusDevice *s390_pci_find_dev_by_pci(S390pciState *s,
PCIDevice *pci_dev)
{
S390PCIBusDevice *pbdev;
--
2.49.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 6/7] s390x: Build IPLB for virtio-pci devices
2025-10-20 16:20 ` [PATCH 6/7] s390x: Build IPLB for virtio-pci devices jrossi
@ 2025-10-21 14:08 ` Thomas Huth
2025-10-22 19:35 ` Jared Rossi
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Huth @ 2025-10-21 14:08 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Search for a corresponding S390PCIBusDevice and build an IPLB if a device has
> been indexed for boot but does not identify as a CCW device,
>
> PCI devices are not yet included in boot probing (they must have a boot index).
> Per-device loadparm is not yet supported for PCI devices.
Could you add it? Something similar to what has been done in
scsi_property_add_specifics() in hw/scsi/scsi-disk.c for the SCSI disks?
...
> @@ -346,7 +349,7 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
> static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
> {
> CcwDevice *ccw_dev = NULL;
> - int tmp_dt = CCW_DEVTYPE_NONE;
> + int tmp_dt = S390_DEVTYPE_NONE;
>
> if (dev_st) {
> VirtIONet *virtio_net_dev = (VirtIONet *)
> @@ -393,6 +396,31 @@ static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
> return ccw_dev;
> }
>
> +#define PCI_DEVTYPE_VIRTIO 0x05
Is this for virtio-block only ? If so, I'd maybe rather name it
PCI_DEVTYPE_VIRTIO_BLK or so. Or will this be used for virtio-scsi-pci etc.,
too?
Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/7] s390x: Build IPLB for virtio-pci devices
2025-10-21 14:08 ` Thomas Huth
@ 2025-10-22 19:35 ` Jared Rossi
0 siblings, 0 replies; 25+ messages in thread
From: Jared Rossi @ 2025-10-22 19:35 UTC (permalink / raw)
To: Thomas Huth, qemu-devel, qemu-s390x
Cc: jjherne, alifm, farman, mjrosato, zycai
On 10/21/25 10:08 AM, Thomas Huth wrote:
> On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> Search for a corresponding S390PCIBusDevice and build an IPLB if a
>> device has
>> been indexed for boot but does not identify as a CCW device,
>>
>> PCI devices are not yet included in boot probing (they must have a
>> boot index).
>> Per-device loadparm is not yet supported for PCI devices.
>
> Could you add it? Something similar to what has been done in
> scsi_property_add_specifics() in hw/scsi/scsi-disk.c for the SCSI disks?
>
Sure. It will be included in the next version.
> ...
>> @@ -346,7 +349,7 @@ static void s390_ipl_set_boot_menu(S390IPLState
>> *ipl)
>> static CcwDevice *s390_get_ccw_device(DeviceState *dev_st, int
>> *devtype)
>> {
>> CcwDevice *ccw_dev = NULL;
>> - int tmp_dt = CCW_DEVTYPE_NONE;
>> + int tmp_dt = S390_DEVTYPE_NONE;
>> if (dev_st) {
>> VirtIONet *virtio_net_dev = (VirtIONet *)
>> @@ -393,6 +396,31 @@ static CcwDevice
>> *s390_get_ccw_device(DeviceState *dev_st, int *devtype)
>> return ccw_dev;
>> }
>> +#define PCI_DEVTYPE_VIRTIO 0x05
>
> Is this for virtio-block only ? If so, I'd maybe rather name it
> PCI_DEVTYPE_VIRTIO_BLK or so. Or will this be used for virtio-scsi-pci
> etc., too?
I named it this way because it is the PCI equivalent of
CCW_DEVTYPE_VIRTIO used in the above s390_get_ccw_device() function.
Although The CCW function also has a different designation for
virtio-net devices I don't think that will be useful for PCI (due to the
change with the netboot binary). So, this device type could be used for
both virtio-blk-pci and virtio-net-pci. Virtio-scsi-pci would need a
different designation though (much like how there exists CCW_DEVTYPE_SCSI).
Regards,
Jared Rossi
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 7/7] tests/qtest: Add s390x PCI boot test to cdrom-test.c
2025-10-20 16:20 [PATCH 0/7] s390x: Add support for virtio-blk-pci IPL device jrossi
` (5 preceding siblings ...)
2025-10-20 16:20 ` [PATCH 6/7] s390x: Build IPLB for virtio-pci devices jrossi
@ 2025-10-20 16:20 ` jrossi
2025-10-21 14:09 ` Thomas Huth
6 siblings, 1 reply; 25+ messages in thread
From: jrossi @ 2025-10-20 16:20 UTC (permalink / raw)
To: qemu-devel, qemu-s390x, thuth
Cc: jjherne, alifm, farman, mjrosato, jrossi, zycai
From: Jared Rossi <jrossi@linux.ibm.com>
Add a rudimentary test for s390x IPL to verify that a guest may boot using
virtio-blk-pci device.
Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
---
tests/qtest/cdrom-test.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 56e2d283a9..a65854d2bc 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -246,6 +246,13 @@ static void add_s390x_tests(void)
"-drive if=none,id=d2,media=cdrom,file=",
test_cdboot);
}
+ if (qtest_has_device("virtio-blk-pci")) {
+ qtest_add_data_func("cdrom/boot/pci-bus-with-bootindex",
+ "-device virtio-scsi -device virtio-serial "
+ "-device virtio-blk-pci,drive=d1,bootindex=1 "
+ "-drive if=none,id=d1,media=cdrom,file=",
+ test_cdboot);
+ }
}
int main(int argc, char **argv)
--
2.49.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 7/7] tests/qtest: Add s390x PCI boot test to cdrom-test.c
2025-10-20 16:20 ` [PATCH 7/7] tests/qtest: Add s390x PCI boot test to cdrom-test.c jrossi
@ 2025-10-21 14:09 ` Thomas Huth
0 siblings, 0 replies; 25+ messages in thread
From: Thomas Huth @ 2025-10-21 14:09 UTC (permalink / raw)
To: jrossi, qemu-devel, qemu-s390x; +Cc: jjherne, alifm, farman, mjrosato, zycai
On 20/10/2025 18.20, jrossi@linux.ibm.com wrote:
> From: Jared Rossi <jrossi@linux.ibm.com>
>
> Add a rudimentary test for s390x IPL to verify that a guest may boot using
> virtio-blk-pci device.
>
> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
> ---
> tests/qtest/cdrom-test.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> index 56e2d283a9..a65854d2bc 100644
> --- a/tests/qtest/cdrom-test.c
> +++ b/tests/qtest/cdrom-test.c
> @@ -246,6 +246,13 @@ static void add_s390x_tests(void)
> "-drive if=none,id=d2,media=cdrom,file=",
> test_cdboot);
> }
> + if (qtest_has_device("virtio-blk-pci")) {
> + qtest_add_data_func("cdrom/boot/pci-bus-with-bootindex",
> + "-device virtio-scsi -device virtio-serial "
> + "-device virtio-blk-pci,drive=d1,bootindex=1 "
> + "-drive if=none,id=d1,media=cdrom,file=",
> + test_cdboot);
> + }
> }
>
> int main(int argc, char **argv)
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread