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

[PATCH 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.

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                 | 229 ++++++++++++++++++++++++++
 xen/arch/arm/mpu/setup.c              |  83 +++++++++-
 8 files changed, 374 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH 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.

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
---
 xen/arch/arm/include/asm/mpu/mm.h | 29 ++++++++++++++
 xen/arch/arm/mpu/mm.c             | 66 +++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+)

diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index a7f970b465..a0f0d86d4a 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 half-open
+ * interval inclusive of #base and exclusive of #limit.
+ */
+int mpumap_contain_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..15197339b1 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -12,6 +12,18 @@
 #include <asm/page.h>
 #include <asm/sysregs.h>
 
+#ifdef NDEBUG
+static inline void __attribute__ ((__format__ (__printf__, 1, 2)))
+region_printk(const char *fmt, ...) {}
+#else /* !NDEBUG */
+#define region_printk(fmt, args...)         \
+    do                                      \
+    {                                       \
+        dprintk(XENLOG_ERR, fmt, ## args);  \
+        WARN();                             \
+    } while (0)
+#endif /* NDEBUG */
+
 struct page_info *frame_table;
 
 /* Maximum number of supported MPU memory regions by the EL2 MPU. */
@@ -110,6 +122,60 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
     return region;
 }
 
+int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
+                          paddr_t limit, uint8_t *index)
+{
+    uint8_t i = 0, _index;
+
+    /* Allow index to be NULL */
+    index = index ? index : &_index;
+
+    /* Inside mpumap_contain_region check for inclusive range */
+    limit = limit - 1;
+
+    *index = INVALID_REGION_IDX;
+
+    if ( limit < base )
+    {
+        region_printk("Base address 0x%"PRIpaddr" must be smaller than limit address 0x%"PRIpaddr"\n",
+                      base, limit);
+        return -EINVAL;
+    }
+
+    for ( ; i < nr_regions; i++ )
+    {
+        paddr_t iter_base = pr_get_base(&table[i]);
+        paddr_t iter_limit = pr_get_limit(&table[i]);
+
+        /* Found an exact valid match */
+        if ( (iter_base == base) && (iter_limit == limit) &&
+             region_is_valid(&table[i]) )
+        {
+            *index = i;
+            return MPUMAP_REGION_FOUND;
+        }
+
+        /* No overlapping */
+        if ( (iter_limit < base) || (iter_base > limit) )
+            continue;
+
+        /* Inclusive and valid */
+        if ( (base >= iter_base) && (limit <= iter_limit) &&
+             region_is_valid(&table[i]) )
+        {
+            *index = i;
+            return MPUMAP_REGION_INCLUSIVE;
+        }
+
+        /* Overlap */
+        region_printk("Range 0x%"PRIpaddr" - 0x%"PRIpaddr" overlaps with the existing region 0x%"PRIpaddr" - 0x%"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 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 update the mapping.

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

Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
 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..9daaa96d93 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 */
+extern 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..c2da1e3a05 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 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) )
+        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 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>
---
 xen/arch/arm/include/asm/mpu/mm.h | 12 ++++
 xen/arch/arm/mpu/mm.c             | 96 +++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index a0f0d86d4a..f0f41db210 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 15197339b1..1de28d2120 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>
@@ -41,6 +42,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)
 {
     /*
@@ -176,6 +179,99 @@ int mpumap_contain_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_contain_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
+    if ( (rc < 0) || (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;
+
+    if ( flags_has_rwx(flags) )
+    {
+        region_printk("Mappings should not be both Writeable and Executable\n");
+        return -EINVAL;
+    }
+
+    if ( !IS_ALIGNED(base, PAGE_SIZE) || !IS_ALIGNED(limit, PAGE_SIZE) )
+    {
+        region_printk("base address 0x%"PRIpaddr", or limit address 0x%"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 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>
---
 xen/arch/arm/include/asm/mpu.h        |  2 +
 xen/arch/arm/include/asm/mpu/cpregs.h |  4 ++
 xen/arch/arm/mpu/mm.c                 | 71 ++++++++++++++++++++++++++-
 3 files changed, 75 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 1de28d2120..23230936f7 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -199,6 +199,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.
@@ -217,11 +253,11 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
     ASSERT(spin_is_locked(&xen_mpumap_lock));
 
     rc = mpumap_contain_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
-    if ( (rc < 0) || (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 )
@@ -232,6 +268,22 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
         write_protection_region(&xen_mpumap[idx], idx);
     }
 
+    if ( !(flags & _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )
+    {
+        /*
+         * Currently, 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.
+         */
+        if ( MPUMAP_REGION_INCLUSIVE == rc )
+        {
+            region_printk("mpu: part-region removing is not supported\n");
+            return -EINVAL;
+        }
+
+        disable_mpu_region_from_index(idx);
+    }
+
     return 0;
 }
 
@@ -261,6 +313,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 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>
---
 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 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>
---
 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 2/6] xen/arm: Introduce flags_has_rwx helper

[Ayan Kumar Halder]

Hi,

On 20/06/2025 10:49, Hari Limaye wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> 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 update the mapping.
>
> This check was already present in pt.c but since it will
> be used also for MPU system, it's wrapped into a function
> now.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

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

[Ayan Kumar Halder]

Hi Hari,

Some questions.

On 20/06/2025 10:49, Hari Limaye wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> 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>
> ---
>   xen/arch/arm/include/asm/mpu/mm.h | 12 ++++
>   xen/arch/arm/mpu/mm.c             | 96 +++++++++++++++++++++++++++++++
>   2 files changed, 108 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index a0f0d86d4a..f0f41db210 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 15197339b1..1de28d2120 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>
> @@ -41,6 +42,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)
>   {
>       /*
> @@ -176,6 +179,99 @@ int mpumap_contain_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_contain_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
> +    if ( (rc < 0) || (rc > MPUMAP_REGION_NOTFOUND) )
if ( !(rc == MPUMAP_REGION_NOTFOUND) ) <<-- Does it read better ?
> +        return -EINVAL;
> +
> +    /* We are inserting a mapping => Create new region. */
> +    if ( flags & _PAGE_PRESENT )

Why do we need to check for this flag ? Or where have we set it.

If we have reached here, doesn't it mean that the region does not exist. 
So we need to create one.

> +    {
> +        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;
> +
> +    if ( flags_has_rwx(flags) )
> +    {
> +        region_printk("Mappings should not be both Writeable and Executable\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( !IS_ALIGNED(base, PAGE_SIZE) || !IS_ALIGNED(limit, PAGE_SIZE) )
> +    {
> +        region_printk("base address 0x%"PRIpaddr", or limit address 0x%"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");

Rest look good to me.

- Ayan

> --
> 2.34.1
>
>

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

[Ayan Kumar Halder]

Hi Hari,

On 20/06/2025 10:49, Hari Limaye wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> 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>
> ---
>   xen/arch/arm/include/asm/mpu.h        |  2 +
>   xen/arch/arm/include/asm/mpu/cpregs.h |  4 ++
>   xen/arch/arm/mpu/mm.c                 | 71 ++++++++++++++++++++++++++-
>   3 files changed, 75 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 1de28d2120..23230936f7 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -199,6 +199,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);

NIT.

These 2 lines we can move before the if { ..}. So that the region is 
zeroed even if the region is disabled. This will add a small overhead, 
but we will be sure that the region is zeroed whenever it is disabled.

> +
> +    /*
> +     * 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.
> @@ -217,11 +253,11 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>       ASSERT(spin_is_locked(&xen_mpumap_lock));
>
>       rc = mpumap_contain_region(xen_mpumap, max_mpu_regions, base, limit, &idx);
> -    if ( (rc < 0) || (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) )

Same question in this patch , why do we need to check for _PAGE_PRESENT. 
Can't we just rely on MPUMAP_REGION_XXX ?

>       {
>           rc = xen_mpumap_alloc_entry(&idx);
>           if ( rc )
> @@ -232,6 +268,22 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>           write_protection_region(&xen_mpumap[idx], idx);
>       }
>
> +    if ( !(flags & _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )
> +    {
> +        /*
> +         * Currently, 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.
> +         */
> +        if ( MPUMAP_REGION_INCLUSIVE == rc )
> +        {
> +            region_printk("mpu: part-region removing is not supported\n");
> +            return -EINVAL;
> +        }

NIT.

Can we keep this ^^^ outside of the outer if condition ie "if ( !(flags 
& _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )" ?

> +
> +        disable_mpu_region_from_index(idx);
> +    }
> +
>       return 0;
>   }
>
> @@ -261,6 +313,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);

Can we have these asserts in xen_mpumap_update() as well ?

> +
> +    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
- Ayan
>
>

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

[Ayan Kumar Halder]

Hi,

On 20/06/2025 10:49, Hari Limaye wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> 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.
>
> 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>
> ---
>   xen/arch/arm/include/asm/mpu/mm.h | 29 ++++++++++++++
>   xen/arch/arm/mpu/mm.c             | 66 +++++++++++++++++++++++++++++++
>   2 files changed, 95 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index a7f970b465..a0f0d86d4a 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 half-open
> + * interval inclusive of #base and exclusive of #limit.
> + */
> +int mpumap_contain_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..15197339b1 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -12,6 +12,18 @@
>   #include <asm/page.h>
>   #include <asm/sysregs.h>
>
> +#ifdef NDEBUG
> +static inline void __attribute__ ((__format__ (__printf__, 1, 2)))
> +region_printk(const char *fmt, ...) {}
> +#else /* !NDEBUG */
> +#define region_printk(fmt, args...)         \
> +    do                                      \
> +    {                                       \
> +        dprintk(XENLOG_ERR, fmt, ## args);  \
> +        WARN();                             \
> +    } while (0)
> +#endif /* NDEBUG */
> +
>   struct page_info *frame_table;
>
>   /* Maximum number of supported MPU memory regions by the EL2 MPU. */
> @@ -110,6 +122,60 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
>       return region;
>   }
>
> +int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> +                          paddr_t limit, uint8_t *index)
> +{
> +    uint8_t i = 0, _index;
> +
> +    /* Allow index to be NULL */
> +    index = index ? index : &_index;
> +
> +    /* Inside mpumap_contain_region check for inclusive range */
> +    limit = limit - 1;
> +
> +    *index = INVALID_REGION_IDX;
> +
> +    if ( limit < base )
> +    {
> +        region_printk("Base address 0x%"PRIpaddr" must be smaller than limit address 0x%"PRIpaddr"\n",
> +                      base, limit);
> +        return -EINVAL;
> +    }
> +
> +    for ( ; i < nr_regions; i++ )
> +    {
> +        paddr_t iter_base = pr_get_base(&table[i]);
> +        paddr_t iter_limit = pr_get_limit(&table[i]);
> +
> +        /* Found an exact valid match */
> +        if ( (iter_base == base) && (iter_limit == limit) &&
> +             region_is_valid(&table[i]) )
> +        {
> +            *index = i;
> +            return MPUMAP_REGION_FOUND;
> +        }
> +
> +        /* No overlapping */
> +        if ( (iter_limit < base) || (iter_base > limit) )
> +            continue;
> +
> +        /* Inclusive and valid */
> +        if ( (base >= iter_base) && (limit <= iter_limit) &&
> +             region_is_valid(&table[i]) )
> +        {
> +            *index = i;
> +            return MPUMAP_REGION_INCLUSIVE;
> +        }
> +
> +        /* Overlap */
> +        region_printk("Range 0x%"PRIpaddr" - 0x%"PRIpaddr" overlaps with the existing region 0x%"PRIpaddr" - 0x%"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");

LGTM.

- Ayan

> --
> 2.34.1
>
>

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

[Ayan Kumar Halder]

On 20/06/2025 10:49, Hari Limaye wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> 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>
> ---
>   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;
>   }

LGTM

Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

>
>   /*
> --
> 2.34.1
>
>

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

[Ayan Kumar Halder]

Hi,

On 20/06/2025 10:49, Hari Limaye wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> 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>

I have tested the series on R52 and R82 and it works fine.

- Ayan

> ---
>   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 1/6] arm/mpu: Find MPU region by range

[Orzel, Michal]

On 20/06/2025 11:49, 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.
The commit msg should also mention why a change is needed.

> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> ---
>  xen/arch/arm/include/asm/mpu/mm.h | 29 ++++++++++++++
>  xen/arch/arm/mpu/mm.c             | 66 +++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index a7f970b465..a0f0d86d4a 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`.
NIT: in other places you refer to already mentioned parameters using #param and
not `param`.

> + * @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 half-open
> + * interval inclusive of #base and exclusive of #limit.
What does half-open interval mean?

> + */
> +int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
NIT: mpumap is a table (singular), so it should be s/contain/contains

> +                          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..15197339b1 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -12,6 +12,18 @@
>  #include <asm/page.h>
>  #include <asm/sysregs.h>
>  
> +#ifdef NDEBUG
> +static inline void __attribute__ ((__format__ (__printf__, 1, 2)))
> +region_printk(const char *fmt, ...) {}
> +#else /* !NDEBUG */
> +#define region_printk(fmt, args...)         \
> +    do                                      \
> +    {                                       \
> +        dprintk(XENLOG_ERR, fmt, ## args);  \
> +        WARN();                             \
> +    } while (0)
> +#endif /* NDEBUG */
> +
>  struct page_info *frame_table;
>  
>  /* Maximum number of supported MPU memory regions by the EL2 MPU. */
> @@ -110,6 +122,60 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
>      return region;
>  }
>  
> +int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> +                          paddr_t limit, uint8_t *index)
> +{
> +    uint8_t i = 0, _index;
Why underscore? I don't know what MISRA thinks of this but looks similar to
reserve identifiers and I don't think there is a need to use it here.

> +
> +    /* Allow index to be NULL */
> +    index = index ? index : &_index;
If index argument is NULL, why bother setting this internal variable _index?

> +
> +    /* Inside mpumap_contain_region check for inclusive range */
What does this comment supposed to mean (we are already in this function)

> +    limit = limit - 1;
> +
> +    *index = INVALID_REGION_IDX;
> +
> +    if ( limit < base )
> +    {
> +        region_printk("Base address 0x%"PRIpaddr" must be smaller than limit address 0x%"PRIpaddr"\n",
Why not normal printk? I think it's important to see such message.

Also %# is preferred over 0x%

> +                      base, limit);
> +        return -EINVAL;
> +    }
> +
> +    for ( ; i < nr_regions; i++ )
> +    {
> +        paddr_t iter_base = pr_get_base(&table[i]);
> +        paddr_t iter_limit = pr_get_limit(&table[i]);
> +
> +        /* Found an exact valid match */
> +        if ( (iter_base == base) && (iter_limit == limit) &&
> +             region_is_valid(&table[i]) )
I think the check for valid region should be first. No need for other two if
region is invalid. Also, shouldn't we check for region being valid right at the
start of the loop?

> +        {
> +            *index = i;
> +            return MPUMAP_REGION_FOUND;
> +        }
> +
> +        /* No overlapping */
> +        if ( (iter_limit < base) || (iter_base > limit) )
> +            continue;
> +
> +        /* Inclusive and valid */
> +        if ( (base >= iter_base) && (limit <= iter_limit) &&
> +             region_is_valid(&table[i]) )
> +        {
> +            *index = i;
> +            return MPUMAP_REGION_INCLUSIVE;
> +        }
> +
> +        /* Overlap */
> +        region_printk("Range 0x%"PRIpaddr" - 0x%"PRIpaddr" overlaps with the existing region 0x%"PRIpaddr" - 0x%"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");

~Michal

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

[Orzel, Michal]

On 20/06/2025 11:49, Hari Limaye wrote:
> 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 update the mapping.
NIT: s/update/updating/

> 
> This check was already present in pt.c but since it will
> be used also for MPU system, it's wrapped into a function
> now.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Hari, your SOB is missing.

> ---
>  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..9daaa96d93 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 */
> +extern bool flags_has_rwx(unsigned int flags);
Please, don't use extern for function prototypes. We don't use them anymore.

>  /* 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..c2da1e3a05 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 writeable and
NIT: you can take opportunity to correct misspelling: s/writeable/writable/

Otherwise:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

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

[Hari Limaye]

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

Hi Michal,

Thank you very much for the review.

I just had a small clarification regarding the following comment:
>> +
> +    /* Allow index to be NULL */
> +    index = index ? index : &_index;
>If index argument is NULL, why bother setting this internal variable _index?

This assignment is intended to support `index` as an optional output parameter: callers can pass NULL if they only care about the return value. This approach avoids repeated `if (index)` checks by aliasing to a local dummy variable upfront.
Would you be happy for me to retain this pattern, renaming the dummy variable to make it clearer, e.g.:
```
uint8_t dummy_index;
index = index ? index : &dummy_index;
```
I would also update the documentation to clarify that index is optional.

Alternatively, if you’d prefer to disallow NULL for index and require the caller to always provide a valid pointer, I’m happy to change it accordingly.

Many thanks,
Hari

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

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

[Orzel, Michal]

On 30/06/2025 13:45, Hari Limaye wrote:
> Hi Michal,
> 
>  
> 
> Thank you very much for the review.
> 
>  
> 
> I just had a small clarification regarding the following comment:
> 
>>> +
> 
>> +    /* Allow index to be NULL */
> 
>> +    index = index ? index : &_index;
> 
>>If index argument is NULL, why bother setting this internal variable _index?
> 
>  
> 
> This assignment is intended to support `index` as an optional output parameter:
> callers can pass NULL if they only care about the return value. This approach
> avoids repeated `if (index)` checks by aliasing to a local dummy variable upfront.
You don't need multiple if ( index ). Just one at the end of a function by
modifying it to use goto e.g.:

rc = <return_code>;
goto out;

 out:
 if ( index )
	*index = i;

> 
> Would you be happy for me to retain this pattern, renaming the dummy variable to
> make it clearer, e.g.:
> 
> ```
> 
> uint8_t dummy_index;
> 
> index = index ? index : &dummy_index;
> 
> ```
> 
> I would also update the documentation to clarify that index is optional.
This does not address my point about having and setting a variable that may not
be used.
> 
>  
> 
> Alternatively, if you’d prefer to disallow NULL for index and require the caller
> to always provide a valid pointer, I’m happy to change it accordingly.
No, that's not what I had in mind. That said it's up to you to decide how you
want the caller to behave. If you think that obtaining index is not necessary in
all the scenarios, then you should retain your current functionality.

~Michal

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

[Hari Limaye]

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

Hi Ayan,

Thank you for the review. I have just a couple of clarifications before I
re-spin the series to address all the comments:

> > -    if ( flags & _PAGE_PRESENT )
> > +    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
>
> Same question in this patch , why do we need to check for _PAGE_PRESENT.
> Can't we just rely on MPUMAP_REGION_XXX ?

The _PAGE_PRESENT flag indicates intent - whether the caller intends to create
or remove a region. The MPUMAP_REGION_XXX values only describe the current state
of the MPU map - whether the region already exists - not the caller's purpose.

For example, if the function is called to remove a region and it turns out to be
MPUMAP_REGION_NOTFOUND, we don't want to accidentally create it.

The flag is passed via the `flags` argument in calls to `map_pages_to_xen()`.
In this patch:
https://patchwork.kernel.org/project/xen-devel/patch/deccb1566ced5fa64f6de5c988ab968b76dc945a.1750411205.git.hari.limaye@arm.com/
`flags` is set to PAGE_HYPERVISOR_RO, and as defined in
xen/arch/arm/include/asm/page.h, this includes _PAGE_PRESENT.

It is also used in a similar way in `xen_pt_update_entry()` in
xen/arch/arm/mmu/pt.c.

> > +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);
>
> NIT.
>
> These 2 lines we can move before the if { ..}. So that the region is
> zeroed even if the region is disabled. This will add a small overhead,
> but we will be sure that the region is zeroed whenever it is disabled.

Thank you for the suggestion - just to clarify, unless I am missing something
I think that moving those lines above the if block would actually break the
logic.

The memset() call zeroes the region in the xen_mpumap data structure, and this
is what region_is_valid() inspects. So if we zero the region before calling
region_is_valid(), that check will always fail, and we would exit early without
updating the hardware.

It’s the subsequent lines that actually write the region to the MPU register. So
if we exit early, the hardware MPU state remains unchanged.

That said, I believe the current logic is sound - the early return path should
only be hit if the region is already known to be disabled both in software and
hardware. This function assumes it is the sole point of disabling regions, so
the check should be reliable.

Many Thanks,
Hari

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

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

[Ayan Kumar Halder]

On 01/07/2025 15:56, Hari Limaye wrote:
>
> Hi Ayan,
>
Hi Hari,
>
> Thank you for the review. I have just a couple of clarifications before I
>
> re-spin the series to address all the comments:
>
> > > -    if ( flags & _PAGE_PRESENT )
>
> > > +    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
>
> >
>
> > Same question in this patch , why do we need to check for _PAGE_PRESENT.
>
> > Can't we just rely on MPUMAP_REGION_XXX ?
>
> The _PAGE_PRESENTflag indicates intent - whether the caller intends to 
> create
>
> or remove a region.
>
If so, then I misunderstood the code. However, looking at 
xen_pt_check_entry(), it seems _PAGE_PRESENTindicates if the page table 
entry exists. If so, using _PAGE_PRESENTis not making sense  to me 
atleast. May be others can chime in.
>
> The MPUMAP_REGION_XXX values only describe the current state
>
> of the MPU map - whether the region already exists - not the caller's 
> purpose.
>
> For example, if the function is called to remove a region and it turns 
> out to be
>
> MPUMAP_REGION_NOTFOUND, we don't want to accidentally create it.
>
> The flag is passed via the `flags` argument in calls to 
> `map_pages_to_xen()`.
>
> In this patch:
>
> https://patchwork.kernel.org/project/xen-devel/patch/deccb1566ced5fa64f6de5c988ab968b76dc945a.1750411205.git.hari.limaye@arm.com/
>
> `flags` is set to PAGE_HYPERVISOR_RO, and as defined in
>
> xen/arch/arm/include/asm/page.h, this includes _PAGE_PRESENT.
>
> It is also used in a similar way in `xen_pt_update_entry()` in
>
> xen/arch/arm/mmu/pt.c.
>
> > > +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);
>
> >
>
> > NIT.
>
> >
>
> > These 2 lines we can move before the if { ..}. So that the region is
>
> > zeroed even if the region is disabled. This will add a small overhead,
>
> > but we will be sure that the region is zeroed whenever it is disabled.
>
> Thank you for the suggestion - just to clarify, unless I am missing 
> something
>
> I think that moving those lines above the if block would actually 
> break the
>
> logic.
>
Ah my mistake. I meant these two lines should be moved *inside* the if 
{...} condition.

This is a minor suggestion so feel free to ignore.

> The memset() call zeroes the region in the xen_mpumap data structure, 
> and this
>
> is what region_is_valid() inspects. So if we zero the region before 
> calling
>
> region_is_valid(), that check will always fail, and we would exit 
> early without
>
> updating the hardware.
>
> It’s the subsequent lines that actually write the region to the MPU 
> register. So
>
> if we exit early, the hardware MPU state remains unchanged.
>
All good.

- Ayan

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

[Luca Fancellu]

Hi Ayan,

> On 2 Jul 2025, at 14:11, Ayan Kumar Halder <ayankuma@amd.com> wrote:
> 
> 
> On 01/07/2025 15:56, Hari Limaye wrote:
>> 
>> Hi Ayan,
>> 
> Hi Hari,
>> 
>> Thank you for the review. I have just a couple of clarifications before I
>> 
>> re-spin the series to address all the comments:
>> 
>> > > -    if ( flags & _PAGE_PRESENT )
>> 
>> > > +    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
>> 
>> >
>> 
>> > Same question in this patch , why do we need to check for _PAGE_PRESENT.
>> 
>> > Can't we just rely on MPUMAP_REGION_XXX ?
>> 
>> The _PAGE_PRESENTflag indicates intent - whether the caller intends to create
>> 
>> or remove a region.
>> 
> If so, then I misunderstood the code. However, looking at xen_pt_check_entry(), it seems _PAGE_PRESENTindicates if the page table entry exists. If so, using _PAGE_PRESENTis not making sense  to me atleast. May be others can chime in.

But it seems to me that _PAGE_PRESENT is used in the MPU code in the same way as the MMU code, to check
if the caller has intention to add/modify a region if it’s set, otherwise to delete it.

I’m not sure why you say it’s different, can you point out which line in case, because I’ve had a look on xen_pt_check_entry
but I didn’t get your point.

Cheers,
Luca

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

[Ayan Kumar Halder]

On 02/07/2025 14:44, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
>
>> On 2 Jul 2025, at 14:11, Ayan Kumar Halder <ayankuma@amd.com> wrote:
>>
>>
>> On 01/07/2025 15:56, Hari Limaye wrote:
>>> Hi Ayan,
>>>
>> Hi Hari,
>>> Thank you for the review. I have just a couple of clarifications before I
>>>
>>> re-spin the series to address all the comments:
>>>
>>>>> -    if ( flags & _PAGE_PRESENT )
>>>>> +    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
>>>> Same question in this patch , why do we need to check for _PAGE_PRESENT.
>>>> Can't we just rely on MPUMAP_REGION_XXX ?
>>> The _PAGE_PRESENTflag indicates intent - whether the caller intends to create
>>>
>>> or remove a region.
>>>
>> If so, then I misunderstood the code. However, looking at xen_pt_check_entry(), it seems _PAGE_PRESENTindicates if the page table entry exists. If so, using _PAGE_PRESENTis not making sense  to me atleast. May be others can chime in.
> But it seems to me that _PAGE_PRESENT is used in the MPU code in the same way as the MMU code, to check
> if the caller has intention to add/modify a region if it’s set, otherwise to delete it.

I had a discussion with Michal and yes, Hari is correct. Please 
disregard my comments.

Sorry for the noise.

- Ayan