* [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses
2025-04-15 15:32 [PATCH v2 0/4] xen/x86: fix implementation of subpage r/o MMIO Roger Pau Monne
@ 2025-04-15 15:32 ` Roger Pau Monne
2025-04-16 6:28 ` dmkhn
` (2 more replies)
2025-04-15 15:32 ` [PATCH v2 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages Roger Pau Monne
` (2 subsequent siblings)
3 siblings, 3 replies; 14+ messages in thread
From: Roger Pau Monne @ 2025-04-15 15:32 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper, Anthony PERARD,
Michal Orzel, Julien Grall, Stefano Stabellini
Several handlers have the same necessity of reading or writing from or to
an MMIO region using 1, 2, 4 or 8 bytes accesses. So far this has been
open-coded in the function itself. Instead provide a new set of handlers
that encapsulate the accesses.
Since the added helpers are not architecture specific, introduce a new
generic io.h header.
No functional change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
- Limit {read,write}q() to 64bit architectures.
- Always have a default case in switch statement.
---
xen/arch/x86/hvm/vmsi.c | 47 ++--------------------------
xen/arch/x86/mm.c | 32 +++++--------------
xen/drivers/vpci/msix.c | 47 ++--------------------------
xen/include/xen/io.h | 68 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 81 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..4495a04c403e
--- /dev/null
+++ b/xen/include/xen/io.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * IO related routines.
+ *
+ * Copyright (c) 2025 Cloud Software Group
+ */
+#ifndef XEN_IO_H
+#define XEN_IO_H
+
+#include <xen/bug.h>
+
+#include <asm/io.h>
+
+static inline unsigned long read_mmio(const volatile void __iomem *mem,
+ unsigned int size)
+{
+ switch ( size )
+ {
+ case 1:
+ return readb(mem);
+
+ case 2:
+ return readw(mem);
+
+ case 4:
+ return readl(mem);
+
+#ifdef CONFIG_64BIT
+ case 8:
+ return readq(mem);
+#endif
+
+ default:
+ ASSERT_UNREACHABLE();
+ return ~0UL;
+ }
+}
+
+static inline void write_mmio(volatile void __iomem *mem, unsigned long data,
+ unsigned int size)
+{
+ switch ( size )
+ {
+ case 1:
+ writeb(data, mem);
+ break;
+
+ case 2:
+ writew(data, mem);
+ break;
+
+ case 4:
+ writel(data, mem);
+ break;
+
+#ifdef CONFIG_64BIT
+ case 8:
+ writeq(data, mem);
+ break;
+#endif
+
+ default:
+ ASSERT_UNREACHABLE();
+ break;
+ }
+}
+
+#endif /* XEN_IO_H */
--
2.48.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses
2025-04-15 15:32 ` [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses Roger Pau Monne
@ 2025-04-16 6:28 ` dmkhn
2025-04-16 8:29 ` Andrew Cooper
2025-04-17 7:43 ` Jan Beulich
2 siblings, 0 replies; 14+ messages in thread
From: dmkhn @ 2025-04-16 6:28 UTC (permalink / raw)
To: Roger Pau Monne
Cc: xen-devel, Jan Beulich, Andrew Cooper, Anthony PERARD,
Michal Orzel, Julien Grall, Stefano Stabellini
On Tue, Apr 15, 2025 at 05:32:43PM +0200, Roger Pau Monne wrote:
> Several handlers have the same necessity of reading or writing from or to
> an MMIO region using 1, 2, 4 or 8 bytes accesses. So far this has been
> open-coded in the function itself. Instead provide a new set of handlers
> that encapsulate the accesses.
>
> Since the added helpers are not architecture specific, introduce a new
> generic io.h header.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v1:
> - Limit {read,write}q() to 64bit architectures.
> - Always have a default case in switch statement.
> ---
> xen/arch/x86/hvm/vmsi.c | 47 ++--------------------------
> xen/arch/x86/mm.c | 32 +++++--------------
> xen/drivers/vpci/msix.c | 47 ++--------------------------
> xen/include/xen/io.h | 68 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 81 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..4495a04c403e
> --- /dev/null
> +++ b/xen/include/xen/io.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * IO related routines.
> + *
> + * Copyright (c) 2025 Cloud Software Group
> + */
> +#ifndef XEN_IO_H
> +#define XEN_IO_H
> +
> +#include <xen/bug.h>
> +
> +#include <asm/io.h>
> +
> +static inline unsigned long read_mmio(const volatile void __iomem *mem,
> + unsigned int size)
> +{
> + switch ( size )
> + {
> + case 1:
> + return readb(mem);
> +
> + case 2:
> + return readw(mem);
> +
> + case 4:
> + return readl(mem);
> +
> +#ifdef CONFIG_64BIT
> + case 8:
> + return readq(mem);
> +#endif
> +
> + default:
> + ASSERT_UNREACHABLE();
> + return ~0UL;
> + }
> +}
> +
> +static inline void write_mmio(volatile void __iomem *mem, unsigned long data,
> + unsigned int size)
> +{
> + switch ( size )
> + {
> + case 1:
> + writeb(data, mem);
> + break;
> +
> + case 2:
> + writew(data, mem);
> + break;
> +
> + case 4:
> + writel(data, mem);
> + break;
> +
> +#ifdef CONFIG_64BIT
> + case 8:
> + writeq(data, mem);
> + break;
> +#endif
> +
> + default:
> + ASSERT_UNREACHABLE();
> + break;
> + }
> +}
> +
> +#endif /* XEN_IO_H */
> --
> 2.48.1
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses
2025-04-15 15:32 ` [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses Roger Pau Monne
2025-04-16 6:28 ` dmkhn
@ 2025-04-16 8:29 ` Andrew Cooper
2025-04-16 8:40 ` Jan Beulich
2025-04-17 7:43 ` Jan Beulich
2 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2025-04-16 8:29 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Jan Beulich, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini
On 15/04/2025 4:32 pm, Roger Pau Monne wrote:
> diff --git a/xen/include/xen/io.h b/xen/include/xen/io.h
> new file mode 100644
> index 000000000000..4495a04c403e
> --- /dev/null
> +++ b/xen/include/xen/io.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * IO related routines.
> + *
> + * Copyright (c) 2025 Cloud Software Group
> + */
> +#ifndef XEN_IO_H
> +#define XEN_IO_H
> +
> +#include <xen/bug.h>
> +
> +#include <asm/io.h>
> +
> +static inline unsigned long read_mmio(const volatile void __iomem *mem,
void *__iomem. (i.e. without the __iomem in the middle of the type).
It seems most examples of this in Xen are wrong.
~Andrew
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses
2025-04-16 8:29 ` Andrew Cooper
@ 2025-04-16 8:40 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2025-04-16 8:40 UTC (permalink / raw)
To: Andrew Cooper
Cc: Anthony PERARD, Michal Orzel, Julien Grall, Stefano Stabellini,
Roger Pau Monne, xen-devel
On 16.04.2025 10:29, Andrew Cooper wrote:
> On 15/04/2025 4:32 pm, Roger Pau Monne wrote:
>> diff --git a/xen/include/xen/io.h b/xen/include/xen/io.h
>> new file mode 100644
>> index 000000000000..4495a04c403e
>> --- /dev/null
>> +++ b/xen/include/xen/io.h
>> @@ -0,0 +1,68 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * IO related routines.
>> + *
>> + * Copyright (c) 2025 Cloud Software Group
>> + */
>> +#ifndef XEN_IO_H
>> +#define XEN_IO_H
>> +
>> +#include <xen/bug.h>
>> +
>> +#include <asm/io.h>
>> +
>> +static inline unsigned long read_mmio(const volatile void __iomem *mem,
>
> void *__iomem. (i.e. without the __iomem in the middle of the type).
> It seems most examples of this in Xen are wrong.
Hmm, perhaps in turn because I don't see why it would need to be that
placement. The attribute describes what's pointed to, not the variable
(or parameter here) itself.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses
2025-04-15 15:32 ` [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses Roger Pau Monne
2025-04-16 6:28 ` dmkhn
2025-04-16 8:29 ` Andrew Cooper
@ 2025-04-17 7:43 ` Jan Beulich
2025-04-17 14:16 ` Roger Pau Monné
2 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2025-04-17 7:43 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, xen-devel
On 15.04.2025 17:32, Roger Pau Monne wrote:
> @@ -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);
> }
Should probably have noticed this on v1 already: The log message is now lost
for the write-ignored case. It looks easy enough to have the function return
a boolean indicating "done", to retain original behavior here.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses
2025-04-17 7:43 ` Jan Beulich
@ 2025-04-17 14:16 ` Roger Pau Monné
2025-04-17 14:22 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Roger Pau Monné @ 2025-04-17 14:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, xen-devel
On Thu, Apr 17, 2025 at 09:43:09AM +0200, Jan Beulich wrote:
> On 15.04.2025 17:32, Roger Pau Monne wrote:
> > @@ -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);
> > }
>
> Should probably have noticed this on v1 already: The log message is now lost
> for the write-ignored case. It looks easy enough to have the function return
> a boolean indicating "done", to retain original behavior here.
Hm, I didn't seem to me the message wants conserving, as it's
unreachable code. I can try to add again, but we don't print such
message in other cases.
Thanks, Roger.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses
2025-04-17 14:16 ` Roger Pau Monné
@ 2025-04-17 14:22 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2025-04-17 14:22 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
Stefano Stabellini, xen-devel
On 17.04.2025 16:16, Roger Pau Monné wrote:
> On Thu, Apr 17, 2025 at 09:43:09AM +0200, Jan Beulich wrote:
>> On 15.04.2025 17:32, Roger Pau Monne wrote:
>>> @@ -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);
>>> }
>>
>> Should probably have noticed this on v1 already: The log message is now lost
>> for the write-ignored case. It looks easy enough to have the function return
>> a boolean indicating "done", to retain original behavior here.
>
> Hm, I didn't seem to me the message wants conserving, as it's
> unreachable code. I can try to add again, but we don't print such
> message in other cases.
This sub-page stuff is special, but I wouldn't mind if we dropped the
message altogether. The "unreachable code" argument is slightly weak,
as it's in particular when the assumption is violated that we would
want to know about it.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages
2025-04-15 15:32 [PATCH v2 0/4] xen/x86: fix implementation of subpage r/o MMIO Roger Pau Monne
2025-04-15 15:32 ` [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses Roger Pau Monne
@ 2025-04-15 15:32 ` Roger Pau Monne
2025-04-17 7:57 ` Jan Beulich
2025-04-15 15:32 ` [PATCH v2 3/4] x86/hvm: only register the r/o subpage ops when needed Roger Pau Monne
2025-04-15 15:32 ` [PATCH v2 4/4] x86/mm: move mmio_ro_emulated_write() to PV only file Roger Pau Monne
3 siblings, 1 reply; 14+ messages in thread
From: Roger Pau Monne @ 2025-04-15 15:32 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper
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 call site
in hvm_hap_nested_page_fault(). 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
usage of emulation for faulting p2m_mmio_direct types is limited to
addresses in the r/o MMIO range. The page present check is dropped as type
p2m_mmio_direct must have the present bit set in the PTE.
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>
---
Changes since v1:
- Introduce hvm/mmio.c to place the r/o MMIO handlers.
- Add comment about pf-fixup and r/o MMIO range checking.
- Expand commit message about dropping the PTE present check and usage of
the emulator.
- Return X86EMUL_OKAY in the read handler if the MMIO region is not found.
- Check the faulting address is in the mmio_ro_ranges before sending for
emulation.
---
xen/arch/x86/hvm/Makefile | 1 +
xen/arch/x86/hvm/emulate.c | 51 ++-----------
xen/arch/x86/hvm/hvm.c | 17 ++---
xen/arch/x86/hvm/mmio.c | 100 +++++++++++++++++++++++++
xen/arch/x86/include/asm/hvm/emulate.h | 1 -
xen/arch/x86/include/asm/hvm/io.h | 3 +
xen/arch/x86/include/asm/mm.h | 12 +++
xen/arch/x86/mm.c | 37 +--------
8 files changed, 132 insertions(+), 90 deletions(-)
create mode 100644 xen/arch/x86/hvm/mmio.c
diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
index 4c1fa5c6c2bf..6ec2c8f2db56 100644
--- a/xen/arch/x86/hvm/Makefile
+++ b/xen/arch/x86/hvm/Makefile
@@ -15,6 +15,7 @@ obj-y += intercept.o
obj-y += io.o
obj-y += ioreq.o
obj-y += irq.o
+obj-y += mmio.o
obj-y += monitor.o
obj-y += mtrr.o
obj-y += nestedhvm.o
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 9fff1b82f7c6..fbe8dd6ccb0b 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -370,7 +370,12 @@ 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) &&
+ /*
+ * Do not attempt to fixup accesses to r/o MMIO regions, they
+ * are expected to be terminated by the null handler below.
+ */
+ !rangeset_contains_singleton(mmio_ro_ranges, PFN_DOWN(addr)) )
{
/*
* PVH dom0 is likely missing MMIO mappings on the p2m, due to
@@ -2856,50 +2861,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..6b998387e3d8 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>
@@ -692,6 +692,8 @@ int hvm_domain_initialise(struct domain *d,
register_portio_handler(d, XEN_HVM_DEBUGCONS_IOPORT, 1, hvm_print_line);
+ register_subpage_ro_handler(d);
+
if ( hvm_tsc_scaling_supported )
d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio;
@@ -1981,7 +1983,10 @@ 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 &&
+ rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn))))) )
{
if ( !handle_mmio_with_translation(gla, gfn, npfec) )
hvm_inject_hw_exception(X86_EXC_GP, 0);
@@ -2033,14 +2038,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/hvm/mmio.c b/xen/arch/x86/hvm/mmio.c
new file mode 100644
index 000000000000..ee29d9e5039e
--- /dev/null
+++ b/xen/arch/x86/hvm/mmio.c
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * MMIO related routines.
+ *
+ * Copyright (c) 2025 Cloud Software Group
+ */
+
+#include <xen/io.h>
+#include <xen/mm.h>
+
+#include <asm/p2m.h>
+
+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_OKAY;
+ }
+
+ mem = subpage_mmio_map_page(entry);
+ if ( !mem )
+ {
+ put_gfn(d, addr);
+ gprintk(XENLOG_ERR,
+ "Failed to map page for MMIO read at %#lx -> %#lx\n",
+ addr, 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;
+}
+
+void register_subpage_ro_handler(struct domain *d)
+{
+ static const struct hvm_mmio_ops subpage_mmio_ops = {
+ .check = subpage_mmio_accept,
+ .read = subpage_mmio_read,
+ .write = subpage_mmio_write,
+ };
+
+ register_mmio_handler(d, &subpage_mmio_ops);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
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/hvm/io.h b/xen/arch/x86/include/asm/hvm/io.h
index 565bad300d20..c12f099a037c 100644
--- a/xen/arch/x86/include/asm/hvm/io.h
+++ b/xen/arch/x86/include/asm/hvm/io.h
@@ -135,6 +135,9 @@ void destroy_vpci_mmcfg(struct domain *d);
/* Remove MMCFG regions from a domain ->iomem_caps. */
int vpci_mmcfg_deny_access(struct domain *d);
+/* r/o MMIO subpage access handler. */
+void register_subpage_ro_handler(struct domain *d);
+
#endif /* __ASM_X86_HVM_IO_H__ */
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages
2025-04-15 15:32 ` [PATCH v2 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages Roger Pau Monne
@ 2025-04-17 7:57 ` Jan Beulich
2025-04-17 12:19 ` Marek Marczykowski-Górecki
2025-04-17 13:59 ` Roger Pau Monné
0 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2025-04-17 7:57 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: Andrew Cooper, xen-devel
On 15.04.2025 17:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -370,7 +370,12 @@ 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) &&
> + /*
> + * Do not attempt to fixup accesses to r/o MMIO regions, they
> + * are expected to be terminated by the null handler below.
> + */
> + !rangeset_contains_singleton(mmio_ro_ranges, PFN_DOWN(addr)) )
> {
> /*
> * PVH dom0 is likely missing MMIO mappings on the p2m, due to
Doesn't this need limiting to writes, i.e. permitting reads to still be
handled right here?
> --- /dev/null
> +++ b/xen/arch/x86/hvm/mmio.c
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * MMIO related routines.
> + *
> + * Copyright (c) 2025 Cloud Software Group
> + */
> +
> +#include <xen/io.h>
> +#include <xen/mm.h>
> +
> +#include <asm/p2m.h>
> +
> +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);
Don't you need to use PFN_DOWN() on addr?
> + 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);
Same here and further down, and in the write case?
> + 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_OKAY;
> + }
> +
> + mem = subpage_mmio_map_page(entry);
> + if ( !mem )
> + {
> + put_gfn(d, addr);
> + gprintk(XENLOG_ERR,
> + "Failed to map page for MMIO read at %#lx -> %#lx\n",
> + addr, mfn_to_maddr(mfn) + PAGE_OFFSET(addr));
> + return X86EMUL_OKAY;
> + }
> +
> + *data = read_mmio(mem + PAGE_OFFSET(addr), len);
What if this crosses the trailing page boundary? Imo subpage_mmio_accept()
would better reject misaligned accesses (at least until we know we need to
handle such for some obscure reason).
> + put_gfn(d, addr);
> + return X86EMUL_OKAY;
> +}
Thinking of it - why do reads need handling here? The guest has read access?
Hmm, yes, read-modify-write operations may take this path. Maybe worth
having a comment here.
> +void register_subpage_ro_handler(struct domain *d)
> +{
> + static const struct hvm_mmio_ops subpage_mmio_ops = {
> + .check = subpage_mmio_accept,
> + .read = subpage_mmio_read,
> + .write = subpage_mmio_write,
> + };
> +
> + register_mmio_handler(d, &subpage_mmio_ops);
Are there any criteria by which we could limit the registration of this
handler?
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages
2025-04-17 7:57 ` Jan Beulich
@ 2025-04-17 12:19 ` Marek Marczykowski-Górecki
2025-04-17 13:59 ` Roger Pau Monné
1 sibling, 0 replies; 14+ messages in thread
From: Marek Marczykowski-Górecki @ 2025-04-17 12:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monne, Andrew Cooper, xen-devel
[-- Attachment #1: Type: text/plain, Size: 626 bytes --]
On Thu, Apr 17, 2025 at 09:57:29AM +0200, Jan Beulich wrote:
> On 15.04.2025 17:32, Roger Pau Monne wrote:
> > +void register_subpage_ro_handler(struct domain *d)
> > +{
> > + static const struct hvm_mmio_ops subpage_mmio_ops = {
> > + .check = subpage_mmio_accept,
> > + .read = subpage_mmio_read,
> > + .write = subpage_mmio_write,
> > + };
> > +
> > + register_mmio_handler(d, &subpage_mmio_ops);
>
> Are there any criteria by which we could limit the registration of this
> handler?
Yes, see patch 3.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages
2025-04-17 7:57 ` Jan Beulich
2025-04-17 12:19 ` Marek Marczykowski-Górecki
@ 2025-04-17 13:59 ` Roger Pau Monné
1 sibling, 0 replies; 14+ messages in thread
From: Roger Pau Monné @ 2025-04-17 13:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
On Thu, Apr 17, 2025 at 09:57:29AM +0200, Jan Beulich wrote:
> On 15.04.2025 17:32, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -370,7 +370,12 @@ 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) &&
> > + /*
> > + * Do not attempt to fixup accesses to r/o MMIO regions, they
> > + * are expected to be terminated by the null handler below.
> > + */
> > + !rangeset_contains_singleton(mmio_ro_ranges, PFN_DOWN(addr)) )
> > {
> > /*
> > * PVH dom0 is likely missing MMIO mappings on the p2m, due to
>
> Doesn't this need limiting to writes, i.e. permitting reads to still be
> handled right here?
Oh, I see, yes, it would be fine to attempt to fixup read accesses to
mmio_ro_ranges ranges.
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/mmio.c
> > @@ -0,0 +1,100 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * MMIO related routines.
> > + *
> > + * Copyright (c) 2025 Cloud Software Group
> > + */
> > +
> > +#include <xen/io.h>
> > +#include <xen/mm.h>
> > +
> > +#include <asm/p2m.h>
> > +
> > +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);
>
> Don't you need to use PFN_DOWN() on addr?
>
> > + 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);
>
> Same here and further down, and in the write case?
Hm, yes I do.
> > + 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_OKAY;
> > + }
> > +
> > + mem = subpage_mmio_map_page(entry);
> > + if ( !mem )
> > + {
> > + put_gfn(d, addr);
> > + gprintk(XENLOG_ERR,
> > + "Failed to map page for MMIO read at %#lx -> %#lx\n",
> > + addr, mfn_to_maddr(mfn) + PAGE_OFFSET(addr));
> > + return X86EMUL_OKAY;
> > + }
> > +
> > + *data = read_mmio(mem + PAGE_OFFSET(addr), len);
>
> What if this crosses the trailing page boundary? Imo subpage_mmio_accept()
> would better reject misaligned accesses (at least until we know we need to
> handle such for some obscure reason).
Yes, the previous mmio_ro_emulated_write() did already reject such
accesses.
Thanks, Roger.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/4] x86/hvm: only register the r/o subpage ops when needed
2025-04-15 15:32 [PATCH v2 0/4] xen/x86: fix implementation of subpage r/o MMIO Roger Pau Monne
2025-04-15 15:32 ` [PATCH v2 1/4] xen/io: provide helpers for multi size MMIO accesses Roger Pau Monne
2025-04-15 15:32 ` [PATCH v2 2/4] x86/hvm: fix handling of accesses to partial r/o MMIO pages Roger Pau Monne
@ 2025-04-15 15:32 ` Roger Pau Monne
2025-04-15 15:32 ` [PATCH v2 4/4] x86/mm: move mmio_ro_emulated_write() to PV only file Roger Pau Monne
3 siblings, 0 replies; 14+ messages in thread
From: Roger Pau Monne @ 2025-04-15 15:32 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper
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>
---
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 | 3 ++-
xen/arch/x86/include/asm/mm.h | 1 +
xen/arch/x86/mm.c | 16 ++++++++++++++++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6b998387e3d8..4cb2e13046d1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -692,7 +692,8 @@ int hvm_domain_initialise(struct domain *d,
register_portio_handler(d, XEN_HVM_DEBUGCONS_IOPORT, 1, hvm_print_line);
- register_subpage_ro_handler(d);
+ if ( subpage_ro_active() )
+ register_subpage_ro_handler(d);
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 4/4] x86/mm: move mmio_ro_emulated_write() to PV only file
2025-04-15 15:32 [PATCH v2 0/4] xen/x86: fix implementation of subpage r/o MMIO Roger Pau Monne
` (2 preceding siblings ...)
2025-04-15 15:32 ` [PATCH v2 3/4] x86/hvm: only register the r/o subpage ops when needed Roger Pau Monne
@ 2025-04-15 15:32 ` Roger Pau Monne
3 siblings, 0 replies; 14+ messages in thread
From: Roger Pau Monne @ 2025-04-15 15:32 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Jan Beulich, Andrew Cooper
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>
---
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..3dd795288379 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
^ permalink raw reply related [flat|nested] 14+ messages in thread