Re: [PATCH v4 11/15] pc-bios/s390-ccw: Introduce virtio-pci functions

[Eric Farman]

On Tue, 2026-03-03 at 21:59 -0500, jro...@linux.ibm.com wrote:
> From: Jared Rossi 
> 
> Define common functionality for interacting with virtio-pci devices.
> 
> Signed-off-by: Jared Rossi 
> ---
>  pc-bios/s390-ccw/Makefile |   2 +-
>  pc-bios/s390-ccw/virtio-pci.c | 167 ++
>  pc-bios/s390-ccw/virtio-pci.h |  80 
>  pc-bios/s390-ccw/virtio.h |   5 +
>  4 files changed, 253 insertions(+), 1 deletion(-)
>  create mode 100644 pc-bios/s390-ccw/virtio-pci.c
>  create mode 100644 pc-bios/s390-ccw/virtio-pci.h
> 

...snip...
(Only comment I had in here was the s/0x1402/0x1042/ that Thomas pointed out.)


> diff --git a/pc-bios/s390-ccw/virtio-pci.h b/pc-bios/s390-ccw/virtio-pci.h
> new file mode 100644
> index 00..54c524f698
> --- /dev/null
> +++ b/pc-bios/s390-ccw/virtio-pci.h
> @@ -0,0 +1,80 @@
> +/*
> + * Definitions for virtio-pci
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Jared Rossi 
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef VIRTIO_PCI_H
> +#define VIRTIO_PCI_H
> +
> +/* Common configuration */
> +#define VPCI_CAP_COMMON_CFG  1
> +/* Notifications */
> +#define VPCI_CAP_NOTIFY_CFG  2
> +/* ISR access */
> +#define VPCI_CAP_ISR_CFG 3
> +/* Device specific configuration */
> +#define VPCI_CAP_DEVICE_CFG  4
> +/* PCI configuration access */
> +#define VPCI_CAP_PCI_CFG 5
> +/* Additional shared memory capability */
> +#define VPCI_CAP_SHARED_MEMORY_CFG   8
> +/* PCI vendor data configuration */
> +#define VPCI_CAP_VENDOR_CFG  9
> +
> +/* Offsets within capability header */
> +#define VPCI_CAP_VNDR0
> +#define VPCI_CAP_NEXT1
> +#define VPCI_CAP_LEN 2
> +#define VPCI_CAP_CFG_TYPE3
> +#define VPCI_CAP_BAR 4
> +#define VPCI_CAP_OFFSET  8
> +#define VPCI_CAP_LENGTH  12
> +
> +#define VPCI_N_CAP_MULT 16 /* Notify multiplier, VPCI_CAP_NOTIFY_CFG only */
> +
> +/* Common Area Offsets for virtio-pci queue */
> +#define VPCI_C_OFFSET_DFSELECT  0
> +#define VPCI_C_OFFSET_DF4
> +#define VPCI_C_OFFSET_GFSELECT  8
> +#define VPCI_C_OFFSET_GF12
> +#define VPCI_C_COMMON_NUMQ  18
> +#define VPCI_C_OFFSET_STATUS20
> +#define VPCI_C_OFFSET_Q_SELECT  22
> +#define VPCI_C_OFFSET_Q_SIZE24
> +#define VPCI_C_OFFSET_Q_ENABLE  28
> +#define VPCI_C_OFFSET_Q_NOFF30
> +#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 VIRTIO_F_VERSION_1  1   /* Feature bit 32 */
> +
> +struct VirtioPciCap {
> +uint8_t bar; /* Which PCIAS it's in */
> +uint32_t off;/* Offset within bar */
> +};
> +typedef struct VirtioPciCap  VirtioPciCap;
> +
> +void virtio_pci_id2type(VDev *vdev, uint16_t device_id);
> +int virtio_pci_reset(VDev *vdev);
> +long virtio_pci_notify(int vq_id);
> +
> +int vpci_read_flex(uint64_t offset, uint8_t pcias, void *buf, int len);
> +int vpci_read_bswap64(uint64_t offset, uint8_t pcias, uint64_t *buf);
> +int vpci_read_bswap32(uint64_t offset, uint8_t pcias, uint32_t *buf);
> +int vpci_read_bswap16(uint64_t offset, uint8_t pcias, uint16_t *buf);
> +int vpci_read_byte(uint64_t offset, uint8_t pcias, uint8_t *buf);
> +
> +int vpci_bswap64_write(uint64_t offset, uint8_t pcias, uint64_t data);
> +int vpci_bswap32_write(uint64_t offset, uint8_t pcias, uint32_t data);
> +int vpci_bswap16_write(uint64_t offset, uint8_t pcias, uint16_t data);
> +int vpci_write_byte(uint64_t offset, uint8_t pcias, uint8_t data);

I know it's pedantic, but since the reads are vpci_read_b while the writes 
are vcpi_b_write
(except the single byte case) could we make them consistent, please? (Soft 
preference for
vpci_read/write_b, but I don't much care.)

> +
> +#endif
> diff --git a/pc-bios/s390-ccw/virtio.h b/pc-bios/s390-ccw/virtio.h
> index c3cb5a6ee3..0e7dbdb64c 100644
> --- a/pc-bios/s390-ccw/virtio.h
> +++ b/pc-bios/s390-ccw/virtio.h
> @@ -18,6 +18,10 @@
>  #define VIRTIO_CONFIG_S_DRIVER  2
>  /* Driver has used its parts of the config, and is happy */
>  #define VIRTIO_CONFIG_S_DRIVER_OK   4
> +/* Feature negotiation complete */
> +#define VIRTIO_CONFIG_S_FEATURES_OK 8
> +/* Clear status byte */
> +#define VIRTIO_CONFIG_S_RESET   0x00

Heh, in v3 I wondered if you could add the new status bit(s) here. Bt... :)

Virtio section 2.1 defines "NEEDS_RESET" as 64 (0x40). Of course it says that 
the status field
starts as zero and is reinitialized to zero BY a reset, which is what you end 
up doing. Its
placement here between FEATURES_OK and FAILED with their spec-accurate 
definitions, but with a
different definition had me puzzled. Maybe move it to the top of this list, and 
rename it to
..._S_INIT or ..._S_Z

Re: [PATCH v4 11/15] pc-bios/s390-ccw: Introduce virtio-pci functions

[Jared Rossi]

On 3/4/26 4:18 AM, Thomas Huth wrote:

On 04/03/2026 03.59, jro...@linux.ibm.com wrote:

From: Jared Rossi 

Define common functionality for interacting with virtio-pci devices.

Signed-off-by: Jared Rossi 
---

...
diff --git a/pc-bios/s390-ccw/virtio-pci.c 
b/pc-bios/s390-ccw/virtio-pci.c

new file mode 100644
index 00..2cfb84bf66
--- /dev/null
+++ b/pc-bios/s390-ccw/virtio-pci.c
@@ -0,0 +1,167 @@
+/*
+ * Functionality for virtio-pci
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "clp.h"
+#include "pci.h"
+#include "helper.h"
+#include "virtio.h"
+#include "bswap.h"
+#include "virtio-pci.h"
+#include "s390-time.h"
+#include 
+
+/* Variable offsets used for reads/writes to modern memory regions */
+VirtioPciCap c_cap; /* Common capabilities  */
+VirtioPciCap d_cap; /* Device capabilities  */
+VirtioPciCap n_cap; /* Notify capabilities  */
+uint32_t notify_mult;
+uint16_t q_notify_offset;
+
+static int virtio_pci_set_status(uint8_t status)
+{
+    int rc = vpci_write_byte(c_cap.off + VPCI_C_OFFSET_STATUS, 
c_cap.bar, status);

+    if (rc) {
+    puts("Failed to write virtio-pci status");
+    return -EIO;
+    }
+
+    return 0;
+}
+
+static int virtio_pci_get_status(uint8_t *status)
+{
+    int rc = vpci_read_byte(c_cap.off + VPCI_C_OFFSET_STATUS, 
c_cap.bar, status);

+    if (rc) {
+    puts("Failed to read virtio-pci status");
+    return -EIO;
+    }
+
+    return 0;
+}
+
+/* virtio spec v1.3 section 4.1.2.1 */
+void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
+{
+    switch (device_id) {
+    case 0x1402:


I think it's 0x1042, not 0x1402 ?


Good catch.  You are correct.  I'll fix it.

Thanks again,
Jared Rossi

Re: [PATCH v4 11/15] pc-bios/s390-ccw: Introduce virtio-pci functions

[Thomas Huth]

On 04/03/2026 03.59, jro...@linux.ibm.com wrote:

From: Jared Rossi 

Define common functionality for interacting with virtio-pci devices.

Signed-off-by: Jared Rossi 
---

...

diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
new file mode 100644
index 00..2cfb84bf66
--- /dev/null
+++ b/pc-bios/s390-ccw/virtio-pci.c
@@ -0,0 +1,167 @@
+/*
+ * Functionality for virtio-pci
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "clp.h"
+#include "pci.h"
+#include "helper.h"
+#include "virtio.h"
+#include "bswap.h"
+#include "virtio-pci.h"
+#include "s390-time.h"
+#include 
+
+/* Variable offsets used for reads/writes to modern memory regions */
+VirtioPciCap c_cap; /* Common capabilities  */
+VirtioPciCap d_cap; /* Device capabilities  */
+VirtioPciCap n_cap; /* Notify capabilities  */
+uint32_t notify_mult;
+uint16_t q_notify_offset;
+
+static int virtio_pci_set_status(uint8_t status)
+{
+int rc = vpci_write_byte(c_cap.off + VPCI_C_OFFSET_STATUS, c_cap.bar, 
status);
+if (rc) {
+puts("Failed to write virtio-pci status");
+return -EIO;
+}
+
+return 0;
+}
+
+static int virtio_pci_get_status(uint8_t *status)
+{
+int rc = vpci_read_byte(c_cap.off + VPCI_C_OFFSET_STATUS, c_cap.bar, 
status);
+if (rc) {
+puts("Failed to read virtio-pci status");
+return -EIO;
+}
+
+return 0;
+}
+
+/* virtio spec v1.3 section 4.1.2.1 */
+void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
+{
+switch (device_id) {
+case 0x1402:


I think it's 0x1042, not 0x1402 ?


+case 0x1001:
+vdev->dev_type = VIRTIO_ID_BLOCK;
+break;
+default:
+vdev->dev_type = 0;
+}
+}


 Thomas

[PATCH v4 11/15] pc-bios/s390-ccw: Introduce virtio-pci functions

[jrossi]

From: Jared Rossi 

Define common functionality for interacting with virtio-pci devices.

Signed-off-by: Jared Rossi 
---
 pc-bios/s390-ccw/Makefile |   2 +-
 pc-bios/s390-ccw/virtio-pci.c | 167 ++
 pc-bios/s390-ccw/virtio-pci.h |  80 
 pc-bios/s390-ccw/virtio.h |   5 +
 4 files changed, 253 insertions(+), 1 deletion(-)
 create mode 100644 pc-bios/s390-ccw/virtio-pci.c
 create mode 100644 pc-bios/s390-ccw/virtio-pci.h

diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
index a62fc9d766..3e5dfb64d5 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
 
diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
new file mode 100644
index 00..2cfb84bf66
--- /dev/null
+++ b/pc-bios/s390-ccw/virtio-pci.c
@@ -0,0 +1,167 @@
+/*
+ * Functionality for virtio-pci
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Jared Rossi 
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "clp.h"
+#include "pci.h"
+#include "helper.h"
+#include "virtio.h"
+#include "bswap.h"
+#include "virtio-pci.h"
+#include "s390-time.h"
+#include 
+
+/* Variable offsets used for reads/writes to modern memory regions */
+VirtioPciCap c_cap; /* Common capabilities  */
+VirtioPciCap d_cap; /* Device capabilities  */
+VirtioPciCap n_cap; /* Notify capabilities  */
+uint32_t notify_mult;
+uint16_t q_notify_offset;
+
+static int virtio_pci_set_status(uint8_t status)
+{
+int rc = vpci_write_byte(c_cap.off + VPCI_C_OFFSET_STATUS, c_cap.bar, 
status);
+if (rc) {
+puts("Failed to write virtio-pci status");
+return -EIO;
+}
+
+return 0;
+}
+
+static int virtio_pci_get_status(uint8_t *status)
+{
+int rc = vpci_read_byte(c_cap.off + VPCI_C_OFFSET_STATUS, c_cap.bar, 
status);
+if (rc) {
+puts("Failed to read virtio-pci status");
+return -EIO;
+}
+
+return 0;
+}
+
+/* virtio spec v1.3 section 4.1.2.1 */
+void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
+{
+switch (device_id) {
+case 0x1402:
+case 0x1001:
+vdev->dev_type = VIRTIO_ID_BLOCK;
+break;
+default:
+vdev->dev_type = 0;
+}
+}
+
+int virtio_pci_reset(VDev *vdev)
+{
+int rc;
+uint8_t status = VIRTIO_CONFIG_S_RESET;
+
+rc = virtio_pci_set_status(status);
+rc |= virtio_pci_get_status(&status);
+
+if (rc || status) {
+puts("Failed to reset virtio-pci device");
+return 1;
+}
+
+return 0;
+}
+
+long virtio_pci_notify(int vq_id)
+{
+uint32_t offset = n_cap.off + notify_mult * q_notify_offset;
+return vpci_bswap16_write(offset, n_cap.bar, (uint16_t) vq_id);
+}
+
+/*
+ * Wrappers to byte swap common data sizes then write
+ */
+int vpci_write_byte(uint64_t offset, uint8_t pcias, uint8_t data)
+{
+return pci_write(virtio_get_device()->pci_fh, offset, pcias, (uint64_t) 
data, 1);
+}
+
+int vpci_bswap16_write(uint64_t offset, uint8_t pcias, uint16_t data)
+{
+uint64_t le_data = bswap16(data);
+return pci_write(virtio_get_device()->pci_fh, offset, pcias, le_data, 2);
+}
+
+int vpci_bswap32_write(uint64_t offset, uint8_t pcias, uint32_t data)
+{
+uint64_t le_data = bswap32(data);
+return pci_write(virtio_get_device()->pci_fh, offset, pcias, le_data, 4);
+}
+
+int vpci_bswap64_write(uint64_t offset, uint8_t pcias, uint64_t data)
+{
+uint64_t le_data = bswap64(data);
+return pci_write(virtio_get_device()->pci_fh, offset, pcias, le_data, 8);
+}
+
+/*
+ * Wrappers to read common data sizes then byte swap
+ */
+int vpci_read_byte(uint64_t offset, uint8_t pcias, uint8_t *buf)
+{
+return pci_read(virtio_get_device()->pci_fh, offset, pcias, buf, 1);
+}
+
+int vpci_read_bswap16(uint64_t offset, uint8_t pcias, uint16_t *buf)
+{
+int rc = pci_read(virtio_get_device()->pci_fh, offset, pcias, buf, 2);
+*buf = bswap16(*buf);
+return rc;
+}
+
+int vpci_read_bswap32(uint64_t offset, uint8_t pcias, uint32_t *buf)
+{
+int rc = pci_read(virtio_get_device()->pci_fh, offset, pcias, buf, 4);
+*buf = bswap32(*buf);
+return rc;
+}
+
+int vpci_read_bswap64(uint64_t offset, uint8_t pcias, uint64_t *buf)
+{
+int rc = pci_read(virtio_get_device()->pci_fh, offset, pcias, buf, 8);
+*buf = bswap64(*buf);
+return rc;
+}
+
+/*
+ * Read to an arbitrary length buffer without byte swapping
+ */
+int vpci_read_flex(uint64_t offset, uint8_t pcias, void *buf, int len)
+{
+uint8_t readlen;
+int rc;
+int remaining = len;
+
+/* Read bytes in powers of 2, up to a maximum of 8 bytes per read */
+while (remaining) {
+for (int i = 3; i