[PATCH 0/6] Fourth MPU series

[PATCH 0/6] Fourth MPU series

[Harry Ramsey]

This series aims to further the ongoing work to introduce support for MPU
systems in xen.

The patches in this series implement various memory functions and enable the
hypervisor timer.

Luca Fancellu (3):
  arm/mpu: Implement copy_from_paddr for MPU systems
  arm/mpu: Implement vmap functions for MPU
  arm/mpu: Introduce modify_after_init_mappings

Penny Zheng (3):
  arm/mpu: Implement free_init_memory for MPU systems
  arm: Use secure hypervisor timer in MPU system
  arm/mpu: Map domain page in AArch64 MPU systems

 xen/arch/arm/Kconfig                     |   6 +
 xen/arch/arm/include/asm/arm64/sysregs.h |  12 ++
 xen/arch/arm/include/asm/mpu/mm.h        |  11 ++
 xen/arch/arm/include/asm/setup.h         |   5 +
 xen/arch/arm/mmu/setup.c                 |  15 +++
 xen/arch/arm/mpu/Makefile                |   1 +
 xen/arch/arm/mpu/domain-page.c           |  53 ++++++++
 xen/arch/arm/mpu/mm.c                    | 160 ++++++++++++++++++-----
 xen/arch/arm/mpu/setup.c                 |  54 +++++++-
 xen/arch/arm/mpu/vmap.c                  |  14 +-
 xen/arch/arm/setup.c                     |  15 +--
 11 files changed, 296 insertions(+), 50 deletions(-)
 create mode 100644 xen/arch/arm/mpu/domain-page.c

--
2.43.0

[PATCH 1/6] arm/mpu: Implement copy_from_paddr for MPU systems

[Harry Ramsey]

From: Luca Fancellu <luca.fancellu@arm.com>

Implement the function copy_from_paddr variant for MPU systems, using
the map_pages_to_xen/destroy_xen_mappings to temporarily map the memory
range to be copied.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
 xen/arch/arm/mpu/setup.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
index 163573b932..ec264f54f2 100644
--- a/xen/arch/arm/mpu/setup.c
+++ b/xen/arch/arm/mpu/setup.c
@@ -91,7 +91,19 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
  */
 void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
 {
-    BUG_ON("unimplemented");
+    paddr_t start_pg = round_pgdown(paddr);
+    paddr_t end_pg   = round_pgup(paddr + len);
+    unsigned long nr_mfns = (end_pg - start_pg) >> PAGE_SHIFT;
+    mfn_t mfn = maddr_to_mfn(start_pg);
+
+    if ( map_pages_to_xen(start_pg, mfn, nr_mfns, PAGE_HYPERVISOR_WC) )
+        panic("Unable to map range for copy_from_paddr\n");
+
+    memcpy(dst, maddr_to_virt(paddr), len);
+    clean_dcache_va_range(dst, len);
+
+    if ( destroy_xen_mappings(start_pg, end_pg) )
+        panic("Unable to unmap range for copy_from_paddr\n");
 }
 
 void __init remove_early_mappings(void)
-- 
2.43.0

[PATCH 2/6] arm/mpu: Implement vmap functions for MPU

[Harry Ramsey]

From: Luca Fancellu <luca.fancellu@arm.com>

HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
in places across common code. In order to keep the existing code and
maintain correct functionality, implement the `vmap_contig` and `vunmap`
functions for MPU systems.

Introduce a helper function `destroy_entire_xen_mapping` to aid with
unmapping an entire region when only the start address is known.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
 xen/arch/arm/include/asm/mpu/mm.h | 11 +++++
 xen/arch/arm/mpu/mm.c             | 67 +++++++++++++++++++++++++------
 xen/arch/arm/mpu/vmap.c           | 14 +++++--
 3 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index e1ded6521d..83ee0e59ca 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -111,6 +111,17 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
 int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
                            paddr_t limit, uint8_t *index);
 
+
+/*
+ * Destroys and frees (if reference count is 0) an entire xen mapping on MPU
+ * systems where only the start address is known.
+ *
+ * @param s     Start address of memory region to be destroyed.
+ *
+ * @return:     0 on success, negative on error.
+ */
+int destroy_entire_xen_mapping(paddr_t s);
+
 #endif /* __ARM_MPU_MM_H__ */
 
 /*
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 687dec3bc6..29d8e7ff11 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -290,6 +290,35 @@ static void disable_mpu_region_from_index(uint8_t index)
         write_protection_region(&xen_mpumap[index], index);
 }
 
+/*
+ * Free a xen_mpumap entry given the index. A mpu region is actually disabled
+ * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
+ *
+ * @param idx                   Index of the mpumap entry.
+ * @param region_found_type     Either MPUMAP_REGION_FOUND or MPUMAP_REGION_INCLUSIVE.
+ * @return                      0 on success, otherwise negative on error.
+ */
+static int xen_mpumap_free_entry(uint8_t idx, int region_found_type)
+{
+    ASSERT(spin_is_locked(&xen_mpumap_lock));
+    ASSERT(idx != INVALID_REGION_IDX);
+
+    if ( xen_mpumap[idx].refcount == 0 )
+    {
+        if ( MPUMAP_REGION_FOUND == region_found_type )
+            disable_mpu_region_from_index(idx);
+        else
+        {
+            printk(XENLOG_ERR "Cannot remove a partial region\n");
+            return -EINVAL;
+        }
+    }
+    else
+        xen_mpumap[idx].refcount -= 1;
+
+    return 0;
+}
+
 /*
  * Update the entry in the MPU memory region mapping table (xen_mpumap) for the
  * given memory range and flags, creating one if none exists.
@@ -357,18 +386,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
             return -EINVAL;
         }
 
-        if ( xen_mpumap[idx].refcount == 0 )
-        {
-            if ( MPUMAP_REGION_FOUND == rc )
-                disable_mpu_region_from_index(idx);
-            else
-            {
-                printk("Cannot remove a partial region\n");
-                return -EINVAL;
-            }
-        }
-        else
-            xen_mpumap[idx].refcount -= 1;
+        return xen_mpumap_free_entry(idx, rc);
     }
 
     return 0;
@@ -418,6 +436,31 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
     return xen_mpumap_update(s, e, 0);
 }
 
+int destroy_entire_xen_mapping(paddr_t s)
+{
+    int rc;
+    uint8_t idx;
+
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+
+    spin_lock(&xen_mpumap_lock);
+
+    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s + PAGE_SIZE,
+                                &idx);
+    if ( rc == MPUMAP_REGION_NOTFOUND )
+    {
+        printk(XENLOG_ERR "Cannot remove entry that does not exist");
+        rc = -EINVAL;
+    }
+
+    /* As we are unmapping entire region use MPUMAP_REGION_FOUND instead */
+    rc = xen_mpumap_free_entry(idx, MPUMAP_REGION_FOUND);
+
+    spin_unlock(&xen_mpumap_lock);
+
+    return rc;
+}
+
 int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
                      unsigned int flags)
 {
diff --git a/xen/arch/arm/mpu/vmap.c b/xen/arch/arm/mpu/vmap.c
index f977b79cd4..d3037ae98a 100644
--- a/xen/arch/arm/mpu/vmap.c
+++ b/xen/arch/arm/mpu/vmap.c
@@ -1,19 +1,27 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
 #include <xen/bug.h>
+#include <xen/mm.h>
 #include <xen/mm-frame.h>
 #include <xen/types.h>
 #include <xen/vmap.h>
 
 void *vmap_contig(mfn_t mfn, unsigned int nr)
 {
-    BUG_ON("unimplemented");
-    return NULL;
+    paddr_t base = mfn_to_maddr(mfn);
+
+    if ( map_pages_to_xen(base, mfn, nr, PAGE_HYPERVISOR ) )
+        return NULL;
+
+    return maddr_to_virt(base);
 }
 
 void vunmap(const void *va)
 {
-    BUG_ON("unimplemented");
+    paddr_t base = virt_to_maddr(va);
+
+    if ( destroy_entire_xen_mapping(base) )
+        panic("Failed to vunmap region\n");
 }
 
 /*
-- 
2.43.0

[PATCH 3/6] arm/mpu: Implement free_init_memory for MPU systems

[Harry Ramsey]

From: Penny Zheng <Penny.Zheng@arm.com>

Implement the function `free_init_memory` for MPU systems. In order to
support this, the function `modify_xen_mappings` is implemented.

On MPU systems, we map the init text and init data sections using
separate MPU memory regions. Therefore these are removed separately in
`free_init_memory`.

Additionally remove warning messages from `is_mm_attr_match` as some
permissions can now be updated by `xen_mpumap_update_entry`.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
 xen/arch/arm/include/asm/setup.h |  2 +
 xen/arch/arm/mpu/mm.c            | 91 +++++++++++++++++++++++++-------
 2 files changed, 73 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 1eaf13bd66..005cf7be59 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -65,6 +65,8 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
 int map_range_to_domain(const struct dt_device_node *dev,
                         uint64_t addr, uint64_t len, void *data);
 
+extern const char __init_data_begin[], __bss_start[], __bss_end[];
+
 struct init_info
 {
     /* Pointer to the stack, used by head.S when entering in C */
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 29d8e7ff11..8446dddde8 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -174,28 +174,13 @@ int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
 static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
 {
     if ( region->prbar.reg.ro != PAGE_RO_MASK(attributes) )
-    {
-        printk(XENLOG_WARNING
-               "Mismatched Access Permission attributes (%#x instead of %#x)\n",
-               region->prbar.reg.ro, PAGE_RO_MASK(attributes));
         return false;
-    }
 
     if ( region->prbar.reg.xn != PAGE_XN_MASK(attributes) )
-    {
-        printk(XENLOG_WARNING
-               "Mismatched Execute Never attributes (%#x instead of %#x)\n",
-               region->prbar.reg.xn, PAGE_XN_MASK(attributes));
         return false;
-    }
 
     if ( region->prlar.reg.ai != PAGE_AI_MASK(attributes) )
-    {
-        printk(XENLOG_WARNING
-               "Mismatched Memory Attribute Index (%#x instead of %#x)\n",
-               region->prlar.reg.ai, PAGE_AI_MASK(attributes));
         return false;
-    }
 
     return true;
 }
@@ -352,8 +337,33 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
     {
         if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
         {
-            printk("Modifying an existing entry is not supported\n");
-            return -EINVAL;
+            if ( rc == MPUMAP_REGION_INCLUSIVE )
+            {
+                printk(XENLOG_ERR
+                       "Cannot modify partial region permissions\n");
+                return -EINVAL;
+            }
+
+            if ( xen_mpumap[idx].prlar.reg.ai != PAGE_AI_MASK(flags) )
+            {
+                printk(XENLOG_ERR
+                       "Modifying memory attribute is not supported\n");
+                return -EINVAL;
+            }
+
+            if ( xen_mpumap[idx].refcount != 0 )
+            {
+                printk(XENLOG_ERR
+                       "Cannot modify memory permissions for a region mapped multiple time\n");
+                return -EINVAL;
+            }
+
+            /* Set new permission */
+            xen_mpumap[idx].prbar.reg.ro = PAGE_RO_MASK(flags);
+            xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
+
+            write_protection_region(&xen_mpumap[idx], idx);
+            return 0;
         }
 
         /* Check for overflow of refcount before incrementing.  */
@@ -499,8 +509,7 @@ void __init setup_mm_helper(void)
 
 int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
 {
-    BUG_ON("unimplemented");
-    return -EINVAL;
+    return xen_mpumap_update(s, e, nf);
 }
 
 void dump_hyp_walk(vaddr_t addr)
@@ -511,7 +520,49 @@ void dump_hyp_walk(vaddr_t addr)
 /* Release all __init and __initdata ranges to be reused */
 void free_init_memory(void)
 {
-    BUG_ON("unimplemented");
+    unsigned long inittext_end = round_pgup((unsigned long)__init_data_begin);
+    unsigned long len = __init_end - __init_begin;
+    uint8_t idx;
+    int rc;
+
+    rc = modify_xen_mappings((unsigned long)__init_begin, inittext_end,
+                             PAGE_HYPERVISOR_RW);
+    if ( rc )
+        panic("Unable to map RW the init text section (rc = %d)\n", rc);
+
+    /*
+     * From now on, init will not be used for execution anymore,
+     * so nuke the instruction cache to remove entries related to init.
+     */
+    invalidate_icache_local();
+
+    /* Zeroing the memory before returning it */
+    memset(__init_begin, 0, len);
+
+    rc = destroy_xen_mappings((unsigned long)__init_begin, inittext_end);
+    if ( rc )
+        panic("Unable to remove init text section (rc = %d)\n", rc);
+
+    /*
+     * The initdata and bss sections are mapped using a single MPU region, so
+     * modify the start of this region to remove the initdata section.
+     */
+    spin_lock(&xen_mpumap_lock);
+
+    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions,
+                                (unsigned long)__init_data_begin,
+                                (unsigned long)__bss_end,
+                                &idx);
+    if ( rc < MPUMAP_REGION_FOUND )
+        panic("Unable to find bss data section (rc = %d)\n", rc);
+
+    /* bss data section is shrunk and now starts from __bss_start */
+    pr_set_base(&xen_mpumap[idx], (unsigned long)__bss_start);
+
+    write_protection_region(&xen_mpumap[idx], idx);
+    context_sync_mpu();
+
+    spin_unlock(&xen_mpumap_lock);
 }
 
 void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int flags)
-- 
2.43.0

[PATCH 4/6] arm/mpu: Introduce modify_after_init_mappings

[Harry Ramsey]

From: Luca Fancellu <luca.fancellu@arm.com>

During `init_done`, Xen sets the permissions of all symbols marked with
__ro_after_init to be read-only. Currently this is achieved by calling
`modify_xen_mappings` and will shrink the RW mapping on one side and
extend the RO mapping on the other.

This does not work on MPU systems at present because part-region
modification is not supported. Therefore introduce the function
`modify_after_init_mappings` for MMU and MPU, to handle the divergent
approaches to setting permissions of __ro_after_init symbols.

As the new function is marked with __init, it needs to be called before
`free_init_memory`.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
 xen/arch/arm/include/asm/setup.h |  3 +++
 xen/arch/arm/mmu/setup.c         | 15 ++++++++++++
 xen/arch/arm/mpu/mm.c            |  2 +-
 xen/arch/arm/mpu/setup.c         | 40 ++++++++++++++++++++++++++++++++
 xen/arch/arm/setup.c             | 15 ++----------
 5 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 005cf7be59..899e33925c 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -78,6 +78,9 @@ struct init_info
 paddr_t consider_modules(paddr_t s, paddr_t e, uint32_t size, paddr_t align,
                          int first_mod);
 
+/* Modify some mappings after the init is done */
+void modify_after_init_mappings(void);
+
 #endif
 /*
  * Local variables:
diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
index 9b874f8ab2..d042f73597 100644
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -213,6 +213,21 @@ void __init remove_early_mappings(void)
     BUG_ON(rc);
 }
 
+void __init modify_after_init_mappings(void)
+{
+    /*
+     * We have finished booting. Mark the section .data.ro_after_init
+     * read-only.
+     */
+    int rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
+                                 (unsigned long)&__ro_after_init_end,
+                                 PAGE_HYPERVISOR_RO);
+
+    if ( rc )
+        panic("Unable to mark the .data.ro_after_init section read-only (rc = %d)\n",
+              rc);
+}
+
 /*
  * After boot, Xen page-tables should not contain mapping that are both
  * Writable and eXecutables.
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 8446dddde8..f95ba7c749 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -32,7 +32,7 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
 /* EL2 Xen MPU memory region mapping table. */
 pr_t __cacheline_aligned __section(".data") xen_mpumap[MAX_MPU_REGION_NR];
 
-static DEFINE_SPINLOCK(xen_mpumap_lock);
+DEFINE_SPINLOCK(xen_mpumap_lock);
 
 static void __init __maybe_unused build_assertions(void)
 {
diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
index ec264f54f2..55317ee318 100644
--- a/xen/arch/arm/mpu/setup.c
+++ b/xen/arch/arm/mpu/setup.c
@@ -8,11 +8,14 @@
 #include <xen/pfn.h>
 #include <xen/types.h>
 #include <xen/sizes.h>
+#include <xen/spinlock.h>
 #include <asm/setup.h>
 
 static paddr_t __initdata mapped_fdt_base = INVALID_PADDR;
 static paddr_t __initdata mapped_fdt_limit = INVALID_PADDR;
 
+extern spinlock_t xen_mpumap_lock;
+
 void __init setup_pagetables(void) {}
 
 void * __init early_fdt_map(paddr_t fdt_paddr)
@@ -106,6 +109,43 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
         panic("Unable to unmap range for copy_from_paddr\n");
 }
 
+void __init modify_after_init_mappings(void)
+{
+    int rc;
+    uint8_t idx_rodata;
+    uint8_t idx_rwdata;
+
+    spin_lock(&xen_mpumap_lock);
+
+    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions,
+                                (unsigned long)_srodata,
+                                (unsigned long)_erodata,
+                                &idx_rodata);
+
+    if ( rc < MPUMAP_REGION_FOUND )
+        panic("Unable to find rodata section (rc = %d)\n", rc);
+
+    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions,
+                                (unsigned long)__ro_after_init_start,
+                                (unsigned long)__init_begin,
+                                &idx_rwdata);
+
+    if ( rc < MPUMAP_REGION_FOUND )
+        panic("Unable to find rwdata section (rc = %d)\n", rc);
+
+    /* Shrink rwdata section to begin at __ro_after_init_end */
+    pr_set_base(&xen_mpumap[idx_rwdata], (unsigned long)__ro_after_init_end);
+
+    /* Extend rodata section to end at __ro_after_init_end */
+    pr_set_limit(&xen_mpumap[idx_rodata], (unsigned long)__ro_after_init_end);
+
+    write_protection_region(&xen_mpumap[idx_rwdata], idx_rwdata);
+    write_protection_region(&xen_mpumap[idx_rodata], idx_rodata);
+    context_sync_mpu();
+
+    spin_unlock(&xen_mpumap_lock);
+}
+
 void __init remove_early_mappings(void)
 {
     int rc = destroy_xen_mappings(round_pgdown(mapped_fdt_base),
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7ad870e382..6310a47d68 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -66,23 +66,12 @@ domid_t __read_mostly max_init_domid;
 
 static __used void noreturn init_done(void)
 {
-    int rc;
-
     /* Must be done past setting system_state. */
     unregister_init_virtual_region();
 
-    free_init_memory();
+    modify_after_init_mappings();
 
-    /*
-     * We have finished booting. Mark the section .data.ro_after_init
-     * read-only.
-     */
-    rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
-                             (unsigned long)&__ro_after_init_end,
-                             PAGE_HYPERVISOR_RO);
-    if ( rc )
-        panic("Unable to mark the .data.ro_after_init section read-only (rc = %d)\n",
-              rc);
+    free_init_memory();
 
     startup_cpu_idle_loop();
 }
-- 
2.43.0

[PATCH 5/6] arm: Use secure hypervisor timer in MPU system

[Harry Ramsey]

From: Penny Zheng <Penny.Zheng@arm.com>

As MPU systems only have one secure state, we have to use secure EL2
hypervisor timer for Xen in secure EL2.

In this patch, we introduce a new Kconfig option ARM_SECURE_STATE
and a set of secure hypervisor timer registers CNTHPS_*_EL2.
We alias CNTHP_*_EL2 to CNTHPS_*_EL2 to keep the timer code
flow unchanged.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
 xen/arch/arm/Kconfig                     |  5 +++++
 xen/arch/arm/include/asm/arm64/sysregs.h | 12 ++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index cf6af68299..a5c111e08e 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -91,6 +91,7 @@ config MMU
 
 config MPU
 	bool "MPU" if UNSUPPORTED
+	select ARM_SECURE_STATE if ARM_64
 	select STATIC_MEMORY
 	help
 	  Memory Protection Unit (MPU). Select if you plan to run Xen on ARMv8-R
@@ -223,6 +224,10 @@ config HARDEN_BRANCH_PREDICTOR
 
 	  If unsure, say Y.
 
+config ARM_SECURE_STATE
+	bool "Xen will run in Arm Secure State"
+	default n
+
 config ARM64_PTR_AUTH
 	def_bool n
 	depends on ARM_64
diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
index 7440d495e4..29caad7155 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -462,6 +462,18 @@
 #define ZCR_ELx_LEN_SIZE             9
 #define ZCR_ELx_LEN_MASK             0x1ff
 
+#ifdef CONFIG_ARM_SECURE_STATE
+/*
+ * The Armv8-R AArch64 architecture always executes code in Secure
+ * state with EL2 as the highest Exception.
+ *
+ * Hypervisor timer registers for Secure EL2.
+ */
+#define CNTHP_TVAL_EL2  CNTHPS_TVAL_EL2
+#define CNTHP_CTL_EL2   CNTHPS_CTL_EL2
+#define CNTHP_CVAL_EL2  CNTHPS_CVAL_EL2
+#endif
+
 #define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
 #define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
 #define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
-- 
2.43.0

[PATCH 6/6] arm/mpu: Map domain page in AArch64 MPU systems

[Harry Ramsey]

From: Penny Zheng <Penny.Zheng@arm.com>

In MPU systems, we implement map_domain_page()/unmap_domain_page()
through mapping the domain page with a MPU region on demand.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
 xen/arch/arm/Kconfig           |  1 +
 xen/arch/arm/mpu/Makefile      |  1 +
 xen/arch/arm/mpu/domain-page.c | 53 ++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)
 create mode 100644 xen/arch/arm/mpu/domain-page.c

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index a5c111e08e..dac9a16c28 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -91,6 +91,7 @@ config MMU
 
 config MPU
 	bool "MPU" if UNSUPPORTED
+	select ARCH_MAP_DOMAIN_PAGE if ARM_64
 	select ARM_SECURE_STATE if ARM_64
 	select STATIC_MEMORY
 	help
diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
index 4963c8b550..940297af3f 100644
--- a/xen/arch/arm/mpu/Makefile
+++ b/xen/arch/arm/mpu/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_ARM_32) += arm32/
 obj-$(CONFIG_ARM_64) += arm64/
+obj-$(CONFIG_ARM_64) += domain-page.o
 obj-y += mm.o
 obj-y += p2m.o
 obj-y += setup.init.o
diff --git a/xen/arch/arm/mpu/domain-page.c b/xen/arch/arm/mpu/domain-page.c
new file mode 100644
index 0000000000..9248053ff5
--- /dev/null
+++ b/xen/arch/arm/mpu/domain-page.c
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include <xen/bug.h>
+#include <xen/domain_page.h>
+#include <xen/mm.h>
+#include <xen/mm-frame.h>
+#include <xen/types.h>
+
+void *map_domain_page_global(mfn_t mfn)
+{
+    BUG_ON("unimplemented");
+    return NULL;
+}
+
+/* Map a page of domheap memory */
+void *map_domain_page(mfn_t mfn)
+{
+    paddr_t pa = mfn_to_maddr(mfn);
+
+    if ( map_pages_to_xen((unsigned long)pa, mfn, 1, PAGE_HYPERVISOR_RW) )
+        return NULL;
+
+    return maddr_to_virt(pa);
+}
+
+/* Release a mapping taken with map_domain_page() */
+void unmap_domain_page(const void *ptr)
+{
+    paddr_t base = virt_to_maddr(ptr);
+
+    if ( destroy_entire_xen_mapping(base) )
+        panic("Failed to unmap domain page\n");
+}
+
+mfn_t domain_page_map_to_mfn(const void *ptr)
+{
+    BUG_ON("unimplemented");
+    return INVALID_MFN;
+}
+
+void unmap_domain_page_global(const void *va)
+{
+    BUG_ON("unimplemented");
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.43.0

Re: [PATCH 1/6] arm/mpu: Implement copy_from_paddr for MPU systems

[Orzel, Michal]

On 28/11/2025 10:58, Harry Ramsey wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Implement the function copy_from_paddr variant for MPU systems, using
> the map_pages_to_xen/destroy_xen_mappings to temporarily map the memory
> range to be copied.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 2/6] arm/mpu: Implement vmap functions for MPU

[Orzel, Michal]

On 28/11/2025 10:58, Harry Ramsey wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
Just as a reminder, we don't intend to support VMAP on MPU? Asking because it
would otherwise be a duplicate effort to implement only these two helpers.

> in places across common code. In order to keep the existing code and
> maintain correct functionality, implement the `vmap_contig` and `vunmap`
> functions for MPU systems.
> 
> Introduce a helper function `destroy_entire_xen_mapping` to aid with
> unmapping an entire region when only the start address is known.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
>  xen/arch/arm/include/asm/mpu/mm.h | 11 +++++
>  xen/arch/arm/mpu/mm.c             | 67 +++++++++++++++++++++++++------
>  xen/arch/arm/mpu/vmap.c           | 14 +++++--
>  3 files changed, 77 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index e1ded6521d..83ee0e59ca 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -111,6 +111,17 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
>  int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>                             paddr_t limit, uint8_t *index);
>  
> +
> +/*
> + * Destroys and frees (if reference count is 0) an entire xen mapping on MPU
> + * systems where only the start address is known.
> + *
> + * @param s     Start address of memory region to be destroyed.
> + *
> + * @return:     0 on success, negative on error.
> + */
> +int destroy_entire_xen_mapping(paddr_t s);
NIT: I read it as all the mappings which is a bit misleading :)
Maybe something like: destroy_mapping_containing or alike?

> +
>  #endif /* __ARM_MPU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 687dec3bc6..29d8e7ff11 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -290,6 +290,35 @@ static void disable_mpu_region_from_index(uint8_t index)
>          write_protection_region(&xen_mpumap[index], index);
>  }
>  
> +/*
> + * Free a xen_mpumap entry given the index. A mpu region is actually disabled
> + * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
> + *
> + * @param idx                   Index of the mpumap entry.
> + * @param region_found_type     Either MPUMAP_REGION_FOUND or MPUMAP_REGION_INCLUSIVE.
Both of these are unsigned, so why the parameter is int?

> + * @return                      0 on success, otherwise negative on error.
> + */
> +static int xen_mpumap_free_entry(uint8_t idx, int region_found_type)
> +{
> +    ASSERT(spin_is_locked(&xen_mpumap_lock));
> +    ASSERT(idx != INVALID_REGION_IDX);
> +
> +    if ( xen_mpumap[idx].refcount == 0 )
> +    {
> +        if ( MPUMAP_REGION_FOUND == region_found_type )
> +            disable_mpu_region_from_index(idx);
> +        else
> +        {
> +            printk(XENLOG_ERR "Cannot remove a partial region\n");
> +            return -EINVAL;
> +        }
> +    }
> +    else
> +        xen_mpumap[idx].refcount -= 1;
You could avoid nesting if/else by doing:
    if ( xen_mpumap[idx].refcount )
    {
        xen_mpumap[idx].refcount -= 1;
        return 0;
    }

    if ( MPUMAP_REGION_FOUND == region_found_type )
        disable_mpu_region_from_index(idx);
    else
    {
        printk(XENLOG_ERR "Cannot remove a partial region\n");
        return -EINVAL;
    }

> +
> +    return 0;
> +}
> +
>  /*
>   * Update the entry in the MPU memory region mapping table (xen_mpumap) for the
>   * given memory range and flags, creating one if none exists.
> @@ -357,18 +386,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>              return -EINVAL;
>          }
>  
> -        if ( xen_mpumap[idx].refcount == 0 )
> -        {
> -            if ( MPUMAP_REGION_FOUND == rc )
> -                disable_mpu_region_from_index(idx);
> -            else
> -            {
> -                printk("Cannot remove a partial region\n");
> -                return -EINVAL;
> -            }
> -        }
> -        else
> -            xen_mpumap[idx].refcount -= 1;
> +        return xen_mpumap_free_entry(idx, rc);
>      }
>  
>      return 0;
> @@ -418,6 +436,31 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
>      return xen_mpumap_update(s, e, 0);
>  }
>  
> +int destroy_entire_xen_mapping(paddr_t s)
> +{
> +    int rc;
> +    uint8_t idx;
> +
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +
> +    spin_lock(&xen_mpumap_lock);
> +
> +    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s + PAGE_SIZE,
> +                                &idx);
So here you are searching for a region that includes at least s + PAGE_SIZE.

> +    if ( rc == MPUMAP_REGION_NOTFOUND )
> +    {
> +        printk(XENLOG_ERR "Cannot remove entry that does not exist");
> +        rc = -EINVAL;
Here you assing rc but ...

> +    }
> +
> +    /* As we are unmapping entire region use MPUMAP_REGION_FOUND instead */
> +    rc = xen_mpumap_free_entry(idx, MPUMAP_REGION_FOUND);
here you would redefine it.

> +
> +    spin_unlock(&xen_mpumap_lock);
> +
> +    return rc;
> +}
> +
>  int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
>                       unsigned int flags)
>  {
> diff --git a/xen/arch/arm/mpu/vmap.c b/xen/arch/arm/mpu/vmap.c
> index f977b79cd4..d3037ae98a 100644
> --- a/xen/arch/arm/mpu/vmap.c
> +++ b/xen/arch/arm/mpu/vmap.c
> @@ -1,19 +1,27 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
>  #include <xen/bug.h>
> +#include <xen/mm.h>
>  #include <xen/mm-frame.h>
>  #include <xen/types.h>
>  #include <xen/vmap.h>
>  
>  void *vmap_contig(mfn_t mfn, unsigned int nr)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    paddr_t base = mfn_to_maddr(mfn);
> +
> +    if ( map_pages_to_xen(base, mfn, nr, PAGE_HYPERVISOR ) )
> +        return NULL;
> +
> +    return maddr_to_virt(base);
>  }
>  
>  void vunmap(const void *va)
>  {
> -    BUG_ON("unimplemented");
> +    paddr_t base = virt_to_maddr(va);
> +
> +    if ( destroy_entire_xen_mapping(base) )
> +        panic("Failed to vunmap region\n");
Looking at common vunmap() we ignore the return code from
destroy_xen_mappings(). Is it ok to diverge?

~Michal

>  }
>  
>  /*

Re: [PATCH 2/6] arm/mpu: Implement vmap functions for MPU

[Luca Fancellu]

Hi Michal,

thanks for your review, I’ll answer only few of your points and I’ll let Harry take the rest.

> On 8 Dec 2025, at 09:53, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 28/11/2025 10:58, Harry Ramsey wrote:
>> From: Luca Fancellu <luca.fancellu@arm.com>
>> 
>> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
> Just as a reminder, we don't intend to support VMAP on MPU? Asking because it
> would otherwise be a duplicate effort to implement only these two helpers.

Yes we are not going to support VMAP on MPU, here we want only to provide the
implementation of these two helper so that we don’t touch the common code using these.
Are you suggesting some rewording or was it only a curiosity on your side?

>> 
>> +/*
>> + * Free a xen_mpumap entry given the index. A mpu region is actually disabled
>> + * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
>> + *
>> + * @param idx                   Index of the mpumap entry.
>> + * @param region_found_type     Either MPUMAP_REGION_FOUND or MPUMAP_REGION_INCLUSIVE.
> Both of these are unsigned, so why the parameter is int?

Uhm, good catch, I think this is a documentation issue because it might be also MPUMAP_REGION_OVERLAP which is
-1, we should not mandate which value might be here, we should only say MPUMAP_REGION_* values.


>> 
>> 
>> void vunmap(const void *va)
>> {
>> -    BUG_ON("unimplemented");
>> +    paddr_t base = virt_to_maddr(va);
>> +
>> +    if ( destroy_entire_xen_mapping(base) )
>> +        panic("Failed to vunmap region\n");
> Looking at common vunmap() we ignore the return code from
> destroy_xen_mappings(). Is it ok to diverge?

In our tests it’s ok, I asked Harry to have this so that we can catch any vunmap that is not balanced
with a prior mapping. To be fair I’m not really sure why in the vmap.c implementation we are not
reading the error codes from destroy_xen_mappings.

Cheers,
Luca

Re: [PATCH 2/6] arm/mpu: Implement vmap functions for MPU

[Harry Ramsey]

On 08/12/2025 09:53, Orzel, Michal wrote:
>
> On 28/11/2025 10:58, Harry Ramsey wrote:
>> From: Luca Fancellu <luca.fancellu@arm.com>
>>
>> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
> Just as a reminder, we don't intend to support VMAP on MPU? Asking because it
> would otherwise be a duplicate effort to implement only these two helpers.
>
>> in places across common code. In order to keep the existing code and
>> maintain correct functionality, implement the `vmap_contig` and `vunmap`
>> functions for MPU systems.
>>
>> Introduce a helper function `destroy_entire_xen_mapping` to aid with
>> unmapping an entire region when only the start address is known.
>>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
>> ---
>>   xen/arch/arm/include/asm/mpu/mm.h | 11 +++++
>>   xen/arch/arm/mpu/mm.c             | 67 +++++++++++++++++++++++++------
>>   xen/arch/arm/mpu/vmap.c           | 14 +++++--
>>   3 files changed, 77 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
>> index e1ded6521d..83ee0e59ca 100644
>> --- a/xen/arch/arm/include/asm/mpu/mm.h
>> +++ b/xen/arch/arm/include/asm/mpu/mm.h
>> @@ -111,6 +111,17 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
>>   int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>>                              paddr_t limit, uint8_t *index);
>>   
>> +
>> +/*
>> + * Destroys and frees (if reference count is 0) an entire xen mapping on MPU
>> + * systems where only the start address is known.
>> + *
>> + * @param s     Start address of memory region to be destroyed.
>> + *
>> + * @return:     0 on success, negative on error.
>> + */
>> +int destroy_entire_xen_mapping(paddr_t s);
> NIT: I read it as all the mappings which is a bit misleading :)
> Maybe something like: destroy_mapping_containing or alike?
>
>> +
>>   #endif /* __ARM_MPU_MM_H__ */
>>   
>>   /*
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 687dec3bc6..29d8e7ff11 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -290,6 +290,35 @@ static void disable_mpu_region_from_index(uint8_t index)
>>           write_protection_region(&xen_mpumap[index], index);
>>   }
>>   
>> +/*
>> + * Free a xen_mpumap entry given the index. A mpu region is actually disabled
>> + * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
>> + *
>> + * @param idx                   Index of the mpumap entry.
>> + * @param region_found_type     Either MPUMAP_REGION_FOUND or MPUMAP_REGION_INCLUSIVE.
> Both of these are unsigned, so why the parameter is int?
>
>> + * @return                      0 on success, otherwise negative on error.
>> + */
>> +static int xen_mpumap_free_entry(uint8_t idx, int region_found_type)
>> +{
>> +    ASSERT(spin_is_locked(&xen_mpumap_lock));
>> +    ASSERT(idx != INVALID_REGION_IDX);
>> +
>> +    if ( xen_mpumap[idx].refcount == 0 )
>> +    {
>> +        if ( MPUMAP_REGION_FOUND == region_found_type )
>> +            disable_mpu_region_from_index(idx);
>> +        else
>> +        {
>> +            printk(XENLOG_ERR "Cannot remove a partial region\n");
>> +            return -EINVAL;
>> +        }
>> +    }
>> +    else
>> +        xen_mpumap[idx].refcount -= 1;
> You could avoid nesting if/else by doing:
>      if ( xen_mpumap[idx].refcount )
>      {
>          xen_mpumap[idx].refcount -= 1;
>          return 0;
>      }
>
>      if ( MPUMAP_REGION_FOUND == region_found_type )
>          disable_mpu_region_from_index(idx);
>      else
>      {
>          printk(XENLOG_ERR "Cannot remove a partial region\n");
>          return -EINVAL;
>      }
>
>> +
>> +    return 0;
>> +}
>> +
>>   /*
>>    * Update the entry in the MPU memory region mapping table (xen_mpumap) for the
>>    * given memory range and flags, creating one if none exists.
>> @@ -357,18 +386,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>>               return -EINVAL;
>>           }
>>   
>> -        if ( xen_mpumap[idx].refcount == 0 )
>> -        {
>> -            if ( MPUMAP_REGION_FOUND == rc )
>> -                disable_mpu_region_from_index(idx);
>> -            else
>> -            {
>> -                printk("Cannot remove a partial region\n");
>> -                return -EINVAL;
>> -            }
>> -        }
>> -        else
>> -            xen_mpumap[idx].refcount -= 1;
>> +        return xen_mpumap_free_entry(idx, rc);
>>       }
>>   
>>       return 0;
>> @@ -418,6 +436,31 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
>>       return xen_mpumap_update(s, e, 0);
>>   }
>>   
>> +int destroy_entire_xen_mapping(paddr_t s)
>> +{
>> +    int rc;
>> +    uint8_t idx;
>> +
>> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>> +
>> +    spin_lock(&xen_mpumap_lock);
>> +
>> +    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s + PAGE_SIZE,
>> +                                &idx);
> So here you are searching for a region that includes at least s + PAGE_SIZE.
>
>> +    if ( rc == MPUMAP_REGION_NOTFOUND )
>> +    {
>> +        printk(XENLOG_ERR "Cannot remove entry that does not exist");
>> +        rc = -EINVAL;
> Here you assing rc but ...
>
>> +    }
>> +
>> +    /* As we are unmapping entire region use MPUMAP_REGION_FOUND instead */
>> +    rc = xen_mpumap_free_entry(idx, MPUMAP_REGION_FOUND);
> here you would redefine it.


Sorry, this is a mistake and we should just return early as the mapping 
doesn't exist and therefore will have no references to be decremented.

>> +
>> +    spin_unlock(&xen_mpumap_lock);
>> +
>> +    return rc;
>> +}
>> +
>>   int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
>>                        unsigned int flags)
>>   {
>> diff --git a/xen/arch/arm/mpu/vmap.c b/xen/arch/arm/mpu/vmap.c
>> index f977b79cd4..d3037ae98a 100644
>> --- a/xen/arch/arm/mpu/vmap.c
>> +++ b/xen/arch/arm/mpu/vmap.c
>> @@ -1,19 +1,27 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>>   
>>   #include <xen/bug.h>
>> +#include <xen/mm.h>
>>   #include <xen/mm-frame.h>
>>   #include <xen/types.h>
>>   #include <xen/vmap.h>
>>   
>>   void *vmap_contig(mfn_t mfn, unsigned int nr)
>>   {
>> -    BUG_ON("unimplemented");
>> -    return NULL;
>> +    paddr_t base = mfn_to_maddr(mfn);
>> +
>> +    if ( map_pages_to_xen(base, mfn, nr, PAGE_HYPERVISOR ) )
>> +        return NULL;
>> +
>> +    return maddr_to_virt(base);
>>   }
>>   
>>   void vunmap(const void *va)
>>   {
>> -    BUG_ON("unimplemented");
>> +    paddr_t base = virt_to_maddr(va);
>> +
>> +    if ( destroy_entire_xen_mapping(base) )
>> +        panic("Failed to vunmap region\n");
> Looking at common vunmap() we ignore the return code from
> destroy_xen_mappings(). Is it ok to diverge?
>
> ~Michal
>
>>   }
>>   
>>   /*


I will address the code style comments in the next version of these patches.

Thanks,

Harry Ramsey

Re: [PATCH 2/6] arm/mpu: Implement vmap functions for MPU

[Orzel, Michal]

On 08/12/2025 11:20, Luca Fancellu wrote:
> Hi Michal,
> 
> thanks for your review, I’ll answer only few of your points and I’ll let Harry take the rest.
> 
>> On 8 Dec 2025, at 09:53, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>
>>
>>
>> On 28/11/2025 10:58, Harry Ramsey wrote:
>>> From: Luca Fancellu <luca.fancellu@arm.com>
>>>
>>> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
>> Just as a reminder, we don't intend to support VMAP on MPU? Asking because it
>> would otherwise be a duplicate effort to implement only these two helpers.
> 
> Yes we are not going to support VMAP on MPU, here we want only to provide the
> implementation of these two helper so that we don’t touch the common code using these.
> Are you suggesting some rewording or was it only a curiosity on your side?
It was a question to check whether we are aligned.

> 
>>>
>>> +/*
>>> + * Free a xen_mpumap entry given the index. A mpu region is actually disabled
>>> + * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
>>> + *
>>> + * @param idx                   Index of the mpumap entry.
>>> + * @param region_found_type     Either MPUMAP_REGION_FOUND or MPUMAP_REGION_INCLUSIVE.
>> Both of these are unsigned, so why the parameter is int?
> 
> Uhm, good catch, I think this is a documentation issue because it might be also MPUMAP_REGION_OVERLAP which is
> -1, we should not mandate which value might be here, we should only say MPUMAP_REGION_* values.
> 
> 
>>>
>>>
>>> void vunmap(const void *va)
>>> {
>>> -    BUG_ON("unimplemented");
>>> +    paddr_t base = virt_to_maddr(va);
>>> +
>>> +    if ( destroy_entire_xen_mapping(base) )
>>> +        panic("Failed to vunmap region\n");
>> Looking at common vunmap() we ignore the return code from
>> destroy_xen_mappings(). Is it ok to diverge?
> 
> In our tests it’s ok, I asked Harry to have this so that we can catch any vunmap that is not balanced
> with a prior mapping. To be fair I’m not really sure why in the vmap.c implementation we are not
> reading the error codes from destroy_xen_mappings.
I'm ok with that. I don't know why we don't check the rc there. If you look at
x86 destroy_xen_mappings calls, none seem to be checked against rc. It can be
that it's impossible scenario there.

~Michal

> 
> Cheers,
> Luca
> 

Re: [PATCH 3/6] arm/mpu: Implement free_init_memory for MPU systems

[Orzel, Michal]

On 28/11/2025 10:58, Harry Ramsey wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> Implement the function `free_init_memory` for MPU systems. In order to
> support this, the function `modify_xen_mappings` is implemented.
> 
> On MPU systems, we map the init text and init data sections using
> separate MPU memory regions. Therefore these are removed separately in
> `free_init_memory`.
> 
> Additionally remove warning messages from `is_mm_attr_match` as some
> permissions can now be updated by `xen_mpumap_update_entry`.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
>  xen/arch/arm/include/asm/setup.h |  2 +
>  xen/arch/arm/mpu/mm.c            | 91 +++++++++++++++++++++++++-------
>  2 files changed, 73 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 1eaf13bd66..005cf7be59 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -65,6 +65,8 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
>  int map_range_to_domain(const struct dt_device_node *dev,
>                          uint64_t addr, uint64_t len, void *data);
>  
> +extern const char __init_data_begin[], __bss_start[], __bss_end[];
> +
>  struct init_info
>  {
>      /* Pointer to the stack, used by head.S when entering in C */
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 29d8e7ff11..8446dddde8 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -174,28 +174,13 @@ int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>  static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
>  {
>      if ( region->prbar.reg.ro != PAGE_RO_MASK(attributes) )
> -    {
> -        printk(XENLOG_WARNING
> -               "Mismatched Access Permission attributes (%#x instead of %#x)\n",
> -               region->prbar.reg.ro, PAGE_RO_MASK(attributes));
>          return false;
> -    }
>  
>      if ( region->prbar.reg.xn != PAGE_XN_MASK(attributes) )
> -    {
> -        printk(XENLOG_WARNING
> -               "Mismatched Execute Never attributes (%#x instead of %#x)\n",
> -               region->prbar.reg.xn, PAGE_XN_MASK(attributes));
>          return false;
> -    }
>  
>      if ( region->prlar.reg.ai != PAGE_AI_MASK(attributes) )
This check and ...

> -    {
> -        printk(XENLOG_WARNING
> -               "Mismatched Memory Attribute Index (%#x instead of %#x)\n",
> -               region->prlar.reg.ai, PAGE_AI_MASK(attributes));
>          return false;
> -    }
>  
>      return true;
>  }
> @@ -352,8 +337,33 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>      {
>          if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
>          {
> -            printk("Modifying an existing entry is not supported\n");
> -            return -EINVAL;
> +            if ( rc == MPUMAP_REGION_INCLUSIVE )
> +            {
> +                printk(XENLOG_ERR
> +                       "Cannot modify partial region permissions\n");
> +                return -EINVAL;
> +            }
> +
> +            if ( xen_mpumap[idx].prlar.reg.ai != PAGE_AI_MASK(flags) )
this one are identical. Why do we duplicate it here? If this is because
is_mm_attr_match returns just bool, then maybe you want to introduce more
logical return values and parse them here. What you eventually do here is you
allow modifying regions for RO and XN. Therefore is_mm_attr_match could return
true on RO and XN but false on AI.

> +            {
> +                printk(XENLOG_ERR
> +                       "Modifying memory attribute is not supported\n");
> +                return -EINVAL;
> +            }
> +
> +            if ( xen_mpumap[idx].refcount != 0 )
> +            {
> +                printk(XENLOG_ERR
> +                       "Cannot modify memory permissions for a region mapped multiple time\n");
s/time/times/

> +                return -EINVAL;
> +            }
> +
> +            /* Set new permission */
> +            xen_mpumap[idx].prbar.reg.ro = PAGE_RO_MASK(flags);
> +            xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
> +
> +            write_protection_region(&xen_mpumap[idx], idx);
> +            return 0;
>          }
>  
>          /* Check for overflow of refcount before incrementing.  */
> @@ -499,8 +509,7 @@ void __init setup_mm_helper(void)
>  
>  int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
>  {
> -    BUG_ON("unimplemented");
> -    return -EINVAL;
> +    return xen_mpumap_update(s, e, nf);
>  }
>  
>  void dump_hyp_walk(vaddr_t addr)
> @@ -511,7 +520,49 @@ void dump_hyp_walk(vaddr_t addr)
>  /* Release all __init and __initdata ranges to be reused */
>  void free_init_memory(void)
>  {
> -    BUG_ON("unimplemented");
> +    unsigned long inittext_end = round_pgup((unsigned long)__init_data_begin);
Looking at the linker script, __init_data_begin is already page aligned.

> +    unsigned long len = __init_end - __init_begin;
> +    uint8_t idx;
> +    int rc;
> +
> +    rc = modify_xen_mappings((unsigned long)__init_begin, inittext_end,
> +                             PAGE_HYPERVISOR_RW);
So here you modify mapping of text section but...

> +    if ( rc )
> +        panic("Unable to map RW the init text section (rc = %d)\n", rc);
> +
> +    /*
> +     * From now on, init will not be used for execution anymore,
> +     * so nuke the instruction cache to remove entries related to init.
> +     */
> +    invalidate_icache_local();
> +
> +    /* Zeroing the memory before returning it */
> +    memset(__init_begin, 0, len);
here you zero the entire init. Is it because init.data is already RW, so you
don't need to modify the mappings? If so, more informative comments would be
welcome.

> +
> +    rc = destroy_xen_mappings((unsigned long)__init_begin, inittext_end);
> +    if ( rc )
> +        panic("Unable to remove init text section (rc = %d)\n", rc);
> +
> +    /*
> +     * The initdata and bss sections are mapped using a single MPU region, so
> +     * modify the start of this region to remove the initdata section.
> +     */
> +    spin_lock(&xen_mpumap_lock);
> +
> +    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions,
> +                                (unsigned long)__init_data_begin,
> +                                (unsigned long)__bss_end,
> +                                &idx);
> +    if ( rc < MPUMAP_REGION_FOUND )
> +        panic("Unable to find bss data section (rc = %d)\n", rc);
> +
> +    /* bss data section is shrunk and now starts from __bss_start */
> +    pr_set_base(&xen_mpumap[idx], (unsigned long)__bss_start);
> +
> +    write_protection_region(&xen_mpumap[idx], idx);
> +    context_sync_mpu();
> +
> +    spin_unlock(&xen_mpumap_lock);
>  }
>  
>  void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int flags)

~Michal

Re: [PATCH 5/6] arm: Use secure hypervisor timer in MPU system

[Orzel, Michal]

On 28/11/2025 10:58, Harry Ramsey wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> As MPU systems only have one secure state, we have to use secure EL2
> hypervisor timer for Xen in secure EL2.
> 
> In this patch, we introduce a new Kconfig option ARM_SECURE_STATE
> and a set of secure hypervisor timer registers CNTHPS_*_EL2.
> We alias CNTHP_*_EL2 to CNTHPS_*_EL2 to keep the timer code
> flow unchanged.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
>  xen/arch/arm/Kconfig                     |  5 +++++
>  xen/arch/arm/include/asm/arm64/sysregs.h | 12 ++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index cf6af68299..a5c111e08e 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -91,6 +91,7 @@ config MMU
>  
>  config MPU
>  	bool "MPU" if UNSUPPORTED
> +	select ARM_SECURE_STATE if ARM_64
>  	select STATIC_MEMORY
>  	help
>  	  Memory Protection Unit (MPU). Select if you plan to run Xen on ARMv8-R
> @@ -223,6 +224,10 @@ config HARDEN_BRANCH_PREDICTOR
>  
>  	  If unsure, say Y.
>  
> +config ARM_SECURE_STATE
> +	bool "Xen will run in Arm Secure State"
> +	default n
No need, n is a default. I don't think this should be a selectable option.

> +
>  config ARM64_PTR_AUTH
>  	def_bool n
>  	depends on ARM_64
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index 7440d495e4..29caad7155 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -462,6 +462,18 @@
>  #define ZCR_ELx_LEN_SIZE             9
>  #define ZCR_ELx_LEN_MASK             0x1ff
>  
> +#ifdef CONFIG_ARM_SECURE_STATE
> +/*
> + * The Armv8-R AArch64 architecture always executes code in Secure
> + * state with EL2 as the highest Exception.
s/Exception/exception level/

> + *
> + * Hypervisor timer registers for Secure EL2.
> + */
> +#define CNTHP_TVAL_EL2  CNTHPS_TVAL_EL2
TVAL is not used in Xen, so you can drop it

> +#define CNTHP_CTL_EL2   CNTHPS_CTL_EL2
> +#define CNTHP_CVAL_EL2  CNTHPS_CVAL_EL2
EL1 will still use the NS EL1 timer (CNTP)?

> +#endif
> +
>  #define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
>  #define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */
>  #define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */

~Michal

Re: [PATCH 4/6] arm/mpu: Introduce modify_after_init_mappings

[Orzel, Michal]

On 28/11/2025 10:58, Harry Ramsey wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> During `init_done`, Xen sets the permissions of all symbols marked with
> __ro_after_init to be read-only. Currently this is achieved by calling
> `modify_xen_mappings` and will shrink the RW mapping on one side and
> extend the RO mapping on the other.
Can you be more specific about the sides you mention? How did you deduce it?
I assume you are talking about MMU part.

> 
> This does not work on MPU systems at present because part-region
> modification is not supported. Therefore introduce the function
What else is in that region?
Wouldn't it be better to have one region for this __ro_after_init so that we
don't need to shrink/extend the mappings? Is it done because of number of
regions limitation?

~Michal

> `modify_after_init_mappings` for MMU and MPU, to handle the divergent
> approaches to setting permissions of __ro_after_init symbols.
> 
> As the new function is marked with __init, it needs to be called before
> `free_init_memory`.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
>  xen/arch/arm/include/asm/setup.h |  3 +++
>  xen/arch/arm/mmu/setup.c         | 15 ++++++++++++
>  xen/arch/arm/mpu/mm.c            |  2 +-
>  xen/arch/arm/mpu/setup.c         | 40 ++++++++++++++++++++++++++++++++
>  xen/arch/arm/setup.c             | 15 ++----------
>  5 files changed, 61 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 005cf7be59..899e33925c 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -78,6 +78,9 @@ struct init_info
>  paddr_t consider_modules(paddr_t s, paddr_t e, uint32_t size, paddr_t align,
>                           int first_mod);
>  
> +/* Modify some mappings after the init is done */
> +void modify_after_init_mappings(void);
> +
>  #endif
>  /*
>   * Local variables:
> diff --git a/xen/arch/arm/mmu/setup.c b/xen/arch/arm/mmu/setup.c
> index 9b874f8ab2..d042f73597 100644
> --- a/xen/arch/arm/mmu/setup.c
> +++ b/xen/arch/arm/mmu/setup.c
> @@ -213,6 +213,21 @@ void __init remove_early_mappings(void)
>      BUG_ON(rc);
>  }
>  
> +void __init modify_after_init_mappings(void)
> +{
> +    /*
> +     * We have finished booting. Mark the section .data.ro_after_init
> +     * read-only.
> +     */
> +    int rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
> +                                 (unsigned long)&__ro_after_init_end,
> +                                 PAGE_HYPERVISOR_RO);
> +
> +    if ( rc )
> +        panic("Unable to mark the .data.ro_after_init section read-only (rc = %d)\n",
> +              rc);
> +}
> +
>  /*
>   * After boot, Xen page-tables should not contain mapping that are both
>   * Writable and eXecutables.
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 8446dddde8..f95ba7c749 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -32,7 +32,7 @@ DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
>  /* EL2 Xen MPU memory region mapping table. */
>  pr_t __cacheline_aligned __section(".data") xen_mpumap[MAX_MPU_REGION_NR];
>  
> -static DEFINE_SPINLOCK(xen_mpumap_lock);
> +DEFINE_SPINLOCK(xen_mpumap_lock);
>  
>  static void __init __maybe_unused build_assertions(void)
>  {
> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
> index ec264f54f2..55317ee318 100644
> --- a/xen/arch/arm/mpu/setup.c
> +++ b/xen/arch/arm/mpu/setup.c
> @@ -8,11 +8,14 @@
>  #include <xen/pfn.h>
>  #include <xen/types.h>
>  #include <xen/sizes.h>
> +#include <xen/spinlock.h>
>  #include <asm/setup.h>
>  
>  static paddr_t __initdata mapped_fdt_base = INVALID_PADDR;
>  static paddr_t __initdata mapped_fdt_limit = INVALID_PADDR;
>  
> +extern spinlock_t xen_mpumap_lock;
> +
>  void __init setup_pagetables(void) {}
>  
>  void * __init early_fdt_map(paddr_t fdt_paddr)
> @@ -106,6 +109,43 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
>          panic("Unable to unmap range for copy_from_paddr\n");
>  }
>  
> +void __init modify_after_init_mappings(void)
> +{
> +    int rc;
> +    uint8_t idx_rodata;
> +    uint8_t idx_rwdata;
> +
> +    spin_lock(&xen_mpumap_lock);
> +
> +    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions,
> +                                (unsigned long)_srodata,
> +                                (unsigned long)_erodata,
> +                                &idx_rodata);
> +
> +    if ( rc < MPUMAP_REGION_FOUND )
> +        panic("Unable to find rodata section (rc = %d)\n", rc);
> +
> +    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions,
> +                                (unsigned long)__ro_after_init_start,
> +                                (unsigned long)__init_begin,
> +                                &idx_rwdata);
> +
> +    if ( rc < MPUMAP_REGION_FOUND )
> +        panic("Unable to find rwdata section (rc = %d)\n", rc);
> +
> +    /* Shrink rwdata section to begin at __ro_after_init_end */
> +    pr_set_base(&xen_mpumap[idx_rwdata], (unsigned long)__ro_after_init_end);
> +
> +    /* Extend rodata section to end at __ro_after_init_end */
> +    pr_set_limit(&xen_mpumap[idx_rodata], (unsigned long)__ro_after_init_end);
> +
> +    write_protection_region(&xen_mpumap[idx_rwdata], idx_rwdata);
> +    write_protection_region(&xen_mpumap[idx_rodata], idx_rodata);
> +    context_sync_mpu();
> +
> +    spin_unlock(&xen_mpumap_lock);
> +}
> +
>  void __init remove_early_mappings(void)
>  {
>      int rc = destroy_xen_mappings(round_pgdown(mapped_fdt_base),
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 7ad870e382..6310a47d68 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -66,23 +66,12 @@ domid_t __read_mostly max_init_domid;
>  
>  static __used void noreturn init_done(void)
>  {
> -    int rc;
> -
>      /* Must be done past setting system_state. */
>      unregister_init_virtual_region();
>  
> -    free_init_memory();
> +    modify_after_init_mappings();
>  
> -    /*
> -     * We have finished booting. Mark the section .data.ro_after_init
> -     * read-only.
> -     */
> -    rc = modify_xen_mappings((unsigned long)&__ro_after_init_start,
> -                             (unsigned long)&__ro_after_init_end,
> -                             PAGE_HYPERVISOR_RO);
> -    if ( rc )
> -        panic("Unable to mark the .data.ro_after_init section read-only (rc = %d)\n",
> -              rc);
> +    free_init_memory();
>  
>      startup_cpu_idle_loop();
>  }

Re: [PATCH 6/6] arm/mpu: Map domain page in AArch64 MPU systems

[Orzel, Michal]

On 28/11/2025 10:58, Harry Ramsey wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> In MPU systems, we implement map_domain_page()/unmap_domain_page()
> through mapping the domain page with a MPU region on demand.
What prevents you from implementing the remaining few helpers?

~Michal

> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
>  xen/arch/arm/Kconfig           |  1 +
>  xen/arch/arm/mpu/Makefile      |  1 +
>  xen/arch/arm/mpu/domain-page.c | 53 ++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+)
>  create mode 100644 xen/arch/arm/mpu/domain-page.c
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index a5c111e08e..dac9a16c28 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -91,6 +91,7 @@ config MMU
>  
>  config MPU
>  	bool "MPU" if UNSUPPORTED
> +	select ARCH_MAP_DOMAIN_PAGE if ARM_64
>  	select ARM_SECURE_STATE if ARM_64
>  	select STATIC_MEMORY
>  	help
> diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
> index 4963c8b550..940297af3f 100644
> --- a/xen/arch/arm/mpu/Makefile
> +++ b/xen/arch/arm/mpu/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_ARM_32) += arm32/
>  obj-$(CONFIG_ARM_64) += arm64/
> +obj-$(CONFIG_ARM_64) += domain-page.o
>  obj-y += mm.o
>  obj-y += p2m.o
>  obj-y += setup.init.o
> diff --git a/xen/arch/arm/mpu/domain-page.c b/xen/arch/arm/mpu/domain-page.c
> new file mode 100644
> index 0000000000..9248053ff5
> --- /dev/null
> +++ b/xen/arch/arm/mpu/domain-page.c
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/bug.h>
> +#include <xen/domain_page.h>
> +#include <xen/mm.h>
> +#include <xen/mm-frame.h>
> +#include <xen/types.h>
> +
> +void *map_domain_page_global(mfn_t mfn)
> +{
> +    BUG_ON("unimplemented");
> +    return NULL;
> +}
> +
> +/* Map a page of domheap memory */
> +void *map_domain_page(mfn_t mfn)
> +{
> +    paddr_t pa = mfn_to_maddr(mfn);
> +
> +    if ( map_pages_to_xen((unsigned long)pa, mfn, 1, PAGE_HYPERVISOR_RW) )
> +        return NULL;
> +
> +    return maddr_to_virt(pa);
> +}
> +
> +/* Release a mapping taken with map_domain_page() */
> +void unmap_domain_page(const void *ptr)
> +{
> +    paddr_t base = virt_to_maddr(ptr);
> +
> +    if ( destroy_entire_xen_mapping(base) )
> +        panic("Failed to unmap domain page\n");
> +}
> +
> +mfn_t domain_page_map_to_mfn(const void *ptr)
> +{
> +    BUG_ON("unimplemented");
> +    return INVALID_MFN;
> +}
> +
> +void unmap_domain_page_global(const void *va)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Re: [PATCH 6/6] arm/mpu: Map domain page in AArch64 MPU systems

[Luca Fancellu]

Hi Michael,

> On 16 Dec 2025, at 09:26, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 28/11/2025 10:58, Harry Ramsey wrote:
>> From: Penny Zheng <Penny.Zheng@arm.com>
>> 
>> In MPU systems, we implement map_domain_page()/unmap_domain_page()
>> through mapping the domain page with a MPU region on demand.
> What prevents you from implementing the remaining few helpers?

Only the fact that they are not used at this stage, otherwise we would have seen
a panic while running Linux.

Cheers,
Luca

Re: [PATCH 6/6] arm/mpu: Map domain page in AArch64 MPU systems

[Orzel, Michal]

On 16/12/2025 10:29, Luca Fancellu wrote:
> Hi Michael,
> 
>> On 16 Dec 2025, at 09:26, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>
>>
>>
>> On 28/11/2025 10:58, Harry Ramsey wrote:
>>> From: Penny Zheng <Penny.Zheng@arm.com>
>>>
>>> In MPU systems, we implement map_domain_page()/unmap_domain_page()
>>> through mapping the domain page with a MPU region on demand.
>> What prevents you from implementing the remaining few helpers?
> 
> Only the fact that they are not used at this stage, otherwise we would have seen
> a panic while running Linux.
Sure but it looks like that they would also be a few-liners hence there is a
feeling that they could all be done in one go for completeness sake.

~Michal

Re: [PATCH 6/6] arm/mpu: Map domain page in AArch64 MPU systems

[Luca Fancellu]

> On 16 Dec 2025, at 09:32, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 16/12/2025 10:29, Luca Fancellu wrote:
>> Hi Michael,
>> 
>>> On 16 Dec 2025, at 09:26, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>> 
>>> 
>>> 
>>> On 28/11/2025 10:58, Harry Ramsey wrote:
>>>> From: Penny Zheng <Penny.Zheng@arm.com>
>>>> 
>>>> In MPU systems, we implement map_domain_page()/unmap_domain_page()
>>>> through mapping the domain page with a MPU region on demand.
>>> What prevents you from implementing the remaining few helpers?
>> 
>> Only the fact that they are not used at this stage, otherwise we would have seen
>> a panic while running Linux.
> Sure but it looks like that they would also be a few-liners hence there is a
> feeling that they could all be done in one go for completeness sake.

yeah but we don’t have the confidence it works because we can’t test

Cheers,
Luca

Re: [PATCH 4/6] arm/mpu: Introduce modify_after_init_mappings

[Luca Fancellu]

Hi Michael,

> On 16 Dec 2025, at 09:15, Orzel, Michal <Michal.Orzel@amd.com> wrote:
> 
> 
> 
> On 28/11/2025 10:58, Harry Ramsey wrote:
>> From: Luca Fancellu <luca.fancellu@arm.com>
>> 
>> During `init_done`, Xen sets the permissions of all symbols marked with
>> __ro_after_init to be read-only. Currently this is achieved by calling
>> `modify_xen_mappings` and will shrink the RW mapping on one side and
>> extend the RO mapping on the other.
> Can you be more specific about the sides you mention? How did you deduce it?
> I assume you are talking about MMU part.

You are right, this sentence is a bit misleading.
So what was written here was meant to say that on MPU modify_xen_mappings
should shrink the RW mapping region and extend the RO mapping region because
as of now they are declared like this in xen.lds.S:

read only data:
+------------------+
| _srodata         |
| _erodata         |
+-------------------+

RW data:
+---------------------------+
| __ro_after_init_start |
| __ro_after_init_end  |
| __init_begin              |
+---------------------------+

And in head.S we map like this:

/* Xen read-only data section. */
ldr x1, =_srodata
ldr x2, =_erodata
prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR

/* Xen read-only after init and data section. (RW data) */
ldr x1, =__ro_after_init_start
ldr x2, =__init_begin
prepare_xen_region x0, x1, x2, x3, x4, x5

Now, because (__ro_after_init_start, __ro_after_init_end) needs to become RO,
it means that RO section will be extended to (_srodata, __ro_after_init_end) and
RW section will be shrinked to (__ro_after_init_end, __init_begin):

read only data region:
+--------------------------+
| _srodata                  |
| __ro_after_init_end |
+--------------------------+

RW data region:
+---------------------------+
| __ro_after_init_end  |
| __init_begin              |
+---------------------------+

So what we’ve done is taking (__ro_after_init_start, __ro_after_init_end) from
the RW region and attach it to the RO region.

> 
>> 
>> This does not work on MPU systems at present because part-region
>> modification is not supported. Therefore introduce the function
> What else is in that region?
> Wouldn't it be better to have one region for this __ro_after_init so that we
> don't need to shrink/extend the mappings? Is it done because of number of
> regions limitation?

So if we do in this way we waste one region, because we will have 2 region declared
RO that are also contiguous, so easily mergeable, like how we are doing above by
Extending/shrinking.

Cheers,
Luca

Re: [PATCH 4/6] arm/mpu: Introduce modify_after_init_mappings

[Orzel, Michal]

On 16/12/2025 14:11, Luca Fancellu wrote:
> Hi Michael,
> 
>> On 16 Dec 2025, at 09:15, Orzel, Michal <Michal.Orzel@amd.com> wrote:
>>
>>
>>
>> On 28/11/2025 10:58, Harry Ramsey wrote:
>>> From: Luca Fancellu <luca.fancellu@arm.com>
>>>
>>> During `init_done`, Xen sets the permissions of all symbols marked with
>>> __ro_after_init to be read-only. Currently this is achieved by calling
>>> `modify_xen_mappings` and will shrink the RW mapping on one side and
>>> extend the RO mapping on the other.
>> Can you be more specific about the sides you mention? How did you deduce it?
>> I assume you are talking about MMU part.
> 
> You are right, this sentence is a bit misleading.
> So what was written here was meant to say that on MPU modify_xen_mappings
> should shrink the RW mapping region and extend the RO mapping region because
> as of now they are declared like this in xen.lds.S:
> 
> read only data:
> +------------------+
> | _srodata         |
> | _erodata         |
> +-------------------+
> 
> RW data:
> +---------------------------+
> | __ro_after_init_start |
> | __ro_after_init_end  |
> | __init_begin              |
> +---------------------------+
> 
> And in head.S we map like this:
> 
> /* Xen read-only data section. */
> ldr x1, =_srodata
> ldr x2, =_erodata
> prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> 
> /* Xen read-only after init and data section. (RW data) */
> ldr x1, =__ro_after_init_start
> ldr x2, =__init_begin
> prepare_xen_region x0, x1, x2, x3, x4, x5
> 
> Now, because (__ro_after_init_start, __ro_after_init_end) needs to become RO,
> it means that RO section will be extended to (_srodata, __ro_after_init_end) and
> RW section will be shrinked to (__ro_after_init_end, __init_begin):
> 
> read only data region:
> +--------------------------+
> | _srodata                  |
> | __ro_after_init_end |
> +--------------------------+
> 
> RW data region:
> +---------------------------+
> | __ro_after_init_end  |
> | __init_begin              |
> +---------------------------+
> 
> So what we’ve done is taking (__ro_after_init_start, __ro_after_init_end) from
> the RW region and attach it to the RO region.
> 
>>
>>>
>>> This does not work on MPU systems at present because part-region
>>> modification is not supported. Therefore introduce the function
>> What else is in that region?
>> Wouldn't it be better to have one region for this __ro_after_init so that we
>> don't need to shrink/extend the mappings? Is it done because of number of
>> regions limitation?
> 
> So if we do in this way we waste one region, because we will have 2 region declared
> RO that are also contiguous, so easily mergeable, like how we are doing above by
> Extending/shrinking.
Ok, that makes more sense. I thought the descrption in commit msg was somehow
about MMU. It's not ideal to depend on the regions layout but I guess it's ok
if we don't want to waste regions.

~Michal