Re: [PATCH 2/5] xen/io: provide helpers for multi size MMIO accesses

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Friday, April 11th, 2025 at 3:54 AM, Roger Pau Monne <roger.pau@xxxxxxxxxx> 
wrote:

> 
> 
> Several handlers have the same necessity of reading from an MMIO region
> using 1, 2, 4 or 8 bytes accesses. So far this has been open-coded in the
> function itself. Instead provide a new handler that encapsulates the
> accesses.
> 
> Since the added helpers are not architecture specific, introduce a new
> generic io.h header.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné roger.pau@xxxxxxxxxx
> 
> ---
> xen/arch/x86/hvm/vmsi.c | 47 ++----------------------------
> xen/arch/x86/mm.c | 32 +++++----------------
> xen/drivers/vpci/msix.c | 47 ++----------------------------
> xen/include/xen/io.h | 63 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 76 insertions(+), 113 deletions(-)
> create mode 100644 xen/include/xen/io.h
> 
> diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
> index fd83abb929ec..61b89834d97d 100644
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -24,6 +24,7 @@
> * Will be merged it with virtual IOAPIC logic, since most is the same
> */
> 
> +#include <xen/io.h>
> 
> #include <xen/types.h>
> 
> #include <xen/mm.h>
> 
> #include <xen/xmalloc.h>
> 
> @@ -304,28 +305,7 @@ static void adjacent_read(
> 
> hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
> 
> - switch ( len )
> - {
> - case 1:
> - *pval = readb(hwaddr);
> - break;
> -
> - case 2:
> - *pval = readw(hwaddr);
> - break;
> -
> - case 4:
> - *pval = readl(hwaddr);
> - break;
> -
> - case 8:
> - *pval = readq(hwaddr);
> - break;
> -
> - default:
> - ASSERT_UNREACHABLE();
> - break;
> - }
> + *pval = read_mmio(hwaddr, len);
> }
> 
> static void adjacent_write(
> @@ -344,28 +324,7 @@ static void adjacent_write(
> 
> hwaddr = fix_to_virt(fixmap_idx) + PAGE_OFFSET(address);
> 
> - switch ( len )
> - {
> - case 1:
> - writeb(val, hwaddr);
> - break;
> -
> - case 2:
> - writew(val, hwaddr);
> - break;
> -
> - case 4:
> - writel(val, hwaddr);
> - break;
> -
> - case 8:
> - writeq(val, hwaddr);
> - break;
> -
> - default:
> - ASSERT_UNREACHABLE();
> - break;
> - }
> + write_mmio(hwaddr, val, len);
> }
> 
> static int cf_check msixtbl_read(
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 1cf236516789..989e62e7ce6f 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -95,6 +95,7 @@
> #include <xen/guest_access.h>
> 
> #include <xen/hypercall.h>
> 
> #include <xen/init.h>
> 
> +#include <xen/io.h>
> 
> #include <xen/iocap.h>
> 
> #include <xen/ioreq.h>
> 
> #include <xen/irq.h>
> 
> @@ -116,7 +117,6 @@
> #include <asm/flushtlb.h>
> 
> #include <asm/guest.h>
> 
> #include <asm/idt.h>
> 
> -#include <asm/io.h>
> 
> #include <asm/io_apic.h>
> 
> #include <asm/ldt.h>
> 
> #include <asm/mem_sharing.h>
> 
> @@ -5102,7 +5102,7 @@ static void __iomem *subpage_mmio_map_page(
> static void subpage_mmio_write_emulate(
> mfn_t mfn,
> unsigned int offset,
> - const void *data,
> + unsigned long data,
> unsigned int len)
> {
> struct subpage_ro_range *entry;
> @@ -5115,7 +5115,6 @@ static void subpage_mmio_write_emulate(
> 
> if ( test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) )
> 
> {
> - write_ignored:
> gprintk(XENLOG_WARNING,
> "ignoring write to R/O MMIO 0x%"PRI_mfn"%03x len %u\n",
> mfn_x(mfn), offset, len);
> @@ -5131,26 +5130,7 @@ static void subpage_mmio_write_emulate(
> return;
> }
> 
> - addr += offset;
> - switch ( len )
> - {
> - case 1:
> - writeb((const uint8_t)data, addr);
> - break;
> - case 2:
> - writew((const uint16_t)data, addr);
> - break;
> - case 4:
> - writel((const uint32_t)data, addr);
> - break;
> - case 8:
> - writeq((const uint64_t)data, addr);
> - break;
> - default:
> - /* mmio_ro_emulated_write() already validated the size */
> - ASSERT_UNREACHABLE();
> - goto write_ignored;
> - }
> + write_mmio(addr + offset, data, len);
> }
> 
> #ifdef CONFIG_HVM
> @@ -5185,18 +5165,20 @@ int cf_check mmio_ro_emulated_write(
> struct x86_emulate_ctxt *ctxt)
> {
> struct mmio_ro_emulate_ctxt *mmio_ro_ctxt = ctxt->data;
> 
> + unsigned long data = 0;
> 
> /* Only allow naturally-aligned stores at the original %cr2 address. */
> if ( ((bytes | offset) & (bytes - 1)) || !bytes ||
> - offset != mmio_ro_ctxt->cr2 )
> 
> + offset != mmio_ro_ctxt->cr2 || bytes > sizeof(data) )
> 
> {
> gdprintk(XENLOG_WARNING, "bad access (cr2=%lx, addr=%lx, bytes=%u)\n",
> mmio_ro_ctxt->cr2, offset, bytes);
> 
> return X86EMUL_UNHANDLEABLE;
> }
> 
> + memcpy(&data, p_data, bytes);
> subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset),
> 
> - p_data, bytes);
> + data, bytes);
> 
> return X86EMUL_OKAY;
> }
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 6bd8c55bb48e..6455f2a03a01 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -17,6 +17,7 @@
> * License along with this program; If not, see http://www.gnu.org/licenses/.
> 
> */
> 
> +#include <xen/io.h>
> 
> #include <xen/sched.h>
> 
> #include <xen/vpci.h>
> 
> 
> @@ -344,28 +345,7 @@ static int adjacent_read(const struct domain *d, const 
> struct vpci_msix *msix,
> return X86EMUL_OKAY;
> }
> 
> - switch ( len )
> - {
> - case 1:
> - *data = readb(mem + PAGE_OFFSET(addr));
> - break;
> -
> - case 2:
> - *data = readw(mem + PAGE_OFFSET(addr));
> - break;
> -
> - case 4:
> - *data = readl(mem + PAGE_OFFSET(addr));
> - break;
> -
> - case 8:
> - *data = readq(mem + PAGE_OFFSET(addr));
> - break;
> -
> - default:
> - ASSERT_UNREACHABLE();
> - break;
> - }
> + *data = read_mmio(mem + PAGE_OFFSET(addr), len);
> spin_unlock(&vpci->lock);
> 
> 
> return X86EMUL_OKAY;
> @@ -493,28 +473,7 @@ static int adjacent_write(const struct domain *d, const 
> struct vpci_msix *msix,
> return X86EMUL_OKAY;
> }
> 
> - switch ( len )
> - {
> - case 1:
> - writeb(data, mem + PAGE_OFFSET(addr));
> - break;
> -
> - case 2:
> - writew(data, mem + PAGE_OFFSET(addr));
> - break;
> -
> - case 4:
> - writel(data, mem + PAGE_OFFSET(addr));
> - break;
> -
> - case 8:
> - writeq(data, mem + PAGE_OFFSET(addr));
> - break;
> -
> - default:
> - ASSERT_UNREACHABLE();
> - break;
> - }
> + write_mmio(mem + PAGE_OFFSET(addr), data, len);
> spin_unlock(&vpci->lock);
> 
> 
> return X86EMUL_OKAY;
> diff --git a/xen/include/xen/io.h b/xen/include/xen/io.h
> new file mode 100644
> index 000000000000..5c360ce9dee2
> --- /dev/null
> +++ b/xen/include/xen/io.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0-only /
> +/
> + * Generic helpers for doing MMIO accesses.
> + *
> + * Copyright (c) 2025 Cloud Software Group
> + */
> +#ifndef XEN_IO_H
> +#define XEN_IO_H
> +
> +#include <xen/bug.h>
> 
> +
> +#include <asm/io.h>
> 
> +
> +static inline uint64_t read_mmio(const volatile void __iomem *mem,
> + unsigned int size)
> +{
> + switch ( size )
> + {
> + case 1:
> + return readb(mem);
> +
> + case 2:
> + return readw(mem);
> +
> + case 4:
> + return readl(mem);
> +
> + case 8:
> + return readq(mem);

Suggest adding `default` case for MISRA compliance. 

> + }
> +
> + ASSERT_UNREACHABLE();
> + return ~0UL;
> +}
> +
> +static inline void write_mmio(volatile void __iomem mem, uint64_t data,
> + unsigned int size)
> +{
> + switch ( size )
> + {
> + case 1:
> + writeb(data, mem);
> + break;
> +
> + case 2:
> + writew(data, mem);
> + break;
> +
> + case 4:
> + writel(data, mem);
> + break;
> +
> + case 8:
> + writeq(data, mem);
> + break;
> +
> + default:
> + ASSERT_UNREACHABLE();
> + break;
> + }
> +}
> +
> +#endif / XEN_IO_H */
> --
> 2.48.1