Re: [PATCH v5 1/3] util: Add functions for s390x mmio read/write

[Thomas Huth <thuth@redhat.com>]

On 17/04/2025 19.37, Farhan Ali wrote:
> Starting with z15 (or newer) we can execute mmio
> instructions from userspace. On older platforms
> where we don't have these instructions available
> we can fallback to using system calls to access
> the PCI mapped resources.
> 
> This patch adds helper functions for mmio reads
> and writes for s390x.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>   include/qemu/s390x_pci_mmio.h |  24 ++++++
>   util/meson.build              |   2 +
>   util/s390x_pci_mmio.c         | 148 ++++++++++++++++++++++++++++++++++
>   3 files changed, 174 insertions(+)
>   create mode 100644 include/qemu/s390x_pci_mmio.h
>   create mode 100644 util/s390x_pci_mmio.c
> 
> diff --git a/include/qemu/s390x_pci_mmio.h b/include/qemu/s390x_pci_mmio.h
> new file mode 100644
> index 0000000000..c5f63ecefa
> --- /dev/null
> +++ b/include/qemu/s390x_pci_mmio.h
> @@ -0,0 +1,24 @@
> +/*
> + * s390x PCI MMIO definitions
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef S390X_PCI_MMIO_H
> +#define S390X_PCI_MMIO_H
> +
> +#ifdef __s390x__
> +uint8_t s390x_pci_mmio_read_8(const void *ioaddr);
> +uint16_t s390x_pci_mmio_read_16(const void *ioaddr);
> +uint32_t s390x_pci_mmio_read_32(const void *ioaddr);
> +uint64_t s390x_pci_mmio_read_64(const void *ioaddr);
> +
> +void s390x_pci_mmio_write_8(void *ioaddr, uint8_t val);
> +void s390x_pci_mmio_write_16(void *ioaddr, uint16_t val);
> +void s390x_pci_mmio_write_32(void *ioaddr, uint32_t val);
> +void s390x_pci_mmio_write_64(void *ioaddr, uint64_t val);
> +#endif /* __s390x__ */
> +
> +#endif /* S390X_PCI_MMIO_H */
> diff --git a/util/meson.build b/util/meson.build
> index 780b5977a8..acb21592f9 100644
> --- a/util/meson.build
> +++ b/util/meson.build
> @@ -131,4 +131,6 @@ elif cpu in ['ppc', 'ppc64']
>     util_ss.add(files('cpuinfo-ppc.c'))
>   elif cpu in ['riscv32', 'riscv64']
>     util_ss.add(files('cpuinfo-riscv.c'))
> +elif cpu == 's390x'
> +  util_ss.add(files('s390x_pci_mmio.c'))
>   endif
> diff --git a/util/s390x_pci_mmio.c b/util/s390x_pci_mmio.c
> new file mode 100644
> index 0000000000..820458a026
> --- /dev/null
> +++ b/util/s390x_pci_mmio.c
> @@ -0,0 +1,148 @@
> +/*
> + * s390x PCI MMIO definitions
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include <unistd.h>

unistd.h is already included by osdep.h, so you don't have to include it 
again here.

> +#include <sys/syscall.h>
> +#include "qemu/s390x_pci_mmio.h"
> +#include "elf.h"
> +
> +union register_pair {
> +    unsigned __int128 pair;
> +    struct {
> +        uint64_t even;
> +        uint64_t odd;
> +    };
> +};
> +
> +static bool is_mio_supported;
> +
> +static __attribute__((constructor)) void check_is_mio_supported(void)
> +{
> +    is_mio_supported = !!(qemu_getauxval(AT_HWCAP) & HWCAP_S390_PCI_MIO);
> +}
> +
> +static uint64_t s390x_pcilgi(const void *ioaddr, size_t len)
> +{
> +    union register_pair ioaddr_len = { .even = (uint64_t)ioaddr,
> +                                       .odd = len };
> +    uint64_t val;
> +    int cc;
> +
> +    asm volatile(
> +        /* pcilgi */
> +        ".insn   rre,0xb9d60000,%[val],%[ioaddr_len]\n"
> +        "ipm     %[cc]\n"
> +        "srl     %[cc],28\n"
> +        : [cc] "=d"(cc), [val] "=d"(val),
> +        [ioaddr_len] "+&d"(ioaddr_len.pair) :: "cc");

Do we need the "&" modifier here? ... at least the kernel does not seem to 
use it ...

> +
> +    if (cc) {
> +        val = -1ULL;
> +    }
> +
> +    return val;
> +}
> +
> +static void s390x_pcistgi(void *ioaddr, uint64_t val, size_t len)
> +{
> +    union register_pair ioaddr_len = {.even = (uint64_t)ioaddr, .odd = len};
> +
> +    asm volatile (
> +        /* pcistgi */
> +        ".insn   rre,0xb9d40000,%[val],%[ioaddr_len]\n"
> +        : [ioaddr_len] "+&d" (ioaddr_len.pair)

dito

> +        : [val] "d" (val)
> +        : "cc", "memory");
> +}
...
> +void s390x_pci_mmio_write_64(void *ioaddr, uint64_t val)
> +{
> +    if (is_mio_supported) {
> +        s390x_pcistgi(ioaddr, val, sizeof(val));
> +    } else {
> +        syscall(__NR_s390_pci_mmio_write, ioaddr, &val, sizeof(val));
> +    }
> +}
> +

FWIW, "git am" complains about "new blank line at EOF" here.

Apart from these nits, the patch looks sane to me.

  Thomas