[PATCH v3 0/5] Third series for R82 MPU support

[PATCH v3 0/5] Third series for R82 MPU support

[Hari Limaye]

Hi all,

This series is the third set of patches in the ongoing work to
introduce support for MPU systems and Cortex R82 in Xen.

The patches in this series implement the necessary logic to transiently
map and unmap the static memory banks for initialization.

Changes from v2:
- Changes mentioned in individual patches

Cheers,
Hari

Luca Fancellu (4):
  arm/mpu: Implement setup_frametable_mappings for MPU systems
  arm/mpu: Implement setup_mm for MPU systems
  arm/mpu: Implement transient mapping
  arm/mpu: Implement ioremap_attr for MPU

Penny Zheng (1):
  xen/arm: map static memory on demand

 xen/arch/arm/arm32/asm-offsets.c         |   3 +-
 xen/arch/arm/arm64/asm-offsets.c         |   2 +
 xen/arch/arm/include/asm/arm32/mpu.h     |   2 +
 xen/arch/arm/include/asm/arm64/mpu.h     |   2 +
 xen/arch/arm/include/asm/mmu/mm.h        |   3 +
 xen/arch/arm/include/asm/mpu/mm.h        |  40 +++-
 xen/arch/arm/include/asm/mpu/regions.inc |  17 +-
 xen/arch/arm/mpu/mm.c                    | 269 +++++++++++++++++++++--
 xen/arch/arm/mpu/setup.c                 |  11 +
 xen/include/xen/static-memory.h          |   8 +
 10 files changed, 335 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] arm/mpu: Implement setup_frametable_mappings for MPU systems

[Hari Limaye]

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

Implement the MPU variant of `setup_frametable_mappings`. This function
will be called by `setup_mm` when an implementation for MPU systems is
added in a follow up commit.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes from v1:
- Align ps and pe to page size
- Add sanity checking for frametable size, as in MMU version
- Add Michal's R-b
---
 xen/arch/arm/mpu/mm.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index c6891607ec..3f155b7db2 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -168,6 +168,39 @@ int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
     return MPUMAP_REGION_NOTFOUND;
 }
 
+/* Map a frame table to cover physical addresses ps through pe */
+void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
+{
+    mfn_t base_mfn;
+    paddr_t aligned_ps = ROUNDUP(ps, PAGE_SIZE);
+    paddr_t aligned_pe = ROUNDDOWN(pe, PAGE_SIZE);
+
+    unsigned long nr_pdxs = mfn_to_pdx(mfn_add(maddr_to_mfn(aligned_pe), -1)) -
+                            mfn_to_pdx(maddr_to_mfn(aligned_ps)) + 1;
+    unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
+
+    /*
+     * The size of paddr_t should be sufficient for the complete range of
+     * physical address.
+     */
+    BUILD_BUG_ON((sizeof(paddr_t) * BITS_PER_BYTE) < PADDR_BITS);
+    BUILD_BUG_ON(sizeof(struct page_info) != PAGE_INFO_SIZE);
+
+    if ( frametable_size > FRAMETABLE_SIZE )
+        panic("The frametable cannot cover the physical region %#"PRIpaddr" - %#"PRIpaddr"\n",
+              ps, pe);
+
+    frametable_base_pdx = paddr_to_pdx(aligned_ps);
+    frametable_size = ROUNDUP(frametable_size, PAGE_SIZE);
+
+    base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 1);
+    frame_table = (struct page_info *)mfn_to_virt(mfn_x(base_mfn));
+
+    memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
+    memset(&frame_table[nr_pdxs], -1,
+           frametable_size - (nr_pdxs * sizeof(struct page_info)));
+}
+
 /*
  * Allocate an entry for a new EL2 MPU region in the bitmap xen_mpumap_mask.
  * @param idx   Set to the index of the allocated EL2 MPU region on success.
-- 
2.34.1

[PATCH v3 2/5] arm/mpu: Implement setup_mm for MPU systems

[Hari Limaye]

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

Implement `setup_mm` for MPU systems. This variant doesn't need to set
up the direct map.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes from v2:
- Add Michal's R-b

Changes from v1:
- Fix total_pages dead assignment
- Remove extraneous space
- Remove redundant max_page assignment
---
 xen/arch/arm/mpu/mm.c | 62 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 3f155b7db2..4c517d6e43 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -8,9 +8,12 @@
 #include <xen/sizes.h>
 #include <xen/spinlock.h>
 #include <xen/types.h>
+#include <xen/static-memory.h>
+#include <xen/static-shmem.h>
 #include <asm/mpu.h>
 #include <asm/mpu/mm.h>
 #include <asm/page.h>
+#include <asm/setup.h>
 #include <asm/sysregs.h>
 
 struct page_info *frame_table;
@@ -378,9 +381,66 @@ int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
     return xen_mpumap_update(virt, mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags);
 }
 
+/*
+ * Heap must be statically configured in Device Tree through "xen,static-heap"
+ * on MPU systems.
+ */
+static void __init setup_staticheap_mappings(void)
+{
+    const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
+    unsigned int bank = 0;
+
+    for ( ; bank < reserved_mem->nr_banks; bank++ )
+    {
+        if ( reserved_mem->bank[bank].type == MEMBANK_STATIC_HEAP )
+        {
+            paddr_t bank_start = round_pgup(reserved_mem->bank[bank].start);
+            paddr_t bank_size = round_pgdown(reserved_mem->bank[bank].size);
+            paddr_t bank_end = bank_start + bank_size;
+
+            /* Map static heap with one MPU protection region */
+            if ( xen_mpumap_update(bank_start, bank_end, PAGE_HYPERVISOR) )
+                panic("Failed to map static heap\n");
+
+            break;
+        }
+    }
+
+    if ( bank == reserved_mem->nr_banks )
+        panic("No static heap memory bank found\n");
+}
+
 void __init setup_mm(void)
 {
-    BUG_ON("unimplemented");
+    const struct membanks *mem = bootinfo_get_mem();
+    paddr_t ram_start = INVALID_PADDR, ram_end = 0, ram_size = 0;
+
+    if ( !mem->nr_banks )
+        panic("No memory bank\n");
+
+    init_pdx();
+
+    populate_boot_allocator();
+
+    for ( unsigned int bank = 0; bank < mem->nr_banks; bank++ )
+    {
+        paddr_t bank_start = round_pgup(mem->bank[bank].start);
+        paddr_t bank_size = round_pgdown(mem->bank[bank].size);
+        paddr_t bank_end = bank_start + bank_size;
+
+        ram_size = ram_size + bank_size;
+        ram_start = min(ram_start, bank_start);
+        ram_end = max(ram_end, bank_end);
+    }
+
+    setup_staticheap_mappings();
+
+    total_pages = ram_size >> PAGE_SHIFT;
+
+    setup_frametable_mappings(ram_start, ram_end);
+
+    init_staticmem_pages();
+    init_sharedmem_pages();
 }
 
 int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
-- 
2.34.1

[PATCH v3 3/5] arm/mpu: Implement transient mapping

[Hari Limaye]

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

Add a scheme to distinguish transient MPU regions, to identify MPU
regions which will be mapped for a short period of time. This is needed
for the functions which transiently map and unmap memory ranges on
demand which will be introduced in a future commit.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes from v2:
- Define offsets programmatically, rather than hard-coding these
- Add Michal's R-b

Changes from v1:
- Improve commit message
- Mark parameter in read helper as const
---
 xen/arch/arm/arm32/asm-offsets.c         |  3 ++-
 xen/arch/arm/arm64/asm-offsets.c         |  2 ++
 xen/arch/arm/include/asm/arm32/mpu.h     |  2 ++
 xen/arch/arm/include/asm/arm64/mpu.h     |  2 ++
 xen/arch/arm/include/asm/mpu/mm.h        | 14 +++++++++++++-
 xen/arch/arm/include/asm/mpu/regions.inc | 17 +++++++++++++----
 xen/arch/arm/mpu/mm.c                    | 23 ++++++++++++++---------
 7 files changed, 48 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index c203ce269d..f354bf374d 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -43,7 +43,6 @@ void __dummy__(void)
    OFFSET(UREGS_SP_und, struct cpu_user_regs, sp_und);
    OFFSET(UREGS_LR_und, struct cpu_user_regs, lr_und);
    OFFSET(UREGS_SPSR_und, struct cpu_user_regs, spsr_und);
-
    OFFSET(UREGS_SP_irq, struct cpu_user_regs, sp_irq);
    OFFSET(UREGS_LR_irq, struct cpu_user_regs, lr_irq);
    OFFSET(UREGS_SPSR_irq, struct cpu_user_regs, spsr_irq);
@@ -79,6 +78,8 @@ void __dummy__(void)
 #ifdef CONFIG_MPU
    DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
    DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
+   DEFINE(XEN_MPUMAP_ENTRY_SHIFT, ilog2(sizeof(pr_t)));
+   DEFINE(XEN_MPUMAP_ENTRY_ZERO_OFFSET, sizeof(prbar_t) + sizeof(prlar_t));
    BLANK();
 #endif
 }
diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index 320289b281..38a3894a3b 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -73,6 +73,8 @@ void __dummy__(void)
 #ifdef CONFIG_MPU
    DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
    DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
+   DEFINE(XEN_MPUMAP_ENTRY_SHIFT, ilog2(sizeof(pr_t)));
+   DEFINE(XEN_MPUMAP_ENTRY_ZERO_OFFSET, sizeof(prbar_t) + sizeof(prlar_t));
    BLANK();
 #endif
 }
diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
index 0a6930b3a0..9906d98809 100644
--- a/xen/arch/arm/include/asm/arm32/mpu.h
+++ b/xen/arch/arm/include/asm/arm32/mpu.h
@@ -39,6 +39,8 @@ typedef union {
 typedef struct {
     prbar_t prbar;
     prlar_t prlar;
+    bool transient;
+    uint8_t pad[7]; /* Pad structure to 16 Bytes */
 } pr_t;
 
 #endif /* __ASSEMBLY__ */
diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
index f0ce344e78..1d1843eda0 100644
--- a/xen/arch/arm/include/asm/arm64/mpu.h
+++ b/xen/arch/arm/include/asm/arm64/mpu.h
@@ -38,6 +38,8 @@ typedef union {
 typedef struct {
     prbar_t prbar;
     prlar_t prlar;
+    bool transient;
+    uint8_t pad[15]; /* Pad structure to 32 Bytes */
 } pr_t;
 
 #endif /* __ASSEMBLY__ */
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index e1ded6521d..566d338986 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -55,6 +55,16 @@ static inline void context_sync_mpu(void)
     isb();
 }
 
+static inline bool region_is_transient(const pr_t *pr)
+{
+    return pr->transient;
+}
+
+static inline void region_set_transient(pr_t *pr, bool transient)
+{
+    pr->transient = transient;
+}
+
 /*
  * The following API requires context_sync_mpu() after being used to modify MPU
  * regions:
@@ -75,9 +85,11 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel);
  * @param base      Base address of the range to map (inclusive).
  * @param limit     Limit address of the range to map (exclusive).
  * @param flags     Flags for the memory range to map.
+ * @param transient True for a transient mapping, otherwise False.
  * @return          0 on success, negative on error.
  */
-int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags);
+int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
+                      bool transient);
 
 /*
  * Creates a pr_t structure describing a protection region.
diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
index 23fead3b21..09e1dcf03f 100644
--- a/xen/arch/arm/include/asm/mpu/regions.inc
+++ b/xen/arch/arm/include/asm/mpu/regions.inc
@@ -14,19 +14,25 @@
 #define PRLAR_ELx_EN            0x1
 
 #ifdef CONFIG_ARM_64
-#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
-
 .macro store_pair reg1, reg2, dst
     stp \reg1, \reg2, [\dst]
 .endm
 
-#else
-#define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
+.macro zero_pair dst, offset, tmp1, tmp2
+    stp xzr, xzr, [\dst, \offset]
+.endm
 
+#else
 .macro store_pair reg1, reg2, dst
     strd  \reg1, \reg2, [\dst]
 .endm
 
+.macro zero_pair dst, offset, tmp1, tmp2
+    mov \tmp1, #0
+    mov \tmp2, #0
+    strd \tmp1, \tmp2, [\dst, \offset]
+.endm
+
 #endif
 
 /*
@@ -97,6 +103,9 @@
 
 3:
 
+    /* Clear the rest of the xen_mpumap entry. Clobbers prbar and prlar. */
+    zero_pair \base, #XEN_MPUMAP_ENTRY_ZERO_OFFSET, \prbar, \prlar
+
     add   \sel, \sel, #1
 
 1:
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 4c517d6e43..33333181d5 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -265,13 +265,14 @@ static void disable_mpu_region_from_index(uint8_t index)
  * Update the entry in the MPU memory region mapping table (xen_mpumap) for the
  * given memory range and flags, creating one if none exists.
  *
- * @param base  Base address (inclusive).
- * @param limit Limit address (exclusive).
- * @param flags Region attributes (a combination of PAGE_HYPERVISOR_XXX)
+ * @param base      Base address (inclusive).
+ * @param limit     Limit address (exclusive).
+ * @param flags     Region attributes (a combination of PAGE_HYPERVISOR_XXX)
+ * @param transient True for a transient mapping, otherwise False.
  * @return      0 on success, otherwise negative on error.
  */
 static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
-                                   unsigned int flags)
+                                   unsigned int flags, bool transient)
 {
     bool flags_has_page_present;
     uint8_t idx;
@@ -311,6 +312,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
             return -ENOENT;
 
         xen_mpumap[idx] = pr_of_addr(base, limit, flags);
+        region_set_transient(&xen_mpumap[idx], transient);
 
         write_protection_region(&xen_mpumap[idx], idx);
     }
@@ -330,7 +332,8 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
     return 0;
 }
 
-int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags)
+int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
+                      bool transient)
 {
     int rc;
 
@@ -356,7 +359,7 @@ int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags)
 
     spin_lock(&xen_mpumap_lock);
 
-    rc = xen_mpumap_update_entry(base, limit, flags);
+    rc = xen_mpumap_update_entry(base, limit, flags, transient);
     if ( !rc )
         context_sync_mpu();
 
@@ -371,14 +374,15 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
     ASSERT(IS_ALIGNED(e, PAGE_SIZE));
     ASSERT(s < e);
 
-    return xen_mpumap_update(s, e, 0);
+    return xen_mpumap_update(s, e, 0, false);
 }
 
 int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
                      unsigned int flags)
 {
     /* MPU systems have no translation, ma == va, so pass virt directly */
-    return xen_mpumap_update(virt, mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags);
+    return xen_mpumap_update(virt, mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags,
+                             false);
 }
 
 /*
@@ -399,7 +403,8 @@ static void __init setup_staticheap_mappings(void)
             paddr_t bank_end = bank_start + bank_size;
 
             /* Map static heap with one MPU protection region */
-            if ( xen_mpumap_update(bank_start, bank_end, PAGE_HYPERVISOR) )
+            if ( xen_mpumap_update(bank_start, bank_end, PAGE_HYPERVISOR,
+                                   false) )
                 panic("Failed to map static heap\n");
 
             break;
-- 
2.34.1

[PATCH v3 4/5] arm/mpu: Implement ioremap_attr for MPU

[Hari Limaye]

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

Introduce helpers (un)map_mm_range() in order to allow the transient
mapping of a range of memory, and use these to implement the function
`ioremap_attr` for MPU systems.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
---
Changes from v2:
- Propagate error to caller of is_mm_range_mapped, rather than panic

Changes from v1:
- Use transient instead of temporary, and improve wording of comments
  regarding transient mapping
- Rename start, end -> base, limit
---
 xen/arch/arm/include/asm/mpu/mm.h |  22 +++++
 xen/arch/arm/mpu/mm.c             | 157 ++++++++++++++++++++++++++++--
 2 files changed, 170 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index 566d338986..efb0680e39 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -101,6 +101,28 @@ int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
  */
 pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
 
+/*
+ * Maps transiently a range of memory with attributes `flags`; if the range is
+ * already mapped with the same attributes, including an inclusive match, the
+ * existing mapping is returned. This API is intended for mappings that exist
+ * transiently for a short period between calls to this function and
+ * `unmap_mm_range`.
+ *
+ * @param base      Base address of the range to map (inclusive).
+ * @param limit     Limit address of the range to map (exclusive).
+ * @param flags     Flags for the memory range to map.
+ * @return          Pointer to base of region on success, NULL on error.
+ */
+void *map_mm_range(paddr_t base, paddr_t limit, unsigned int flags);
+
+/*
+ * Unmaps a range of memory if it was previously mapped by map_mm_range,
+ * otherwise it does not remove the mapping.
+ *
+ * @param base     Base address of the range to map (inclusive).
+ */
+void unmap_mm_range(paddr_t base);
+
 /*
  * Checks whether a given memory range is present in the provided table of
  * MPU protection regions.
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 33333181d5..337573f9d7 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -332,31 +332,39 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
     return 0;
 }
 
-int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
-                      bool transient)
+static bool check_mpu_mapping(paddr_t base, paddr_t limit, unsigned int flags)
 {
-    int rc;
-
     if ( flags_has_rwx(flags) )
     {
         printk("Mappings should not be both Writeable and Executable\n");
-        return -EINVAL;
+        return false;
     }
 
     if ( base >= limit )
     {
         printk("Base address %#"PRIpaddr" must be smaller than limit address %#"PRIpaddr"\n",
                base, limit);
-        return -EINVAL;
+        return false;
     }
 
     if ( !IS_ALIGNED(base, PAGE_SIZE) || !IS_ALIGNED(limit, PAGE_SIZE) )
     {
         printk("base address %#"PRIpaddr", or limit address %#"PRIpaddr" is not page aligned\n",
                base, limit);
-        return -EINVAL;
+        return false;
     }
 
+    return true;
+}
+
+int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
+                      bool transient)
+{
+    int rc;
+
+    if ( !check_mpu_mapping(base, limit, flags) )
+        return -EINVAL;
+
     spin_lock(&xen_mpumap_lock);
 
     rc = xen_mpumap_update_entry(base, limit, flags, transient);
@@ -465,10 +473,141 @@ void free_init_memory(void)
     BUG_ON("unimplemented");
 }
 
+static int is_mm_range_mapped(paddr_t start, paddr_t end, uint8_t *idx)
+{
+    ASSERT(spin_is_locked(&xen_mpumap_lock));
+
+    /*
+     * 'idx' will be INVALID_REGION_IDX for rc == MPUMAP_REGION_NOTFOUND and
+     * it will be a proper region index when rc >= MPUMAP_REGION_FOUND.
+     */
+    return mpumap_contains_region(xen_mpumap, max_mpu_regions, start, end, idx);
+}
+
+static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
+{
+    bool ret = true;
+
+    if ( region->prbar.reg.ro != PAGE_RO_MASK(attributes) )
+    {
+        printk(XENLOG_WARNING
+               "Mismatched Access Permission attributes (%#x0 instead of %#x0)\n",
+               region->prbar.reg.ro, PAGE_RO_MASK(attributes));
+        ret = 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));
+        ret = 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));
+        ret = false;
+    }
+
+    return ret;
+}
+
+void *map_mm_range(paddr_t base, paddr_t limit, unsigned int flags)
+{
+    paddr_t start_pg = round_pgdown(base);
+    paddr_t end_pg = round_pgup(limit);
+    void *ret = NULL;
+    uint8_t idx;
+    int rc;
+
+    if ( !check_mpu_mapping(start_pg, end_pg, flags) )
+        return NULL;
+
+    spin_lock(&xen_mpumap_lock);
+
+    rc = is_mm_range_mapped(start_pg, end_pg, &idx);
+    if ( rc < 0 ) {
+        printk(XENLOG_WARNING
+               "Cannot handle overlapping MPU memory protection regions\n");
+        goto out;
+    }
+
+    if ( idx != INVALID_REGION_IDX )
+    {
+        /* Already mapped with different attributes */
+        if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
+        {
+            printk(XENLOG_WARNING
+                   "Range %#"PRIpaddr"-%#"PRIpaddr" already mapped with different flags\n",
+                   start_pg, end_pg);
+            goto out;
+        }
+
+        /* Already mapped with same attributes */
+        ret = maddr_to_virt(base);
+        goto out;
+    }
+
+    if ( !xen_mpumap_update_entry(start_pg, end_pg, flags, true) )
+    {
+        context_sync_mpu();
+        ret = maddr_to_virt(base);
+    }
+
+ out:
+    spin_unlock(&xen_mpumap_lock);
+
+    return ret;
+}
+
+void unmap_mm_range(paddr_t base)
+{
+    uint8_t idx;
+    int rc;
+
+    spin_lock(&xen_mpumap_lock);
+
+    /*
+     * Mappings created via map_mm_range are at least PAGE_SIZE. Find the idx
+     * of the MPU memory region containing `start` mapped through map_mm_range.
+     */
+    rc = is_mm_range_mapped(base, base + PAGE_SIZE, &idx);
+    if ( rc < 0 ) {
+        printk(XENLOG_WARNING
+               "Cannot handle overlapping MPU memory protection regions\n");
+        goto out;
+    }
+
+    if ( idx == INVALID_REGION_IDX )
+    {
+        printk(XENLOG_ERR
+               "Failed to unmap_mm_range MPU memory region at %#"PRIpaddr"\n",
+               base);
+        goto out;
+    }
+
+    /* This API is only meant to unmap transient regions */
+    if ( !region_is_transient(&xen_mpumap[idx]) )
+        goto out;
+
+    /* Disable MPU memory region and clear the associated entry in xen_mpumap */
+    disable_mpu_region_from_index(idx);
+    context_sync_mpu();
+
+ out:
+    spin_unlock(&xen_mpumap_lock);
+}
+
 void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int flags)
 {
-    BUG_ON("unimplemented");
-    return NULL;
+    if ( !map_mm_range(start, start + len, flags) )
+        return NULL;
+
+    /* Mapped or already mapped */
+    return maddr_to_virt(start);
 }
 
 /*
-- 
2.34.1

[PATCH v3 5/5] xen/arm: map static memory on demand

[Hari Limaye]

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

In the function `init_staticmem_pages` we need to have mapped static
memory banks for initialization. Unlike on an MMU system, we cannot map
the entire RAM on an MPU system as we have a limited number of MPU
memory regions. To solve this, transiently map the static memory banks
for initialization.

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>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes from v2:
- Add Michal's R-b
---
 xen/arch/arm/include/asm/mmu/mm.h |  3 +++
 xen/arch/arm/include/asm/mpu/mm.h |  4 ++++
 xen/arch/arm/mpu/setup.c          | 11 +++++++++++
 xen/include/xen/static-memory.h   |  8 ++++++++
 4 files changed, 26 insertions(+)

diff --git a/xen/arch/arm/include/asm/mmu/mm.h b/xen/arch/arm/include/asm/mmu/mm.h
index 7f4d59137d..645a0ea3cb 100644
--- a/xen/arch/arm/include/asm/mmu/mm.h
+++ b/xen/arch/arm/include/asm/mmu/mm.h
@@ -110,6 +110,9 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
 extern void switch_ttbr(uint64_t ttbr);
 extern void relocate_and_switch_ttbr(uint64_t ttbr);
 
+static inline void map_staticmem_pages_to_xen(paddr_t start, paddr_t end) {}
+static inline void unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end) {}
+
 #endif /* __ARM_MMU_MM_H__ */
 
 /*
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index efb0680e39..4cc769418e 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -123,6 +123,10 @@ void *map_mm_range(paddr_t base, paddr_t limit, unsigned int flags);
  */
 void unmap_mm_range(paddr_t base);
 
+/* {un}map_staticmem_pages_to_xen used while initializing static memory banks */
+void map_staticmem_pages_to_xen(paddr_t start, paddr_t end);
+void unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end);
+
 /*
  * Checks whether a given memory range is present in the provided table of
  * MPU protection regions.
diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
index 163573b932..dbc3107333 100644
--- a/xen/arch/arm/mpu/setup.c
+++ b/xen/arch/arm/mpu/setup.c
@@ -83,6 +83,17 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     return fdt_virt;
 }
 
+void __init map_staticmem_pages_to_xen(paddr_t start, paddr_t end)
+{
+    if ( !map_mm_range(start, end, PAGE_HYPERVISOR) )
+        panic("Unable to map staticmem pages to Xen!");
+}
+
+void __init unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end)
+{
+    unmap_mm_range(start);
+}
+
 /*
  * copy_from_paddr - copy data from a physical address
  * @dst: destination virtual address
diff --git a/xen/include/xen/static-memory.h b/xen/include/xen/static-memory.h
index e445aa8057..d99abac113 100644
--- a/xen/include/xen/static-memory.h
+++ b/xen/include/xen/static-memory.h
@@ -18,7 +18,15 @@ static inline void init_staticmem_bank(const struct membank *bank)
     if ( mfn_x(bank_end) <= mfn_x(bank_start) )
         return;
 
+    /* Map temporarily before initialization */
+    map_staticmem_pages_to_xen(mfn_to_maddr(bank_start),
+                               mfn_to_maddr(bank_end));
+
     unprepare_staticmem_pages(mfn_to_page(bank_start), bank_pages, false);
+
+    /* Unmap immediately after initialization */
+    unmap_staticmem_pages_to_xen(mfn_to_maddr(bank_start),
+                                 mfn_to_maddr(bank_end));
 }
 
 void allocate_static_memory(struct domain *d, struct kernel_info *kinfo,
-- 
2.34.1

Re: [PATCH v3 4/5] arm/mpu: Implement ioremap_attr for MPU

[Orzel, Michal]

On 28/08/2025 13:12, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Introduce helpers (un)map_mm_range() in order to allow the transient
> mapping of a range of memory, and use these to implement the function
> `ioremap_attr` for MPU systems.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH v3 5/5] xen/arm: map static memory on demand

[Orzel, Michal]

On 28/08/2025 13:12, Hari Limaye wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> In the function `init_staticmem_pages` we need to have mapped static
> memory banks for initialization. Unlike on an MMU system, we cannot map
> the entire RAM on an MPU system as we have a limited number of MPU
> memory regions. To solve this, transiently map the static memory banks
> for initialization.
> 
> 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>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Given that the freeze date will likely be extended, I'd prefer to take this tag
back and request this patch to be changed due to reasons listed below.

> ---
> Changes from v2:
> - Add Michal's R-b
> ---
>  xen/arch/arm/include/asm/mmu/mm.h |  3 +++
>  xen/arch/arm/include/asm/mpu/mm.h |  4 ++++
>  xen/arch/arm/mpu/setup.c          | 11 +++++++++++
>  xen/include/xen/static-memory.h   |  8 ++++++++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mmu/mm.h b/xen/arch/arm/include/asm/mmu/mm.h
> index 7f4d59137d..645a0ea3cb 100644
> --- a/xen/arch/arm/include/asm/mmu/mm.h
> +++ b/xen/arch/arm/include/asm/mmu/mm.h
> @@ -110,6 +110,9 @@ void dump_pt_walk(paddr_t ttbr, paddr_t addr,
>  extern void switch_ttbr(uint64_t ttbr);
>  extern void relocate_and_switch_ttbr(uint64_t ttbr);
>  
> +static inline void map_staticmem_pages_to_xen(paddr_t start, paddr_t end) {}
> +static inline void unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end) {}
> +
>  #endif /* __ARM_MMU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index efb0680e39..4cc769418e 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -123,6 +123,10 @@ void *map_mm_range(paddr_t base, paddr_t limit, unsigned int flags);
>   */
>  void unmap_mm_range(paddr_t base);
>  
> +/* {un}map_staticmem_pages_to_xen used while initializing static memory banks */
> +void map_staticmem_pages_to_xen(paddr_t start, paddr_t end);
> +void unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end);
> +
>  /*
>   * Checks whether a given memory range is present in the provided table of
>   * MPU protection regions.
> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
> index 163573b932..dbc3107333 100644
> --- a/xen/arch/arm/mpu/setup.c
> +++ b/xen/arch/arm/mpu/setup.c
> @@ -83,6 +83,17 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>      return fdt_virt;
>  }
>  
> +void __init map_staticmem_pages_to_xen(paddr_t start, paddr_t end)
> +{
> +    if ( !map_mm_range(start, end, PAGE_HYPERVISOR) )
> +        panic("Unable to map staticmem pages to Xen!");
> +}
> +
> +void __init unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end)
> +{
> +    unmap_mm_range(start);
> +}
> +
>  /*
>   * copy_from_paddr - copy data from a physical address
>   * @dst: destination virtual address
> diff --git a/xen/include/xen/static-memory.h b/xen/include/xen/static-memory.h
> index e445aa8057..d99abac113 100644
> --- a/xen/include/xen/static-memory.h
> +++ b/xen/include/xen/static-memory.h
> @@ -18,7 +18,15 @@ static inline void init_staticmem_bank(const struct membank *bank)
>      if ( mfn_x(bank_end) <= mfn_x(bank_start) )
>          return;
>  
> +    /* Map temporarily before initialization */
> +    map_staticmem_pages_to_xen(mfn_to_maddr(bank_start),
> +                               mfn_to_maddr(bank_end));
Static memory is not Arm only feature, it is common and as such should not (and
does not) make calls to Arm only functions. If at all, such helpers should be
made generic so other arches that could enable static memory can re-define them
if needed (as you pointed out, on MMU you don't need to map/unmap this region
temporarily).

~Michal

Re: [PATCH v3 5/5] xen/arm: map static memory on demand

[Julien Grall]

Hi Hari,

On 28/08/2025 12:12, Hari Limaye wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> In the function `init_staticmem_pages` we need to have mapped static
> memory banks for initialization. Unlike on an MMU system, we cannot map
> the entire RAM

Even on the MMU system we don't always map the full RAM (for instance on 
arm32). This is why we have infrastructure like map_domain_page() 
(Temporary mapping) and map_domain_page_global() (more permanent).

on an MPU system as we have a limited number of MPU
> memory regions. To solve this, transiently map the static memory banks
> for initialization.

I am guessing you implemented the helper because in 
unmap_staticmem_pages_to_xen(), we are calling scrub_one_page(). This 
will be using map_domain_page() and unmap_domain_page(). I am a bit 
confused why we end up with brand new helpers rather than implementation 
map_domain_page() and unmap_domain_page()?

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/5] arm/mpu: Implement setup_mm for MPU systems

[Julien Grall]

Hi,

On 28/08/2025 12:12, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Implement `setup_mm` for MPU systems. This variant doesn't need to set
> up the direct map.

If this is the only difference, then why do we duplicate the rest of the 
code? Why can't we instead setup the directmap helper or make it optional?

In fact, in the future we will want to initially have the directmap 
optional on Arm with MMU, but ultimately will be removed.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 5/5] xen/arm: map static memory on demand

[Luca Fancellu]

Hi Julien,

> On 29 Aug 2025, at 08:22, Julien Grall <julien@xen.org> wrote:
> 
> Hi Hari,
> 
> On 28/08/2025 12:12, Hari Limaye wrote:
>> From: Penny Zheng <Penny.Zheng@arm.com>
>> In the function `init_staticmem_pages` we need to have mapped static
>> memory banks for initialization. Unlike on an MMU system, we cannot map
>> the entire RAM
> 
> Even on the MMU system we don't always map the full RAM (for instance on arm32). This is why we have infrastructure like map_domain_page() (Temporary mapping) and map_domain_page_global() (more permanent).
> 
> on an MPU system as we have a limited number of MPU
>> memory regions. To solve this, transiently map the static memory banks
>> for initialization.
> 
> I am guessing you implemented the helper because in unmap_staticmem_pages_to_xen(), we are calling scrub_one_page(). This will be using map_domain_page() and unmap_domain_page(). I am a bit confused why we end up with brand new helpers rather than implementation map_domain_page() and unmap_domain_page()?

yes I agree, scrub_one_page is already using {un}map_domain_page(), we will investigate about it.

Cheers,
Luca

Re: [PATCH v3 3/5] arm/mpu: Implement transient mapping

[Julien Grall]

Hi,

On 28/08/2025 12:12, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Add a scheme to distinguish transient MPU regions, to identify MPU
> regions which will be mapped for a short period of time. This is needed
> for the functions which transiently map and unmap memory ranges on
> demand which will be introduced in a future commit.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> ---
> Changes from v2:
> - Define offsets programmatically, rather than hard-coding these
> - Add Michal's R-b
> 
> Changes from v1:
> - Improve commit message
> - Mark parameter in read helper as const
> ---
>   xen/arch/arm/arm32/asm-offsets.c         |  3 ++-
>   xen/arch/arm/arm64/asm-offsets.c         |  2 ++
>   xen/arch/arm/include/asm/arm32/mpu.h     |  2 ++
>   xen/arch/arm/include/asm/arm64/mpu.h     |  2 ++
>   xen/arch/arm/include/asm/mpu/mm.h        | 14 +++++++++++++-
>   xen/arch/arm/include/asm/mpu/regions.inc | 17 +++++++++++++----
>   xen/arch/arm/mpu/mm.c                    | 23 ++++++++++++++---------
>   7 files changed, 48 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
> index c203ce269d..f354bf374d 100644
> --- a/xen/arch/arm/arm32/asm-offsets.c
> +++ b/xen/arch/arm/arm32/asm-offsets.c
> @@ -43,7 +43,6 @@ void __dummy__(void)
>      OFFSET(UREGS_SP_und, struct cpu_user_regs, sp_und);
>      OFFSET(UREGS_LR_und, struct cpu_user_regs, lr_und);
>      OFFSET(UREGS_SPSR_und, struct cpu_user_regs, spsr_und);
> -

Spurious change?

>      OFFSET(UREGS_SP_irq, struct cpu_user_regs, sp_irq);
>      OFFSET(UREGS_LR_irq, struct cpu_user_regs, lr_irq);
>      OFFSET(UREGS_SPSR_irq, struct cpu_user_regs, spsr_irq);
> @@ -79,6 +78,8 @@ void __dummy__(void)
>   #ifdef CONFIG_MPU
>      DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
>      DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
> +   DEFINE(XEN_MPUMAP_ENTRY_SHIFT, ilog2(sizeof(pr_t)));
> +   DEFINE(XEN_MPUMAP_ENTRY_ZERO_OFFSET, sizeof(prbar_t) + sizeof(prlar_t));
>      BLANK();
>   #endif
>   }
> diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
> index 320289b281..38a3894a3b 100644
> --- a/xen/arch/arm/arm64/asm-offsets.c
> +++ b/xen/arch/arm/arm64/asm-offsets.c
> @@ -73,6 +73,8 @@ void __dummy__(void)
>   #ifdef CONFIG_MPU
>      DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
>      DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
> +   DEFINE(XEN_MPUMAP_ENTRY_SHIFT, ilog2(sizeof(pr_t)));
> +   DEFINE(XEN_MPUMAP_ENTRY_ZERO_OFFSET, sizeof(prbar_t) + sizeof(prlar_t));
>      BLANK();
>   #endif
>   }
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h b/xen/arch/arm/include/asm/arm32/mpu.h
> index 0a6930b3a0..9906d98809 100644
> --- a/xen/arch/arm/include/asm/arm32/mpu.h
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -39,6 +39,8 @@ typedef union {
>   typedef struct {
>       prbar_t prbar;
>       prlar_t prlar;
> +    bool transient;
 > +    uint8_t pad[7]; /* Pad structure to 16 Bytes */>   } pr_t;
>   
>   #endif /* __ASSEMBLY__ */
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
> index f0ce344e78..1d1843eda0 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -38,6 +38,8 @@ typedef union {
>   typedef struct {
>       prbar_t prbar;
>       prlar_t prlar;
> +    bool transient;
> +    uint8_t pad[15]; /* Pad structure to 32 Bytes */
>   } pr_t;
>   
>   #endif /* __ASSEMBLY__ */
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index e1ded6521d..566d338986 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -55,6 +55,16 @@ static inline void context_sync_mpu(void)
>       isb();
>   }
>   
> +static inline bool region_is_transient(const pr_t *pr)
> +{
> +    return pr->transient;
> +}
> +
> +static inline void region_set_transient(pr_t *pr, bool transient)
> +{
> +    pr->transient = transient;
> +}
> +
>   /*
>    * The following API requires context_sync_mpu() after being used to modify MPU
>    * regions:
> @@ -75,9 +85,11 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel);
>    * @param base      Base address of the range to map (inclusive).
>    * @param limit     Limit address of the range to map (exclusive).
>    * @param flags     Flags for the memory range to map.
> + * @param transient True for a transient mapping, otherwise False.

Why can't this be part of the flags?

>    * @return          0 on success, negative on error.
>    */
> -int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags);
> +int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
> +                      bool transient);
>   
>   /*
>    * Creates a pr_t structure describing a protection region.
> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc b/xen/arch/arm/include/asm/mpu/regions.inc
> index 23fead3b21..09e1dcf03f 100644
> --- a/xen/arch/arm/include/asm/mpu/regions.inc
> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
> @@ -14,19 +14,25 @@
>   #define PRLAR_ELx_EN            0x1
>   
>   #ifdef CONFIG_ARM_64
> -#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
> -
>   .macro store_pair reg1, reg2, dst
>       stp \reg1, \reg2, [\dst]
>   .endm
>   
> -#else
> -#define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
> +.macro zero_pair dst, offset, tmp1, tmp2
 > +    stp xzr, xzr, [\dst, \offset]> +.endm
>   
> +#else
>   .macro store_pair reg1, reg2, dst
>       strd  \reg1, \reg2, [\dst]
>   .endm
>   
> +.macro zero_pair dst, offset, tmp1, tmp2
> +    mov \tmp1, #0
> +    mov \tmp2, #0
> +    strd \tmp1, \tmp2, [\dst, \offset]
> +.endm
 > +

TBH the addition of zero_pair feels a bit overkill when there is one 
user. Why can't we open-code the use case below and use ``store_pair``?

This would also avoid the "tmp1" and "tmp2" which are not used by arm64.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 4/5] arm/mpu: Implement ioremap_attr for MPU

[Julien Grall]

Hi Hari,

On 28/08/2025 12:12, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Introduce helpers (un)map_mm_range() in order to allow the transient
> mapping of a range of memory, and use these to implement the function
> `ioremap_attr` for MPU systems.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> ---
> Changes from v2:
> - Propagate error to caller of is_mm_range_mapped, rather than panic
> 
> Changes from v1:
> - Use transient instead of temporary, and improve wording of comments
>    regarding transient mapping
> - Rename start, end -> base, limit
> ---
>   xen/arch/arm/include/asm/mpu/mm.h |  22 +++++
>   xen/arch/arm/mpu/mm.c             | 157 ++++++++++++++++++++++++++++--
>   2 files changed, 170 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index 566d338986..efb0680e39 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -101,6 +101,28 @@ int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
>    */
>   pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
>   
> +/*
> + * Maps transiently a range of memory with attributes `flags`; if the range is
> + * already mapped with the same attributes, including an inclusive match, the
> + * existing mapping is returned. This API is intended for mappings that exist
> + * transiently for a short period between calls to this function and
> + * `unmap_mm_range`.
> + *
> + * @param base      Base address of the range to map (inclusive).
> + * @param limit     Limit address of the range to map (exclusive).
> + * @param flags     Flags for the memory range to map.
> + * @return          Pointer to base of region on success, NULL on error.
> + */
 > +void *map_mm_range(paddr_t base, paddr_t limit, unsigned int flags);

I think I am missing a couple of things:

  * The current use seems to be ioremap() which tends to be mapping for 
the lifetime of Xen. So I would not describe them as transient.
  * It is unclear to me why we need a different interface and can't use 
exist one. The name also doesn't make very clear that the mapping will 
be "transient" (whatever that means).

> +
> +/*
> + * Unmaps a range of memory if it was previously mapped by map_mm_range,
> + * otherwise it does not remove the mapping.
> + *
> + * @param base     Base address of the range to map (inclusive).
> + */
> +void unmap_mm_range(paddr_t base);
> +
>   /*
>    * Checks whether a given memory range is present in the provided table of
>    * MPU protection regions.
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 33333181d5..337573f9d7 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -332,31 +332,39 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>       return 0;
>   }
>   
> -int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
> -                      bool transient)
> +static bool check_mpu_mapping(paddr_t base, paddr_t limit, unsigned int flags)
>   {
> -    int rc;
> -
>       if ( flags_has_rwx(flags) )
>       {
>           printk("Mappings should not be both Writeable and Executable\n");
> -        return -EINVAL;
> +        return false;
>       }
>   
>       if ( base >= limit )
>       {
>           printk("Base address %#"PRIpaddr" must be smaller than limit address %#"PRIpaddr"\n",
>                  base, limit);
> -        return -EINVAL;
> +        return false;
>       }
>   
>       if ( !IS_ALIGNED(base, PAGE_SIZE) || !IS_ALIGNED(limit, PAGE_SIZE) )
>       {
>           printk("base address %#"PRIpaddr", or limit address %#"PRIpaddr" is not page aligned\n",
>                  base, limit);
> -        return -EINVAL;
> +        return false;
>       }
>   
> +    return true;
> +}
> +
> +int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags,
> +                      bool transient)
> +{
> +    int rc;
> +
> +    if ( !check_mpu_mapping(base, limit, flags) )
> +        return -EINVAL;
> +
>       spin_lock(&xen_mpumap_lock);
>   
>       rc = xen_mpumap_update_entry(base, limit, flags, transient);
> @@ -465,10 +473,141 @@ void free_init_memory(void)
>       BUG_ON("unimplemented");
>   }
>   
> +static int is_mm_range_mapped(paddr_t start, paddr_t end, uint8_t *idx)
> +{
> +    ASSERT(spin_is_locked(&xen_mpumap_lock));
> +
> +    /*
> +     * 'idx' will be INVALID_REGION_IDX for rc == MPUMAP_REGION_NOTFOUND and
> +     * it will be a proper region index when rc >= MPUMAP_REGION_FOUND.
> +     */


I feels this comment belongs on top of ``is_mm_range_mapped()`` because 
it describe the behavior of the function.

> +    return mpumap_contains_region(xen_mpumap, max_mpu_regions, start, end, idx);
> +}
> +
> +static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
> +{
> +    bool ret = true;
> +
> +    if ( region->prbar.reg.ro != PAGE_RO_MASK(attributes) )
> +    {
> +        printk(XENLOG_WARNING
> +               "Mismatched Access Permission attributes (%#x0 instead of %#x0)\n",
> +               region->prbar.reg.ro, PAGE_RO_MASK(attributes));
> +        ret = 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));
> +        ret = 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));
> +        ret = false;
> +    }
> +
> +    return ret;
> +}
> +
> +void *map_mm_range(paddr_t base, paddr_t limit, unsigned int flags)
> +{
> +    paddr_t start_pg = round_pgdown(base);
> +    paddr_t end_pg = round_pgup(limit);
> +    void *ret = NULL;
> +    uint8_t idx;
> +    int rc;
> +
> +    if ( !check_mpu_mapping(start_pg, end_pg, flags) )
> +        return NULL;
> +
> +    spin_lock(&xen_mpumap_lock);
> +
> +    rc = is_mm_range_mapped(start_pg, end_pg, &idx);
> +    if ( rc < 0 ) {

Coding style:

if ( rc < 0 )
{

> +        printk(XENLOG_WARNING
> +               "Cannot handle overlapping MPU memory protection regions\n");
> +        goto out;
> +    }
> +
> +    if ( idx != INVALID_REGION_IDX )
> +    {
> +        /* Already mapped with different attributes */
> +        if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
> +        {
> +            printk(XENLOG_WARNING
> +                   "Range %#"PRIpaddr"-%#"PRIpaddr" already mapped with different flags\n",
> +                   start_pg, end_pg);
> +            goto out;
> +        }
> +
> +        /* Already mapped with same attributes */
> +        ret = maddr_to_virt(base);
> +        goto out;
> +    }
> +
> +    if ( !xen_mpumap_update_entry(start_pg, end_pg, flags, true) )
> +    {
> +        context_sync_mpu();
> +        ret = maddr_to_virt(base);
> +    }
> +
> + out:
> +    spin_unlock(&xen_mpumap_lock);
> +
> +    return ret;
> +}
> +
> +void unmap_mm_range(paddr_t base)
> +{
> +    uint8_t idx;
> +    int rc;
> +
> +    spin_lock(&xen_mpumap_lock);
> +
> +    /*
> +     * Mappings created via map_mm_range are at least PAGE_SIZE. Find the idx
> +     * of the MPU memory region containing `start` mapped through map_mm_range.
> +     */
> +    rc = is_mm_range_mapped(base, base + PAGE_SIZE, &idx);
> +    if ( rc < 0 ) {

Style:

if ( rc < 0 )
{

> +        printk(XENLOG_WARNING
> +               "Cannot handle overlapping MPU memory protection regions\n");

I think this message is incorrect. AFACT, If is_mm_range_mapped() 
returns a negative value, then it means the region is unmapped.

> +        goto out;
> +    }
> +
> +    if ( idx == INVALID_REGION_IDX )
> +    {
> +        printk(XENLOG_ERR
> +               "Failed to unmap_mm_range MPU memory region at %#"PRIpaddr"\n",
> +               base);
> +        goto out;
> +    }
> +
> +    /* This API is only meant to unmap transient regions */

So the transient flag is more for debugging purpose? But if it really 
happens then...

> +    if ( !region_is_transient(&xen_mpumap[idx]) )
 > +        goto out;

... why do we silently fail? Surely this would indicate an error on the 
system which we want to know in debug build.

> +
> +    /* Disable MPU memory region and clear the associated entry in xen_mpumap */
> +    disable_mpu_region_from_index(idx);
> +    context_sync_mpu();
> +
> + out:
> +    spin_unlock(&xen_mpumap_lock);
> +}
> +
>   void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int flags)
>   {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    if ( !map_mm_range(start, start + len, flags) )
> +        return NULL;
> +
> +    /* Mapped or already mapped */
> +    return maddr_to_virt(start);
>   }
>   
>   /*

Cheers,

-- 
Julien Grall