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.0Hi 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..]
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
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
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
© 2016 - 2025 Red Hat, Inc.