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

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

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

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

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

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