[PATCH 0/5] xen/x86: fix implementation of subpage r/o MMIO

[PATCH 0/5] xen/x86: fix implementation of subpage r/o MMIO

[Roger Pau Monne]

Hello,

Following series include some bugfixes and improvements to the handling
of subpage r/o MMIO accesses.

First patch is a bugfix that applies to subpage accesses for both PV and
HVM.

Patch 2 introduces a helper to reduce code duplication when performing
MMIO accesses.

Patch 3 is a bugfix for using subpage in HVM guest, and patch 4 is an
optimization to only register the HVM subpage handler when needed.

Finally patch 5 moves some PV only code to a PV specific file.

Thanks, Roger.

Roger Pau Monne (5):
  x86/mm: account for the offset when performing subpage r/o MMIO access
  xen/io: provide helpers for multi size MMIO accesses
  x86/hvm: fix handling of accesses to partial r/o MMIO pages
  x86/hvm: only register the r/o subpage ops when needed
  x86/mm: move mmio_ro_emulated_write() to PV only file

 xen/arch/x86/hvm/emulate.c             |  47 +-----------
 xen/arch/x86/hvm/hvm.c                 |  90 +++++++++++++++++++---
 xen/arch/x86/hvm/vmsi.c                |  47 +-----------
 xen/arch/x86/include/asm/hvm/emulate.h |   1 -
 xen/arch/x86/include/asm/mm.h          |  21 ++---
 xen/arch/x86/mm.c                      | 102 ++++++-------------------
 xen/arch/x86/pv/ro-page-fault.c        |  34 +++++++++
 xen/drivers/vpci/msix.c                |  47 +-----------
 xen/include/xen/io.h                   |  63 +++++++++++++++
 9 files changed, 218 insertions(+), 234 deletions(-)
 create mode 100644 xen/include/xen/io.h

-- 
2.48.1

[PATCH 1/5] x86/mm: account for the offset when performing subpage r/o MMIO access

[Roger Pau Monne]

The current logic in subpage_mmio_write_emulate() doesn't take into account
the page offset, and always performs the writes at offset 0 (start of the
page).

Fix this by accounting for the offset before performing the write.

Fixes: 8847d6e23f97 ('x86/mm: add API for marking only part of a MMIO page read only')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/mm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 4fecd37aeca0..1cf236516789 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5131,6 +5131,7 @@ static void subpage_mmio_write_emulate(
         return;
     }
 
+    addr += offset;
     switch ( len )
     {
     case 1:
-- 
2.48.1

Re: [PATCH 1/5] x86/mm: account for the offset when performing subpage r/o MMIO access

[Andrew Cooper]

On 11/04/2025 11:54 am, Roger Pau Monne wrote:
> The current logic in subpage_mmio_write_emulate() doesn't take into account
> the page offset, and always performs the writes at offset 0 (start of the
> page).
>
> Fix this by accounting for the offset before performing the write.
>
> Fixes: 8847d6e23f97 ('x86/mm: add API for marking only part of a MMIO page read only')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Roger Pau Monne]

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@citrix.com>
---
 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);
+    }
+
+    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

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

[Denis Mukhin]

On Friday, April 11th, 2025 at 3:54 AM, Roger Pau Monne <roger.pau@citrix.com> 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@citrix.com
> 
> ---
> 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

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

[Jan Beulich]

On 11.04.2025 12:54, Roger Pau Monne 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.

Except that ...

> --- /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);

... this and ...

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

... this may (generally will) not work on 32-bit architectures. Add
CONFIG_64BIT conditionals? At which point return type / last parameter
type could move from uint64_t to unsigned long.

As to the top comment of the file: io.h is, to me at least, wider than
just dealing with MMIO accesses. IOW I fear that sentence may go stale
at some point, without anyone paying attention.

Jan

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

[Julien Grall]

Hi,

On 14/04/2025 15:07, Jan Beulich wrote:
> On 11.04.2025 12:54, Roger Pau Monne 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.
> 
> Except that ...
> 
>> --- /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);
> 
> ... this and ...
> 
>> +    }
>> +
>> +    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;
> 
> ... this may (generally will) not work on 32-bit architectures. Add
> CONFIG_64BIT conditionals? At which point return type / last parameter
> type could move from uint64_t to unsigned long.

Technically arm32 bit supports 64-bit write because we mandate LPAE. I 
see this is used by the vPCI code. Are we expecting to have any 64-bit 
access?

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

On 14.04.2025 09:49, Julien Grall wrote:
> On 14/04/2025 15:07, Jan Beulich wrote:
>> On 11.04.2025 12:54, Roger Pau Monne 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.
>>
>> Except that ...
>>
>>> --- /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);
>>
>> ... this and ...
>>
>>> +    }
>>> +
>>> +    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;
>>
>> ... this may (generally will) not work on 32-bit architectures. Add
>> CONFIG_64BIT conditionals? At which point return type / last parameter
>> type could move from uint64_t to unsigned long.
> 
> Technically arm32 bit supports 64-bit write because we mandate LPAE. I see this is used by the vPCI code. Are we expecting to have any 64-bit access?

vPCI is, I think, supposed to not see 64-bit accesses (to config space).
However, vMSI-X already may see such.

Jan

[PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Roger Pau Monne]

The current logic to handle accesses to MMIO pages partially read-only is
based on the (now removed) logic used to handle accesses to the r/o MMCFG
region(s) for PVH v1 dom0.  However that has issues when running on AMD
hardware, as in that case the guest linear address that triggered the fault
is not provided as part of the VM exit.  This caused
mmio_ro_emulated_write() to always fail before calling
subpage_mmio_write_emulate() when running on AMD and called from an HVM
context.

Take a different approach and convert the handling of partial read-only
MMIO page accesses into an HVM MMIO ops handler, as that's the more natural
way to handle this kind of emulation for HVM domains.

This allows getting rid of hvm_emulate_one_mmio() and it's single cal site
in hvm_hap_nested_page_fault().

Note a small adjustment is needed to the `pf-fixup` dom0 PVH logic: avoid
attempting to fixup faults resulting from accesses to read-only MMIO
regions, as handling of those accesses is now done by handle_mmio().

Fixes: 33c19df9a5a0 ('x86/PCI: intercept accesses to RO MMIO from dom0s in HVM containers')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
The fixes tag is maybe a bit wonky, it's either this or:

8847d6e23f97 ('x86/mm: add API for marking only part of a MMIO page read only')

However the addition of subpage r/o access handling to the existing
mmio_ro_emulated_write() function was done based on the assumption that the
current code was working - which turned out to not be the case for AMD,
hence my preference for blaming the commit that actually introduced the
broken logic.
---
 xen/arch/x86/hvm/emulate.c             | 47 +-------------
 xen/arch/x86/hvm/hvm.c                 | 89 +++++++++++++++++++++++---
 xen/arch/x86/include/asm/hvm/emulate.h |  1 -
 xen/arch/x86/include/asm/mm.h          | 12 ++++
 xen/arch/x86/mm.c                      | 37 +----------
 5 files changed, 96 insertions(+), 90 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 9fff1b82f7c6..ed888f0b49d3 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -370,7 +370,8 @@ static int hvmemul_do_io(
         /* If there is no suitable backing DM, just ignore accesses */
         if ( !s )
         {
-            if ( is_mmio && is_hardware_domain(currd) )
+            if ( is_mmio && is_hardware_domain(currd) &&
+                 !rangeset_contains_singleton(mmio_ro_ranges, PFN_DOWN(addr)) )
             {
                 /*
                  * PVH dom0 is likely missing MMIO mappings on the p2m, due to
@@ -2856,50 +2857,6 @@ int hvm_emulate_one(
     return _hvm_emulate_one(hvmemul_ctxt, &hvm_emulate_ops, completion);
 }
 
-int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
-{
-    static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = {
-        .read       = x86emul_unhandleable_rw,
-        .insn_fetch = hvmemul_insn_fetch,
-        .write      = mmio_ro_emulated_write,
-        .validate   = hvmemul_validate,
-    };
-    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla, .mfn = _mfn(mfn) };
-    struct hvm_emulate_ctxt ctxt;
-    unsigned int seg, bdf;
-    int rc;
-
-    if ( pci_ro_mmcfg_decode(mfn, &seg, &bdf) )
-    {
-        /* Should be always handled by vPCI for PVH dom0. */
-        gdprintk(XENLOG_ERR, "unhandled MMCFG access for %pp\n",
-                 &PCI_SBDF(seg, bdf));
-        ASSERT_UNREACHABLE();
-        return X86EMUL_UNHANDLEABLE;
-    }
-
-    hvm_emulate_init_once(&ctxt, x86_insn_is_mem_write,
-                          guest_cpu_user_regs());
-    ctxt.ctxt.data = &mmio_ro_ctxt;
-
-    switch ( rc = _hvm_emulate_one(&ctxt, &hvm_ro_emulate_ops_mmio,
-                                   VIO_no_completion) )
-    {
-    case X86EMUL_UNHANDLEABLE:
-    case X86EMUL_UNIMPLEMENTED:
-        hvm_dump_emulation_state(XENLOG_G_WARNING, "r/o MMIO", &ctxt, rc);
-        break;
-    case X86EMUL_EXCEPTION:
-        hvm_inject_event(&ctxt.ctxt.event);
-        /* fallthrough */
-    default:
-        hvm_emulate_writeback(&ctxt);
-        break;
-    }
-
-    return rc;
-}
-
 void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
     unsigned int errcode)
 {
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6f1174c5127e..21f005b0947c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -8,6 +8,7 @@
  */
 
 #include <xen/init.h>
+#include <xen/io.h>
 #include <xen/ioreq.h>
 #include <xen/lib.h>
 #include <xen/trace.h>
@@ -35,7 +36,6 @@
 #include <asm/current.h>
 #include <asm/debugreg.h>
 #include <asm/e820.h>
-#include <asm/io.h>
 #include <asm/regs.h>
 #include <asm/cpufeature.h>
 #include <asm/processor.h>
@@ -585,9 +585,81 @@ static int cf_check hvm_print_line(
     return X86EMUL_OKAY;
 }
 
+static int cf_check subpage_mmio_accept(struct vcpu *v, unsigned long addr)
+{
+    p2m_type_t t;
+    mfn_t mfn = get_gfn_query_unlocked(v->domain, addr, &t);
+
+    return !mfn_eq(mfn, INVALID_MFN) && t == p2m_mmio_direct &&
+           !!subpage_mmio_find_page(mfn);
+}
+
+static int cf_check subpage_mmio_read(
+    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long *data)
+{
+    struct domain *d = v->domain;
+    p2m_type_t t;
+    mfn_t mfn = get_gfn_query(d, addr, &t);
+    struct subpage_ro_range *entry;
+    volatile void __iomem *mem;
+
+    *data = ~0UL;
+
+    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
+    {
+        put_gfn(d, addr);
+        return X86EMUL_RETRY;
+    }
+
+    entry = subpage_mmio_find_page(mfn);
+    if ( !entry )
+    {
+        put_gfn(d, addr);
+        return X86EMUL_RETRY;
+    }
+
+    mem = subpage_mmio_map_page(entry);
+    if ( !mem )
+    {
+        put_gfn(d, addr);
+        gprintk(XENLOG_ERR, "Failed to map page for MMIO read at %#lx\n",
+                mfn_to_maddr(mfn) + PAGE_OFFSET(addr));
+        return X86EMUL_OKAY;
+    }
+
+    *data = read_mmio(mem + PAGE_OFFSET(addr), len);
+
+    put_gfn(d, addr);
+    return X86EMUL_OKAY;
+}
+
+static int cf_check subpage_mmio_write(
+    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
+{
+    struct domain *d = v->domain;
+    p2m_type_t t;
+    mfn_t mfn = get_gfn_query(d, addr, &t);
+
+    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
+    {
+        put_gfn(d, addr);
+        return X86EMUL_RETRY;
+    }
+
+    subpage_mmio_write_emulate(mfn, PAGE_OFFSET(addr), data, len);
+
+    put_gfn(d, addr);
+    return X86EMUL_OKAY;
+}
+
 int hvm_domain_initialise(struct domain *d,
                           const struct xen_domctl_createdomain *config)
 {
+    static const struct hvm_mmio_ops subpage_mmio_ops = {
+        .check = subpage_mmio_accept,
+        .read = subpage_mmio_read,
+        .write = subpage_mmio_write,
+    };
     unsigned int nr_gsis;
     int rc;
 
@@ -692,6 +764,9 @@ int hvm_domain_initialise(struct domain *d,
 
     register_portio_handler(d, XEN_HVM_DEBUGCONS_IOPORT, 1, hvm_print_line);
 
+    /* Handler for r/o MMIO subpage accesses. */
+    register_mmio_handler(d, &subpage_mmio_ops);
+
     if ( hvm_tsc_scaling_supported )
         d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio;
 
@@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      */
     if ( (p2mt == p2m_mmio_dm) ||
          (npfec.write_access &&
-          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
+          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
+           /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
+           (p2mt == p2m_mmio_direct))) )
     {
         if ( !handle_mmio_with_translation(gla, gfn, npfec) )
             hvm_inject_hw_exception(X86_EXC_GP, 0);
@@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
         goto out_put_gfn;
     }
 
-    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&
-         (is_hardware_domain(currd) || subpage_mmio_write_accept(mfn, gla)) &&
-         (hvm_emulate_one_mmio(mfn_x(mfn), gla) == X86EMUL_OKAY) )
-    {
-        rc = 1;
-        goto out_put_gfn;
-    }
-
     /* If we fell through, the vcpu will retry now that access restrictions have
      * been removed. It may fault again if the p2m entry type still requires so.
      * Otherwise, this is an error condition. */
diff --git a/xen/arch/x86/include/asm/hvm/emulate.h b/xen/arch/x86/include/asm/hvm/emulate.h
index c7a2d2a5be4e..178ac32e151f 100644
--- a/xen/arch/x86/include/asm/hvm/emulate.h
+++ b/xen/arch/x86/include/asm/hvm/emulate.h
@@ -86,7 +86,6 @@ void hvmemul_cancel(struct vcpu *v);
 struct segment_register *hvmemul_get_seg_reg(
     enum x86_segment seg,
     struct hvm_emulate_ctxt *hvmemul_ctxt);
-int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla);
 
 static inline bool handle_mmio(void)
 {
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index a1bc8cc27451..c2e9ef6e5023 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -554,6 +554,18 @@ int cf_check mmio_ro_emulated_write(
     enum x86_segment seg, unsigned long offset, void *p_data,
     unsigned int bytes, struct x86_emulate_ctxt *ctxt);
 
+/* r/o MMIO subpage access handlers. */
+struct subpage_ro_range {
+    struct list_head list;
+    mfn_t mfn;
+    void __iomem *mapped;
+    DECLARE_BITMAP(ro_elems, PAGE_SIZE / MMIO_RO_SUBPAGE_GRAN);
+};
+struct subpage_ro_range *subpage_mmio_find_page(mfn_t mfn);
+void __iomem *subpage_mmio_map_page(struct subpage_ro_range *entry);
+void subpage_mmio_write_emulate(
+    mfn_t mfn, unsigned int offset, unsigned long data, unsigned int len);
+
 int audit_adjust_pgtables(struct domain *d, int dir, int noisy);
 
 extern int pagefault_by_memadd(unsigned long addr, struct cpu_user_regs *regs);
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 989e62e7ce6f..f59c7816fba5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -157,13 +157,6 @@ struct rangeset *__read_mostly mmio_ro_ranges;
 static uint32_t __ro_after_init base_disallow_mask;
 
 /* Handling sub-page read-only MMIO regions */
-struct subpage_ro_range {
-    struct list_head list;
-    mfn_t mfn;
-    void __iomem *mapped;
-    DECLARE_BITMAP(ro_elems, PAGE_SIZE / MMIO_RO_SUBPAGE_GRAN);
-};
-
 static LIST_HEAD_RO_AFTER_INIT(subpage_ro_ranges);
 static DEFINE_SPINLOCK(subpage_ro_lock);
 
@@ -4929,7 +4922,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     return rc;
 }
 
-static struct subpage_ro_range *subpage_mmio_find_page(mfn_t mfn)
+struct subpage_ro_range *subpage_mmio_find_page(mfn_t mfn)
 {
     struct subpage_ro_range *entry;
 
@@ -5074,7 +5067,7 @@ int __init subpage_mmio_ro_add(
     return rc;
 }
 
-static void __iomem *subpage_mmio_map_page(
+void __iomem *subpage_mmio_map_page(
     struct subpage_ro_range *entry)
 {
     void __iomem *mapped_page;
@@ -5099,7 +5092,7 @@ static void __iomem *subpage_mmio_map_page(
     return entry->mapped;
 }
 
-static void subpage_mmio_write_emulate(
+void subpage_mmio_write_emulate(
     mfn_t mfn,
     unsigned int offset,
     unsigned long data,
@@ -5133,30 +5126,6 @@ static void subpage_mmio_write_emulate(
     write_mmio(addr + offset, data, len);
 }
 
-#ifdef CONFIG_HVM
-bool subpage_mmio_write_accept(mfn_t mfn, unsigned long gla)
-{
-    unsigned int offset = PAGE_OFFSET(gla);
-    const struct subpage_ro_range *entry;
-
-    entry = subpage_mmio_find_page(mfn);
-    if ( !entry )
-        return false;
-
-    if ( !test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) )
-    {
-        /*
-         * We don't know the write size at this point yet, so it could be
-         * an unaligned write, but accept it here anyway and deal with it
-         * later.
-         */
-        return true;
-    }
-
-    return false;
-}
-#endif
-
 int cf_check mmio_ro_emulated_write(
     enum x86_segment seg,
     unsigned long offset,
-- 
2.48.1

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich]

On 11.04.2025 12:54, Roger Pau Monne wrote:
> The current logic to handle accesses to MMIO pages partially read-only is
> based on the (now removed) logic used to handle accesses to the r/o MMCFG
> region(s) for PVH v1 dom0.  However that has issues when running on AMD
> hardware, as in that case the guest linear address that triggered the fault
> is not provided as part of the VM exit.  This caused
> mmio_ro_emulated_write() to always fail before calling
> subpage_mmio_write_emulate() when running on AMD and called from an HVM
> context.
> 
> Take a different approach and convert the handling of partial read-only
> MMIO page accesses into an HVM MMIO ops handler, as that's the more natural
> way to handle this kind of emulation for HVM domains.
> 
> This allows getting rid of hvm_emulate_one_mmio() and it's single cal site
> in hvm_hap_nested_page_fault().
> 
> Note a small adjustment is needed to the `pf-fixup` dom0 PVH logic: avoid
> attempting to fixup faults resulting from accesses to read-only MMIO
> regions, as handling of those accesses is now done by handle_mmio().
> 
> Fixes: 33c19df9a5a0 ('x86/PCI: intercept accesses to RO MMIO from dom0s in HVM containers')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> The fixes tag is maybe a bit wonky, it's either this or:
> 
> 8847d6e23f97 ('x86/mm: add API for marking only part of a MMIO page read only')
> 
> However the addition of subpage r/o access handling to the existing
> mmio_ro_emulated_write() function was done based on the assumption that the
> current code was working - which turned out to not be the case for AMD,
> hence my preference for blaming the commit that actually introduced the
> broken logic.

I agree.

> ---
>  xen/arch/x86/hvm/emulate.c             | 47 +-------------
>  xen/arch/x86/hvm/hvm.c                 | 89 +++++++++++++++++++++++---

It's quite a bit of rather special code you add to this catch-all file. How
about introducing a small new one, e.g. mmio.c (to later maybe become home
to some further stuff)?

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -370,7 +370,8 @@ static int hvmemul_do_io(
>          /* If there is no suitable backing DM, just ignore accesses */
>          if ( !s )
>          {
> -            if ( is_mmio && is_hardware_domain(currd) )
> +            if ( is_mmio && is_hardware_domain(currd) &&
> +                 !rangeset_contains_singleton(mmio_ro_ranges, PFN_DOWN(addr)) )

I think this needs a brief comment - it otherwise looks misplaced here.

> @@ -585,9 +585,81 @@ static int cf_check hvm_print_line(
>      return X86EMUL_OKAY;
>  }
>  
> +static int cf_check subpage_mmio_accept(struct vcpu *v, unsigned long addr)
> +{
> +    p2m_type_t t;
> +    mfn_t mfn = get_gfn_query_unlocked(v->domain, addr, &t);
> +
> +    return !mfn_eq(mfn, INVALID_MFN) && t == p2m_mmio_direct &&
> +           !!subpage_mmio_find_page(mfn);

The !! isn't needed here, is it?

> +}
> +
> +static int cf_check subpage_mmio_read(
> +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long *data)
> +{
> +    struct domain *d = v->domain;
> +    p2m_type_t t;
> +    mfn_t mfn = get_gfn_query(d, addr, &t);
> +    struct subpage_ro_range *entry;
> +    volatile void __iomem *mem;
> +
> +    *data = ~0UL;
> +
> +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
> +    {
> +        put_gfn(d, addr);
> +        return X86EMUL_RETRY;
> +    }
> +
> +    entry = subpage_mmio_find_page(mfn);
> +    if ( !entry )
> +    {
> +        put_gfn(d, addr);
> +        return X86EMUL_RETRY;
> +    }
> +
> +    mem = subpage_mmio_map_page(entry);
> +    if ( !mem )
> +    {
> +        put_gfn(d, addr);
> +        gprintk(XENLOG_ERR, "Failed to map page for MMIO read at %#lx\n",
> +                mfn_to_maddr(mfn) + PAGE_OFFSET(addr));

Makes me wonder whether the function parameter wouldn't better be named
"curr" and the local variable "currd", to reflect that this log message
will report appropriate context.

Would also logging the guest physical address perhaps be worthwhile here?

> +        return X86EMUL_OKAY;
> +    }
> +
> +    *data = read_mmio(mem + PAGE_OFFSET(addr), len);
> +
> +    put_gfn(d, addr);
> +    return X86EMUL_OKAY;
> +}
> +
> +static int cf_check subpage_mmio_write(
> +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
> +{
> +    struct domain *d = v->domain;
> +    p2m_type_t t;
> +    mfn_t mfn = get_gfn_query(d, addr, &t);
> +
> +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
> +    {
> +        put_gfn(d, addr);
> +        return X86EMUL_RETRY;
> +    }
> +
> +    subpage_mmio_write_emulate(mfn, PAGE_OFFSET(addr), data, len);
> +
> +    put_gfn(d, addr);
> +    return X86EMUL_OKAY;
> +}

Unlike the read path this doesn't return RETRY when subpage_mmio_find_page()
fails (indicating something must have changed after "accept" said yes).

> @@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>       */
>      if ( (p2mt == p2m_mmio_dm) ||
>           (npfec.write_access &&
> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
> +           /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
> +           (p2mt == p2m_mmio_direct))) )
>      {
>          if ( !handle_mmio_with_translation(gla, gfn, npfec) )
>              hvm_inject_hw_exception(X86_EXC_GP, 0);

Aren't we handing too many things to handle_mmio_with_translation() this
way? At the very least you're losing ...

> @@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>          goto out_put_gfn;
>      }
>  
> -    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&

... the .present check.

I'm also concerned of e.g. VT-x'es APIC access MFN, which is
p2m_mmio_direct.

Jan

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Roger Pau Monné]

On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
> On 11.04.2025 12:54, Roger Pau Monne wrote:
> > The current logic to handle accesses to MMIO pages partially read-only is
> > based on the (now removed) logic used to handle accesses to the r/o MMCFG
> > region(s) for PVH v1 dom0.  However that has issues when running on AMD
> > hardware, as in that case the guest linear address that triggered the fault
> > is not provided as part of the VM exit.  This caused
> > mmio_ro_emulated_write() to always fail before calling
> > subpage_mmio_write_emulate() when running on AMD and called from an HVM
> > context.
> > 
> > Take a different approach and convert the handling of partial read-only
> > MMIO page accesses into an HVM MMIO ops handler, as that's the more natural
> > way to handle this kind of emulation for HVM domains.
> > 
> > This allows getting rid of hvm_emulate_one_mmio() and it's single cal site
> > in hvm_hap_nested_page_fault().
> > 
> > Note a small adjustment is needed to the `pf-fixup` dom0 PVH logic: avoid
> > attempting to fixup faults resulting from accesses to read-only MMIO
> > regions, as handling of those accesses is now done by handle_mmio().
> > 
> > Fixes: 33c19df9a5a0 ('x86/PCI: intercept accesses to RO MMIO from dom0s in HVM containers')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > The fixes tag is maybe a bit wonky, it's either this or:
> > 
> > 8847d6e23f97 ('x86/mm: add API for marking only part of a MMIO page read only')
> > 
> > However the addition of subpage r/o access handling to the existing
> > mmio_ro_emulated_write() function was done based on the assumption that the
> > current code was working - which turned out to not be the case for AMD,
> > hence my preference for blaming the commit that actually introduced the
> > broken logic.
> 
> I agree.
> 
> > ---
> >  xen/arch/x86/hvm/emulate.c             | 47 +-------------
> >  xen/arch/x86/hvm/hvm.c                 | 89 +++++++++++++++++++++++---
> 
> It's quite a bit of rather special code you add to this catch-all file. How
> about introducing a small new one, e.g. mmio.c (to later maybe become home
> to some further stuff)?

Yes, I didn't find any suitable place, I was also considering
hvm/io.c or hvm/intercept.c, but none looked very good TBH.

The main benefit of placing it in hvm.c is that the functions can all
be static and confined to hvm.c as it's only user.

> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -370,7 +370,8 @@ static int hvmemul_do_io(
> >          /* If there is no suitable backing DM, just ignore accesses */
> >          if ( !s )
> >          {
> > -            if ( is_mmio && is_hardware_domain(currd) )
> > +            if ( is_mmio && is_hardware_domain(currd) &&
> > +                 !rangeset_contains_singleton(mmio_ro_ranges, PFN_DOWN(addr)) )
> 
> I think this needs a brief comment - it otherwise looks misplaced here.
> 
> > @@ -585,9 +585,81 @@ static int cf_check hvm_print_line(
> >      return X86EMUL_OKAY;
> >  }
> >  
> > +static int cf_check subpage_mmio_accept(struct vcpu *v, unsigned long addr)
> > +{
> > +    p2m_type_t t;
> > +    mfn_t mfn = get_gfn_query_unlocked(v->domain, addr, &t);
> > +
> > +    return !mfn_eq(mfn, INVALID_MFN) && t == p2m_mmio_direct &&
> > +           !!subpage_mmio_find_page(mfn);
> 
> The !! isn't needed here, is it?

IIRC clang complained about conversion from pointer to integer without
a cast, but maybe that was before adding the further conditions here.

> > +}
> > +
> > +static int cf_check subpage_mmio_read(
> > +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long *data)
> > +{
> > +    struct domain *d = v->domain;
> > +    p2m_type_t t;
> > +    mfn_t mfn = get_gfn_query(d, addr, &t);
> > +    struct subpage_ro_range *entry;
> > +    volatile void __iomem *mem;
> > +
> > +    *data = ~0UL;
> > +
> > +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
> > +    {
> > +        put_gfn(d, addr);
> > +        return X86EMUL_RETRY;
> > +    }
> > +
> > +    entry = subpage_mmio_find_page(mfn);
> > +    if ( !entry )
> > +    {
> > +        put_gfn(d, addr);
> > +        return X86EMUL_RETRY;
> > +    }
> > +
> > +    mem = subpage_mmio_map_page(entry);
> > +    if ( !mem )
> > +    {
> > +        put_gfn(d, addr);
> > +        gprintk(XENLOG_ERR, "Failed to map page for MMIO read at %#lx\n",
> > +                mfn_to_maddr(mfn) + PAGE_OFFSET(addr));
> 
> Makes me wonder whether the function parameter wouldn't better be named
> "curr" and the local variable "currd", to reflect that this log message
> will report appropriate context.

Sure, can adjust.

> Would also logging the guest physical address perhaps be worthwhile here?

Possibly, my first apporahc was to print gfn -> mfn, but ended up
copying the same message from  subpage_mmio_write_emulate() for
symmetry reasons.

> > +        return X86EMUL_OKAY;
> > +    }
> > +
> > +    *data = read_mmio(mem + PAGE_OFFSET(addr), len);
> > +
> > +    put_gfn(d, addr);
> > +    return X86EMUL_OKAY;
> > +}
> > +
> > +static int cf_check subpage_mmio_write(
> > +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
> > +{
> > +    struct domain *d = v->domain;
> > +    p2m_type_t t;
> > +    mfn_t mfn = get_gfn_query(d, addr, &t);
> > +
> > +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
> > +    {
> > +        put_gfn(d, addr);
> > +        return X86EMUL_RETRY;
> > +    }
> > +
> > +    subpage_mmio_write_emulate(mfn, PAGE_OFFSET(addr), data, len);
> > +
> > +    put_gfn(d, addr);
> > +    return X86EMUL_OKAY;
> > +}
> 
> Unlike the read path this doesn't return RETRY when subpage_mmio_find_page()
> fails (indicating something must have changed after "accept" said yes).

Yeah, I've noticed this, but didn't feel like modifying
subpage_mmio_write_emulate() for this.  The list of partial r/o MMIO
pages cannot change after system boot, so accept returning yes and not
finding a page here would likely warrant an ASSERT_UNRECHABLE().

Would you like me to modify subpage_mmio_write_emulate to make it
return an error code?

> > @@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> >       */
> >      if ( (p2mt == p2m_mmio_dm) ||
> >           (npfec.write_access &&
> > -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> > +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
> > +           /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
> > +           (p2mt == p2m_mmio_direct))) )
> >      {
> >          if ( !handle_mmio_with_translation(gla, gfn, npfec) )
> >              hvm_inject_hw_exception(X86_EXC_GP, 0);
> 
> Aren't we handing too many things to handle_mmio_with_translation() this
> way? At the very least you're losing ...
> 
> > @@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> >          goto out_put_gfn;
> >      }
> >  
> > -    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&
> 
> ... the .present check.

Isn't the p2mt == p2m_mmio_direct check already ensuring the entry is
present?  Otherwise it's type would be p2m_invalid or p2m_mmio_dm?

It did seem to me the other checks in this function already assume
that by having a valid type the entry is present.

> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
> p2m_mmio_direct.

But that won't go into hvm_hap_nested_page_fault() when using
cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
mapped as p2m_mmio_direct)?

It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
differently?

Thanks, Roger.

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich]

On 14.04.2025 15:53, Roger Pau Monné wrote:
> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
>> On 11.04.2025 12:54, Roger Pau Monne wrote:
>>> ---
>>>  xen/arch/x86/hvm/emulate.c             | 47 +-------------
>>>  xen/arch/x86/hvm/hvm.c                 | 89 +++++++++++++++++++++++---
>>
>> It's quite a bit of rather special code you add to this catch-all file. How
>> about introducing a small new one, e.g. mmio.c (to later maybe become home
>> to some further stuff)?
> 
> Yes, I didn't find any suitable place, I was also considering
> hvm/io.c or hvm/intercept.c, but none looked very good TBH.
> 
> The main benefit of placing it in hvm.c is that the functions can all
> be static and confined to hvm.c as it's only user.

I understand that. Still, if we went by that criteria, we'd best put all of
our code in a single file ;-)

>>> +static int cf_check subpage_mmio_write(
>>> +    struct vcpu *v, unsigned long addr, unsigned int len, unsigned long data)
>>> +{
>>> +    struct domain *d = v->domain;
>>> +    p2m_type_t t;
>>> +    mfn_t mfn = get_gfn_query(d, addr, &t);
>>> +
>>> +    if ( mfn_eq(mfn, INVALID_MFN) || t != p2m_mmio_direct )
>>> +    {
>>> +        put_gfn(d, addr);
>>> +        return X86EMUL_RETRY;
>>> +    }
>>> +
>>> +    subpage_mmio_write_emulate(mfn, PAGE_OFFSET(addr), data, len);
>>> +
>>> +    put_gfn(d, addr);
>>> +    return X86EMUL_OKAY;
>>> +}
>>
>> Unlike the read path this doesn't return RETRY when subpage_mmio_find_page()
>> fails (indicating something must have changed after "accept" said yes).
> 
> Yeah, I've noticed this, but didn't feel like modifying
> subpage_mmio_write_emulate() for this.  The list of partial r/o MMIO
> pages cannot change after system boot, so accept returning yes and not
> finding a page here would likely warrant an ASSERT_UNRECHABLE().
> 
> Would you like me to modify subpage_mmio_write_emulate to make it
> return an error code?

I'd be happy with the two paths being in sync in whichever way that's
achieved. The argument you give equally holds for the read path, after
all.

>>> @@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>>>       */
>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>           (npfec.write_access &&
>>> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
>>> +           /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
>>> +           (p2mt == p2m_mmio_direct))) )
>>>      {
>>>          if ( !handle_mmio_with_translation(gla, gfn, npfec) )
>>>              hvm_inject_hw_exception(X86_EXC_GP, 0);
>>
>> Aren't we handing too many things to handle_mmio_with_translation() this
>> way? At the very least you're losing ...
>>
>>> @@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>>>          goto out_put_gfn;
>>>      }
>>>  
>>> -    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&
>>
>> ... the .present check.
> 
> Isn't the p2mt == p2m_mmio_direct check already ensuring the entry is
> present?  Otherwise it's type would be p2m_invalid or p2m_mmio_dm?

Yes (to the 1st question), it kind of is.

> It did seem to me the other checks in this function already assume
> that by having a valid type the entry is present.

Except for the code above, where we decided to play safe. AT the very least
if you drop such a check, please say a justifying word in the description.

>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
>> p2m_mmio_direct.
> 
> But that won't go into hvm_hap_nested_page_fault() when using
> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
> mapped as p2m_mmio_direct)?
> 
> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
> differently?

All true as long as things work as expected (potentially including the guest
also behaving as expected). Also this was explicitly only an example I could
readily think of. I'm simply wary of handle_mmio_with_translation() now
getting things to handle it's not meant to ever see.

Jan

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Roger Pau Monné]

On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
> On 14.04.2025 15:53, Roger Pau Monné wrote:
> > On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
> >> On 11.04.2025 12:54, Roger Pau Monne wrote:
> >>> @@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> >>>       */
> >>>      if ( (p2mt == p2m_mmio_dm) ||
> >>>           (npfec.write_access &&
> >>> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> >>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
> >>> +           /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
> >>> +           (p2mt == p2m_mmio_direct))) )
> >>>      {
> >>>          if ( !handle_mmio_with_translation(gla, gfn, npfec) )
> >>>              hvm_inject_hw_exception(X86_EXC_GP, 0);
> >>
> >> Aren't we handing too many things to handle_mmio_with_translation() this
> >> way? At the very least you're losing ...
> >>
> >>> @@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> >>>          goto out_put_gfn;
> >>>      }
> >>>  
> >>> -    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&
> >>
> >> ... the .present check.
> > 
> > Isn't the p2mt == p2m_mmio_direct check already ensuring the entry is
> > present?  Otherwise it's type would be p2m_invalid or p2m_mmio_dm?
> 
> Yes (to the 1st question), it kind of is.
> 
> > It did seem to me the other checks in this function already assume
> > that by having a valid type the entry is present.
> 
> Except for the code above, where we decided to play safe. AT the very least
> if you drop such a check, please say a justifying word in the description.

I've added:

"As part of the fix r/o MMIO accesses are now handled by
handle_mmio_with_translation(), re-using the same logic that was used
for other read-only types part of p2m_is_discard_write().  The page
present check is dropped as type p2m_mmio_direct must have the
present bit set in the PTE."

Let me know if you think that's enough.

> >> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
> >> p2m_mmio_direct.
> > 
> > But that won't go into hvm_hap_nested_page_fault() when using
> > cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
> > mapped as p2m_mmio_direct)?
> > 
> > It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
> > differently?
> 
> All true as long as things work as expected (potentially including the guest
> also behaving as expected). Also this was explicitly only an example I could
> readily think of. I'm simply wary of handle_mmio_with_translation() now
> getting things to handle it's not meant to ever see.

How was access to MMIO r/o regions supposed to be handled before
33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
(~2010), yet I can't figure out how writes would be handled back then
that didn't result in a p2m fault and crashing of the domain.

I'm happy to look at other ways to handling this, but given there's
current logic for handling accesses to read-only regions in
hvm_hap_nested_page_fault() I think re-using that was the best way to
also handle accesses to MMIO read-only regions.

Arguably it would already be the case that for other reasons Xen would
need to emulate an instruction that accesses a read-only MMIO region?

Thanks, Roger.

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich]

On 14.04.2025 18:13, Roger Pau Monné wrote:
> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
>> On 14.04.2025 15:53, Roger Pau Monné wrote:
>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
>>>> On 11.04.2025 12:54, Roger Pau Monne wrote:
>>>>> @@ -1981,7 +2056,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>>>>>       */
>>>>>      if ( (p2mt == p2m_mmio_dm) ||
>>>>>           (npfec.write_access &&
>>>>> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
>>>>> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
>>>>> +           /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
>>>>> +           (p2mt == p2m_mmio_direct))) )
>>>>>      {
>>>>>          if ( !handle_mmio_with_translation(gla, gfn, npfec) )
>>>>>              hvm_inject_hw_exception(X86_EXC_GP, 0);
>>>>
>>>> Aren't we handing too many things to handle_mmio_with_translation() this
>>>> way? At the very least you're losing ...
>>>>
>>>>> @@ -2033,14 +2110,6 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>>>>>          goto out_put_gfn;
>>>>>      }
>>>>>  
>>>>> -    if ( (p2mt == p2m_mmio_direct) && npfec.write_access && npfec.present &&
>>>>
>>>> ... the .present check.
>>>
>>> Isn't the p2mt == p2m_mmio_direct check already ensuring the entry is
>>> present?  Otherwise it's type would be p2m_invalid or p2m_mmio_dm?
>>
>> Yes (to the 1st question), it kind of is.
>>
>>> It did seem to me the other checks in this function already assume
>>> that by having a valid type the entry is present.
>>
>> Except for the code above, where we decided to play safe. AT the very least
>> if you drop such a check, please say a justifying word in the description.
> 
> I've added:
> 
> "As part of the fix r/o MMIO accesses are now handled by
> handle_mmio_with_translation(), re-using the same logic that was used
> for other read-only types part of p2m_is_discard_write().  The page
> present check is dropped as type p2m_mmio_direct must have the
> present bit set in the PTE."
> 
> Let me know if you think that's enough.

That's fine; it's even more verbose than I was hoping for.

Independently (i.e. not for this patch) we may want to actually assert
that npfec.present is set for P2M types where we demand that to always
be the case. (I wouldn't be too surprised if we actually found such an
assertion to trigger.)

>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
>>>> p2m_mmio_direct.
>>>
>>> But that won't go into hvm_hap_nested_page_fault() when using
>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
>>> mapped as p2m_mmio_direct)?
>>>
>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
>>> differently?
>>
>> All true as long as things work as expected (potentially including the guest
>> also behaving as expected). Also this was explicitly only an example I could
>> readily think of. I'm simply wary of handle_mmio_with_translation() now
>> getting things to handle it's not meant to ever see.
> 
> How was access to MMIO r/o regions supposed to be handled before
> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
> (~2010), yet I can't figure out how writes would be handled back then
> that didn't result in a p2m fault and crashing of the domain.

Was that handled at all before said change? mmio_ro_do_page_fault() was
(and still is) invoked for the hardware domain only, and quite likely
the need for handling (discarding) writes for PVHv1 had been overlooked
until someone was hit by the lack thereof.

> I'm happy to look at other ways to handling this, but given there's
> current logic for handling accesses to read-only regions in
> hvm_hap_nested_page_fault() I think re-using that was the best way to
> also handle accesses to MMIO read-only regions.
> 
> Arguably it would already be the case that for other reasons Xen would
> need to emulate an instruction that accesses a read-only MMIO region?

Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
p2m_mmio_direct (after all, "direct" means we expect no emulation is
needed; while arguably wrong for the introspection case, I'm not sure
that and pass-through actually go together). Hence it's down to
hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
which means that if hvm_io_intercept() can't handle it, the access
will be forwarded to the responsible DM, or be "processed" by the
internal null handler.

Given this, perhaps what you do is actually fine. At the same time
note how several functions in hvm/emulate.c simply fail upon
encountering p2m_mmio_direct. These are all REP handlers though, so
the main emulator would then try emulating the insn the non-REP way.

Jan

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Roger Pau Monné]

On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
> On 14.04.2025 18:13, Roger Pau Monné wrote:
> > On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
> >> On 14.04.2025 15:53, Roger Pau Monné wrote:
> >>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
> >>>> On 11.04.2025 12:54, Roger Pau Monne wrote:
> Independently (i.e. not for this patch) we may want to actually assert
> that npfec.present is set for P2M types where we demand that to always
> be the case. (I wouldn't be too surprised if we actually found such an
> assertion to trigger.)
> 
> >>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
> >>>> p2m_mmio_direct.
> >>>
> >>> But that won't go into hvm_hap_nested_page_fault() when using
> >>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
> >>> mapped as p2m_mmio_direct)?
> >>>
> >>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
> >>> differently?
> >>
> >> All true as long as things work as expected (potentially including the guest
> >> also behaving as expected). Also this was explicitly only an example I could
> >> readily think of. I'm simply wary of handle_mmio_with_translation() now
> >> getting things to handle it's not meant to ever see.
> > 
> > How was access to MMIO r/o regions supposed to be handled before
> > 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
> > added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
> > (~2010), yet I can't figure out how writes would be handled back then
> > that didn't result in a p2m fault and crashing of the domain.
> 
> Was that handled at all before said change?

Not really AFAICT, hence me wondering how where write accesses to r/o
MMIO regions supposed to be handled by (non-priv) domains.  Was the
expectation that those writes trigger an p2m violation thus crashing
the domain?

> mmio_ro_do_page_fault() was
> (and still is) invoked for the hardware domain only, and quite likely
> the need for handling (discarding) writes for PVHv1 had been overlooked
> until someone was hit by the lack thereof.

I see, I didn't realize r/o MMIO was only handled for PV hardware
domains only.  I could arguably do the same for HVM in
hvm_hap_nested_page_fault().

Not sure whether the subpage stuff is supposed to be functional for
domains different than the hardware domain?  It seems to be available
to the hanrdware domain only for PV guests, while for HVM is available
for both PV and HVM domains:

is_hardware_domain(currd) || subpage_mmio_write_accept(mfn, gla)

In hvm_hap_nested_page_fault().

> > I'm happy to look at other ways to handling this, but given there's
> > current logic for handling accesses to read-only regions in
> > hvm_hap_nested_page_fault() I think re-using that was the best way to
> > also handle accesses to MMIO read-only regions.
> > 
> > Arguably it would already be the case that for other reasons Xen would
> > need to emulate an instruction that accesses a read-only MMIO region?
> 
> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
> p2m_mmio_direct (after all, "direct" means we expect no emulation is
> needed; while arguably wrong for the introspection case, I'm not sure
> that and pass-through actually go together). Hence it's down to
> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
> which means that if hvm_io_intercept() can't handle it, the access
> will be forwarded to the responsible DM, or be "processed" by the
> internal null handler.
> 
> Given this, perhaps what you do is actually fine. At the same time
> note how several functions in hvm/emulate.c simply fail upon
> encountering p2m_mmio_direct. These are all REP handlers though, so
> the main emulator would then try emulating the insn the non-REP way.

I'm open to alternative ways of handling such accesses, just used what
seemed more natural in the context of hvm_hap_nested_page_fault().

Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
perspective, that would "just" result in the guest getting a #GP
injected.

Would you like me to add some of your reasoning above to the commit
message?

Thanks, Roger.

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich]

On 15.04.2025 10:34, Roger Pau Monné wrote:
> On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
>> On 14.04.2025 18:13, Roger Pau Monné wrote:
>>> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
>>>> On 14.04.2025 15:53, Roger Pau Monné wrote:
>>>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
>>>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
>>>>>> p2m_mmio_direct.
>>>>>
>>>>> But that won't go into hvm_hap_nested_page_fault() when using
>>>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
>>>>> mapped as p2m_mmio_direct)?
>>>>>
>>>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
>>>>> differently?
>>>>
>>>> All true as long as things work as expected (potentially including the guest
>>>> also behaving as expected). Also this was explicitly only an example I could
>>>> readily think of. I'm simply wary of handle_mmio_with_translation() now
>>>> getting things to handle it's not meant to ever see.
>>>
>>> How was access to MMIO r/o regions supposed to be handled before
>>> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
>>> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
>>> (~2010), yet I can't figure out how writes would be handled back then
>>> that didn't result in a p2m fault and crashing of the domain.
>>
>> Was that handled at all before said change?
> 
> Not really AFAICT, hence me wondering how where write accesses to r/o
> MMIO regions supposed to be handled by (non-priv) domains.  Was the
> expectation that those writes trigger an p2m violation thus crashing
> the domain?

I think so, yes. Devices with such special areas weren't (aren't?) supposed
to be handed to DomU-s.

>> mmio_ro_do_page_fault() was
>> (and still is) invoked for the hardware domain only, and quite likely
>> the need for handling (discarding) writes for PVHv1 had been overlooked
>> until someone was hit by the lack thereof.
> 
> I see, I didn't realize r/o MMIO was only handled for PV hardware
> domains only.  I could arguably do the same for HVM in
> hvm_hap_nested_page_fault().
> 
> Not sure whether the subpage stuff is supposed to be functional for
> domains different than the hardware domain?  It seems to be available
> to the hanrdware domain only for PV guests, while for HVM is available
> for both PV and HVM domains:

DYM Dom0 and DomU here?

> is_hardware_domain(currd) || subpage_mmio_write_accept(mfn, gla)
> 
> In hvm_hap_nested_page_fault().

See the three XHCI_SHARE_* modes. When it's XHCI_SHARE_ANY, even DomU-s
would require this handling. It looks like a mistake that we permit the
path to be taken for DomU-s even when the mode is XHCI_SHARE_HWDOM. It
also looks like a mistake that the PV path has remained Dom0-only, even
in the XHCI_SHARE_ANY case. Cc-ing Marek once again ...

>>> I'm happy to look at other ways to handling this, but given there's
>>> current logic for handling accesses to read-only regions in
>>> hvm_hap_nested_page_fault() I think re-using that was the best way to
>>> also handle accesses to MMIO read-only regions.
>>>
>>> Arguably it would already be the case that for other reasons Xen would
>>> need to emulate an instruction that accesses a read-only MMIO region?
>>
>> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
>> p2m_mmio_direct (after all, "direct" means we expect no emulation is
>> needed; while arguably wrong for the introspection case, I'm not sure
>> that and pass-through actually go together). Hence it's down to
>> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
>> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
>> which means that if hvm_io_intercept() can't handle it, the access
>> will be forwarded to the responsible DM, or be "processed" by the
>> internal null handler.
>>
>> Given this, perhaps what you do is actually fine. At the same time
>> note how several functions in hvm/emulate.c simply fail upon
>> encountering p2m_mmio_direct. These are all REP handlers though, so
>> the main emulator would then try emulating the insn the non-REP way.
> 
> I'm open to alternative ways of handling such accesses, just used what
> seemed more natural in the context of hvm_hap_nested_page_fault().
> 
> Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
> perspective, that would "just" result in the guest getting a #GP
> injected.

That's not the part I'm worried about. What worries me is that we open up
another (or better: we're widening a) way to hit the emulator in the first
place. (Plus, as said, the issue with the not really tidy P2M type system.)

> Would you like me to add some of your reasoning above to the commit
> message?

While I'd still be a little hesitant as to ack-ing of the result, I think
that's all I'm really asking for, yes.

Jan

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Roger Pau Monné]

On Tue, Apr 15, 2025 at 11:41:27AM +0200, Jan Beulich wrote:
> On 15.04.2025 10:34, Roger Pau Monné wrote:
> > On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
> >> On 14.04.2025 18:13, Roger Pau Monné wrote:
> >>> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
> >>>> On 14.04.2025 15:53, Roger Pau Monné wrote:
> >>>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
> >>>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
> >>>>>> p2m_mmio_direct.
> >>>>>
> >>>>> But that won't go into hvm_hap_nested_page_fault() when using
> >>>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
> >>>>> mapped as p2m_mmio_direct)?
> >>>>>
> >>>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
> >>>>> differently?
> >>>>
> >>>> All true as long as things work as expected (potentially including the guest
> >>>> also behaving as expected). Also this was explicitly only an example I could
> >>>> readily think of. I'm simply wary of handle_mmio_with_translation() now
> >>>> getting things to handle it's not meant to ever see.
> >>>
> >>> How was access to MMIO r/o regions supposed to be handled before
> >>> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
> >>> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
> >>> (~2010), yet I can't figure out how writes would be handled back then
> >>> that didn't result in a p2m fault and crashing of the domain.
> >>
> >> Was that handled at all before said change?
> > 
> > Not really AFAICT, hence me wondering how where write accesses to r/o
> > MMIO regions supposed to be handled by (non-priv) domains.  Was the
> > expectation that those writes trigger an p2m violation thus crashing
> > the domain?
> 
> I think so, yes. Devices with such special areas weren't (aren't?) supposed
> to be handed to DomU-s.

Oh, I see.  That makes stuff a bit clearer.  I think we would then
also want to add some checks to {ept_}p2m_type_to_flags()?

I wonder why handling of mmio_ro_ranges was added to the HVM p2m code
in ~2010 then.  If mmio_ro_ranges is only supposed to be relevant for
the hardware domain in ~2010 an HVM dom0 was not even in sight?

Sorry to ask so many questions, I'm a bit confused about how this
was/is supposed to work.

> >> mmio_ro_do_page_fault() was
> >> (and still is) invoked for the hardware domain only, and quite likely
> >> the need for handling (discarding) writes for PVHv1 had been overlooked
> >> until someone was hit by the lack thereof.
> > 
> > I see, I didn't realize r/o MMIO was only handled for PV hardware
> > domains only.  I could arguably do the same for HVM in
> > hvm_hap_nested_page_fault().
> > 
> > Not sure whether the subpage stuff is supposed to be functional for
> > domains different than the hardware domain?  It seems to be available
> > to the hanrdware domain only for PV guests, while for HVM is available
> > for both PV and HVM domains:
> 
> DYM Dom0 and DomU here?

Indeed, sorry.

> > is_hardware_domain(currd) || subpage_mmio_write_accept(mfn, gla)
> > 
> > In hvm_hap_nested_page_fault().
> 
> See the three XHCI_SHARE_* modes. When it's XHCI_SHARE_ANY, even DomU-s
> would require this handling. It looks like a mistake that we permit the
> path to be taken for DomU-s even when the mode is XHCI_SHARE_HWDOM.

Arguable a domU will never get the device assigned in the first place
unless the share mode is set to XHCI_SHARE_ANY.  For the other modes
the device is hidden, and hence couldn't be assigned to a domU anyway.

> It
> also looks like a mistake that the PV path has remained Dom0-only, even
> in the XHCI_SHARE_ANY case. Cc-ing Marek once again ...
> 
> >>> I'm happy to look at other ways to handling this, but given there's
> >>> current logic for handling accesses to read-only regions in
> >>> hvm_hap_nested_page_fault() I think re-using that was the best way to
> >>> also handle accesses to MMIO read-only regions.
> >>>
> >>> Arguably it would already be the case that for other reasons Xen would
> >>> need to emulate an instruction that accesses a read-only MMIO region?
> >>
> >> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
> >> p2m_mmio_direct (after all, "direct" means we expect no emulation is
> >> needed; while arguably wrong for the introspection case, I'm not sure
> >> that and pass-through actually go together). Hence it's down to
> >> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
> >> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
> >> which means that if hvm_io_intercept() can't handle it, the access
> >> will be forwarded to the responsible DM, or be "processed" by the
> >> internal null handler.
> >>
> >> Given this, perhaps what you do is actually fine. At the same time
> >> note how several functions in hvm/emulate.c simply fail upon
> >> encountering p2m_mmio_direct. These are all REP handlers though, so
> >> the main emulator would then try emulating the insn the non-REP way.
> > 
> > I'm open to alternative ways of handling such accesses, just used what
> > seemed more natural in the context of hvm_hap_nested_page_fault().
> > 
> > Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
> > perspective, that would "just" result in the guest getting a #GP
> > injected.
> 
> That's not the part I'm worried about. What worries me is that we open up
> another (or better: we're widening a) way to hit the emulator in the first
> place. (Plus, as said, the issue with the not really tidy P2M type system.)

But the hit would be limited to domains having r/o p2m_mmio_direct
entries in the p2m, as otherwise the path would be unreachable?

> > Would you like me to add some of your reasoning above to the commit
> > message?
> 
> While I'd still be a little hesitant as to ack-ing of the result, I think
> that's all I'm really asking for, yes.

As said before - I'm happy to consider suggestions here, I don't want
to fix this with yet another bodge that will cause us further issues
down the road.  What I proposed seemed like the most natural way to
handle those accesses IMO, but I'm not an expert on the emulator.

Thanks, Roger.

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich]

On 15.04.2025 12:04, Roger Pau Monné wrote:
> On Tue, Apr 15, 2025 at 11:41:27AM +0200, Jan Beulich wrote:
>> On 15.04.2025 10:34, Roger Pau Monné wrote:
>>> On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
>>>> On 14.04.2025 18:13, Roger Pau Monné wrote:
>>>>> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
>>>>>> On 14.04.2025 15:53, Roger Pau Monné wrote:
>>>>>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
>>>>>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
>>>>>>>> p2m_mmio_direct.
>>>>>>>
>>>>>>> But that won't go into hvm_hap_nested_page_fault() when using
>>>>>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
>>>>>>> mapped as p2m_mmio_direct)?
>>>>>>>
>>>>>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
>>>>>>> differently?
>>>>>>
>>>>>> All true as long as things work as expected (potentially including the guest
>>>>>> also behaving as expected). Also this was explicitly only an example I could
>>>>>> readily think of. I'm simply wary of handle_mmio_with_translation() now
>>>>>> getting things to handle it's not meant to ever see.
>>>>>
>>>>> How was access to MMIO r/o regions supposed to be handled before
>>>>> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
>>>>> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
>>>>> (~2010), yet I can't figure out how writes would be handled back then
>>>>> that didn't result in a p2m fault and crashing of the domain.
>>>>
>>>> Was that handled at all before said change?
>>>
>>> Not really AFAICT, hence me wondering how where write accesses to r/o
>>> MMIO regions supposed to be handled by (non-priv) domains.  Was the
>>> expectation that those writes trigger an p2m violation thus crashing
>>> the domain?
>>
>> I think so, yes. Devices with such special areas weren't (aren't?) supposed
>> to be handed to DomU-s.
> 
> Oh, I see.  That makes stuff a bit clearer.  I think we would then
> also want to add some checks to {ept_}p2m_type_to_flags()?
> 
> I wonder why handling of mmio_ro_ranges was added to the HVM p2m code
> in ~2010 then.  If mmio_ro_ranges is only supposed to be relevant for
> the hardware domain in ~2010 an HVM dom0 was not even in sight?

I fear because I was wrong with what I said in the earlier reply: There's
one exception - the MSI-X tables of devices. DomU-s (and even Dom0) aren't
supposed to access them directly, but we'd permit reads (which, at least
back at the time, were also required to keep qemu working).

> Sorry to ask so many questions, I'm a bit confused about how this
> was/is supposed to work.

No worries - as you can see, I'm not getting it quite straight either.

>>>> mmio_ro_do_page_fault() was
>>>> (and still is) invoked for the hardware domain only, and quite likely
>>>> the need for handling (discarding) writes for PVHv1 had been overlooked
>>>> until someone was hit by the lack thereof.
>>>
>>> I see, I didn't realize r/o MMIO was only handled for PV hardware
>>> domains only.  I could arguably do the same for HVM in
>>> hvm_hap_nested_page_fault().
>>>
>>> Not sure whether the subpage stuff is supposed to be functional for
>>> domains different than the hardware domain?  It seems to be available
>>> to the hanrdware domain only for PV guests, while for HVM is available
>>> for both PV and HVM domains:
>>
>> DYM Dom0 and DomU here?
> 
> Indeed, sorry.
> 
>>> is_hardware_domain(currd) || subpage_mmio_write_accept(mfn, gla)
>>>
>>> In hvm_hap_nested_page_fault().
>>
>> See the three XHCI_SHARE_* modes. When it's XHCI_SHARE_ANY, even DomU-s
>> would require this handling. It looks like a mistake that we permit the
>> path to be taken for DomU-s even when the mode is XHCI_SHARE_HWDOM.
> 
> Arguable a domU will never get the device assigned in the first place
> unless the share mode is set to XHCI_SHARE_ANY.  For the other modes
> the device is hidden, and hence couldn't be assigned to a domU anyway.

Correct. Yet then we permit a code path to be taken which is supposedly
unnecessary, but potentially (if something went wrong) harmful.

>>>>> I'm happy to look at other ways to handling this, but given there's
>>>>> current logic for handling accesses to read-only regions in
>>>>> hvm_hap_nested_page_fault() I think re-using that was the best way to
>>>>> also handle accesses to MMIO read-only regions.
>>>>>
>>>>> Arguably it would already be the case that for other reasons Xen would
>>>>> need to emulate an instruction that accesses a read-only MMIO region?
>>>>
>>>> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
>>>> p2m_mmio_direct (after all, "direct" means we expect no emulation is
>>>> needed; while arguably wrong for the introspection case, I'm not sure
>>>> that and pass-through actually go together). Hence it's down to
>>>> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
>>>> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
>>>> which means that if hvm_io_intercept() can't handle it, the access
>>>> will be forwarded to the responsible DM, or be "processed" by the
>>>> internal null handler.
>>>>
>>>> Given this, perhaps what you do is actually fine. At the same time
>>>> note how several functions in hvm/emulate.c simply fail upon
>>>> encountering p2m_mmio_direct. These are all REP handlers though, so
>>>> the main emulator would then try emulating the insn the non-REP way.
>>>
>>> I'm open to alternative ways of handling such accesses, just used what
>>> seemed more natural in the context of hvm_hap_nested_page_fault().
>>>
>>> Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
>>> perspective, that would "just" result in the guest getting a #GP
>>> injected.
>>
>> That's not the part I'm worried about. What worries me is that we open up
>> another (or better: we're widening a) way to hit the emulator in the first
>> place. (Plus, as said, the issue with the not really tidy P2M type system.)
> 
> But the hit would be limited to domains having r/o p2m_mmio_direct
> entries in the p2m, as otherwise the path would be unreachable?

I fear I don't follow - all you look for in the newly extended conditional
is the type being p2m_mmio_direct. There's no r/o-ness being checked for
until we'd make it through the emulator and into subpage_mmio_accept().

Jan

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Marek Marczykowski]

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

On Tue, Apr 15, 2025 at 12:18:04PM +0200, Jan Beulich wrote:
> On 15.04.2025 12:04, Roger Pau Monné wrote:
> > On Tue, Apr 15, 2025 at 11:41:27AM +0200, Jan Beulich wrote:
> >> On 15.04.2025 10:34, Roger Pau Monné wrote:
> >>> On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
> >>>> On 14.04.2025 18:13, Roger Pau Monné wrote:
> >>>>> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
> >>>>>> On 14.04.2025 15:53, Roger Pau Monné wrote:
> >>>>>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
> >>>>>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
> >>>>>>>> p2m_mmio_direct.
> >>>>>>>
> >>>>>>> But that won't go into hvm_hap_nested_page_fault() when using
> >>>>>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
> >>>>>>> mapped as p2m_mmio_direct)?
> >>>>>>>
> >>>>>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
> >>>>>>> differently?
> >>>>>>
> >>>>>> All true as long as things work as expected (potentially including the guest
> >>>>>> also behaving as expected). Also this was explicitly only an example I could
> >>>>>> readily think of. I'm simply wary of handle_mmio_with_translation() now
> >>>>>> getting things to handle it's not meant to ever see.
> >>>>>
> >>>>> How was access to MMIO r/o regions supposed to be handled before
> >>>>> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
> >>>>> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
> >>>>> (~2010), yet I can't figure out how writes would be handled back then
> >>>>> that didn't result in a p2m fault and crashing of the domain.
> >>>>
> >>>> Was that handled at all before said change?
> >>>
> >>> Not really AFAICT, hence me wondering how where write accesses to r/o
> >>> MMIO regions supposed to be handled by (non-priv) domains.  Was the
> >>> expectation that those writes trigger an p2m violation thus crashing
> >>> the domain?
> >>
> >> I think so, yes. Devices with such special areas weren't (aren't?) supposed
> >> to be handed to DomU-s.
> > 
> > Oh, I see.  That makes stuff a bit clearer.  I think we would then
> > also want to add some checks to {ept_}p2m_type_to_flags()?
> > 
> > I wonder why handling of mmio_ro_ranges was added to the HVM p2m code
> > in ~2010 then.  If mmio_ro_ranges is only supposed to be relevant for
> > the hardware domain in ~2010 an HVM dom0 was not even in sight?
> 
> I fear because I was wrong with what I said in the earlier reply: There's
> one exception - the MSI-X tables of devices. DomU-s (and even Dom0) aren't
> supposed to access them directly, but we'd permit reads (which, at least
> back at the time, were also required to keep qemu working).

And there is also a case where some devices have other registers on the
same page as MSI-X tables. But this case is handled specially in the
MSI-X code, not via sub-page R/O API.

> > Sorry to ask so many questions, I'm a bit confused about how this
> > was/is supposed to work.
> 
> No worries - as you can see, I'm not getting it quite straight either.
> 
> >>>> mmio_ro_do_page_fault() was
> >>>> (and still is) invoked for the hardware domain only, and quite likely
> >>>> the need for handling (discarding) writes for PVHv1 had been overlooked
> >>>> until someone was hit by the lack thereof.
> >>>
> >>> I see, I didn't realize r/o MMIO was only handled for PV hardware
> >>> domains only.  I could arguably do the same for HVM in
> >>> hvm_hap_nested_page_fault().
> >>>
> >>> Not sure whether the subpage stuff is supposed to be functional for
> >>> domains different than the hardware domain?  It seems to be available
> >>> to the hanrdware domain only for PV guests, while for HVM is available
> >>> for both PV and HVM domains:
> >>
> >> DYM Dom0 and DomU here?
> > 
> > Indeed, sorry.

I'm not sure about the PV case and domU. I think I tested it at some
iteration, but it isn't configuration that I care much about. If it
doesn't work (and fixing it would make it even more complex), IMO we can
simply adjust documentation of XHCI_SHARE_ANY to say it works only with
HVM domU.

The domU case exists mostly (only?) to enable automated testing. I do a
lot of that on laptops, which have only a single USB controller (no way
to plug any extra one), and I need that USB controller in a domU for
several tests. In fact, the XHCI console is a debugging feature in the
first place. So, the domU part doesn't need security support, can
require extra hoops to jump through etc.

> >>> is_hardware_domain(currd) || subpage_mmio_write_accept(mfn, gla)
> >>>
> >>> In hvm_hap_nested_page_fault().
> >>
> >> See the three XHCI_SHARE_* modes. When it's XHCI_SHARE_ANY, even DomU-s
> >> would require this handling. It looks like a mistake that we permit the
> >> path to be taken for DomU-s even when the mode is XHCI_SHARE_HWDOM.
> > 
> > Arguable a domU will never get the device assigned in the first place
> > unless the share mode is set to XHCI_SHARE_ANY.  For the other modes
> > the device is hidden, and hence couldn't be assigned to a domU anyway.
> 
> Correct. Yet then we permit a code path to be taken which is supposedly
> unnecessary, but potentially (if something went wrong) harmful.

Since the XHCI_SHARE_ANY case is rare (and not security-supported),
maybe there should be a global variable guarding this part? It would be
set to true only if XHCI_SHARE_ANY is used (or some future use of this
subpage-ro API with a domU). Then, that code would still be potentially
reachable for all domUs (if XHCI_SHARE_ANY is used), but that's still
better?
Anyway, I'm still not sure what the concern is. What is the (not purely
theoretical) case where domU gains access to the emulator, where without
this feature it wouldn't have it already? Any HVM can hit the emulator
already, regardless of this feature, no?

> >>>>> I'm happy to look at other ways to handling this, but given there's
> >>>>> current logic for handling accesses to read-only regions in
> >>>>> hvm_hap_nested_page_fault() I think re-using that was the best way to
> >>>>> also handle accesses to MMIO read-only regions.
> >>>>>
> >>>>> Arguably it would already be the case that for other reasons Xen would
> >>>>> need to emulate an instruction that accesses a read-only MMIO region?
> >>>>
> >>>> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
> >>>> p2m_mmio_direct (after all, "direct" means we expect no emulation is
> >>>> needed; while arguably wrong for the introspection case, I'm not sure
> >>>> that and pass-through actually go together). Hence it's down to
> >>>> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
> >>>> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
> >>>> which means that if hvm_io_intercept() can't handle it, the access
> >>>> will be forwarded to the responsible DM, or be "processed" by the
> >>>> internal null handler.
> >>>>
> >>>> Given this, perhaps what you do is actually fine. At the same time
> >>>> note how several functions in hvm/emulate.c simply fail upon
> >>>> encountering p2m_mmio_direct. These are all REP handlers though, so
> >>>> the main emulator would then try emulating the insn the non-REP way.
> >>>
> >>> I'm open to alternative ways of handling such accesses, just used what
> >>> seemed more natural in the context of hvm_hap_nested_page_fault().
> >>>
> >>> Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
> >>> perspective, that would "just" result in the guest getting a #GP
> >>> injected.
> >>
> >> That's not the part I'm worried about. What worries me is that we open up
> >> another (or better: we're widening a) way to hit the emulator in the first
> >> place. (Plus, as said, the issue with the not really tidy P2M type system.)
> > 
> > But the hit would be limited to domains having r/o p2m_mmio_direct
> > entries in the p2m, as otherwise the path would be unreachable?
> 
> I fear I don't follow - all you look for in the newly extended conditional
> is the type being p2m_mmio_direct. There's no r/o-ness being checked for
> until we'd make it through the emulator and into subpage_mmio_accept().

But EPT violation can be hit on p2m_mmio_direct page only if it's a
write and the page is read-only, no? Is there any other case that exists
today?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

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

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich]

On 15.04.2025 12:40, Marek Marczykowski wrote:
> On Tue, Apr 15, 2025 at 12:18:04PM +0200, Jan Beulich wrote:
>> On 15.04.2025 12:04, Roger Pau Monné wrote:
>>> On Tue, Apr 15, 2025 at 11:41:27AM +0200, Jan Beulich wrote:
>>>> On 15.04.2025 10:34, Roger Pau Monné wrote:
>>>>> Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
>>>>> perspective, that would "just" result in the guest getting a #GP
>>>>> injected.
>>>>
>>>> That's not the part I'm worried about. What worries me is that we open up
>>>> another (or better: we're widening a) way to hit the emulator in the first
>>>> place. (Plus, as said, the issue with the not really tidy P2M type system.)
>>>
>>> But the hit would be limited to domains having r/o p2m_mmio_direct
>>> entries in the p2m, as otherwise the path would be unreachable?
>>
>> I fear I don't follow - all you look for in the newly extended conditional
>> is the type being p2m_mmio_direct. There's no r/o-ness being checked for
>> until we'd make it through the emulator and into subpage_mmio_accept().
> 
> But EPT violation can be hit on p2m_mmio_direct page only if it's a
> write and the page is read-only, no? Is there any other case that exists
> today?

Today and if everything works as it should - yes, I think so.

Jan

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Roger Pau Monné]

On Tue, Apr 15, 2025 at 12:18:04PM +0200, Jan Beulich wrote:
> On 15.04.2025 12:04, Roger Pau Monné wrote:
> > On Tue, Apr 15, 2025 at 11:41:27AM +0200, Jan Beulich wrote:
> >> On 15.04.2025 10:34, Roger Pau Monné wrote:
> >>> On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
> >>>> On 14.04.2025 18:13, Roger Pau Monné wrote:
> >>>>> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
> >>>>>> On 14.04.2025 15:53, Roger Pau Monné wrote:
> >>>>>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
> >>>>>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
> >>>>>>>> p2m_mmio_direct.
> >>>>>>>
> >>>>>>> But that won't go into hvm_hap_nested_page_fault() when using
> >>>>>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
> >>>>>>> mapped as p2m_mmio_direct)?
> >>>>>>>
> >>>>>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
> >>>>>>> differently?
> >>>>>>
> >>>>>> All true as long as things work as expected (potentially including the guest
> >>>>>> also behaving as expected). Also this was explicitly only an example I could
> >>>>>> readily think of. I'm simply wary of handle_mmio_with_translation() now
> >>>>>> getting things to handle it's not meant to ever see.
> >>>>>
> >>>>> How was access to MMIO r/o regions supposed to be handled before
> >>>>> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
> >>>>> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
> >>>>> (~2010), yet I can't figure out how writes would be handled back then
> >>>>> that didn't result in a p2m fault and crashing of the domain.
> >>>>
> >>>> Was that handled at all before said change?
> >>>
> >>> Not really AFAICT, hence me wondering how where write accesses to r/o
> >>> MMIO regions supposed to be handled by (non-priv) domains.  Was the
> >>> expectation that those writes trigger an p2m violation thus crashing
> >>> the domain?
> >>
> >> I think so, yes. Devices with such special areas weren't (aren't?) supposed
> >> to be handed to DomU-s.
> > 
> > Oh, I see.  That makes stuff a bit clearer.  I think we would then
> > also want to add some checks to {ept_}p2m_type_to_flags()?
> > 
> > I wonder why handling of mmio_ro_ranges was added to the HVM p2m code
> > in ~2010 then.  If mmio_ro_ranges is only supposed to be relevant for
> > the hardware domain in ~2010 an HVM dom0 was not even in sight?
> 
> I fear because I was wrong with what I said in the earlier reply: There's
> one exception - the MSI-X tables of devices. DomU-s (and even Dom0) aren't
> supposed to access them directly, but we'd permit reads (which, at least
> back at the time, were also required to keep qemu working).

Hm, but reads to the MSI-X table for HVM domains will go through QEMU,
and hence not hit the r/o MMIO path, because the MSI-X table will
never be mapped to an HVM guest p2m?

Reads from QEMU are indeed different, but those where always made from
a PV domain.  As said above - HVM guests never got to see the native
MSI-X table at all.

> >>>>> I'm happy to look at other ways to handling this, but given there's
> >>>>> current logic for handling accesses to read-only regions in
> >>>>> hvm_hap_nested_page_fault() I think re-using that was the best way to
> >>>>> also handle accesses to MMIO read-only regions.
> >>>>>
> >>>>> Arguably it would already be the case that for other reasons Xen would
> >>>>> need to emulate an instruction that accesses a read-only MMIO region?
> >>>>
> >>>> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
> >>>> p2m_mmio_direct (after all, "direct" means we expect no emulation is
> >>>> needed; while arguably wrong for the introspection case, I'm not sure
> >>>> that and pass-through actually go together). Hence it's down to
> >>>> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
> >>>> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
> >>>> which means that if hvm_io_intercept() can't handle it, the access
> >>>> will be forwarded to the responsible DM, or be "processed" by the
> >>>> internal null handler.
> >>>>
> >>>> Given this, perhaps what you do is actually fine. At the same time
> >>>> note how several functions in hvm/emulate.c simply fail upon
> >>>> encountering p2m_mmio_direct. These are all REP handlers though, so
> >>>> the main emulator would then try emulating the insn the non-REP way.
> >>>
> >>> I'm open to alternative ways of handling such accesses, just used what
> >>> seemed more natural in the context of hvm_hap_nested_page_fault().
> >>>
> >>> Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
> >>> perspective, that would "just" result in the guest getting a #GP
> >>> injected.
> >>
> >> That's not the part I'm worried about. What worries me is that we open up
> >> another (or better: we're widening a) way to hit the emulator in the first
> >> place. (Plus, as said, the issue with the not really tidy P2M type system.)
> > 
> > But the hit would be limited to domains having r/o p2m_mmio_direct
> > entries in the p2m, as otherwise the path would be unreachable?
> 
> I fear I don't follow - all you look for in the newly extended conditional
> is the type being p2m_mmio_direct. There's no r/o-ness being checked for
> until we'd make it through the emulator and into subpage_mmio_accept().

Well, it's a write page-fault of a type with p2m_mmio_direct.  What
about limiting the path even further by checking for mmio_ro_ranges:

    if ( (p2mt == p2m_mmio_dm) ||
         (npfec.write_access &&
          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
           /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
           (p2mt == p2m_mmio_direct &&
            rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn))))) )
    {

Thanks, Roger.

Re: [PATCH 3/5] x86/hvm: fix handling of accesses to partial r/o MMIO pages

[Jan Beulich]

On 15.04.2025 14:47, Roger Pau Monné wrote:
> On Tue, Apr 15, 2025 at 12:18:04PM +0200, Jan Beulich wrote:
>> On 15.04.2025 12:04, Roger Pau Monné wrote:
>>> On Tue, Apr 15, 2025 at 11:41:27AM +0200, Jan Beulich wrote:
>>>> On 15.04.2025 10:34, Roger Pau Monné wrote:
>>>>> On Tue, Apr 15, 2025 at 09:32:37AM +0200, Jan Beulich wrote:
>>>>>> On 14.04.2025 18:13, Roger Pau Monné wrote:
>>>>>>> On Mon, Apr 14, 2025 at 05:24:32PM +0200, Jan Beulich wrote:
>>>>>>>> On 14.04.2025 15:53, Roger Pau Monné wrote:
>>>>>>>>> On Mon, Apr 14, 2025 at 08:33:44AM +0200, Jan Beulich wrote:
>>>>>>>>>> I'm also concerned of e.g. VT-x'es APIC access MFN, which is
>>>>>>>>>> p2m_mmio_direct.
>>>>>>>>>
>>>>>>>>> But that won't go into hvm_hap_nested_page_fault() when using
>>>>>>>>> cpu_has_vmx_virtualize_apic_accesses (and thus having an APIC page
>>>>>>>>> mapped as p2m_mmio_direct)?
>>>>>>>>>
>>>>>>>>> It would instead be an EXIT_REASON_APIC_ACCESS vmexit which is handled
>>>>>>>>> differently?
>>>>>>>>
>>>>>>>> All true as long as things work as expected (potentially including the guest
>>>>>>>> also behaving as expected). Also this was explicitly only an example I could
>>>>>>>> readily think of. I'm simply wary of handle_mmio_with_translation() now
>>>>>>>> getting things to handle it's not meant to ever see.
>>>>>>>
>>>>>>> How was access to MMIO r/o regions supposed to be handled before
>>>>>>> 33c19df9a5a0 (~2015)?  I see that setting r/o MMIO p2m entries was
>>>>>>> added way before to p2m_type_to_flags() and ept_p2m_type_to_flags()
>>>>>>> (~2010), yet I can't figure out how writes would be handled back then
>>>>>>> that didn't result in a p2m fault and crashing of the domain.
>>>>>>
>>>>>> Was that handled at all before said change?
>>>>>
>>>>> Not really AFAICT, hence me wondering how where write accesses to r/o
>>>>> MMIO regions supposed to be handled by (non-priv) domains.  Was the
>>>>> expectation that those writes trigger an p2m violation thus crashing
>>>>> the domain?
>>>>
>>>> I think so, yes. Devices with such special areas weren't (aren't?) supposed
>>>> to be handed to DomU-s.
>>>
>>> Oh, I see.  That makes stuff a bit clearer.  I think we would then
>>> also want to add some checks to {ept_}p2m_type_to_flags()?
>>>
>>> I wonder why handling of mmio_ro_ranges was added to the HVM p2m code
>>> in ~2010 then.  If mmio_ro_ranges is only supposed to be relevant for
>>> the hardware domain in ~2010 an HVM dom0 was not even in sight?
>>
>> I fear because I was wrong with what I said in the earlier reply: There's
>> one exception - the MSI-X tables of devices. DomU-s (and even Dom0) aren't
>> supposed to access them directly, but we'd permit reads (which, at least
>> back at the time, were also required to keep qemu working).
> 
> Hm, but reads to the MSI-X table for HVM domains will go through QEMU,
> and hence not hit the r/o MMIO path, because the MSI-X table will
> never be mapped to an HVM guest p2m?

Right, just wanted to be on the safe side there.

>>>>>>> I'm happy to look at other ways to handling this, but given there's
>>>>>>> current logic for handling accesses to read-only regions in
>>>>>>> hvm_hap_nested_page_fault() I think re-using that was the best way to
>>>>>>> also handle accesses to MMIO read-only regions.
>>>>>>>
>>>>>>> Arguably it would already be the case that for other reasons Xen would
>>>>>>> need to emulate an instruction that accesses a read-only MMIO region?
>>>>>>
>>>>>> Aiui hvm_translate_get_page() will yield HVMTRANS_bad_gfn_to_mfn for
>>>>>> p2m_mmio_direct (after all, "direct" means we expect no emulation is
>>>>>> needed; while arguably wrong for the introspection case, I'm not sure
>>>>>> that and pass-through actually go together). Hence it's down to
>>>>>> hvmemul_linear_mmio_access() -> hvmemul_phys_mmio_access() ->
>>>>>> hvmemul_do_mmio_buffer() -> hvmemul_do_io_buffer() -> hvmemul_do_io(),
>>>>>> which means that if hvm_io_intercept() can't handle it, the access
>>>>>> will be forwarded to the responsible DM, or be "processed" by the
>>>>>> internal null handler.
>>>>>>
>>>>>> Given this, perhaps what you do is actually fine. At the same time
>>>>>> note how several functions in hvm/emulate.c simply fail upon
>>>>>> encountering p2m_mmio_direct. These are all REP handlers though, so
>>>>>> the main emulator would then try emulating the insn the non-REP way.
>>>>>
>>>>> I'm open to alternative ways of handling such accesses, just used what
>>>>> seemed more natural in the context of hvm_hap_nested_page_fault().
>>>>>
>>>>> Emulation of r/o MMIO accesses failing wouldn't be an issue from Xen's
>>>>> perspective, that would "just" result in the guest getting a #GP
>>>>> injected.
>>>>
>>>> That's not the part I'm worried about. What worries me is that we open up
>>>> another (or better: we're widening a) way to hit the emulator in the first
>>>> place. (Plus, as said, the issue with the not really tidy P2M type system.)
>>>
>>> But the hit would be limited to domains having r/o p2m_mmio_direct
>>> entries in the p2m, as otherwise the path would be unreachable?
>>
>> I fear I don't follow - all you look for in the newly extended conditional
>> is the type being p2m_mmio_direct. There's no r/o-ness being checked for
>> until we'd make it through the emulator and into subpage_mmio_accept().
> 
> Well, it's a write page-fault of a type with p2m_mmio_direct.  What
> about limiting the path even further by checking for mmio_ro_ranges:
> 
>     if ( (p2mt == p2m_mmio_dm) ||
>          (npfec.write_access &&
>           (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
>            /* MMIO entries can be r/o if the target mfn is in mmio_ro_ranges. */
>            (p2mt == p2m_mmio_direct &&
>             rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn))))) )
>     {

Yes, this would make me quite a bit less nervous.

Jan

[PATCH 4/5] x86/hvm: only register the r/o subpage ops when needed

[Roger Pau Monne]

MMIO operation handlers can be expensive to process, hence attempt to
register only those that will be needed by the domain.

Subpage r/o MMIO regions are added exclusively at boot, further limit their
addition to strictly before the initial domain gets created, so by the time
initial domain creation happens Xen knows whether subpage is required or
not.  This allows only registering the MMIO handler when there are
subpage regions to handle.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Could possibly be part of the previous patch, but strictly speaking is an
improvement, as even before the previous patch subpage r/o was always
called even when no subpage regions are registered.
---
 xen/arch/x86/hvm/hvm.c        |  5 +++--
 xen/arch/x86/include/asm/mm.h |  1 +
 xen/arch/x86/mm.c             | 16 ++++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 21f005b0947c..1a5dfc07813d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -764,8 +764,9 @@ int hvm_domain_initialise(struct domain *d,
 
     register_portio_handler(d, XEN_HVM_DEBUGCONS_IOPORT, 1, hvm_print_line);
 
-    /* Handler for r/o MMIO subpage accesses. */
-    register_mmio_handler(d, &subpage_mmio_ops);
+    if ( subpage_ro_active() )
+        /* Handler for r/o MMIO subpage accesses. */
+        register_mmio_handler(d, &subpage_mmio_ops);
 
     if ( hvm_tsc_scaling_supported )
         d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio;
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index c2e9ef6e5023..aeb8ebcf2d56 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -561,6 +561,7 @@ struct subpage_ro_range {
     void __iomem *mapped;
     DECLARE_BITMAP(ro_elems, PAGE_SIZE / MMIO_RO_SUBPAGE_GRAN);
 };
+bool subpage_ro_active(void);
 struct subpage_ro_range *subpage_mmio_find_page(mfn_t mfn);
 void __iomem *subpage_mmio_map_page(struct subpage_ro_range *entry);
 void subpage_mmio_write_emulate(
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f59c7816fba5..3bc6304d831c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4922,6 +4922,11 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     return rc;
 }
 
+bool subpage_ro_active(void)
+{
+    return !list_empty(&subpage_ro_ranges);
+}
+
 struct subpage_ro_range *subpage_mmio_find_page(mfn_t mfn)
 {
     struct subpage_ro_range *entry;
@@ -5011,6 +5016,17 @@ int __init subpage_mmio_ro_add(
          !IS_ALIGNED(size, MMIO_RO_SUBPAGE_GRAN) )
         return -EINVAL;
 
+    /*
+     * Force all r/o subregions to be registered before initial domain
+     * creation, so that the emulation handlers can be added only when there
+     * are pages registered.
+     */
+    if ( system_state >= SYS_STATE_smp_boot )
+    {
+        ASSERT_UNREACHABLE();
+        return -EILSEQ;
+    }
+
     if ( !size )
         return 0;
 
-- 
2.48.1

Re: [PATCH 4/5] x86/hvm: only register the r/o subpage ops when needed

[Jan Beulich]

On 11.04.2025 12:54, Roger Pau Monne wrote:
> MMIO operation handlers can be expensive to process, hence attempt to
> register only those that will be needed by the domain.
> 
> Subpage r/o MMIO regions are added exclusively at boot, further limit their
> addition to strictly before the initial domain gets created, so by the time
> initial domain creation happens Xen knows whether subpage is required or
> not.  This allows only registering the MMIO handler when there are
> subpage regions to handle.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

[PATCH 5/5] x86/mm: move mmio_ro_emulated_write() to PV only file

[Roger Pau Monne]

mmio_ro_emulated_write() is only used in pv/ro-page-fault.c, move the
function to that file and make it static.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/mm.h   | 12 ------------
 xen/arch/x86/mm.c               | 26 -------------------------
 xen/arch/x86/pv/ro-page-fault.c | 34 +++++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index aeb8ebcf2d56..2665daa6f74f 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -542,18 +542,6 @@ void memguard_unguard_stack(void *p);
 int subpage_mmio_ro_add(paddr_t start, size_t size);
 bool subpage_mmio_write_accept(mfn_t mfn, unsigned long gla);
 
-struct mmio_ro_emulate_ctxt {
-        unsigned long cr2;
-        /* Used only for mmcfg case */
-        unsigned int seg, bdf;
-        /* Used only for non-mmcfg case */
-        mfn_t mfn;
-};
-
-int cf_check mmio_ro_emulated_write(
-    enum x86_segment seg, unsigned long offset, void *p_data,
-    unsigned int bytes, struct x86_emulate_ctxt *ctxt);
-
 /* r/o MMIO subpage access handlers. */
 struct subpage_ro_range {
     struct list_head list;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 3bc6304d831c..7c6a5fde5ebd 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5142,32 +5142,6 @@ void subpage_mmio_write_emulate(
     write_mmio(addr + offset, data, len);
 }
 
-int cf_check mmio_ro_emulated_write(
-    enum x86_segment seg,
-    unsigned long offset,
-    void *p_data,
-    unsigned int bytes,
-    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 || 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),
-                               data, bytes);
-
-    return X86EMUL_OKAY;
-}
-
 /*
  * For these PTE APIs, the caller must follow the alloc-map-unmap-free
  * lifecycle, which means explicitly mapping the PTE pages before accessing
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index 11b01c479e43..8b1c25e60c17 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -298,6 +298,14 @@ static int ptwr_do_page_fault(struct x86_emulate_ctxt *ctxt,
  * fault handling for read-only MMIO pages
  */
 
+struct mmio_ro_emulate_ctxt {
+        unsigned long cr2;
+        /* Used only for mmcfg case */
+        unsigned int seg, bdf;
+        /* Used only for non-mmcfg case */
+        mfn_t mfn;
+};
+
 static int cf_check mmcfg_intercept_write(
     enum x86_segment seg,
     unsigned long offset,
@@ -329,6 +337,32 @@ static int cf_check mmcfg_intercept_write(
     return X86EMUL_OKAY;
 }
 
+static int cf_check mmio_ro_emulated_write(
+    enum x86_segment seg,
+    unsigned long offset,
+    void *p_data,
+    unsigned int bytes,
+    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 || 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),
+                               data, bytes);
+
+    return X86EMUL_OKAY;
+}
+
 static const struct x86_emulate_ops mmio_ro_emulate_ops = {
     .read       = x86emul_unhandleable_rw,
     .insn_fetch = ptwr_emulated_insn_fetch,
-- 
2.48.1

Re: [PATCH 5/5] x86/mm: move mmio_ro_emulated_write() to PV only file

[Jan Beulich]

On 11.04.2025 12:54, Roger Pau Monne wrote:
> mmio_ro_emulated_write() is only used in pv/ro-page-fault.c, move the
> function to that file and make it static.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

> --- a/xen/arch/x86/pv/ro-page-fault.c
> +++ b/xen/arch/x86/pv/ro-page-fault.c
> @@ -298,6 +298,14 @@ static int ptwr_do_page_fault(struct x86_emulate_ctxt *ctxt,
>   * fault handling for read-only MMIO pages
>   */
>  
> +struct mmio_ro_emulate_ctxt {
> +        unsigned long cr2;
> +        /* Used only for mmcfg case */
> +        unsigned int seg, bdf;
> +        /* Used only for non-mmcfg case */
> +        mfn_t mfn;

... the unnecessarily deep indentation corrected here.

Jan