* [PATCH v5 1/3] util: Add functions for s390x mmio read/write
2025-04-17 17:37 [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
@ 2025-04-17 17:37 ` Farhan Ali
2025-04-25 9:00 ` Thomas Huth
2025-04-17 17:38 ` [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions Farhan Ali
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Farhan Ali @ 2025-04-17 17:37 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, stefanha, alifm, mjrosato, schnelle, philmd,
kwolf, hreitz, thuth, fam
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>
+#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");
+
+ 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)
+ : [val] "d" (val)
+ : "cc", "memory");
+}
+
+uint8_t s390x_pci_mmio_read_8(const void *ioaddr)
+{
+ uint8_t val = 0;
+
+ if (is_mio_supported) {
+ val = s390x_pcilgi(ioaddr, sizeof(val));
+ } else {
+ syscall(__NR_s390_pci_mmio_read, ioaddr, &val, sizeof(val));
+ }
+ return val;
+}
+
+uint16_t s390x_pci_mmio_read_16(const void *ioaddr)
+{
+ uint16_t val = 0;
+
+ if (is_mio_supported) {
+ val = s390x_pcilgi(ioaddr, sizeof(val));
+ } else {
+ syscall(__NR_s390_pci_mmio_read, ioaddr, &val, sizeof(val));
+ }
+ return val;
+}
+
+uint32_t s390x_pci_mmio_read_32(const void *ioaddr)
+{
+ uint32_t val = 0;
+
+ if (is_mio_supported) {
+ val = s390x_pcilgi(ioaddr, sizeof(val));
+ } else {
+ syscall(__NR_s390_pci_mmio_read, ioaddr, &val, sizeof(val));
+ }
+ return val;
+}
+
+uint64_t s390x_pci_mmio_read_64(const void *ioaddr)
+{
+ uint64_t val = 0;
+
+ if (is_mio_supported) {
+ val = s390x_pcilgi(ioaddr, sizeof(val));
+ } else {
+ syscall(__NR_s390_pci_mmio_read, ioaddr, &val, sizeof(val));
+ }
+ return val;
+}
+
+void s390x_pci_mmio_write_8(void *ioaddr, uint8_t val)
+{
+ if (is_mio_supported) {
+ s390x_pcistgi(ioaddr, val, sizeof(val));
+ } else {
+ syscall(__NR_s390_pci_mmio_write, ioaddr, &val, sizeof(val));
+ }
+}
+
+void s390x_pci_mmio_write_16(void *ioaddr, uint16_t val)
+{
+ if (is_mio_supported) {
+ s390x_pcistgi(ioaddr, val, sizeof(val));
+ } else {
+ syscall(__NR_s390_pci_mmio_write, ioaddr, &val, sizeof(val));
+ }
+}
+
+void s390x_pci_mmio_write_32(void *ioaddr, uint32_t val)
+{
+ if (is_mio_supported) {
+ s390x_pcistgi(ioaddr, val, sizeof(val));
+ } else {
+ syscall(__NR_s390_pci_mmio_write, ioaddr, &val, sizeof(val));
+ }
+}
+
+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));
+ }
+}
+
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5 1/3] util: Add functions for s390x mmio read/write
2025-04-17 17:37 ` [PATCH v5 1/3] util: Add functions for s390x mmio read/write Farhan Ali
@ 2025-04-25 9:00 ` Thomas Huth
2025-04-25 10:29 ` Niklas Schnelle
0 siblings, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2025-04-25 9:00 UTC (permalink / raw)
To: Farhan Ali, qemu-devel
Cc: alex.williamson, stefanha, mjrosato, schnelle, philmd, kwolf,
hreitz, fam
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 1/3] util: Add functions for s390x mmio read/write
2025-04-25 9:00 ` Thomas Huth
@ 2025-04-25 10:29 ` Niklas Schnelle
2025-04-25 14:09 ` Heiko Carstens
0 siblings, 1 reply; 18+ messages in thread
From: Niklas Schnelle @ 2025-04-25 10:29 UTC (permalink / raw)
To: Thomas Huth, Farhan Ali, qemu-devel, Heiko Carstens
Cc: alex.williamson, stefanha, mjrosato, philmd, kwolf, hreitz, fam
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");
> > +}
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 1/3] util: Add functions for s390x mmio read/write
2025-04-25 10:29 ` Niklas Schnelle
@ 2025-04-25 14:09 ` Heiko Carstens
2025-04-30 16:42 ` Farhan Ali
0 siblings, 1 reply; 18+ messages in thread
From: Heiko Carstens @ 2025-04-25 14:09 UTC (permalink / raw)
To: Niklas Schnelle
Cc: Thomas Huth, Farhan Ali, qemu-devel, alex.williamson, stefanha,
mjrosato, philmd, kwolf, hreitz, fam
On Fri, Apr 25, 2025 at 12:29:35PM +0200, Niklas Schnelle wrote:
> On Fri, 2025-04-25 at 11:00 +0200, Thomas Huth wrote:
> > On 17/04/2025 19.37, Farhan Ali wrote:
> > > + 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
It is not needed, since all inputs are consumed before to any output
is written to.
> > > + asm volatile (
> > > + /* pcistgi */
> > > + ".insn rre,0xb9d40000,%[val],%[ioaddr_len]\n"
> > > + : [ioaddr_len] "+&d" (ioaddr_len.pair)
> >
> > dito
Same here, it is not needed.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 1/3] util: Add functions for s390x mmio read/write
2025-04-25 14:09 ` Heiko Carstens
@ 2025-04-30 16:42 ` Farhan Ali
0 siblings, 0 replies; 18+ messages in thread
From: Farhan Ali @ 2025-04-30 16:42 UTC (permalink / raw)
To: Heiko Carstens, Niklas Schnelle
Cc: Thomas Huth, qemu-devel, alex.williamson, stefanha, mjrosato,
philmd, kwolf, hreitz, fam
On 4/25/2025 7:09 AM, Heiko Carstens wrote:
> On Fri, Apr 25, 2025 at 12:29:35PM +0200, Niklas Schnelle wrote:
>> On Fri, 2025-04-25 at 11:00 +0200, Thomas Huth wrote:
>>> On 17/04/2025 19.37, Farhan Ali wrote:
>>>> + 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
> It is not needed, since all inputs are consumed before to any output
> is written to.
Ack, will update the patch.
>>>> + asm volatile (
>>>> + /* pcistgi */
>>>> + ".insn rre,0xb9d40000,%[val],%[ioaddr_len]\n"
>>>> + : [ioaddr_len] "+&d" (ioaddr_len.pair)
>>> dito
> Same here, it is not needed.
Ack, will update the patch here as well.
Thanks
Farhan
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions
2025-04-17 17:37 [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
2025-04-17 17:37 ` [PATCH v5 1/3] util: Add functions for s390x mmio read/write Farhan Ali
@ 2025-04-17 17:38 ` Farhan Ali
2025-04-22 14:45 ` Stefan Hajnoczi
2025-04-25 9:17 ` Thomas Huth
2025-04-17 17:38 ` [PATCH v5 3/3] block/nvme: Use host PCI MMIO API Farhan Ali
2025-04-24 16:24 ` [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
3 siblings, 2 replies; 18+ messages in thread
From: Farhan Ali @ 2025-04-17 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, stefanha, alifm, mjrosato, schnelle, philmd,
kwolf, hreitz, thuth, fam
Add a generic API for host PCI MMIO reads/writes
(e.g. Linux VFIO BAR accesses). The functions access
little endian memory and returns the result in
host cpu endianness.
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
include/qemu/host-pci-mmio.h | 141 +++++++++++++++++++++++++++++++++++
1 file changed, 141 insertions(+)
create mode 100644 include/qemu/host-pci-mmio.h
diff --git a/include/qemu/host-pci-mmio.h b/include/qemu/host-pci-mmio.h
new file mode 100644
index 0000000000..c93f77dcd4
--- /dev/null
+++ b/include/qemu/host-pci-mmio.h
@@ -0,0 +1,141 @@
+/*
+ * API for host PCI MMIO accesses (e.g. Linux VFIO BARs)
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Farhan Ali <alifm@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HOST_PCI_MMIO_H
+#define HOST_PCI_MMIO_H
+
+#include "qemu/bswap.h"
+#include "qemu/s390x_pci_mmio.h"
+
+
+static inline uint8_t host_pci_ldub_p(const void *ioaddr)
+{
+ uint8_t ret = 0;
+#ifdef __s390x__
+ ret = s390x_pci_mmio_read_8(ioaddr);
+#else
+ ret = ldub_p(ioaddr);
+#endif
+
+ return ret;
+}
+
+static inline uint16_t host_pci_lduw_le_p(const void *ioaddr)
+{
+ uint16_t ret = 0;
+#ifdef __s390x__
+ ret = le16_to_cpu(s390x_pci_mmio_read_16(ioaddr));
+#else
+ ret = lduw_le_p(ioaddr);
+#endif
+
+ return ret;
+}
+
+static inline uint32_t host_pci_ldl_le_p(const void *ioaddr)
+{
+ uint32_t ret = 0;
+#ifdef __s390x__
+ ret = le32_to_cpu(s390x_pci_mmio_read_32(ioaddr));
+#else
+ ret = (uint32_t)ldl_le_p(ioaddr);
+#endif
+
+ return ret;
+}
+
+static inline uint64_t host_pci_ldq_le_p(const void *ioaddr)
+{
+ uint64_t ret = 0;
+#ifdef __s390x__
+ ret = le64_to_cpu(s390x_pci_mmio_read_64(ioaddr));
+#else
+ ret = ldq_le_p(ioaddr);
+#endif
+
+ return ret;
+}
+
+static inline void host_pci_stb_le_p(void *ioaddr, uint8_t val)
+{
+
+#ifdef __s390x__
+ s390x_pci_mmio_write_8(ioaddr, val);
+#else
+ stb_p(ioaddr, val);
+#endif
+}
+
+static inline void host_pci_stw_le_p(void *ioaddr, uint16_t val)
+{
+
+#ifdef __s390x__
+ s390x_pci_mmio_write_16(ioaddr, cpu_to_le16(val));
+#else
+ stw_le_p(ioaddr, val);
+#endif
+}
+
+static inline void host_pci_stl_le_p(void *ioaddr, uint32_t val)
+{
+
+#ifdef __s390x__
+ s390x_pci_mmio_write_32(ioaddr, cpu_to_le32(val));
+#else
+ stl_le_p(ioaddr, val);
+#endif
+}
+
+static inline void host_pci_stq_le_p(void *ioaddr, uint64_t val)
+{
+
+#ifdef __s390x__
+ s390x_pci_mmio_write_64(ioaddr, cpu_to_le64(val));
+#else
+ stq_le_p(ioaddr, val);
+#endif
+}
+
+static inline uint64_t host_pci_ldn_le_p(const void *ioaddr, int sz)
+{
+ switch (sz) {
+ case 1:
+ return host_pci_ldub_p(ioaddr);
+ case 2:
+ return host_pci_lduw_le_p(ioaddr);
+ case 4:
+ return host_pci_ldl_le_p(ioaddr);
+ case 8:
+ return host_pci_ldq_le_p(ioaddr);
+ default:
+ g_assert_not_reached();
+ }
+}
+
+static inline void host_pci_stn_le_p(void *ioaddr, int sz, uint64_t v)
+{
+ switch (sz) {
+ case 1:
+ host_pci_stb_le_p(ioaddr, v);
+ break;
+ case 2:
+ host_pci_stw_le_p(ioaddr, v);
+ break;
+ case 4:
+ host_pci_stl_le_p(ioaddr, v);
+ break;
+ case 8:
+ host_pci_stq_le_p(ioaddr, v);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+}
+
+#endif
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions
2025-04-17 17:38 ` [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions Farhan Ali
@ 2025-04-22 14:45 ` Stefan Hajnoczi
2025-04-25 9:17 ` Thomas Huth
1 sibling, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2025-04-22 14:45 UTC (permalink / raw)
To: Farhan Ali
Cc: qemu-devel, alex.williamson, mjrosato, schnelle, philmd, kwolf,
hreitz, thuth, fam
[-- Attachment #1: Type: text/plain, Size: 524 bytes --]
On Thu, Apr 17, 2025 at 10:38:00AM -0700, Farhan Ali wrote:
> Add a generic API for host PCI MMIO reads/writes
> (e.g. Linux VFIO BAR accesses). The functions access
> little endian memory and returns the result in
> host cpu endianness.
>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
> include/qemu/host-pci-mmio.h | 141 +++++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
> create mode 100644 include/qemu/host-pci-mmio.h
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions
2025-04-17 17:38 ` [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions Farhan Ali
2025-04-22 14:45 ` Stefan Hajnoczi
@ 2025-04-25 9:17 ` Thomas Huth
2025-04-30 16:47 ` Farhan Ali
1 sibling, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2025-04-25 9:17 UTC (permalink / raw)
To: Farhan Ali, qemu-devel
Cc: alex.williamson, stefanha, mjrosato, schnelle, philmd, kwolf,
hreitz, fam
On 17/04/2025 19.38, Farhan Ali wrote:
> Add a generic API for host PCI MMIO reads/writes
> (e.g. Linux VFIO BAR accesses). The functions access
> little endian memory and returns the result in
> host cpu endianness.
>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
> include/qemu/host-pci-mmio.h | 141 +++++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
> create mode 100644 include/qemu/host-pci-mmio.h
>
> diff --git a/include/qemu/host-pci-mmio.h b/include/qemu/host-pci-mmio.h
> new file mode 100644
> index 0000000000..c93f77dcd4
> --- /dev/null
> +++ b/include/qemu/host-pci-mmio.h
> @@ -0,0 +1,141 @@
> +/*
> + * API for host PCI MMIO accesses (e.g. Linux VFIO BARs)
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HOST_PCI_MMIO_H
> +#define HOST_PCI_MMIO_H
> +
> +#include "qemu/bswap.h"
> +#include "qemu/s390x_pci_mmio.h"
> +
> +
Cosmetic nit: It's more common in QEMU to only use one empty line instead of
two.
> +static inline uint8_t host_pci_ldub_p(const void *ioaddr)
> +{
> + uint8_t ret = 0;
> +#ifdef __s390x__
> + ret = s390x_pci_mmio_read_8(ioaddr);
> +#else
> + ret = ldub_p(ioaddr);
> +#endif
> +
> + return ret;
> +}
> +
> +static inline uint16_t host_pci_lduw_le_p(const void *ioaddr)
> +{
> + uint16_t ret = 0;
> +#ifdef __s390x__
> + ret = le16_to_cpu(s390x_pci_mmio_read_16(ioaddr));
> +#else
> + ret = lduw_le_p(ioaddr);
> +#endif
> +
> + return ret;
> +}
> +
> +static inline uint32_t host_pci_ldl_le_p(const void *ioaddr)
> +{
> + uint32_t ret = 0;
> +#ifdef __s390x__
> + ret = le32_to_cpu(s390x_pci_mmio_read_32(ioaddr));
> +#else
> + ret = (uint32_t)ldl_le_p(ioaddr);
This is the only spot where you used a cast. Is it necessary, or could it be
omitted?
> +#endif
> +
> + return ret;
> +}
> +
> +static inline uint64_t host_pci_ldq_le_p(const void *ioaddr)
> +{
> + uint64_t ret = 0;
> +#ifdef __s390x__
> + ret = le64_to_cpu(s390x_pci_mmio_read_64(ioaddr));
> +#else
> + ret = ldq_le_p(ioaddr);
> +#endif
> +
> + return ret;
> +}
> +
> +static inline void host_pci_stb_le_p(void *ioaddr, uint8_t val)
> +{
> +
Remove the empty line, please.
> +#ifdef __s390x__
> + s390x_pci_mmio_write_8(ioaddr, val);
> +#else
> + stb_p(ioaddr, val);
> +#endif
> +}
> +
> +static inline void host_pci_stw_le_p(void *ioaddr, uint16_t val)
> +{
> +
dito.
> +#ifdef __s390x__
> + s390x_pci_mmio_write_16(ioaddr, cpu_to_le16(val));
> +#else
> + stw_le_p(ioaddr, val);
> +#endif
> +}
> +
> +static inline void host_pci_stl_le_p(void *ioaddr, uint32_t val)
> +{
> +
dito.
> +#ifdef __s390x__
> + s390x_pci_mmio_write_32(ioaddr, cpu_to_le32(val));
> +#else
> + stl_le_p(ioaddr, val);
> +#endif
> +}
> +
> +static inline void host_pci_stq_le_p(void *ioaddr, uint64_t val)
> +{
> +
dito
> +#ifdef __s390x__
> + s390x_pci_mmio_write_64(ioaddr, cpu_to_le64(val));
> +#else
> + stq_le_p(ioaddr, val);
> +#endif
> +}
> +
> +static inline uint64_t host_pci_ldn_le_p(const void *ioaddr, int sz)
> +{
> + switch (sz) {
> + case 1:
> + return host_pci_ldub_p(ioaddr);
> + case 2:
> + return host_pci_lduw_le_p(ioaddr);
> + case 4:
> + return host_pci_ldl_le_p(ioaddr);
> + case 8:
> + return host_pci_ldq_le_p(ioaddr);
> + default:
> + g_assert_not_reached();
> + }
> +}
> +
> +static inline void host_pci_stn_le_p(void *ioaddr, int sz, uint64_t v)
> +{
> + switch (sz) {
> + case 1:
> + host_pci_stb_le_p(ioaddr, v);
> + break;
> + case 2:
> + host_pci_stw_le_p(ioaddr, v);
> + break;
> + case 4:
> + host_pci_stl_le_p(ioaddr, v);
> + break;
> + case 8:
> + host_pci_stq_le_p(ioaddr, v);
> + break;
> + default:
> + g_assert_not_reached();
> + }
> +}
> +
> +#endif
Apart from the nits, patch looks good to me.
Thomas
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions
2025-04-25 9:17 ` Thomas Huth
@ 2025-04-30 16:47 ` Farhan Ali
2025-04-30 17:52 ` Thomas Huth
0 siblings, 1 reply; 18+ messages in thread
From: Farhan Ali @ 2025-04-30 16:47 UTC (permalink / raw)
To: Thomas Huth, qemu-devel
Cc: alex.williamson, stefanha, mjrosato, schnelle, philmd, kwolf,
hreitz, fam
..snip...
>> +static inline uint32_t host_pci_ldl_le_p(const void *ioaddr)
>> +{
>> + uint32_t ret = 0;
>> +#ifdef __s390x__
>> + ret = le32_to_cpu(s390x_pci_mmio_read_32(ioaddr));
>> +#else
>> + ret = (uint32_t)ldl_le_p(ioaddr);
>
> This is the only spot where you used a cast. Is it necessary, or could
> it be omitted?
Yes, the ldl_le_p returns an int. We do similar cast here
https://github.com/qemu/qemu/blob/73d29ea2417b58ca55fba1aa468ba38e3607b583/include/qemu/bswap.h#L416
>
>> +#endif
>> +
>> + return ret;
>> +}
>> +
>> +static inline uint64_t host_pci_ldq_le_p(const void *ioaddr)
>> +{
>> + uint64_t ret = 0;
>> +#ifdef __s390x__
>> + ret = le64_to_cpu(s390x_pci_mmio_read_64(ioaddr));
>> +#else
>> + ret = ldq_le_p(ioaddr);
>> +#endif
>> +
>> + return ret;
>> +}
>> +
>> +static inline void host_pci_stb_le_p(void *ioaddr, uint8_t val)
>> +{
>> +
>
> Remove the empty line, please.
>
>> +#ifdef __s390x__
>> + s390x_pci_mmio_write_8(ioaddr, val);
>> +#else
>> + stb_p(ioaddr, val);
>> +#endif
>> +}
>> +
>> +static inline void host_pci_stw_le_p(void *ioaddr, uint16_t val)
>> +{
>> +
>
> dito.
>
>> +#ifdef __s390x__
>> + s390x_pci_mmio_write_16(ioaddr, cpu_to_le16(val));
>> +#else
>> + stw_le_p(ioaddr, val);
>> +#endif
>> +}
>> +
>> +static inline void host_pci_stl_le_p(void *ioaddr, uint32_t val)
>> +{
>> +
>
> dito.
>
>> +#ifdef __s390x__
>> + s390x_pci_mmio_write_32(ioaddr, cpu_to_le32(val));
>> +#else
>> + stl_le_p(ioaddr, val);
>> +#endif
>> +}
>> +
>> +static inline void host_pci_stq_le_p(void *ioaddr, uint64_t val)
>> +{
>> +
>
> dito
>
>> +#ifdef __s390x__
>> + s390x_pci_mmio_write_64(ioaddr, cpu_to_le64(val));
>> +#else
>> + stq_le_p(ioaddr, val);
>> +#endif
>> +}
>> +
>> +static inline uint64_t host_pci_ldn_le_p(const void *ioaddr, int sz)
>> +{
>> + switch (sz) {
>> + case 1:
>> + return host_pci_ldub_p(ioaddr);
>> + case 2:
>> + return host_pci_lduw_le_p(ioaddr);
>> + case 4:
>> + return host_pci_ldl_le_p(ioaddr);
>> + case 8:
>> + return host_pci_ldq_le_p(ioaddr);
>> + default:
>> + g_assert_not_reached();
>> + }
>> +}
>> +
>> +static inline void host_pci_stn_le_p(void *ioaddr, int sz, uint64_t v)
>> +{
>> + switch (sz) {
>> + case 1:
>> + host_pci_stb_le_p(ioaddr, v);
>> + break;
>> + case 2:
>> + host_pci_stw_le_p(ioaddr, v);
>> + break;
>> + case 4:
>> + host_pci_stl_le_p(ioaddr, v);
>> + break;
>> + case 8:
>> + host_pci_stq_le_p(ioaddr, v);
>> + break;
>> + default:
>> + g_assert_not_reached();
>> + }
>> +}
>> +
>> +#endif
>
> Apart from the nits, patch looks good to me.
>
> Thomas
Thanks for reviewing! will fix the nits in the next revision.
Thanks
Farhan
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions
2025-04-30 16:47 ` Farhan Ali
@ 2025-04-30 17:52 ` Thomas Huth
2025-04-30 18:32 ` Farhan Ali
0 siblings, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2025-04-30 17:52 UTC (permalink / raw)
To: Farhan Ali, qemu-devel
Cc: alex.williamson, stefanha, mjrosato, schnelle, philmd, kwolf,
hreitz, fam
On 30/04/2025 18.47, Farhan Ali wrote:
>
> ..snip...
>
>>> +static inline uint32_t host_pci_ldl_le_p(const void *ioaddr)
>>> +{
>>> + uint32_t ret = 0;
>>> +#ifdef __s390x__
>>> + ret = le32_to_cpu(s390x_pci_mmio_read_32(ioaddr));
>>> +#else
>>> + ret = (uint32_t)ldl_le_p(ioaddr);
>>
>> This is the only spot where you used a cast. Is it necessary, or could it
>> be omitted?
>
> Yes, the ldl_le_p returns an int. We do similar cast here https://
> github.com/qemu/qemu/blob/73d29ea2417b58ca55fba1aa468ba38e3607b583/include/
> qemu/bswap.h#L416
... but that function there returns an 64-bit value, while you are assigning
the value to a 32-bit variable here, and you also only return a 32-bit value
from the function here. So there is no way that this could accidentally be
sign-extended, could it?
Thomas
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions
2025-04-30 17:52 ` Thomas Huth
@ 2025-04-30 18:32 ` Farhan Ali
2025-04-30 18:38 ` Thomas Huth
0 siblings, 1 reply; 18+ messages in thread
From: Farhan Ali @ 2025-04-30 18:32 UTC (permalink / raw)
To: Thomas Huth, qemu-devel
Cc: alex.williamson, stefanha, mjrosato, schnelle, philmd, kwolf,
hreitz, fam
On 4/30/2025 10:52 AM, Thomas Huth wrote:
> On 30/04/2025 18.47, Farhan Ali wrote:
>>
>> ..snip...
>>
>>>> +static inline uint32_t host_pci_ldl_le_p(const void *ioaddr)
>>>> +{
>>>> + uint32_t ret = 0;
>>>> +#ifdef __s390x__
>>>> + ret = le32_to_cpu(s390x_pci_mmio_read_32(ioaddr));
>>>> +#else
>>>> + ret = (uint32_t)ldl_le_p(ioaddr);
>>>
>>> This is the only spot where you used a cast. Is it necessary, or
>>> could it be omitted?
>>
>> Yes, the ldl_le_p returns an int. We do similar cast here https://
>> github.com/qemu/qemu/blob/73d29ea2417b58ca55fba1aa468ba38e3607b583/include/
>> qemu/bswap.h#L416
>
> ... but that function there returns an 64-bit value, while you are
> assigning the value to a 32-bit variable here, and you also only
> return a 32-bit value from the function here. So there is no way that
> this could accidentally be sign-extended, could it?
>
> Thomas
>
>
I checked the Linux kernel and there too we seem to be returning
unsigned 32 bit values (readl) for 32 bit mmio reads. So I think it
should be safe. Do you think we should remove the cast?
Thanks
Farhan
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions
2025-04-30 18:32 ` Farhan Ali
@ 2025-04-30 18:38 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2025-04-30 18:38 UTC (permalink / raw)
To: Farhan Ali, qemu-devel
Cc: alex.williamson, stefanha, mjrosato, schnelle, philmd, kwolf,
hreitz, fam
On 30/04/2025 20.32, Farhan Ali wrote:
>
> On 4/30/2025 10:52 AM, Thomas Huth wrote:
>> On 30/04/2025 18.47, Farhan Ali wrote:
>>>
>>> ..snip...
>>>
>>>>> +static inline uint32_t host_pci_ldl_le_p(const void *ioaddr)
>>>>> +{
>>>>> + uint32_t ret = 0;
>>>>> +#ifdef __s390x__
>>>>> + ret = le32_to_cpu(s390x_pci_mmio_read_32(ioaddr));
>>>>> +#else
>>>>> + ret = (uint32_t)ldl_le_p(ioaddr);
>>>>
>>>> This is the only spot where you used a cast. Is it necessary, or could
>>>> it be omitted?
>>>
>>> Yes, the ldl_le_p returns an int. We do similar cast here https://
>>> github.com/qemu/qemu/blob/73d29ea2417b58ca55fba1aa468ba38e3607b583/
>>> include/ qemu/bswap.h#L416
>>
>> ... but that function there returns an 64-bit value, while you are
>> assigning the value to a 32-bit variable here, and you also only return a
>> 32-bit value from the function here. So there is no way that this could
>> accidentally be sign-extended, could it?
>>
>> Thomas
>>
>>
> I checked the Linux kernel and there too we seem to be returning unsigned 32
> bit values (readl) for 32 bit mmio reads. So I think it should be safe. Do
> you think we should remove the cast?
It looks redundant to me, so I'd vote to remove it, yes!
Thomas
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5 3/3] block/nvme: Use host PCI MMIO API
2025-04-17 17:37 [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
2025-04-17 17:37 ` [PATCH v5 1/3] util: Add functions for s390x mmio read/write Farhan Ali
2025-04-17 17:38 ` [PATCH v5 2/3] include: Add a header to define host PCI MMIO functions Farhan Ali
@ 2025-04-17 17:38 ` Farhan Ali
2025-04-25 9:22 ` Thomas Huth
2025-04-24 16:24 ` [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
3 siblings, 1 reply; 18+ messages in thread
From: Farhan Ali @ 2025-04-17 17:38 UTC (permalink / raw)
To: qemu-devel
Cc: alex.williamson, stefanha, alifm, mjrosato, schnelle, philmd,
kwolf, hreitz, thuth, fam
Use the host PCI MMIO functions to read/write
to NVMe registers, rather than directly accessing
them.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
block/nvme.c | 41 +++++++++++++++++++++++------------------
1 file changed, 23 insertions(+), 18 deletions(-)
diff --git a/block/nvme.c b/block/nvme.c
index bbf7c23dcd..8df53ee4ca 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -18,6 +18,7 @@
#include "qobject/qstring.h"
#include "qemu/defer-call.h"
#include "qemu/error-report.h"
+#include "qemu/host-pci-mmio.h"
#include "qemu/main-loop.h"
#include "qemu/module.h"
#include "qemu/cutils.h"
@@ -60,7 +61,7 @@ typedef struct {
uint8_t *queue;
uint64_t iova;
/* Hardware MMIO register */
- volatile uint32_t *doorbell;
+ uint32_t *doorbell;
} NVMeQueue;
typedef struct {
@@ -100,7 +101,7 @@ struct BDRVNVMeState {
QEMUVFIOState *vfio;
void *bar0_wo_map;
/* Memory mapped registers */
- volatile struct {
+ struct {
uint32_t sq_tail;
uint32_t cq_head;
} *doorbells;
@@ -292,7 +293,7 @@ static void nvme_kick(NVMeQueuePair *q)
assert(!(q->sq.tail & 0xFF00));
/* Fence the write to submission queue entry before notifying the device. */
smp_wmb();
- *q->sq.doorbell = cpu_to_le32(q->sq.tail);
+ host_pci_stl_le_p(q->sq.doorbell, q->sq.tail);
q->inflight += q->need_kick;
q->need_kick = 0;
}
@@ -441,7 +442,7 @@ static bool nvme_process_completion(NVMeQueuePair *q)
if (progress) {
/* Notify the device so it can post more completions. */
smp_mb_release();
- *q->cq.doorbell = cpu_to_le32(q->cq.head);
+ host_pci_stl_le_p(q->cq.doorbell, q->cq.head);
nvme_wake_free_req_locked(q);
}
@@ -460,7 +461,7 @@ static void nvme_process_completion_bh(void *opaque)
* so notify the device that it has space to fill in more completions now.
*/
smp_mb_release();
- *q->cq.doorbell = cpu_to_le32(q->cq.head);
+ host_pci_stl_le_p(q->cq.doorbell, q->cq.head);
nvme_wake_free_req_locked(q);
nvme_process_completion(q);
@@ -749,9 +750,10 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
int ret;
uint64_t cap;
uint32_t ver;
+ uint32_t cc;
uint64_t timeout_ms;
uint64_t deadline, now;
- volatile NvmeBar *regs = NULL;
+ NvmeBar *regs = NULL;
qemu_co_mutex_init(&s->dma_map_lock);
qemu_co_queue_init(&s->dma_flush_queue);
@@ -779,7 +781,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
/* Perform initialize sequence as described in NVMe spec "7.6.1
* Initialization". */
- cap = le64_to_cpu(regs->cap);
+ cap = host_pci_ldq_le_p(®s->cap);
trace_nvme_controller_capability_raw(cap);
trace_nvme_controller_capability("Maximum Queue Entries Supported",
1 + NVME_CAP_MQES(cap));
@@ -805,16 +807,17 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
bs->bl.request_alignment = s->page_size;
timeout_ms = MIN(500 * NVME_CAP_TO(cap), 30000);
- ver = le32_to_cpu(regs->vs);
+ ver = host_pci_ldl_le_p(®s->vs);
trace_nvme_controller_spec_version(extract32(ver, 16, 16),
extract32(ver, 8, 8),
extract32(ver, 0, 8));
/* Reset device to get a clean state. */
- regs->cc = cpu_to_le32(le32_to_cpu(regs->cc) & 0xFE);
+ cc = host_pci_ldl_le_p(®s->cc);
+ host_pci_stl_le_p(®s->cc, cc & 0xFE);
/* Wait for CSTS.RDY = 0. */
deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ms * SCALE_MS;
- while (NVME_CSTS_RDY(le32_to_cpu(regs->csts))) {
+ while (NVME_CSTS_RDY(host_pci_ldl_le_p(®s->csts))) {
if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
error_setg(errp, "Timeout while waiting for device to reset (%"
PRId64 " ms)",
@@ -843,19 +846,21 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
s->queues[INDEX_ADMIN] = q;
s->queue_count = 1;
QEMU_BUILD_BUG_ON((NVME_QUEUE_SIZE - 1) & 0xF000);
- regs->aqa = cpu_to_le32(((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
- ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
- regs->asq = cpu_to_le64(q->sq.iova);
- regs->acq = cpu_to_le64(q->cq.iova);
+ host_pci_stl_le_p(®s->aqa,
+ ((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
+ ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
+ host_pci_stq_le_p(®s->asq, q->sq.iova);
+ host_pci_stq_le_p(®s->acq, q->cq.iova);
/* After setting up all control registers we can enable device now. */
- regs->cc = cpu_to_le32((ctz32(NVME_CQ_ENTRY_BYTES) << CC_IOCQES_SHIFT) |
- (ctz32(NVME_SQ_ENTRY_BYTES) << CC_IOSQES_SHIFT) |
- CC_EN_MASK);
+ host_pci_stl_le_p(®s->cc,
+ (ctz32(NVME_CQ_ENTRY_BYTES) << CC_IOCQES_SHIFT) |
+ (ctz32(NVME_SQ_ENTRY_BYTES) << CC_IOSQES_SHIFT) |
+ CC_EN_MASK);
/* Wait for CSTS.RDY = 1. */
now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
deadline = now + timeout_ms * SCALE_MS;
- while (!NVME_CSTS_RDY(le32_to_cpu(regs->csts))) {
+ while (!NVME_CSTS_RDY(host_pci_ldl_le_p(®s->csts))) {
if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
error_setg(errp, "Timeout while waiting for device to start (%"
PRId64 " ms)",
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v5 3/3] block/nvme: Use host PCI MMIO API
2025-04-17 17:38 ` [PATCH v5 3/3] block/nvme: Use host PCI MMIO API Farhan Ali
@ 2025-04-25 9:22 ` Thomas Huth
0 siblings, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2025-04-25 9:22 UTC (permalink / raw)
To: Farhan Ali, qemu-devel
Cc: alex.williamson, stefanha, mjrosato, schnelle, philmd, kwolf,
hreitz, fam
On 17/04/2025 19.38, Farhan Ali wrote:
> Use the host PCI MMIO functions to read/write
> to NVMe registers, rather than directly accessing
> them.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
> block/nvme.c | 41 +++++++++++++++++++++++------------------
> 1 file changed, 23 insertions(+), 18 deletions(-)
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x
2025-04-17 17:37 [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
` (2 preceding siblings ...)
2025-04-17 17:38 ` [PATCH v5 3/3] block/nvme: Use host PCI MMIO API Farhan Ali
@ 2025-04-24 16:24 ` Farhan Ali
2025-04-25 9:24 ` Thomas Huth
2025-04-25 9:59 ` Philippe Mathieu-Daudé
3 siblings, 2 replies; 18+ messages in thread
From: Farhan Ali @ 2025-04-24 16:24 UTC (permalink / raw)
To: qemu-devel, Alex Williamson
Cc: stefanha, mjrosato, schnelle, philmd, kwolf, hreitz, thuth, fam
Hi Alex,
Polite ping. Please let me know if there are any concerns with this
version of the patches.
Thanks
Farhan
On 4/17/2025 10:37 AM, Farhan Ali wrote:
> Hi,
>
> Recently on s390x we have enabled mmap support for vfio-pci devices [1].
> This allows us to take advantage and use userspace drivers on s390x. However,
> on s390x we have special instructions for MMIO access. Starting with z15
> (and newer platforms) we have new PCI Memory I/O (MIO) instructions which
> operate on virtually mapped PCI memory spaces, and can be used from userspace.
> On older platforms we would fallback to using existing system calls for MMIO access.
>
> This patch series introduces support the PCI MIO instructions, and enables s390x
> support for the userspace NVMe driver on s390x. I would appreciate any review/feedback
> on the patches.
>
> Thanks
> Farhan
>
> [1] https://lore.kernel.org/linux-s390/20250226-vfio_pci_mmap-v7-0-c5c0f1d26efd@linux.ibm.com/
>
> ChangeLog
> ---------
> v4 series https://lore.kernel.org/qemu-devel/20250414213616.2675-1-alifm@linux.ibm.com/
> v4 -> v5
> - Fixup typo in PCI MMIO API (patch 2).
>
> v3 series https://lore.kernel.org/qemu-devel/20250401172246.2688-1-alifm@linux.ibm.com/
> v3 -> v4
> - Use generic ld/st functions for non s390x PCI access suggested by Alex (patch 2).
> - Removed R-b for patch 2 as the host PCI MMIO access API changed for non-s390x.
> Would appreciate review on this again.
>
> v2 series https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06847.html
> v2 -> v3
> - Update the PCI MMIO APIs to reflect that its PCI MMIO access on host
> as suggested by Stefan(patch 2)
> - Move s390x ifdef check to s390x_pci_mmio.h as suggested by Philippe (patch 1)
> - Add R-bs for the respective patches.
>
> v1 series https://mail.gnu.org/archive/html/qemu-devel/2025-03/msg06596.html
> v1 -> v2
> - Add 8 and 16 bit reads/writes for completeness (patch 1)
> - Introduce new QEMU PCI MMIO read/write API as suggested by Stefan (patch 2)
> - Update NVMe userspace driver to use QEMU PCI MMIO functions (patch 3)
>
>
> Farhan Ali (3):
> util: Add functions for s390x mmio read/write
> include: Add a header to define host PCI MMIO functions
> block/nvme: Use host PCI MMIO API
>
> block/nvme.c | 41 +++++-----
> include/qemu/host-pci-mmio.h | 141 ++++++++++++++++++++++++++++++++
> include/qemu/s390x_pci_mmio.h | 24 ++++++
> util/meson.build | 2 +
> util/s390x_pci_mmio.c | 148 ++++++++++++++++++++++++++++++++++
> 5 files changed, 338 insertions(+), 18 deletions(-)
> create mode 100644 include/qemu/host-pci-mmio.h
> create mode 100644 include/qemu/s390x_pci_mmio.h
> create mode 100644 util/s390x_pci_mmio.c
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x
2025-04-24 16:24 ` [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
@ 2025-04-25 9:24 ` Thomas Huth
2025-04-25 9:59 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 18+ messages in thread
From: Thomas Huth @ 2025-04-25 9:24 UTC (permalink / raw)
To: Farhan Ali, qemu-devel, Alex Williamson
Cc: stefanha, mjrosato, schnelle, philmd, kwolf, hreitz, fam
On 24/04/2025 18.24, Farhan Ali wrote:
> Hi Alex,
>
> Polite ping. Please let me know if there are any concerns with this version
> of the patches.
I assumed that Stefan would pick the patches up for the nvme/block tree, but
I can also take them through the s390x tree instead if that's preferred.
But please address/comment on the nits that I found first.
Thanks,
Thomas
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x
2025-04-24 16:24 ` [PATCH v5 0/3] Enable QEMU NVMe userspace driver on s390x Farhan Ali
2025-04-25 9:24 ` Thomas Huth
@ 2025-04-25 9:59 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-04-25 9:59 UTC (permalink / raw)
To: Farhan Ali, qemu-devel, Alex Williamson, Cédric Le Goater
Cc: stefanha, mjrosato, schnelle, kwolf, hreitz, thuth, fam
On 24/4/25 18:24, Farhan Ali wrote:
> Hi Alex,
Cc'ing Cédric as co-maintainer with Alex.
>
> Polite ping. Please let me know if there are any concerns with this
> version of the patches.
>
> Thanks
>
> Farhan
>
> On 4/17/2025 10:37 AM, Farhan Ali wrote:
>> Hi,
>>
>> Recently on s390x we have enabled mmap support for vfio-pci devices [1].
>> This allows us to take advantage and use userspace drivers on s390x.
>> However,
>> on s390x we have special instructions for MMIO access. Starting with z15
>> (and newer platforms) we have new PCI Memory I/O (MIO) instructions which
>> operate on virtually mapped PCI memory spaces, and can be used from
>> userspace.
>> On older platforms we would fallback to using existing system calls
>> for MMIO access.
>>
>> This patch series introduces support the PCI MIO instructions, and
>> enables s390x
>> support for the userspace NVMe driver on s390x. I would appreciate any
>> review/feedback
>> on the patches.
>>
>> Thanks
>> Farhan
>>
>> [1] https://lore.kernel.org/linux-s390/20250226-vfio_pci_mmap-v7-0-
>> c5c0f1d26efd@linux.ibm.com/
>>
>> ChangeLog
>> ---------
>> v4 series https://lore.kernel.org/qemu-devel/20250414213616.2675-1-
>> alifm@linux.ibm.com/
>> v4 -> v5
>> - Fixup typo in PCI MMIO API (patch 2).
>>
>> v3 series https://lore.kernel.org/qemu-devel/20250401172246.2688-1-
>> alifm@linux.ibm.com/
>> v3 -> v4
>> - Use generic ld/st functions for non s390x PCI access suggested
>> by Alex (patch 2).
>> - Removed R-b for patch 2 as the host PCI MMIO access API changed
>> for non-s390x.
>> Would appreciate review on this again.
>>
>> v2 series https://mail.gnu.org/archive/html/qemu-devel/2025-03/
>> msg06847.html
>> v2 -> v3
>> - Update the PCI MMIO APIs to reflect that its PCI MMIO access on
>> host
>> as suggested by Stefan(patch 2)
>> - Move s390x ifdef check to s390x_pci_mmio.h as suggested by
>> Philippe (patch 1)
>> - Add R-bs for the respective patches.
>>
>> v1 series https://mail.gnu.org/archive/html/qemu-devel/2025-03/
>> msg06596.html
>> v1 -> v2
>> - Add 8 and 16 bit reads/writes for completeness (patch 1)
>> - Introduce new QEMU PCI MMIO read/write API as suggested by
>> Stefan (patch 2)
>> - Update NVMe userspace driver to use QEMU PCI MMIO functions
>> (patch 3)
>>
>>
>> Farhan Ali (3):
>> util: Add functions for s390x mmio read/write
>> include: Add a header to define host PCI MMIO functions
>> block/nvme: Use host PCI MMIO API
>>
>> block/nvme.c | 41 +++++-----
>> include/qemu/host-pci-mmio.h | 141 ++++++++++++++++++++++++++++++++
>> include/qemu/s390x_pci_mmio.h | 24 ++++++
>> util/meson.build | 2 +
>> util/s390x_pci_mmio.c | 148 ++++++++++++++++++++++++++++++++++
>> 5 files changed, 338 insertions(+), 18 deletions(-)
>> create mode 100644 include/qemu/host-pci-mmio.h
>> create mode 100644 include/qemu/s390x_pci_mmio.h
>> create mode 100644 util/s390x_pci_mmio.c
>>
^ permalink raw reply [flat|nested] 18+ messages in thread