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

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Fri, 2025-04-25 at 11:00 +0200, Thomas Huth wrote:
> 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 ...
> 

From my understanding it's not strictly needed, but I also used it in
the rdma-core user-space code where I had pointed Farhan. I looked at
the PR again but didn't find or remember why I added it. We do have the
same constraint in the kernel implementation of the syscalll
(__pcilg_mio_inuser()) that I worked on in the same time frame but that
also has other instructions before the PCILGI. I see it in other uses
of "union register_pair" in the kernel too. Reading the gcc docs again,
I also think it does give gcc the right information here. The
address/length pair is only written to after it is read what it
should/can do with this information I don't know. Adding Heiko who
knows way more than I about inline assembly.

> > +
> > +    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");
> > +}