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

[Thomas Huth <thuth@redhat.com>]

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