Re: [PATCH v2 26/40] xen/mpu: destroy an existing entry in Xen MPU memory mapping table

[Julien Grall]

Hi Penny,

On 13/01/2023 05:28, Penny Zheng wrote:

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

We define a new helper "control_xen_mpumap_region_from_index" to enable/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.

Signed-off-by: Penny Zheng <penny.zh...@arm.com>
Signed-off-by: Wei Chen <wei.c...@arm.com>
---
  xen/arch/arm/include/asm/arm64/mpu.h     | 20 ++++++
  xen/arch/arm/include/asm/arm64/sysregs.h |  3 +
  xen/arch/arm/mm_mpu.c                    | 77 ++++++++++++++++++++++--
  3 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
b/xen/arch/arm/include/asm/arm64/mpu.h
index 0044bbf05d..c1dea1c8e9 100644
--- a/xen/arch/arm/include/asm/arm64/mpu.h
+++ b/xen/arch/arm/include/asm/arm64/mpu.h
@@ -16,6 +16,8 @@
   */
  #define ARM_MAX_MPU_MEMORY_REGIONS 255

+#define MPU_PRENR_BITS 32

+
  /* Access permission attributes. */
  /* Read/Write at EL2, No Access at EL1/EL0. */
  #define AP_RW_EL2 0x0
@@ -132,6 +134,24 @@ typedef struct {
      _pr->prlar.reg.en;                                      \
  })

+/*

+ * Access to get base address of MPU protection region(pr_t).
+ * The base address shall be zero extended.
+ */
+#define pr_get_base(pr) ({                                  \
+    pr_t *_pr = pr;                                         \
+    (uint64_t)_pr->prbar.reg.base << MPU_REGION_SHIFT;      \
+})

Can this be a static inline?

+
+/*
+ * Access to get limit address of MPU protection region(pr_t).
+ * The limit address shall be concatenated with 0x3f.
+ */
+#define pr_get_limit(pr) ({                                        \
+    pr_t *_pr = pr;                                                \
+    (uint64_t)((_pr->prlar.reg.limit << MPU_REGION_SHIFT) | 0x3f); \
+})

Same.

+
  #endif /* __ASSEMBLY__ */

#endif /* __ARM64_MPU_H__ */

diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h 
b/xen/arch/arm/include/asm/arm64/sysregs.h
index aca9bca5b1..c46daf6f69 100644
--- a/xen/arch/arm/include/asm/arm64/sysregs.h
+++ b/xen/arch/arm/include/asm/arm64/sysregs.h
@@ -505,6 +505,9 @@
  /* MPU Type registers encode */
  #define MPUIR_EL2 S3_4_C0_C0_4

+/* MPU Protection Region Enable Register encode */

+#define PRENR_EL2 S3_4_C6_C1_1
+
  #endif

/* Access to system registers */

diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
index d2e19e836c..3a0d110b13 100644
--- a/xen/arch/arm/mm_mpu.c
+++ b/xen/arch/arm/mm_mpu.c
@@ -385,6 +385,45 @@ static int mpumap_contain_region(pr_t *mpu, uint64_t 
nr_regions,
      return MPUMAP_REGION_FAILED;
  }

+/* Disable or enable EL2 MPU memory region at index #index */

+static void control_mpu_region_from_index(uint64_t index, bool enable)
+{
+    pr_t region;
+
+    access_protection_region(true, &region, NULL, index);
+    if ( (region_is_valid(&region) && enable) ||
+         (!region_is_valid(&region) && !enable) )

You could write:

!(region_is_valid(&region) ^ enable)

+    {
+        printk(XENLOG_WARNING
+               "mpu: MPU memory region[%lu] is already %s\n", index,
+               enable ? "enabled" : "disabled");
+        return;
+    }
+
+    /*
+     * ARM64v8R provides PRENR_EL2 to have direct access to the
+     * PRLAR_EL2.EN bits of EL2 MPU regions from 0 to 31.
+     */
+    if ( index < MPU_PRENR_BITS )
+    {
+        uint64_t orig, after;
+
+        orig = READ_SYSREG(PRENR_EL2);
+        if ( enable )
+            /* Set respective bit */
+            after = orig | (1UL << index);
+        else
+            /* Clear respective bit */
+            after = orig & (~(1UL << index));
+        WRITE_SYSREG(after, PRENR_EL2);

Don't you need an isb (or similar) to ensure this is visible before...

+    }
+    else
+    {
+        region.prlar.reg.en = enable ? 1 : 0;
+        access_protection_region(false, NULL, (const pr_t*)&region, index);
+    }
+}
+
  /*
   * Update an entry at the index @idx.
   * @base:  base address
@@ -449,6 +488,30 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t 
limit,
          if ( system_state <= SYS_STATE_active )
              update_boot_xen_mpumap_idx(idx);
      }
+    else
+    {
+        /*
+         * Currently, we only support destroying a *WHOLE* MPU memory region,
+         * part-region removing is not supported, as in worst case, it will
+         * lead to two fragments in result after destroying.
+         * part-region removing will be introduced only when actual usage
+         * comes.
+         */
+        if ( rc == MPUMAP_REGION_INCLUSIVE )
+        {
+            region_printk("mpu: part-region removing is not supported\n");
+            return -EINVAL;
+        }
+
+        /* We are removing the region */
+        if ( rc != MPUMAP_REGION_FOUND )
+            return -EINVAL;
+
+        control_mpu_region_from_index(idx, false);
+
+        /* Clear the according MPU memory region entry.*/
+        memset(&xen_mpumap[idx], 0, sizeof(pr_t));

... zeroing the entry? Also, you could use memzero() here.

+    }

return 0;

  }
@@ -589,6 +652,15 @@ static void __init map_mpu_memory_section_on_boot(enum 
mpu_section_info type,
      }
  }

+int destroy_xen_mappings(unsigned long s, unsigned long e)

+{
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
+    ASSERT(s <= e);
+
+    return xen_mpumap_update(s, e, 0);
+}
+
  /*
   * System RAM is statically partitioned into different functionality
   * section in Device Tree, including static xenheap, guest memory
@@ -656,11 +728,6 @@ void *ioremap(paddr_t pa, size_t len)
      return NULL;
  }

-int destroy_xen_mappings(unsigned long s, unsigned long e)

-{
-    return -ENOSYS;
-}
-
  int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
  {
      return -ENOSYS;

Cheers,

--
Julien Grall