Several handlers have the same necessity of reading or writing from or to
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 set of handlers
that encapsulate 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@citrix.com>
---
Changes since v3:
 - Rebase on top of Jan's fix to mmio_ro_emulated_write().
---
 xen/arch/x86/hvm/vmsi.c | 47 ++-------------------------
 xen/arch/x86/mm.c       | 32 ++++++-------------
 xen/drivers/vpci/msix.c | 47 ++-------------------------
 xen/include/xen/io.h    | 70 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 85 insertions(+), 111 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 ac5b51d17aca..732ca1366f33 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;
@@ -5131,26 +5131,8 @@ 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();
+    if ( !write_mmio(addr + offset, data, len) )
         goto write_ignored;
-    }
 }
 
 #ifdef CONFIG_HVM
@@ -5185,6 +5167,7 @@ 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 ||
@@ -5195,9 +5178,12 @@ int cf_check mmio_ro_emulated_write(
         return X86EMUL_UNHANDLEABLE;
     }
 
-    if ( bytes <= 8 )
+    if ( bytes <= sizeof(data) )
+    {
+        memcpy(&data, p_data, bytes);
         subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset),
-                                   p_data, bytes);
+                                   data, bytes);
+    }
     else if ( subpage_mmio_find_page(mmio_ro_ctxt->mfn) )
         gprintk(XENLOG_WARNING,
                 "unsupported %u-byte write to R/O MMIO 0x%"PRI_mfn"%03lx\n",
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 3568f2a65183..74211301ba10 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>
 
@@ -338,28 +339,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;
@@ -487,28 +467,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..164a20c5d7b5
--- /dev/null
+++ b/xen/include/xen/io.h
@@ -0,0 +1,70 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * IO related routines.
+ *
+ * Copyright (c) 2025 Cloud Software Group
+ */
+#ifndef XEN_IO_H
+#define XEN_IO_H
+
+#include <xen/bug.h>
+
+#include <asm/io.h>
+
+static inline unsigned long 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);
+
+#ifdef CONFIG_64BIT
+    case 8:
+        return readq(mem);
+#endif
+
+    default:
+        ASSERT_UNREACHABLE();
+        return ~0UL;
+    }
+}
+
+static inline bool write_mmio(volatile void __iomem *mem, unsigned long data,
+                              unsigned int size)
+{
+    switch ( size )
+    {
+    case 1:
+        writeb(data, mem);
+        break;
+
+    case 2:
+        writew(data, mem);
+        break;
+
+    case 4:
+        writel(data, mem);
+        break;
+
+#ifdef CONFIG_64BIT
+    case 8:
+        writeq(data, mem);
+        break;
+#endif
+
+    default:
+        ASSERT_UNREACHABLE();
+        return false;
+    }
+
+    return true;
+}
+
+#endif /* XEN_IO_H */
-- 
2.48.1

On 29.04.2025 12:12, Roger Pau Monne wrote:
> Several handlers have the same necessity of reading or writing from or to
> 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 set of handlers
> that encapsulate 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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
preferably with ...

> @@ -5185,6 +5167,7 @@ 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;


... this moved ...

> @@ -5195,9 +5178,12 @@ int cf_check mmio_ro_emulated_write(
>          return X86EMUL_UNHANDLEABLE;
>      }
>  
> -    if ( bytes <= 8 )
> +    if ( bytes <= sizeof(data) )
> +    {
> +        memcpy(&data, p_data, bytes);
>          subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset),
> -                                   p_data, bytes);
> +                                   data, bytes);
> +    }

... into this more narrow scope.

Jan

On Tue, Apr 29, 2025 at 12:23:05PM +0200, Jan Beulich wrote:
> On 29.04.2025 12:12, Roger Pau Monne wrote:
> > Several handlers have the same necessity of reading or writing from or to
> > 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 set of handlers
> > that encapsulate 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@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> preferably with ...
> 
> > @@ -5185,6 +5167,7 @@ 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;
> 
> 
> ... this moved ...
> 
> > @@ -5195,9 +5178,12 @@ int cf_check mmio_ro_emulated_write(
> >          return X86EMUL_UNHANDLEABLE;
> >      }
> >  
> > -    if ( bytes <= 8 )
> > +    if ( bytes <= sizeof(data) )
> > +    {
> > +        memcpy(&data, p_data, bytes);
> >          subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset),
> > -                                   p_data, bytes);
> > +                                   data, bytes);
> > +    }
> 
> ... into this more narrow scope.

Hm, but if I move it I can no longer use sizeof(data) in the condition
check, that's why data is defined in the outside.

Let me know if you are OK with this.

Thanks, Roger.

On 29.04.2025 12:46, Roger Pau Monné wrote:
> On Tue, Apr 29, 2025 at 12:23:05PM +0200, Jan Beulich wrote:
>> On 29.04.2025 12:12, Roger Pau Monne wrote:
>>> Several handlers have the same necessity of reading or writing from or to
>>> 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 set of handlers
>>> that encapsulate 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@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> preferably with ...
>>
>>> @@ -5185,6 +5167,7 @@ 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;
>>
>>
>> ... this moved ...
>>
>>> @@ -5195,9 +5178,12 @@ int cf_check mmio_ro_emulated_write(
>>>          return X86EMUL_UNHANDLEABLE;
>>>      }
>>>  
>>> -    if ( bytes <= 8 )
>>> +    if ( bytes <= sizeof(data) )
>>> +    {
>>> +        memcpy(&data, p_data, bytes);
>>>          subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, PAGE_OFFSET(offset),
>>> -                                   p_data, bytes);
>>> +                                   data, bytes);
>>> +    }
>>
>> ... into this more narrow scope.
> 
> Hm, but if I move it I can no longer use sizeof(data) in the condition
> check, that's why data is defined in the outside.

Oh, I didn't pay attention to that. Yes - please keep as you have it.

Jan