[PATCH v2 0/6] Second series for R82 MPU Support

[PATCH v2 0/6] Second series for R82 MPU Support

[Hari Limaye]

Hi all,

This series is the second 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 map and
unmap the Device Tree Blob in the early stages of the boot process.

Changes from v1:
- Changes mentioned in individual patches

Cheers,
Hari

Luca Fancellu (4):
  arm/mpu: Find MPU region by range
  xen/arm: Introduce flags_has_rwx helper
  arm/mpu: Implement early_fdt_map support in MPU systems
  arm/mpu: Implement remove_early_mappings for MPU systems

Penny Zheng (2):
  arm/mpu: Populate a new region in Xen MPU mapping table
  arm/mpu: Destroy an existing entry in Xen MPU memory mapping table

 xen/arch/arm/include/asm/mm.h         |   2 +
 xen/arch/arm/include/asm/mpu.h        |   2 +
 xen/arch/arm/include/asm/mpu/cpregs.h |   4 +
 xen/arch/arm/include/asm/mpu/mm.h     |  41 +++++
 xen/arch/arm/mm.c                     |  15 ++
 xen/arch/arm/mmu/pt.c                 |   9 +-
 xen/arch/arm/mpu/mm.c                 | 217 ++++++++++++++++++++++++++
 xen/arch/arm/mpu/setup.c              |  83 +++++++++-
 8 files changed, 362 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] arm/mpu: Find MPU region by range

[Hari Limaye]

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

Implement a function to find the index of a MPU region in the xen_mpumap
MPU region array. This function will be used in future commits to
implement creating and destroying MPU regions.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
---
Changes from v1:
- Update commit message
- Remove internal _index variable
- Simplify logic by disallowing NULL index parameter
- Use normal printk
- Reorder conditional checks
- Update some comments
---
 xen/arch/arm/include/asm/mpu/mm.h | 29 +++++++++++++++++
 xen/arch/arm/mpu/mm.c             | 52 +++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index a7f970b465..81e47b9d0b 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -10,6 +10,13 @@
 #include <asm/mm.h>
 #include <asm/mpu.h>
 
+#define MPUMAP_REGION_OVERLAP      -1
+#define MPUMAP_REGION_NOTFOUND      0
+#define MPUMAP_REGION_FOUND         1
+#define MPUMAP_REGION_INCLUSIVE     2
+
+#define INVALID_REGION_IDX     0xFFU
+
 extern struct page_info *frame_table;
 
 extern uint8_t max_mpu_regions;
@@ -75,6 +82,28 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel);
  */
 pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
 
+/*
+ * Checks whether a given memory range is present in the provided table of
+ * MPU protection regions.
+ *
+ * @param table         Array of pr_t protection regions.
+ * @param r_regions     Number of elements in `table`.
+ * @param base          Start of the memory region to be checked (inclusive).
+ * @param limit         End of the memory region to be checked (exclusive).
+ * @param index         Set to the index of the region if an exact or inclusive
+ *                      match is found, and INVALID_REGION otherwise.
+ * @return: Return code indicating the result of the search:
+ *          MPUMAP_REGION_NOTFOUND: no part of the range is present in `table`
+ *          MPUMAP_REGION_FOUND: found an exact match in `table`
+ *          MPUMAP_REGION_INCLUSIVE: found an inclusive match in `table`
+ *          MPUMAP_REGION_OVERLAP: found an overlap with a mapping in `table`
+ *
+ * Note: make sure that the range [`base`, `limit`) refers to the memory region
+ * inclusive of `base` and exclusive of `limit`.
+ */
+int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
+                          paddr_t limit, uint8_t *index);
+
 #endif /* __ARM_MPU_MM_H__ */
 
 /*
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index ccfb37a67b..25e7f36c1e 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -110,6 +110,58 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
     return region;
 }
 
+int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
+                          paddr_t limit, uint8_t *index)
+{
+    ASSERT(index);
+    *index = INVALID_REGION_IDX;
+
+    /* Convert [base, limit) to [base, limit - 1] for inclusive comparison */
+    limit = limit - 1;
+
+    if ( limit < base )
+    {
+        printk("Base address %#"PRIpaddr" must be smaller than limit address %#"PRIpaddr"\n",
+               base, limit);
+        return -EINVAL;
+    }
+
+    for (uint8_t i = 0; i < nr_regions; i++ )
+    {
+        paddr_t iter_base = pr_get_base(&table[i]);
+        paddr_t iter_limit = pr_get_limit(&table[i]);
+
+        /* Skip invalid (disabled) regions */
+        if ( !region_is_valid(&table[i]) )
+            continue;
+
+        /* No match */
+        if ( (iter_limit < base) || (iter_base > limit) )
+            continue;
+
+        /* Exact match */
+        if ( (iter_base == base) && (iter_limit == limit) )
+        {
+            *index = i;
+            return MPUMAP_REGION_FOUND;
+        }
+
+        /* Inclusive match */
+        if ( (base >= iter_base) && (limit <= iter_limit) )
+        {
+            *index = i;
+            return MPUMAP_REGION_INCLUSIVE;
+        }
+
+        /* Overlap */
+        printk("Range %#"PRIpaddr" - %#"PRIpaddr" overlaps with the existing region %#"PRIpaddr" - %#"PRIpaddr"\n",
+               base, limit + 1, iter_base, iter_limit + 1);
+        return MPUMAP_REGION_OVERLAP;
+    }
+
+    return MPUMAP_REGION_NOTFOUND;
+}
+
 void __init setup_mm(void)
 {
     BUG_ON("unimplemented");
-- 
2.34.1

[PATCH v2 2/6] xen/arm: Introduce flags_has_rwx helper

[Hari Limaye]

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

Introduce flags_has_rwx() function that will check if a
mapping is both writable and executable when modifying
or updating the mapping.

This check was already present in pt.c but since it will
be used also for MPU systems, it's wrapped into a function
now.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
Changes from v1:
- Add Ayan and Michal's R-b
- Fix typos
- Don't use extern for function prototype
---
 xen/arch/arm/include/asm/mm.h |  2 ++
 xen/arch/arm/mm.c             | 15 +++++++++++++++
 xen/arch/arm/mmu/pt.c         |  9 +--------
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 011bc1fd30..fb79aeb088 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -192,6 +192,8 @@ extern unsigned long frametable_base_pdx;
 
 /* Boot-time pagetable setup */
 extern void setup_pagetables(void);
+/* Check that the mapping flag has no W and X together */
+bool flags_has_rwx(unsigned int flags);
 /* Map FDT in boot pagetable */
 extern void *early_fdt_map(paddr_t fdt_paddr);
 /* Remove early mappings */
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0613c19169..77e21f5f29 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -24,6 +24,21 @@
 
 unsigned long frametable_base_pdx __read_mostly;
 
+bool flags_has_rwx(unsigned int flags)
+{
+    /*
+     * The hardware was configured to forbid mapping both writable and
+     * executable.
+     * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
+     * prevent any update if this happen.
+     */
+    if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
+         !PAGE_XN_MASK(flags) )
+        return true;
+    else
+        return false;
+}
+
 void flush_page_to_ram(unsigned long mfn, bool sync_icache)
 {
     void *v = map_domain_page(_mfn(mfn));
diff --git a/xen/arch/arm/mmu/pt.c b/xen/arch/arm/mmu/pt.c
index 4726e713ef..621b47dbf5 100644
--- a/xen/arch/arm/mmu/pt.c
+++ b/xen/arch/arm/mmu/pt.c
@@ -610,14 +610,7 @@ static int xen_pt_update(unsigned long virt,
      */
     const mfn_t root = maddr_to_mfn(READ_SYSREG64(TTBR0_EL2));
 
-    /*
-     * The hardware was configured to forbid mapping both writeable and
-     * executable.
-     * When modifying/creating mapping (i.e _PAGE_PRESENT is set),
-     * prevent any update if this happen.
-     */
-    if ( (flags & _PAGE_PRESENT) && !PAGE_RO_MASK(flags) &&
-         !PAGE_XN_MASK(flags) )
+    if ( flags_has_rwx(flags) )
     {
         mm_printk("Mappings should not be both Writeable and Executable.\n");
         return -EINVAL;
-- 
2.34.1

[PATCH v2 3/6] arm/mpu: Populate a new region in Xen MPU mapping table

[Hari Limaye]

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

Introduce map_pages_to_xen() that is implemented using a new helper,
xen_mpumap_update(), which is responsible for updating Xen MPU memory
mapping table(xen_mpumap), including creating a new entry, updating
or destroying an existing one, it is equivalent to xen_pt_update in MMU.

This commit only implements populating a new entry in Xen MPU memory mapping
table(xen_mpumap).

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>
---
Changes from v1:
- Simplify if condition
- Use normal printk
- Use %# over 0x%
- Add same asserts as in Patch 4
---
 xen/arch/arm/include/asm/mpu/mm.h |  12 ++++
 xen/arch/arm/mpu/mm.c             | 100 ++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index 81e47b9d0b..101002f7d4 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -64,6 +64,7 @@ static inline void context_sync_mpu(void)
  * The following API requires context_sync_mpu() after being used to modify MPU
  * regions:
  *  - write_protection_region
+ *  - xen_mpumap_update
  */
 
 /* Reads the MPU region (into @pr_read) with index @sel from the HW */
@@ -72,6 +73,17 @@ void read_protection_region(pr_t *pr_read, uint8_t sel);
 /* Writes the MPU region (from @pr_write) with index @sel to the HW */
 void write_protection_region(const pr_t *pr_write, uint8_t sel);
 
+/*
+ * Maps an address range into the MPU data structure and updates the HW.
+ * Equivalent to xen_pt_update in an MMU system.
+ *
+ * @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          0 on success, negative on error.
+ */
+int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags);
+
 /*
  * Creates a pr_t structure describing a protection region.
  *
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 25e7f36c1e..dd54b66901 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -6,6 +6,7 @@
 #include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/sizes.h>
+#include <xen/spinlock.h>
 #include <xen/types.h>
 #include <asm/mpu.h>
 #include <asm/mpu/mm.h>
@@ -29,6 +30,8 @@ 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);
+
 static void __init __maybe_unused build_assertions(void)
 {
     /*
@@ -162,6 +165,103 @@ int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
     return MPUMAP_REGION_NOTFOUND;
 }
 
+/*
+ * Allocate a new free EL2 MPU memory region, based on bitmap xen_mpumap_mask.
+ * @param idx   Set to the index of the allocated EL2 MPU region on success.
+ * @return      0 on success, otherwise -ENOENT on failure.
+ */
+static int xen_mpumap_alloc_entry(uint8_t *idx)
+{
+    ASSERT(spin_is_locked(&xen_mpumap_lock));
+
+    *idx = find_first_zero_bit(xen_mpumap_mask, max_mpu_regions);
+    if ( *idx == max_mpu_regions )
+    {
+        printk(XENLOG_ERR "mpu: EL2 MPU memory region mapping pool exhausted\n");
+        return -ENOENT;
+    }
+
+    set_bit(*idx, xen_mpumap_mask);
+    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.
+ *
+ * @param base  Base address (inclusive).
+ * @param limit Limit address (exclusive).
+ * @param flags Region attributes (a combination of PAGE_HYPERVISOR_XXX)
+ * @return      0 on success, otherwise negative on error.
+ */
+static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
+                                   unsigned int flags)
+{
+    uint8_t idx;
+    int rc;
+
+    ASSERT(spin_is_locked(&xen_mpumap_lock));
+
+    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
+    if ( !(rc == MPUMAP_REGION_NOTFOUND) )
+        return -EINVAL;
+
+    /* We are inserting a mapping => Create new region. */
+    if ( flags & _PAGE_PRESENT )
+    {
+        rc = xen_mpumap_alloc_entry(&idx);
+        if ( rc )
+            return -ENOENT;
+
+        xen_mpumap[idx] = pr_of_addr(base, limit, flags);
+
+        write_protection_region(&xen_mpumap[idx], idx);
+    }
+
+    return 0;
+}
+
+int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags)
+{
+    int rc;
+
+    ASSERT(IS_ALIGNED(base, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(limit, PAGE_SIZE));
+    ASSERT(base <= limit);
+
+    if ( flags_has_rwx(flags) )
+    {
+        printk("Mappings should not be both Writeable and Executable\n");
+        return -EINVAL;
+    }
+
+    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;
+    }
+
+    spin_lock(&xen_mpumap_lock);
+
+    rc = xen_mpumap_update_entry(base, limit, flags);
+
+    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)
+{
+    int rc = xen_mpumap_update(mfn_to_maddr(mfn),
+                               mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags);
+    if ( !rc )
+        context_sync_mpu();
+
+    return rc;
+}
+
 void __init setup_mm(void)
 {
     BUG_ON("unimplemented");
-- 
2.34.1

[PATCH v2 4/6] arm/mpu: Destroy an existing entry in Xen MPU memory mapping table

[Hari Limaye]

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

This commit expands xen_mpumap_update/xen_mpumap_update_entry to include
destroying an existing entry.

We define a new helper "disable_mpu_region_from_index" to disable the MPU
region based on index. If region is within [0, 31], we could quickly
disable the MPU region through PRENR_EL2 which provides direct access to the
PRLAR_EL2.EN bits of EL2 MPU regions.

Rignt now, we only support destroying a *WHOLE* MPU memory region,
part-region removing is not supported, as in worst case, it will
leave two fragments behind.

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>
---
Changes from v1:
- Move check for part-region removal outside if condition
- Use normal printk
---
 xen/arch/arm/include/asm/mpu.h        |  2 +
 xen/arch/arm/include/asm/mpu/cpregs.h |  4 ++
 xen/arch/arm/mpu/mm.c                 | 69 ++++++++++++++++++++++++++-
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
index 63560c613b..5053edaf63 100644
--- a/xen/arch/arm/include/asm/mpu.h
+++ b/xen/arch/arm/include/asm/mpu.h
@@ -23,6 +23,8 @@
 #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
 #define MAX_MPU_REGION_NR       NUM_MPU_REGIONS_MASK
 
+#define PRENR_MASK  GENMASK(31, 0)
+
 #ifndef __ASSEMBLY__
 
 /*
diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
index bb15e02df6..9f3b32acd7 100644
--- a/xen/arch/arm/include/asm/mpu/cpregs.h
+++ b/xen/arch/arm/include/asm/mpu/cpregs.h
@@ -6,6 +6,9 @@
 /* CP15 CR0: MPU Type Register */
 #define HMPUIR          p15,4,c0,c0,4
 
+/* CP15 CR6: Protection Region Enable Register */
+#define HPRENR          p15,4,c6,c1,1
+
 /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
 #define HPRSELR         p15,4,c6,c2,1
 #define HPRBAR          p15,4,c6,c3,0
@@ -82,6 +85,7 @@
 /* Alphabetically... */
 #define MPUIR_EL2       HMPUIR
 #define PRBAR_EL2       HPRBAR
+#define PRENR_EL2       HPRENR
 #define PRLAR_EL2       HPRLAR
 #define PRSELR_EL2      HPRSELR
 #endif /* CONFIG_ARM_32 */
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index dd54b66901..2e88c467d5 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -185,6 +185,42 @@ static int xen_mpumap_alloc_entry(uint8_t *idx)
     return 0;
 }
 
+/*
+ * Disable and remove an MPU region from the data structure and MPU registers.
+ *
+ * @param index Index of the MPU region to be disabled.
+ */
+static void disable_mpu_region_from_index(uint8_t index)
+{
+    ASSERT(spin_is_locked(&xen_mpumap_lock));
+    ASSERT(index != INVALID_REGION_IDX);
+
+    if ( !region_is_valid(&xen_mpumap[index]) )
+    {
+        printk(XENLOG_WARNING
+               "mpu: MPU memory region[%u] is already disabled\n", index);
+        return;
+    }
+
+    /* Zeroing the region will also zero the region enable */
+    memset(&xen_mpumap[index], 0, sizeof(pr_t));
+    clear_bit(index, xen_mpumap_mask);
+
+    /*
+     * Both Armv8-R AArch64 and AArch32 have direct access to the enable bit for
+     * MPU regions numbered from 0 to 31.
+     */
+    if ( (index & PRENR_MASK) != 0 )
+    {
+        /* Clear respective bit */
+        uint64_t val = READ_SYSREG(PRENR_EL2) & (~(1UL << index));
+
+        WRITE_SYSREG(val, PRENR_EL2);
+    }
+    else
+        write_protection_region(&xen_mpumap[index], 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.
@@ -203,11 +239,11 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
     ASSERT(spin_is_locked(&xen_mpumap_lock));
 
     rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
-    if ( !(rc == MPUMAP_REGION_NOTFOUND) )
+    if ( rc < 0 )
         return -EINVAL;
 
     /* We are inserting a mapping => Create new region. */
-    if ( flags & _PAGE_PRESENT )
+    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
     {
         rc = xen_mpumap_alloc_entry(&idx);
         if ( rc )
@@ -218,6 +254,20 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
         write_protection_region(&xen_mpumap[idx], idx);
     }
 
+    /*
+     * Currently, we only support destroying a *WHOLE* MPU memory region.
+     * Part-region removal is not supported as in the worst case it will leave
+     * two fragments behind.
+     */
+    if ( rc == MPUMAP_REGION_INCLUSIVE )
+    {
+        printk("mpu: part-region removal is not supported\n");
+        return -EINVAL;
+    }
+
+    if ( !(flags & _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )
+        disable_mpu_region_from_index(idx);
+
     return 0;
 }
 
@@ -251,6 +301,21 @@ int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags)
     return rc;
 }
 
+int destroy_xen_mappings(unsigned long s, unsigned long e)
+{
+    int rc;
+
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
+    ASSERT(s <= e);
+
+    rc = xen_mpumap_update(virt_to_maddr(s), virt_to_maddr(e), 0);
+    if ( !rc )
+        context_sync_mpu();
+
+    return rc;
+}
+
 int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
                      unsigned int flags)
 {
-- 
2.34.1

[PATCH v2 5/6] arm/mpu: Implement early_fdt_map support in MPU systems

[Hari Limaye]

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

Implement the function early_fdt_map(), which is responsible for mapping
the Device Tree Blob in the early stages of the boot process, for MPU
systems.

We make use of the map_pages_to_xen() and destroy_xen_mappings() APIs.
In particular the latter function is necessary in the case that the
initial mapping of the fdt_header is insufficient to cover the entire
DTB, as we must destroy and then remap the region due to the APIs no
providing support for extending the size of an existing region.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from v1:
- Add Ayan's R-b
---
 xen/arch/arm/mpu/setup.c | 74 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
index b4da77003f..ab00cb944b 100644
--- a/xen/arch/arm/mpu/setup.c
+++ b/xen/arch/arm/mpu/setup.c
@@ -1,17 +1,87 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/bootfdt.h>
 #include <xen/bug.h>
 #include <xen/init.h>
+#include <xen/libfdt/libfdt.h>
 #include <xen/mm.h>
+#include <xen/pfn.h>
 #include <xen/types.h>
 #include <asm/setup.h>
 
+static paddr_t __initdata mapped_fdt_paddr = INVALID_PADDR;
+static paddr_t __initdata mapped_fdt_limit = INVALID_PADDR;
+
 void __init setup_pagetables(void) {}
 
 void * __init early_fdt_map(paddr_t fdt_paddr)
 {
-    BUG_ON("unimplemented");
-    return NULL;
+    /* Map at least a page containing the DTB address, exclusive range */
+    paddr_t base = round_pgdown(fdt_paddr);
+    paddr_t limit = round_pgup(fdt_paddr + sizeof(struct fdt_header));
+    unsigned int flags = PAGE_HYPERVISOR_RO;
+    void *fdt_virt = (void *)fdt_paddr; /* virt == paddr for MPU */
+    int rc;
+    unsigned long nr_mfns;
+
+    /*
+     * Check whether the physical FDT address is set and meets the minimum
+     * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at
+     * least 8 bytes so that we always access the magic and size fields
+     * of the FDT header after mapping the first chunk, double check if
+     * that is indeed the case.
+     */
+    BUILD_BUG_ON(MIN_FDT_ALIGN < 8);
+    if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
+        return NULL;
+
+    /* DTB starting at this address has already been mapped. */
+    if ( mapped_fdt_paddr == fdt_paddr )
+        return fdt_virt;
+
+    /*
+     * DTB starting at a different address has been mapped, so destroy this
+     * before continuing.
+     */
+    if ( mapped_fdt_paddr != INVALID_PADDR )
+    {
+        rc = destroy_xen_mappings(round_pgdown(mapped_fdt_paddr),
+                                  mapped_fdt_limit);
+        if ( rc )
+            panic("Unable to unmap existing device-tree.\n");
+    }
+
+    nr_mfns = (limit - base) >> PAGE_SHIFT;
+
+    rc = map_pages_to_xen(base, maddr_to_mfn(base), nr_mfns, flags);
+    if ( rc )
+        panic("Unable to map the device-tree.\n");
+
+    mapped_fdt_paddr = fdt_paddr;
+    mapped_fdt_limit = limit;
+
+    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
+        return NULL;
+
+    limit = round_pgup(fdt_paddr + fdt_totalsize(fdt_virt));
+
+    /* If the mapped range is not enough, map the rest of the DTB. */
+    if ( limit > mapped_fdt_limit )
+    {
+        rc = destroy_xen_mappings(base, mapped_fdt_limit);
+        if ( rc )
+            panic("Unable to unmap the device-tree header.\n");
+
+        nr_mfns = (limit - base) >> PAGE_SHIFT;
+
+        rc = map_pages_to_xen(base, maddr_to_mfn(base), nr_mfns, flags);
+        if ( rc )
+            panic("Unable to map the device-tree.\n");
+
+        mapped_fdt_limit = limit;
+    }
+
+    return fdt_virt;
 }
 
 /*
-- 
2.34.1

[PATCH v2 6/6] arm/mpu: Implement remove_early_mappings for MPU systems

[Hari Limaye]

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

Implement remove_early_mappings for MPU systems.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
---
Changes from v1:
- Add Ayan's R-b
---
 xen/arch/arm/mpu/setup.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
index ab00cb944b..5928b534d5 100644
--- a/xen/arch/arm/mpu/setup.c
+++ b/xen/arch/arm/mpu/setup.c
@@ -97,7 +97,14 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
 
 void __init remove_early_mappings(void)
 {
-    BUG_ON("unimplemented");
+    int rc;
+
+    if ( mapped_fdt_paddr == INVALID_PADDR )
+        return;
+
+    rc = destroy_xen_mappings(round_pgdown(mapped_fdt_paddr), mapped_fdt_limit);
+    if ( rc )
+        panic("Unable to unmap the device-tree.\n");
 }
 
 /*
-- 
2.34.1

Re: [PATCH v2 1/6] arm/mpu: Find MPU region by range

[Orzel, Michal]

On 02/07/2025 16:13, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Implement a function to find the index of a MPU region in the xen_mpumap
> MPU region array. This function will be used in future commits to
> implement creating and destroying MPU regions.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> ---
> Changes from v1:
> - Update commit message
> - Remove internal _index variable
> - Simplify logic by disallowing NULL index parameter
> - Use normal printk
> - Reorder conditional checks
> - Update some comments
> ---
>  xen/arch/arm/include/asm/mpu/mm.h | 29 +++++++++++++++++
>  xen/arch/arm/mpu/mm.c             | 52 +++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index a7f970b465..81e47b9d0b 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -10,6 +10,13 @@
>  #include <asm/mm.h>
>  #include <asm/mpu.h>
>  
> +#define MPUMAP_REGION_OVERLAP      -1
> +#define MPUMAP_REGION_NOTFOUND      0
> +#define MPUMAP_REGION_FOUND         1
> +#define MPUMAP_REGION_INCLUSIVE     2
> +
> +#define INVALID_REGION_IDX     0xFFU
> +
>  extern struct page_info *frame_table;
>  
>  extern uint8_t max_mpu_regions;
> @@ -75,6 +82,28 @@ void write_protection_region(const pr_t *pr_write, uint8_t sel);
>   */
>  pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
>  
> +/*
> + * Checks whether a given memory range is present in the provided table of
> + * MPU protection regions.
> + *
> + * @param table         Array of pr_t protection regions.
> + * @param r_regions     Number of elements in `table`.
> + * @param base          Start of the memory region to be checked (inclusive).
> + * @param limit         End of the memory region to be checked (exclusive).
> + * @param index         Set to the index of the region if an exact or inclusive
> + *                      match is found, and INVALID_REGION otherwise.
> + * @return: Return code indicating the result of the search:
> + *          MPUMAP_REGION_NOTFOUND: no part of the range is present in `table`
> + *          MPUMAP_REGION_FOUND: found an exact match in `table`
> + *          MPUMAP_REGION_INCLUSIVE: found an inclusive match in `table`
> + *          MPUMAP_REGION_OVERLAP: found an overlap with a mapping in `table`
> + *
> + * Note: make sure that the range [`base`, `limit`) refers to the memory region
> + * inclusive of `base` and exclusive of `limit`.
> + */
> +int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> +                          paddr_t limit, uint8_t *index);
> +
>  #endif /* __ARM_MPU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index ccfb37a67b..25e7f36c1e 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -110,6 +110,58 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
>      return region;
>  }
>  
> +int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> +                          paddr_t limit, uint8_t *index)
> +{
> +    ASSERT(index);
> +    *index = INVALID_REGION_IDX;
> +
> +    /* Convert [base, limit) to [base, limit - 1] for inclusive comparison */
> +    limit = limit - 1;
> +
> +    if ( limit < base )
> +    {
> +        printk("Base address %#"PRIpaddr" must be smaller than limit address %#"PRIpaddr"\n",
Given this message, what about region being empty i.e. limit == base? Is it
worth continuing the function?

> +               base, limit);
Here you print limit as inclusive but below as exclusive. Why the difference?

> +        return -EINVAL;
> +    }
> +
> +    for (uint8_t i = 0; i < nr_regions; i++ )
Space before ( and uint8_t.

~Michal

Re: [PATCH v2 3/6] arm/mpu: Populate a new region in Xen MPU mapping table

[Orzel, Michal]

On 02/07/2025 16:13, Hari Limaye wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> Introduce map_pages_to_xen() that is implemented using a new helper,
> xen_mpumap_update(), which is responsible for updating Xen MPU memory
> mapping table(xen_mpumap), including creating a new entry, updating
> or destroying an existing one, it is equivalent to xen_pt_update in MMU.
> 
> This commit only implements populating a new entry in Xen MPU memory mapping
> table(xen_mpumap).
> 
> 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>
> ---
> Changes from v1:
> - Simplify if condition
> - Use normal printk
> - Use %# over 0x%
> - Add same asserts as in Patch 4
> ---
>  xen/arch/arm/include/asm/mpu/mm.h |  12 ++++
>  xen/arch/arm/mpu/mm.c             | 100 ++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index 81e47b9d0b..101002f7d4 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -64,6 +64,7 @@ static inline void context_sync_mpu(void)
>   * The following API requires context_sync_mpu() after being used to modify MPU
>   * regions:
>   *  - write_protection_region
> + *  - xen_mpumap_update
>   */
>  
>  /* Reads the MPU region (into @pr_read) with index @sel from the HW */
> @@ -72,6 +73,17 @@ void read_protection_region(pr_t *pr_read, uint8_t sel);
>  /* Writes the MPU region (from @pr_write) with index @sel to the HW */
>  void write_protection_region(const pr_t *pr_write, uint8_t sel);
>  
> +/*
> + * Maps an address range into the MPU data structure and updates the HW.
> + * Equivalent to xen_pt_update in an MMU system.
> + *
> + * @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          0 on success, negative on error.
> + */
> +int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags);
> +
>  /*
>   * Creates a pr_t structure describing a protection region.
>   *
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 25e7f36c1e..dd54b66901 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -6,6 +6,7 @@
>  #include <xen/lib.h>
>  #include <xen/mm.h>
>  #include <xen/sizes.h>
> +#include <xen/spinlock.h>
>  #include <xen/types.h>
>  #include <asm/mpu.h>
>  #include <asm/mpu/mm.h>
> @@ -29,6 +30,8 @@ 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);
> +
>  static void __init __maybe_unused build_assertions(void)
>  {
>      /*
> @@ -162,6 +165,103 @@ int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>      return MPUMAP_REGION_NOTFOUND;
>  }
>  
> +/*
> + * Allocate a new free EL2 MPU memory region, based on bitmap xen_mpumap_mask.
I would clarify that you allocate entry in bitmap, not a region.

> + * @param idx   Set to the index of the allocated EL2 MPU region on success.
> + * @return      0 on success, otherwise -ENOENT on failure.
> + */
> +static int xen_mpumap_alloc_entry(uint8_t *idx)
> +{
> +    ASSERT(spin_is_locked(&xen_mpumap_lock));
> +
> +    *idx = find_first_zero_bit(xen_mpumap_mask, max_mpu_regions);
> +    if ( *idx == max_mpu_regions )
> +    {
> +        printk(XENLOG_ERR "mpu: EL2 MPU memory region mapping pool exhausted\n");
> +        return -ENOENT;
> +    }
> +
> +    set_bit(*idx, xen_mpumap_mask);
Empty line here please.

> +    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.
> + *
> + * @param base  Base address (inclusive).
> + * @param limit Limit address (exclusive).
> + * @param flags Region attributes (a combination of PAGE_HYPERVISOR_XXX)
> + * @return      0 on success, otherwise negative on error.
> + */
> +static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
> +                                   unsigned int flags)
> +{
> +    uint8_t idx;
> +    int rc;
> +
> +    ASSERT(spin_is_locked(&xen_mpumap_lock));
> +
> +    rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
> +    if ( !(rc == MPUMAP_REGION_NOTFOUND) )
Why not ( rc != MPUMAP_REGION_NOTFOUND )?

> +        return -EINVAL;
> +
> +    /* We are inserting a mapping => Create new region. */
> +    if ( flags & _PAGE_PRESENT )
Wouldn't it make more sense to have this check before calling
mpumap_contains_region()?

> +    {
> +        rc = xen_mpumap_alloc_entry(&idx);
> +        if ( rc )
> +            return -ENOENT;
> +
> +        xen_mpumap[idx] = pr_of_addr(base, limit, flags);
> +
> +        write_protection_region(&xen_mpumap[idx], idx);
> +    }
> +
> +    return 0;
> +}
> +
> +int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags)
> +{
> +    int rc;
> +
> +    ASSERT(IS_ALIGNED(base, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(limit, PAGE_SIZE));
What's the purpose of these asserts given the exact same check a few lines below?

> +    ASSERT(base <= limit);
Hmm, given limit being exclusive and later subtraction to become inclusive, if
we pass base == limit, you will end up with limit being smaller than base. Also,
if limit == 0, you will underflow it.

> +
> +    if ( flags_has_rwx(flags) )
> +    {
> +        printk("Mappings should not be both Writeable and Executable\n");
> +        return -EINVAL;
> +    }
> +
> +    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;
> +    }
> +
> +    spin_lock(&xen_mpumap_lock);
> +
> +    rc = xen_mpumap_update_entry(base, limit, flags);
> +
> +    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)
> +{
> +    int rc = xen_mpumap_update(mfn_to_maddr(mfn),
What do you expect to be passed as virt? I would expect maddr which could save
you the conversion here.

> +                               mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags);
> +    if ( !rc )
> +        context_sync_mpu();
Shouldn't this be called inside xen_mpumap_update() and within the locked section?

> +
> +    return rc;
> +}
> +
>  void __init setup_mm(void)
>  {
>      BUG_ON("unimplemented");

~Michal

Re: [PATCH v2 4/6] arm/mpu: Destroy an existing entry in Xen MPU memory mapping table

[Orzel, Michal]

On 02/07/2025 16:13, Hari Limaye wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
> 
> This commit expands xen_mpumap_update/xen_mpumap_update_entry to include
> destroying an existing entry.
> 
> We define a new helper "disable_mpu_region_from_index" to disable the MPU
> region based on index. If region is within [0, 31], we could quickly
> disable the MPU region through PRENR_EL2 which provides direct access to the
> PRLAR_EL2.EN bits of EL2 MPU regions.
> 
> Rignt now, we only support destroying a *WHOLE* MPU memory region,
> part-region removing is not supported, as in worst case, it will
> leave two fragments behind.
> 
> 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>
> ---
> Changes from v1:
> - Move check for part-region removal outside if condition
> - Use normal printk
> ---
>  xen/arch/arm/include/asm/mpu.h        |  2 +
>  xen/arch/arm/include/asm/mpu/cpregs.h |  4 ++
>  xen/arch/arm/mpu/mm.c                 | 69 ++++++++++++++++++++++++++-
>  3 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 63560c613b..5053edaf63 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -23,6 +23,8 @@
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>  #define MAX_MPU_REGION_NR       NUM_MPU_REGIONS_MASK
>  
> +#define PRENR_MASK  GENMASK(31, 0)
> +
>  #ifndef __ASSEMBLY__
>  
>  /*
> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h b/xen/arch/arm/include/asm/mpu/cpregs.h
> index bb15e02df6..9f3b32acd7 100644
> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> @@ -6,6 +6,9 @@
>  /* CP15 CR0: MPU Type Register */
>  #define HMPUIR          p15,4,c0,c0,4
>  
> +/* CP15 CR6: Protection Region Enable Register */
> +#define HPRENR          p15,4,c6,c1,1
> +
>  /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
>  #define HPRSELR         p15,4,c6,c2,1
>  #define HPRBAR          p15,4,c6,c3,0
> @@ -82,6 +85,7 @@
>  /* Alphabetically... */
>  #define MPUIR_EL2       HMPUIR
>  #define PRBAR_EL2       HPRBAR
> +#define PRENR_EL2       HPRENR
>  #define PRLAR_EL2       HPRLAR
>  #define PRSELR_EL2      HPRSELR
>  #endif /* CONFIG_ARM_32 */
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index dd54b66901..2e88c467d5 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -185,6 +185,42 @@ static int xen_mpumap_alloc_entry(uint8_t *idx)
>      return 0;
>  }
>  
> +/*
> + * Disable and remove an MPU region from the data structure and MPU registers.
> + *
> + * @param index Index of the MPU region to be disabled.
> + */
> +static void disable_mpu_region_from_index(uint8_t index)
> +{
> +    ASSERT(spin_is_locked(&xen_mpumap_lock));
> +    ASSERT(index != INVALID_REGION_IDX);
> +
> +    if ( !region_is_valid(&xen_mpumap[index]) )
> +    {
> +        printk(XENLOG_WARNING
> +               "mpu: MPU memory region[%u] is already disabled\n", index);
> +        return;
> +    }
> +
> +    /* Zeroing the region will also zero the region enable */
> +    memset(&xen_mpumap[index], 0, sizeof(pr_t));
Is it ok that for a fast case (i.e. 0-31) our representation of prbar/prlar will
be different from the HW i.e. xen_mpumap[index] is 0 vs only .en bit of prlar
being 0 in HW?

> +    clear_bit(index, xen_mpumap_mask);
> +
> +    /*
> +     * Both Armv8-R AArch64 and AArch32 have direct access to the enable bit for
> +     * MPU regions numbered from 0 to 31.
> +     */
> +    if ( (index & PRENR_MASK) != 0 )
> +    {
> +        /* Clear respective bit */
> +        uint64_t val = READ_SYSREG(PRENR_EL2) & (~(1UL << index));
On AArch32 the register is 32bit, so I think you should use register_t type.

> +
> +        WRITE_SYSREG(val, PRENR_EL2);
> +    }
> +    else
> +        write_protection_region(&xen_mpumap[index], 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.
> @@ -203,11 +239,11 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>      ASSERT(spin_is_locked(&xen_mpumap_lock));
>  
>      rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
> -    if ( !(rc == MPUMAP_REGION_NOTFOUND) )
> +    if ( rc < 0 )
>          return -EINVAL;
>  
>      /* We are inserting a mapping => Create new region. */
> -    if ( flags & _PAGE_PRESENT )
> +    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
I think we need more sanity checking. What if flags has _PAGE_PRESENT but rc !=
MPUMAP_REGION_NOTFOUND, e.g. function called to modify existing entry? You will
silently return success. Maybe a similar function as for MMU is needed to
perform some sanity checks depending on the reason of the call?

>      {
>          rc = xen_mpumap_alloc_entry(&idx);
>          if ( rc )
> @@ -218,6 +254,20 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>          write_protection_region(&xen_mpumap[idx], idx);
>      }
>  
> +    /*
> +     * Currently, we only support destroying a *WHOLE* MPU memory region.
> +     * Part-region removal is not supported as in the worst case it will leave
> +     * two fragments behind.
> +     */
> +    if ( rc == MPUMAP_REGION_INCLUSIVE )
> +    {
> +        printk("mpu: part-region removal is not supported\n");
You mention removal but why do you limit this place to removal only? You don't
have any checks making sure that flags is 0 at this point.

> +        return -EINVAL;
> +    }
> +
> +    if ( !(flags & _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )
> +        disable_mpu_region_from_index(idx);
> +
>      return 0;
>  }
>  
> @@ -251,6 +301,21 @@ int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags)
>      return rc;
>  }
>  
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> +{
> +    int rc;
> +
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> +    ASSERT(s <= e);
> +
> +    rc = xen_mpumap_update(virt_to_maddr(s), virt_to_maddr(e), 0);
> +    if ( !rc )
> +        context_sync_mpu();
> +
> +    return rc;
> +}
> +
>  int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
>                       unsigned int flags)
>  {

~Michal

Re: [PATCH v2 5/6] arm/mpu: Implement early_fdt_map support in MPU systems

[Orzel, Michal]

On 02/07/2025 16:14, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Implement the function early_fdt_map(), which is responsible for mapping
> the Device Tree Blob in the early stages of the boot process, for MPU
> systems.
> 
> We make use of the map_pages_to_xen() and destroy_xen_mappings() APIs.
> In particular the latter function is necessary in the case that the
> initial mapping of the fdt_header is insufficient to cover the entire
> DTB, as we must destroy and then remap the region due to the APIs no
> providing support for extending the size of an existing region.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from v1:
> - Add Ayan's R-b
> ---
>  xen/arch/arm/mpu/setup.c | 74 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
> index b4da77003f..ab00cb944b 100644
> --- a/xen/arch/arm/mpu/setup.c
> +++ b/xen/arch/arm/mpu/setup.c
> @@ -1,17 +1,87 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <xen/bootfdt.h>
>  #include <xen/bug.h>
>  #include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
>  #include <xen/mm.h>
> +#include <xen/pfn.h>
>  #include <xen/types.h>
>  #include <asm/setup.h>
>  
> +static paddr_t __initdata mapped_fdt_paddr = INVALID_PADDR;
I think it would be better to name it mapped_fdt_base, given that in MPU
everything refers to base,limit.

> +static paddr_t __initdata mapped_fdt_limit = INVALID_PADDR;
> +
>  void __init setup_pagetables(void) {}
>  
>  void * __init early_fdt_map(paddr_t fdt_paddr)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    /* Map at least a page containing the DTB address, exclusive range */
> +    paddr_t base = round_pgdown(fdt_paddr);
> +    paddr_t limit = round_pgup(fdt_paddr + sizeof(struct fdt_header));
> +    unsigned int flags = PAGE_HYPERVISOR_RO;
> +    void *fdt_virt = (void *)fdt_paddr; /* virt == paddr for MPU */
> +    int rc;
> +    unsigned long nr_mfns;
> +
> +    /*
> +     * Check whether the physical FDT address is set and meets the minimum
> +     * alignment requirement. Since we are relying on MIN_FDT_ALIGN to be at
> +     * least 8 bytes so that we always access the magic and size fields
> +     * of the FDT header after mapping the first chunk, double check if
> +     * that is indeed the case.
> +     */
> +    BUILD_BUG_ON(MIN_FDT_ALIGN < 8);
> +    if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN )
> +        return NULL;
> +
> +    /* DTB starting at this address has already been mapped. */
When can this happen?

> +    if ( mapped_fdt_paddr == fdt_paddr )
> +        return fdt_virt;
> +
> +    /*
> +     * DTB starting at a different address has been mapped, so destroy this
> +     * before continuing.
> +     */
> +    if ( mapped_fdt_paddr != INVALID_PADDR )
> +    {
> +        rc = destroy_xen_mappings(round_pgdown(mapped_fdt_paddr),
> +                                  mapped_fdt_limit);
> +        if ( rc )
> +            panic("Unable to unmap existing device-tree.\n");
NIT: no need for a dot at the end. It only takes space and is really not needed.

> +    }
> +
> +    nr_mfns = (limit - base) >> PAGE_SHIFT;
> +
> +    rc = map_pages_to_xen(base, maddr_to_mfn(base), nr_mfns, flags);
> +    if ( rc )
> +        panic("Unable to map the device-tree.\n");
> +
> +    mapped_fdt_paddr = fdt_paddr;
> +    mapped_fdt_limit = limit;
> +
> +    if ( fdt_magic(fdt_virt) != FDT_MAGIC )
> +        return NULL;
> +
> +    limit = round_pgup(fdt_paddr + fdt_totalsize(fdt_virt));
You're missing a sanity check for MAX_FDT_SIZE

> +
> +    /* If the mapped range is not enough, map the rest of the DTB. */
> +    if ( limit > mapped_fdt_limit )
> +    {
> +        rc = destroy_xen_mappings(base, mapped_fdt_limit);
> +        if ( rc )
> +            panic("Unable to unmap the device-tree header.\n");
> +
> +        nr_mfns = (limit - base) >> PAGE_SHIFT;
> +
> +        rc = map_pages_to_xen(base, maddr_to_mfn(base), nr_mfns, flags);
> +        if ( rc )
> +            panic("Unable to map the device-tree.\n");
> +
> +        mapped_fdt_limit = limit;
> +    }
> +
> +    return fdt_virt;
>  }
>  
>  /*

~Michal

Re: [PATCH v2 6/6] arm/mpu: Implement remove_early_mappings for MPU systems

[Orzel, Michal]

On 02/07/2025 16:14, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
> 
> Implement remove_early_mappings for MPU systems.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> Changes from v1:
> - Add Ayan's R-b
> ---
>  xen/arch/arm/mpu/setup.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
> index ab00cb944b..5928b534d5 100644
> --- a/xen/arch/arm/mpu/setup.c
> +++ b/xen/arch/arm/mpu/setup.c
> @@ -97,7 +97,14 @@ void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
>  
>  void __init remove_early_mappings(void)
>  {
> -    BUG_ON("unimplemented");
> +    int rc;
> +
> +    if ( mapped_fdt_paddr == INVALID_PADDR )
Hmm, I cannot think of a scenario where this is true. What am I missing?

> +        return;
> +
> +    rc = destroy_xen_mappings(round_pgdown(mapped_fdt_paddr), mapped_fdt_limit);
> +    if ( rc )
> +        panic("Unable to unmap the device-tree.\n");
NIT: please, no dots at the end

>  }
>  
>  /*

~Michal

Re: [PATCH v2 4/6] arm/mpu: Destroy an existing entry in Xen MPU memory mapping table

[Hari Limaye]

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

Hi Michal,
Thank you for reviewing the patch.

> > +    /* Zeroing the region will also zero the region enable */
> > +    memset(&xen_mpumap[index], 0, sizeof(pr_t));
> Is it ok that for a fast case (i.e. 0-31) our representation of prbar/prlar will
> be different from the HW i.e. xen_mpumap[index] is 0 vs only .en bit of prlar
> being 0 in HW?

I think this should not matter - there is nothing reading registers directly and
all the decisions are taken looking into xen_mpumap.

However, if you would prefer, we could alter the logic here to only clear the .en
bit in xen_mpumap[index] in the fast case, so that things remain consistent? Alternatively we could always directly zero the entirety of the registers, but then we would remove the use of the fast case entirely.

Many thanks,
Hari

[-- Attachment #2: Type: text/html, Size: 3766 bytes --]

Re: [PATCH v2 5/6] arm/mpu: Implement early_fdt_map support in MPU systems

[Hari Limaye]

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

Hi Michal,

Thank you for reviewing the patch. To answer the following question:
> > +    /* DTB starting at this address has already been mapped. */
> When can this happen?

In xen/arch/arm/setup.c `start_xen` early_fdt_map(fdt_paddr) is called twice,
before and after `setup_page_tables` - which is a no-op on an MPU system. This
seemed like the neatest way to handle this - but maybe you'd prefer further
clarification in the comment here?

Many thanks,
Hari

[-- Attachment #2: Type: text/html, Size: 2984 bytes --]

Re: [PATCH v2 3/6] arm/mpu: Populate a new region in Xen MPU mapping table

[Hari Limaye]

Hi Michal,

>> +int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
>> +                     unsigned int flags)
>> +{
>> +    int rc = xen_mpumap_update(mfn_to_maddr(mfn),
> What do you expect to be passed as virt? I would expect maddr which could save
> you the conversion here.

I think you should be correct here and it should be fine to use virt directly. However, the one place in common code that I can see xen_mpumap_update is called is in `xen/common/vmap.c`; and here the virtual address is not the same as the machine address, but it looks like this is only used for LIVEPATCH and we perhaps don’t care about this on MPU systems?

Cheers,
Hari

Re: [PATCH v2 4/6] arm/mpu: Destroy an existing entry in Xen MPU memory mapping table

[Orzel, Michal]

On 11/07/2025 08:04, Hari Limaye wrote:
> Hi Michal,
> 
> Thank you for reviewing the patch.
> 
>  
> 
>> > +    /* Zeroing the region will also zero the region enable */
> 
>> > +    memset(&xen_mpumap[index], 0, sizeof(pr_t));
> 
>> Is it ok that for a fast case (i.e. 0-31) our representation of prbar/prlar will
> 
>> be different from the HW i.e. xen_mpumap[index] is 0 vs only .en bit of prlar
> 
>> being 0 in HW?
> 
>  
> 
> I think this should not matter - there is nothing reading registers directly and
> 
> all the decisions are taken looking into xen_mpumap.
> 
>  
> 
> However, if you would prefer, we could alter the logic here to only clear the .en
> 
> bit in xen_mpumap[index] in the fast case, so that things remain consistent?
> Alternatively we could always directly zero the entirety of the registers, but
> then we would remove the use of the fast case entirely.
I don't have a strong preference here. I think we can keep your current solution.

~Michal

Re: [PATCH v2 5/6] arm/mpu: Implement early_fdt_map support in MPU systems

[Orzel, Michal]

On 11/07/2025 08:05, Hari Limaye wrote:
> Hi Michal,
> 
>  
> 
> Thank you for reviewing the patch. To answer the following question:
> 
>> > +    /* DTB starting at this address has already been mapped. */
> 
>> When can this happen?
> 
>  
> 
> In xen/arch/arm/setup.c `start_xen` early_fdt_map(fdt_paddr) is called twice,
> 
> before and after `setup_page_tables` - which is a no-op on an MPU system. This
> 
> seemed like the neatest way to handle this - but maybe you'd prefer further
> 
> clarification in the comment here?
Yes, I would like this comment to be improved a bit.

~Michal

Re: [PATCH v2 3/6] arm/mpu: Populate a new region in Xen MPU mapping table

[Orzel, Michal]

On 11/07/2025 17:10, Hari Limaye wrote:
> Hi Michal,
> 
>>> +int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
>>> +                     unsigned int flags)
>>> +{
>>> +    int rc = xen_mpumap_update(mfn_to_maddr(mfn),
>> What do you expect to be passed as virt? I would expect maddr which could save
>> you the conversion here.
> 
> I think you should be correct here and it should be fine to use virt directly. However, the one place in common code that I can see xen_mpumap_update is called is in `xen/common/vmap.c`; and here the virtual address is not the same as the machine address, but it looks like this is only used for LIVEPATCH and we perhaps don’t care about this on MPU systems?
AFAICT you will not enable VMAP on MPU.

~Michal