Re: [PATCH 4/7] pc-bios/s390-ccw: Introduce PCI device IPL format

[Thomas Huth <thuth@redhat.com>]

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