[PATCH v2 0/3] Enable QEMU NVMe userspace driver on s390x

[PATCH v2 0/3] Enable QEMU NVMe userspace driver on s390x

[Farhan Ali]

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
---------
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 PCI MMIO functions
  block/nvme: Use QEMU PCI MMIO API

 block/nvme.c                  |  37 +++++----
 include/qemu/pci-mmio.h       | 116 ++++++++++++++++++++++++++
 include/qemu/s390x_pci_mmio.h |  23 ++++++
 util/meson.build              |   2 +
 util/s390x_pci_mmio.c         | 148 ++++++++++++++++++++++++++++++++++
 5 files changed, 310 insertions(+), 16 deletions(-)
 create mode 100644 include/qemu/pci-mmio.h
 create mode 100644 include/qemu/s390x_pci_mmio.h
 create mode 100644 util/s390x_pci_mmio.c

-- 
2.43.0

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

[Farhan Ali]

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>
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 include/qemu/s390x_pci_mmio.h |  23 ++++++
 util/meson.build              |   2 +
 util/s390x_pci_mmio.c         | 148 ++++++++++++++++++++++++++++++++++
 3 files changed, 173 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..aead791475
--- /dev/null
+++ b/include/qemu/s390x_pci_mmio.h
@@ -0,0 +1,23 @@
+/*
+ * 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
+
+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
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

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

[Niklas Schnelle]

On Fri, 2025-03-28 at 12:06 -0700, 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>
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  include/qemu/s390x_pci_mmio.h |  23 ++++++
>  util/meson.build              |   2 +
>  util/s390x_pci_mmio.c         | 148 ++++++++++++++++++++++++++++++++++
>  3 files changed, 173 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..aead791475
> --- /dev/null
> +++ b/include/qemu/s390x_pci_mmio.h
> @@ -0,0 +1,23 @@
> +/*
> + * 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
> +
> +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
> 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");
> +}

I can confirm that the PCI MIO inline assembly looks good to me.
Pretty much exactly matches what I did for rdma-core a while back.

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

Thanks for the great work!

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

[PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Farhan Ali]

Add a generic QEMU API for PCI MMIO reads/writes.
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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)
 create mode 100644 include/qemu/pci-mmio.h

diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
new file mode 100644
index 0000000000..2ef92455b1
--- /dev/null
+++ b/include/qemu/pci-mmio.h
@@ -0,0 +1,116 @@
+/*
+ * QEMU PCI MMIO API
+ *
+ * Copyright 2025 IBM Corp.
+ * Author(s): Farhan Ali <alifm@linux.ibm.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QEMU_PCI_MMIO_H
+#define QEMU_PCI_MMIO_H
+
+#ifdef __s390x__
+#include "s390x_pci_mmio.h"
+#endif
+
+static inline uint8_t qemu_pci_mmio_read_8(const void *ioaddr)
+{
+    uint8_t ret = 0;
+#ifdef __s390x__
+    ret = s390x_pci_mmio_read_8(ioaddr);
+#else
+    /* Prevent the compiler from optimizing away the load */
+    ret = *((volatile uint8_t *)ioaddr);
+#endif
+
+    return ret;
+}
+
+static inline uint16_t qemu_pci_mmio_read_16(const void *ioaddr)
+{
+    uint16_t ret = 0;
+#ifdef __s390x__
+    ret = s390x_pci_mmio_read_16(ioaddr);
+#else
+    /* Prevent the compiler from optimizing away the load */
+    ret = *((volatile uint16_t *)ioaddr);
+#endif
+
+    return le16_to_cpu(ret);
+}
+
+static inline uint32_t qemu_pci_mmio_read_32(const void *ioaddr)
+{
+    uint32_t ret = 0;
+#ifdef __s390x__
+    ret = s390x_pci_mmio_read_32(ioaddr);
+#else
+    /* Prevent the compiler from optimizing away the load */
+    ret = *((volatile uint32_t *)ioaddr);
+#endif
+
+    return le32_to_cpu(ret);
+}
+
+static inline uint64_t qemu_pci_mmio_read_64(const void *ioaddr)
+{
+    uint64_t ret = 0;
+#ifdef __s390x__
+    ret = s390x_pci_mmio_read_64(ioaddr);
+#else
+    /* Prevent the compiler from optimizing away the load */
+    ret = *((volatile uint64_t *)ioaddr);
+#endif
+
+    return le64_to_cpu(ret);
+}
+
+static inline void qemu_pci_mmio_write_8(void *ioaddr, uint8_t val)
+{
+
+#ifdef __s390x__
+    s390x_pci_mmio_write_8(ioaddr, val);
+#else
+    /* Prevent the compiler from optimizing away the store */
+    *((volatile uint8_t *)ioaddr) = val;
+#endif
+}
+
+static inline void qemu_pci_mmio_write_16(void *ioaddr, uint16_t val)
+{
+    val = cpu_to_le16(val);
+
+#ifdef __s390x__
+    s390x_pci_mmio_write_16(ioaddr, val);
+#else
+    /* Prevent the compiler from optimizing away the store */
+    *((volatile uint16_t *)ioaddr) = val;
+#endif
+}
+
+static inline void qemu_pci_mmio_write_32(void *ioaddr, uint32_t val)
+{
+    val = cpu_to_le32(val);
+
+#ifdef __s390x__
+    s390x_pci_mmio_write_32(ioaddr, val);
+#else
+    /* Prevent the compiler from optimizing away the store */
+    *((volatile uint32_t *)ioaddr) = val;
+#endif
+}
+
+static inline void qemu_pci_mmio_write_64(void *ioaddr, uint64_t val)
+{
+    val = cpu_to_le64(val);
+
+#ifdef __s390x__
+    s390x_pci_mmio_write_64(ioaddr, val);
+#else
+    /* Prevent the compiler from optimizing away the store */
+    *((volatile uint64_t *)ioaddr) = val;
+#endif
+}
+
+#endif
-- 
2.43.0

Re: [PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Philippe Mathieu-Daudé]

On 28/3/25 20:06, Farhan Ali wrote:
> Add a generic QEMU API for PCI MMIO reads/writes.
> 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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 116 insertions(+)
>   create mode 100644 include/qemu/pci-mmio.h
> 
> diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
> new file mode 100644
> index 0000000000..2ef92455b1
> --- /dev/null
> +++ b/include/qemu/pci-mmio.h
> @@ -0,0 +1,116 @@
> +/*
> + * QEMU PCI MMIO API
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef QEMU_PCI_MMIO_H
> +#define QEMU_PCI_MMIO_H
> +

Missing:

#include "qemu/bswap.h"

> +#ifdef __s390x__
> +#include "s390x_pci_mmio.h"
> +#endif
> +
> +static inline uint8_t qemu_pci_mmio_read_8(const void *ioaddr)
> +{
> +    uint8_t ret = 0;
> +#ifdef __s390x__
> +    ret = s390x_pci_mmio_read_8(ioaddr);
> +#else
> +    /* Prevent the compiler from optimizing away the load */
> +    ret = *((volatile uint8_t *)ioaddr);
> +#endif
> +
> +    return ret;
> +}
> +
> +static inline uint16_t qemu_pci_mmio_read_16(const void *ioaddr)
> +{
> +    uint16_t ret = 0;
> +#ifdef __s390x__
> +    ret = s390x_pci_mmio_read_16(ioaddr);
> +#else
> +    /* Prevent the compiler from optimizing away the load */
> +    ret = *((volatile uint16_t *)ioaddr);
> +#endif
> +
> +    return le16_to_cpu(ret);
> +}

Otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Farhan Ali]

On 3/28/2025 1:38 PM, Philippe Mathieu-Daudé wrote:
> On 28/3/25 20:06, Farhan Ali wrote:
>> Add a generic QEMU API for PCI MMIO reads/writes.
>> 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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 116 insertions(+)
>>   create mode 100644 include/qemu/pci-mmio.h
>>
>> diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
>> new file mode 100644
>> index 0000000000..2ef92455b1
>> --- /dev/null
>> +++ b/include/qemu/pci-mmio.h
>> @@ -0,0 +1,116 @@
>> +/*
>> + * QEMU PCI MMIO API
>> + *
>> + * Copyright 2025 IBM Corp.
>> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef QEMU_PCI_MMIO_H
>> +#define QEMU_PCI_MMIO_H
>> +
>
> Missing:
>
> #include "qemu/bswap.h"
>
Thanks, will add this. Though I didn't have any issue compiling, but i 
think we should add this.


>> +#ifdef __s390x__
>> +#include "s390x_pci_mmio.h"
>> +#endif
>> +
>> +static inline uint8_t qemu_pci_mmio_read_8(const void *ioaddr)
>> +{
>> +    uint8_t ret = 0;
>> +#ifdef __s390x__
>> +    ret = s390x_pci_mmio_read_8(ioaddr);
>> +#else
>> +    /* Prevent the compiler from optimizing away the load */
>> +    ret = *((volatile uint8_t *)ioaddr);
>> +#endif
>> +
>> +    return ret;
>> +}
>> +
>> +static inline uint16_t qemu_pci_mmio_read_16(const void *ioaddr)
>> +{
>> +    uint16_t ret = 0;
>> +#ifdef __s390x__
>> +    ret = s390x_pci_mmio_read_16(ioaddr);
>> +#else
>> +    /* Prevent the compiler from optimizing away the load */
>> +    ret = *((volatile uint16_t *)ioaddr);
>> +#endif
>> +
>> +    return le16_to_cpu(ret);
>> +}
>
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>

Re: [PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Philippe Mathieu-Daudé]

On 28/3/25 20:06, Farhan Ali wrote:
> Add a generic QEMU API for PCI MMIO reads/writes.
> 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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 116 insertions(+)
>   create mode 100644 include/qemu/pci-mmio.h
> 
> diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
> new file mode 100644
> index 0000000000..2ef92455b1
> --- /dev/null
> +++ b/include/qemu/pci-mmio.h
> @@ -0,0 +1,116 @@
> +/*
> + * QEMU PCI MMIO API
> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef QEMU_PCI_MMIO_H
> +#define QEMU_PCI_MMIO_H
> +
> +#ifdef __s390x__
 > +#include "s390x_pci_mmio.h"

Does this ifdef belong to the header instead?
Otherwise remove?

> +#endif

Re: [PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Farhan Ali]

On 3/28/2025 1:44 PM, Philippe Mathieu-Daudé wrote:
> On 28/3/25 20:06, Farhan Ali wrote:
>> Add a generic QEMU API for PCI MMIO reads/writes.
>> 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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 116 insertions(+)
>>   create mode 100644 include/qemu/pci-mmio.h
>>
>> diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
>> new file mode 100644
>> index 0000000000..2ef92455b1
>> --- /dev/null
>> +++ b/include/qemu/pci-mmio.h
>> @@ -0,0 +1,116 @@
>> +/*
>> + * QEMU PCI MMIO API
>> + *
>> + * Copyright 2025 IBM Corp.
>> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef QEMU_PCI_MMIO_H
>> +#define QEMU_PCI_MMIO_H
>> +
>> +#ifdef __s390x__
> > +#include "s390x_pci_mmio.h"
>
> Does this ifdef belong to the header instead?
> Otherwise remove?

Just to clarify, are you suggesting to move this ifdef to the 
s390x_pci_mmio.h header file? so we can include s390x_pci_mmio.h file 
here without any ifdef?


>
>> +#endif
>

Re: [PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Philippe Mathieu-Daudé]

On 29/3/25 07:03, Farhan Ali wrote:
> 
> On 3/28/2025 1:44 PM, Philippe Mathieu-Daudé wrote:
>> On 28/3/25 20:06, Farhan Ali wrote:
>>> Add a generic QEMU API for PCI MMIO reads/writes.
>>> 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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 116 insertions(+)
>>>   create mode 100644 include/qemu/pci-mmio.h
>>>
>>> diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
>>> new file mode 100644
>>> index 0000000000..2ef92455b1
>>> --- /dev/null
>>> +++ b/include/qemu/pci-mmio.h
>>> @@ -0,0 +1,116 @@
>>> +/*
>>> + * QEMU PCI MMIO API
>>> + *
>>> + * Copyright 2025 IBM Corp.
>>> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef QEMU_PCI_MMIO_H
>>> +#define QEMU_PCI_MMIO_H
>>> +
>>> +#ifdef __s390x__
>> > +#include "s390x_pci_mmio.h"
>>
>> Does this ifdef belong to the header instead?
>> Otherwise remove?
> 
> Just to clarify, are you suggesting to move this ifdef to the 
> s390x_pci_mmio.h header file? so we can include s390x_pci_mmio.h file 
> here without any ifdef?

Exactly!

Re: [PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4133 bytes --]

On Fri, Mar 28, 2025 at 12:06:26PM -0700, Farhan Ali wrote:
> Add a generic QEMU API for PCI MMIO reads/writes.
> 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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 include/qemu/pci-mmio.h
> 
> diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
> new file mode 100644
> index 0000000000..2ef92455b1
> --- /dev/null
> +++ b/include/qemu/pci-mmio.h
> @@ -0,0 +1,116 @@
> +/*
> + * QEMU PCI MMIO API

QEMU also emulates PCI devices that handle MMIO accesses. It is easy to
get confused between host PCI MMIO accesses and emulated guest PCI MMIO
accesses if the name is just "PCI MMIO API".

Please update the commit message, filenames, function names, and doc
comments to make it clear that this is only for host PCI MMIO accesses
(e.g. Linux VFIO BAR accesses).

For example "qemu/host-pci-mmio.h", "API for host PCI MMIO accesses
(e.g. Linux VFIO BARs)", and host_pci_mmio_read_8().

> + *
> + * Copyright 2025 IBM Corp.
> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef QEMU_PCI_MMIO_H
> +#define QEMU_PCI_MMIO_H
> +
> +#ifdef __s390x__
> +#include "s390x_pci_mmio.h"
> +#endif
> +
> +static inline uint8_t qemu_pci_mmio_read_8(const void *ioaddr)
> +{
> +    uint8_t ret = 0;
> +#ifdef __s390x__
> +    ret = s390x_pci_mmio_read_8(ioaddr);
> +#else
> +    /* Prevent the compiler from optimizing away the load */
> +    ret = *((volatile uint8_t *)ioaddr);
> +#endif
> +
> +    return ret;
> +}
> +
> +static inline uint16_t qemu_pci_mmio_read_16(const void *ioaddr)
> +{
> +    uint16_t ret = 0;
> +#ifdef __s390x__
> +    ret = s390x_pci_mmio_read_16(ioaddr);
> +#else
> +    /* Prevent the compiler from optimizing away the load */
> +    ret = *((volatile uint16_t *)ioaddr);
> +#endif
> +
> +    return le16_to_cpu(ret);
> +}
> +
> +static inline uint32_t qemu_pci_mmio_read_32(const void *ioaddr)
> +{
> +    uint32_t ret = 0;
> +#ifdef __s390x__
> +    ret = s390x_pci_mmio_read_32(ioaddr);
> +#else
> +    /* Prevent the compiler from optimizing away the load */
> +    ret = *((volatile uint32_t *)ioaddr);
> +#endif
> +
> +    return le32_to_cpu(ret);
> +}
> +
> +static inline uint64_t qemu_pci_mmio_read_64(const void *ioaddr)
> +{
> +    uint64_t ret = 0;
> +#ifdef __s390x__
> +    ret = s390x_pci_mmio_read_64(ioaddr);
> +#else
> +    /* Prevent the compiler from optimizing away the load */
> +    ret = *((volatile uint64_t *)ioaddr);
> +#endif
> +
> +    return le64_to_cpu(ret);
> +}
> +
> +static inline void qemu_pci_mmio_write_8(void *ioaddr, uint8_t val)
> +{
> +
> +#ifdef __s390x__
> +    s390x_pci_mmio_write_8(ioaddr, val);
> +#else
> +    /* Prevent the compiler from optimizing away the store */
> +    *((volatile uint8_t *)ioaddr) = val;
> +#endif
> +}
> +
> +static inline void qemu_pci_mmio_write_16(void *ioaddr, uint16_t val)
> +{
> +    val = cpu_to_le16(val);
> +
> +#ifdef __s390x__
> +    s390x_pci_mmio_write_16(ioaddr, val);
> +#else
> +    /* Prevent the compiler from optimizing away the store */
> +    *((volatile uint16_t *)ioaddr) = val;
> +#endif
> +}
> +
> +static inline void qemu_pci_mmio_write_32(void *ioaddr, uint32_t val)
> +{
> +    val = cpu_to_le32(val);
> +
> +#ifdef __s390x__
> +    s390x_pci_mmio_write_32(ioaddr, val);
> +#else
> +    /* Prevent the compiler from optimizing away the store */
> +    *((volatile uint32_t *)ioaddr) = val;
> +#endif
> +}
> +
> +static inline void qemu_pci_mmio_write_64(void *ioaddr, uint64_t val)
> +{
> +    val = cpu_to_le64(val);
> +
> +#ifdef __s390x__
> +    s390x_pci_mmio_write_64(ioaddr, val);
> +#else
> +    /* Prevent the compiler from optimizing away the store */
> +    *((volatile uint64_t *)ioaddr) = val;
> +#endif
> +}
> +
> +#endif
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/3] include: Add a header to define PCI MMIO functions

[Farhan Ali]

On 3/31/2025 6:46 AM, Stefan Hajnoczi wrote:
> On Fri, Mar 28, 2025 at 12:06:26PM -0700, Farhan Ali wrote:
>> Add a generic QEMU API for PCI MMIO reads/writes.
>> 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/pci-mmio.h | 116 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 116 insertions(+)
>>   create mode 100644 include/qemu/pci-mmio.h
>>
>> diff --git a/include/qemu/pci-mmio.h b/include/qemu/pci-mmio.h
>> new file mode 100644
>> index 0000000000..2ef92455b1
>> --- /dev/null
>> +++ b/include/qemu/pci-mmio.h
>> @@ -0,0 +1,116 @@
>> +/*
>> + * QEMU PCI MMIO API
> QEMU also emulates PCI devices that handle MMIO accesses. It is easy to
> get confused between host PCI MMIO accesses and emulated guest PCI MMIO
> accesses if the name is just "PCI MMIO API".
>
> Please update the commit message, filenames, function names, and doc
> comments to make it clear that this is only for host PCI MMIO accesses
> (e.g. Linux VFIO BAR accesses).
>
> For example "qemu/host-pci-mmio.h", "API for host PCI MMIO accesses
> (e.g. Linux VFIO BARs)", and host_pci_mmio_read_8().

yeah this makes sense. Will update the API names in v3.


>> + *
>> + * Copyright 2025 IBM Corp.
>> + * Author(s): Farhan Ali <alifm@linux.ibm.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef QEMU_PCI_MMIO_H
>> +#define QEMU_PCI_MMIO_H
>> +
>> +#ifdef __s390x__
>> +#include "s390x_pci_mmio.h"
>> +#endif
>> +
>> +static inline uint8_t qemu_pci_mmio_read_8(const void *ioaddr)
>> +{
>> +    uint8_t ret = 0;
>> +#ifdef __s390x__
>> +    ret = s390x_pci_mmio_read_8(ioaddr);
>> +#else
>> +    /* Prevent the compiler from optimizing away the load */
>> +    ret = *((volatile uint8_t *)ioaddr);
>> +#endif
>> +
>> +    return ret;
>> +}
>> +
>> +static inline uint16_t qemu_pci_mmio_read_16(const void *ioaddr)
>> +{
>> +    uint16_t ret = 0;
>> +#ifdef __s390x__
>> +    ret = s390x_pci_mmio_read_16(ioaddr);
>> +#else
>> +    /* Prevent the compiler from optimizing away the load */
>> +    ret = *((volatile uint16_t *)ioaddr);
>> +#endif
>> +
>> +    return le16_to_cpu(ret);
>> +}
>> +
>> +static inline uint32_t qemu_pci_mmio_read_32(const void *ioaddr)
>> +{
>> +    uint32_t ret = 0;
>> +#ifdef __s390x__
>> +    ret = s390x_pci_mmio_read_32(ioaddr);
>> +#else
>> +    /* Prevent the compiler from optimizing away the load */
>> +    ret = *((volatile uint32_t *)ioaddr);
>> +#endif
>> +
>> +    return le32_to_cpu(ret);
>> +}
>> +
>> +static inline uint64_t qemu_pci_mmio_read_64(const void *ioaddr)
>> +{
>> +    uint64_t ret = 0;
>> +#ifdef __s390x__
>> +    ret = s390x_pci_mmio_read_64(ioaddr);
>> +#else
>> +    /* Prevent the compiler from optimizing away the load */
>> +    ret = *((volatile uint64_t *)ioaddr);
>> +#endif
>> +
>> +    return le64_to_cpu(ret);
>> +}
>> +
>> +static inline void qemu_pci_mmio_write_8(void *ioaddr, uint8_t val)
>> +{
>> +
>> +#ifdef __s390x__
>> +    s390x_pci_mmio_write_8(ioaddr, val);
>> +#else
>> +    /* Prevent the compiler from optimizing away the store */
>> +    *((volatile uint8_t *)ioaddr) = val;
>> +#endif
>> +}
>> +
>> +static inline void qemu_pci_mmio_write_16(void *ioaddr, uint16_t val)
>> +{
>> +    val = cpu_to_le16(val);
>> +
>> +#ifdef __s390x__
>> +    s390x_pci_mmio_write_16(ioaddr, val);
>> +#else
>> +    /* Prevent the compiler from optimizing away the store */
>> +    *((volatile uint16_t *)ioaddr) = val;
>> +#endif
>> +}
>> +
>> +static inline void qemu_pci_mmio_write_32(void *ioaddr, uint32_t val)
>> +{
>> +    val = cpu_to_le32(val);
>> +
>> +#ifdef __s390x__
>> +    s390x_pci_mmio_write_32(ioaddr, val);
>> +#else
>> +    /* Prevent the compiler from optimizing away the store */
>> +    *((volatile uint32_t *)ioaddr) = val;
>> +#endif
>> +}
>> +
>> +static inline void qemu_pci_mmio_write_64(void *ioaddr, uint64_t val)
>> +{
>> +    val = cpu_to_le64(val);
>> +
>> +#ifdef __s390x__
>> +    s390x_pci_mmio_write_64(ioaddr, val);
>> +#else
>> +    /* Prevent the compiler from optimizing away the store */
>> +    *((volatile uint64_t *)ioaddr) = val;
>> +#endif
>> +}
>> +
>> +#endif
>> -- 
>> 2.43.0
>>

[PATCH v2 3/3] block/nvme: Use QEMU PCI MMIO API

[Farhan Ali]

Use the QEMU PCI MMIO functions to read/write
to NVMe registers, rather than directly accessing
them.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 block/nvme.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index bbf7c23dcd..ea932609e6 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -23,6 +23,7 @@
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "qemu/memalign.h"
+#include "qemu/pci-mmio.h"
 #include "qemu/vfio-helpers.h"
 #include "block/block-io.h"
 #include "block/block_int.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);
+    qemu_pci_mmio_write_32(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);
+        qemu_pci_mmio_write_32(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);
+    qemu_pci_mmio_write_32(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 = qemu_pci_mmio_read_64(&regs->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 = qemu_pci_mmio_read_32(&regs->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 = qemu_pci_mmio_read_32(&regs->cc);
+    qemu_pci_mmio_write_32(&regs->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(qemu_pci_mmio_read_32(&regs->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);
+    qemu_pci_mmio_write_32(&regs->aqa,
+                        ((NVME_QUEUE_SIZE - 1) << AQA_ACQS_SHIFT) |
+                        ((NVME_QUEUE_SIZE - 1) << AQA_ASQS_SHIFT));
+    qemu_pci_mmio_write_64(&regs->asq, q->sq.iova);
+    qemu_pci_mmio_write_64(&regs->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) |
+    qemu_pci_mmio_write_32(&regs->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(qemu_pci_mmio_read_32(&regs->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

Re: [PATCH v2 3/3] block/nvme: Use QEMU PCI MMIO API

[Philippe Mathieu-Daudé]

On 28/3/25 20:06, Farhan Ali wrote:
> Use the QEMU PCI MMIO functions to read/write
> to NVMe registers, rather than directly accessing
> them.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>   block/nvme.c | 37 +++++++++++++++++++++----------------
>   1 file changed, 21 insertions(+), 16 deletions(-)


> @@ -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 = qemu_pci_mmio_read_32(&regs->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 = qemu_pci_mmio_read_32(&regs->cc);
> +    qemu_pci_mmio_write_32(&regs->cc, (cc & 0xFE));

Extra parenthesis not needed, otherwise:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 3/3] block/nvme: Use QEMU PCI MMIO API

[Farhan Ali]

On 3/28/2025 1:41 PM, Philippe Mathieu-Daudé wrote:
> On 28/3/25 20:06, Farhan Ali wrote:
>> Use the QEMU PCI MMIO functions to read/write
>> to NVMe registers, rather than directly accessing
>> them.
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   block/nvme.c | 37 +++++++++++++++++++++----------------
>>   1 file changed, 21 insertions(+), 16 deletions(-)
>
>
>> @@ -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 = qemu_pci_mmio_read_32(&regs->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 = qemu_pci_mmio_read_32(&regs->cc);
>> +    qemu_pci_mmio_write_32(&regs->cc, (cc & 0xFE));
>
> Extra parenthesis not needed, otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
Sure, will fix. Thanks for reviewing!

Re: [PATCH v2 3/3] block/nvme: Use QEMU PCI MMIO API

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

On Fri, Mar 28, 2025 at 12:06:27PM -0700, Farhan Ali wrote:
> Use the QEMU PCI MMIO functions to read/write
> to NVMe registers, rather than directly accessing
> them.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  block/nvme.c | 37 +++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]