From: Luca Fancellu <luca.fancellu@arm.com>
HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
in places across common code. In order to keep the existing code and
maintain correct functionality, implement the `vmap_contig` and `vunmap`
functions for MPU systems.
Introduce a helper function `destroy_xen_mapping_containing` to aid with
unmapping an entire region when only the start address is known.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
v2:
- Rename `destroy_entire_xen_mapping` to `destroy_xen_mapping_containing`
- Improve code documentation.
- Refactor nested code.
- Fix ignored rc error code in `destroy_xen_mapping_containing`.
---
xen/arch/arm/include/asm/mpu/mm.h | 10 +++++
xen/arch/arm/mpu/mm.c | 74 ++++++++++++++++++++++++++-----
xen/arch/arm/mpu/vmap.c | 14 ++++--
3 files changed, 83 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index e1ded6521d..1b5ffa5b64 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -111,6 +111,16 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
paddr_t limit, uint8_t *index);
+
+/*
+ * Destroys and frees (if reference count is 0) an entire xen mapping on MPU
+ * systems where only the start address is known.
+ *
+ * @param s Start address of memory region to be destroyed.
+ * @return: 0 on success, negative on error.
+ */
+int destroy_xen_mapping_containing(paddr_t s);
+
#endif /* __ARM_MPU_MM_H__ */
/*
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 687dec3bc6..207e8d2d91 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -290,6 +290,42 @@ static void disable_mpu_region_from_index(uint8_t index)
write_protection_region(&xen_mpumap[index], index);
}
+/*
+ * Free a xen_mpumap entry given the index. A mpu region is actually disabled
+ * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
+ *
+ * @param idx Index of the mpumap entry.
+ * @param region_found_type MPUMAP_REGION_* value.
+ * @return 0 on success, otherwise negative on error.
+ */
+static int xen_mpumap_free_entry(uint8_t idx, int region_found_type)
+{
+ ASSERT(spin_is_locked(&xen_mpumap_lock));
+ ASSERT(idx != INVALID_REGION_IDX);
+
+ if ( MPUMAP_REGION_OVERLAP == region_found_type )
+ {
+ printk(XENLOG_ERR "Cannot remove an overlapping region\n");
+ return -EINVAL;
+ }
+
+ if ( xen_mpumap[idx].refcount )
+ {
+ xen_mpumap[idx].refcount -= 1;
+ return 0;
+ }
+
+ if ( MPUMAP_REGION_FOUND == region_found_type )
+ disable_mpu_region_from_index(idx);
+ else
+ {
+ printk(XENLOG_ERR "Cannot remove a partial region\n");
+ return -EINVAL;
+ }
+
+ 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.
@@ -357,18 +393,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
return -EINVAL;
}
- if ( xen_mpumap[idx].refcount == 0 )
- {
- if ( MPUMAP_REGION_FOUND == rc )
- disable_mpu_region_from_index(idx);
- else
- {
- printk("Cannot remove a partial region\n");
- return -EINVAL;
- }
- }
- else
- xen_mpumap[idx].refcount -= 1;
+ return xen_mpumap_free_entry(idx, rc);
}
return 0;
@@ -418,6 +443,31 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
return xen_mpumap_update(s, e, 0);
}
+int destroy_xen_mapping_containing(paddr_t s)
+{
+ int rc;
+ uint8_t idx;
+
+ ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+
+ spin_lock(&xen_mpumap_lock);
+
+ rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s + PAGE_SIZE,
+ &idx);
+ if ( rc == MPUMAP_REGION_NOTFOUND )
+ {
+ printk(XENLOG_ERR "Cannot remove entry that does not exist");
+ return -EINVAL;
+ }
+
+ /* As we are unmapping entire region use MPUMAP_REGION_FOUND instead */
+ rc = xen_mpumap_free_entry(idx, MPUMAP_REGION_FOUND);
+
+ 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)
{
diff --git a/xen/arch/arm/mpu/vmap.c b/xen/arch/arm/mpu/vmap.c
index f977b79cd4..54d949e7ce 100644
--- a/xen/arch/arm/mpu/vmap.c
+++ b/xen/arch/arm/mpu/vmap.c
@@ -1,19 +1,27 @@
/* SPDX-License-Identifier: GPL-2.0-only */
#include <xen/bug.h>
+#include <xen/mm.h>
#include <xen/mm-frame.h>
#include <xen/types.h>
#include <xen/vmap.h>
void *vmap_contig(mfn_t mfn, unsigned int nr)
{
- BUG_ON("unimplemented");
- return NULL;
+ paddr_t base = mfn_to_maddr(mfn);
+
+ if ( map_pages_to_xen(base, mfn, nr, PAGE_HYPERVISOR ) )
+ return NULL;
+
+ return maddr_to_virt(base);
}
void vunmap(const void *va)
{
- BUG_ON("unimplemented");
+ paddr_t base = virt_to_maddr(va);
+
+ if ( destroy_xen_mapping_containing(base) )
+ panic("Failed to vunmap region\n");
}
/*
--
2.43.0Also, two more things here apart from my other remarks.
On 05/01/2026 12:34, Harry Ramsey wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
>
> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
> in places across common code. In order to keep the existing code and
> maintain correct functionality, implement the `vmap_contig` and `vunmap`
> functions for MPU systems.
>
> Introduce a helper function `destroy_xen_mapping_containing` to aid with
> unmapping an entire region when only the start address is known.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
> v2:
> - Rename `destroy_entire_xen_mapping` to `destroy_xen_mapping_containing`
> - Improve code documentation.
> - Refactor nested code.
> - Fix ignored rc error code in `destroy_xen_mapping_containing`.
> ---
...
>
> +int destroy_xen_mapping_containing(paddr_t s)
> +{
> + int rc;
> + uint8_t idx;
> +
> + ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +
> + spin_lock(&xen_mpumap_lock);
Here you take a lock...
> +
> + rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s + PAGE_SIZE,
> + &idx);
> + if ( rc == MPUMAP_REGION_NOTFOUND )
> + {
> + printk(XENLOG_ERR "Cannot remove entry that does not exist");
> + return -EINVAL;
and here you would return while still holding the lock i.e. deadlock.
> + }
> +
> + /* As we are unmapping entire region use MPUMAP_REGION_FOUND instead */
> + rc = xen_mpumap_free_entry(idx, MPUMAP_REGION_FOUND);
Why don't you perform context_sync_mpu() here?
~Michal
On 05/01/2026 12:34, Harry Ramsey wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
>
> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
> in places across common code. In order to keep the existing code and
> maintain correct functionality, implement the `vmap_contig` and `vunmap`
> functions for MPU systems.
>
> Introduce a helper function `destroy_xen_mapping_containing` to aid with
> unmapping an entire region when only the start address is known.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
> v2:
> - Rename `destroy_entire_xen_mapping` to `destroy_xen_mapping_containing`
> - Improve code documentation.
> - Refactor nested code.
> - Fix ignored rc error code in `destroy_xen_mapping_containing`.
> ---
> xen/arch/arm/include/asm/mpu/mm.h | 10 +++++
> xen/arch/arm/mpu/mm.c | 74 ++++++++++++++++++++++++++-----
> xen/arch/arm/mpu/vmap.c | 14 ++++--
> 3 files changed, 83 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index e1ded6521d..1b5ffa5b64 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -111,6 +111,16 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
> int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> paddr_t limit, uint8_t *index);
>
> +
> +/*
> + * Destroys and frees (if reference count is 0) an entire xen mapping on MPU
> + * systems where only the start address is known.
> + *
> + * @param s Start address of memory region to be destroyed.
> + * @return: 0 on success, negative on error.
> + */
> +int destroy_xen_mapping_containing(paddr_t s);
> +
> #endif /* __ARM_MPU_MM_H__ */
>
> /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 687dec3bc6..207e8d2d91 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -290,6 +290,42 @@ static void disable_mpu_region_from_index(uint8_t index)
> write_protection_region(&xen_mpumap[index], index);
> }
>
> +/*
> + * Free a xen_mpumap entry given the index. A mpu region is actually disabled
> + * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
> + *
> + * @param idx Index of the mpumap entry.
> + * @param region_found_type MPUMAP_REGION_* value.
> + * @return 0 on success, otherwise negative on error.
> + */
> +static int xen_mpumap_free_entry(uint8_t idx, int region_found_type)
> +{
> + ASSERT(spin_is_locked(&xen_mpumap_lock));
> + ASSERT(idx != INVALID_REGION_IDX);
> +
> + if ( MPUMAP_REGION_OVERLAP == region_found_type )
> + {
> + printk(XENLOG_ERR "Cannot remove an overlapping region\n");
> + return -EINVAL;
> + }
> +
> + if ( xen_mpumap[idx].refcount )
> + {
> + xen_mpumap[idx].refcount -= 1;
> + return 0;
> + }
> +
> + if ( MPUMAP_REGION_FOUND == region_found_type )
> + disable_mpu_region_from_index(idx);
> + else
> + {
> + printk(XENLOG_ERR "Cannot remove a partial region\n");
Shouldn't this be moved above refcount checking? Do we expect this function to
be called with region_found_type being MPUMAP_REGION_INCLUSIVE?
> + return -EINVAL;
> + }
> +
> + 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.
> @@ -357,18 +393,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
> return -EINVAL;
> }
>
> - if ( xen_mpumap[idx].refcount == 0 )
> - {
> - if ( MPUMAP_REGION_FOUND == rc )
> - disable_mpu_region_from_index(idx);
> - else
> - {
> - printk("Cannot remove a partial region\n");
> - return -EINVAL;
> - }
> - }
> - else
> - xen_mpumap[idx].refcount -= 1;
> + return xen_mpumap_free_entry(idx, rc);
> }
>
> return 0;
> @@ -418,6 +443,31 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
> return xen_mpumap_update(s, e, 0);
> }
>
> +int destroy_xen_mapping_containing(paddr_t s)
> +{
> + int rc;
> + uint8_t idx;
> +
> + ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +
> + spin_lock(&xen_mpumap_lock);
> +
> + rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s + PAGE_SIZE,
> + &idx);
> + if ( rc == MPUMAP_REGION_NOTFOUND )
> + {
> + printk(XENLOG_ERR "Cannot remove entry that does not exist");
Why do we split sanity checking between this and xen_mpumap_free_entry?
What are the possible region types that xen_mpumap_free_entry is expected to
work with? I thought that it should only be MPUMAP_REGION_FOUND.
~Michal
On 09/01/2026 14:00, Orzel, Michal wrote:
>
> On 05/01/2026 12:34, Harry Ramsey wrote:
>> From: Luca Fancellu <luca.fancellu@arm.com>
>>
>> HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
>> in places across common code. In order to keep the existing code and
>> maintain correct functionality, implement the `vmap_contig` and `vunmap`
>> functions for MPU systems.
>>
>> Introduce a helper function `destroy_xen_mapping_containing` to aid with
>> unmapping an entire region when only the start address is known.
>>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
>> ---
>> v2:
>> - Rename `destroy_entire_xen_mapping` to `destroy_xen_mapping_containing`
>> - Improve code documentation.
>> - Refactor nested code.
>> - Fix ignored rc error code in `destroy_xen_mapping_containing`.
>> ---
>> xen/arch/arm/include/asm/mpu/mm.h | 10 +++++
>> xen/arch/arm/mpu/mm.c | 74 ++++++++++++++++++++++++++-----
>> xen/arch/arm/mpu/vmap.c | 14 ++++--
>> 3 files changed, 83 insertions(+), 15 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
>> index e1ded6521d..1b5ffa5b64 100644
>> --- a/xen/arch/arm/include/asm/mpu/mm.h
>> +++ b/xen/arch/arm/include/asm/mpu/mm.h
>> @@ -111,6 +111,16 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
>> int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>> paddr_t limit, uint8_t *index);
>>
>> +
>> +/*
>> + * Destroys and frees (if reference count is 0) an entire xen mapping on MPU
>> + * systems where only the start address is known.
>> + *
>> + * @param s Start address of memory region to be destroyed.
>> + * @return: 0 on success, negative on error.
>> + */
>> +int destroy_xen_mapping_containing(paddr_t s);
>> +
>> #endif /* __ARM_MPU_MM_H__ */
>>
>> /*
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 687dec3bc6..207e8d2d91 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -290,6 +290,42 @@ static void disable_mpu_region_from_index(uint8_t index)
>> write_protection_region(&xen_mpumap[index], index);
>> }
>>
>> +/*
>> + * Free a xen_mpumap entry given the index. A mpu region is actually disabled
>> + * when the refcount is 0 and the region type is MPUMAP_REGION_FOUND.
>> + *
>> + * @param idx Index of the mpumap entry.
>> + * @param region_found_type MPUMAP_REGION_* value.
>> + * @return 0 on success, otherwise negative on error.
>> + */
>> +static int xen_mpumap_free_entry(uint8_t idx, int region_found_type)
>> +{
>> + ASSERT(spin_is_locked(&xen_mpumap_lock));
>> + ASSERT(idx != INVALID_REGION_IDX);
>> +
>> + if ( MPUMAP_REGION_OVERLAP == region_found_type )
>> + {
>> + printk(XENLOG_ERR "Cannot remove an overlapping region\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ( xen_mpumap[idx].refcount )
>> + {
>> + xen_mpumap[idx].refcount -= 1;
>> + return 0;
>> + }
>> +
>> + if ( MPUMAP_REGION_FOUND == region_found_type )
>> + disable_mpu_region_from_index(idx);
>> + else
>> + {
>> + printk(XENLOG_ERR "Cannot remove a partial region\n");
> Shouldn't this be moved above refcount checking? Do we expect this function to
> be called with region_found_type being MPUMAP_REGION_INCLUSIVE?
This function is expected to be given MPUMAP_REGION_INCLUSIVE and in
doing so decrease refcount by 1. If however refcount is already 0, the
region can only be freed by calling the entire region bounds. This
message is moreso a debug error that our MPU API has been used in an
incorrect way, it should not generally occur but I would prefer to keep
it for sanity checking.
>
>> + return -EINVAL;
>> + }
>> +
>> + 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.
>> @@ -357,18 +393,7 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>> return -EINVAL;
>> }
>>
>> - if ( xen_mpumap[idx].refcount == 0 )
>> - {
>> - if ( MPUMAP_REGION_FOUND == rc )
>> - disable_mpu_region_from_index(idx);
>> - else
>> - {
>> - printk("Cannot remove a partial region\n");
>> - return -EINVAL;
>> - }
>> - }
>> - else
>> - xen_mpumap[idx].refcount -= 1;
>> + return xen_mpumap_free_entry(idx, rc);
>> }
>>
>> return 0;
>> @@ -418,6 +443,31 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
>> return xen_mpumap_update(s, e, 0);
>> }
>>
>> +int destroy_xen_mapping_containing(paddr_t s)
>> +{
>> + int rc;
>> + uint8_t idx;
>> +
>> + ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>> +
>> + spin_lock(&xen_mpumap_lock);
>> +
>> + rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s + PAGE_SIZE,
>> + &idx);
>> + if ( rc == MPUMAP_REGION_NOTFOUND )
>> + {
>> + printk(XENLOG_ERR "Cannot remove entry that does not exist");
> Why do we split sanity checking between this and xen_mpumap_free_entry?
> What are the possible region types that xen_mpumap_free_entry is expected to
> work with? I thought that it should only be MPUMAP_REGION_FOUND.
I will move the region checks to xen_mpumap_free_entry since we only
want xen_mpumap_update_entry and xen_mpumap_free_entry to modify our
refcount properties. These functions should then account for all
potential values of MPUMAP_REGION_*.
>
> ~Michal
Thanks,
Harry Ramsey.
On 12/01/2026 10:33, Harry Ramsey wrote:
> On 09/01/2026 14:00, Orzel, Michal wrote:
>>
>> On 05/01/2026 12:34, Harry Ramsey wrote:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + 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.
>>> @@ -357,18 +393,7 @@ static int xen_mpumap_update_entry(paddr_t
>>> base, paddr_t limit,
>>> return -EINVAL;
>>> }
>>> - if ( xen_mpumap[idx].refcount == 0 )
>>> - {
>>> - if ( MPUMAP_REGION_FOUND == rc )
>>> - disable_mpu_region_from_index(idx);
>>> - else
>>> - {
>>> - printk("Cannot remove a partial region\n");
>>> - return -EINVAL;
>>> - }
>>> - }
>>> - else
>>> - xen_mpumap[idx].refcount -= 1;
>>> + return xen_mpumap_free_entry(idx, rc);
>>> }
>>> return 0;
>>> @@ -418,6 +443,31 @@ int destroy_xen_mappings(unsigned long s,
>>> unsigned long e)
>>> return xen_mpumap_update(s, e, 0);
>>> }
>>> +int destroy_xen_mapping_containing(paddr_t s)
>>> +{
>>> + int rc;
>>> + uint8_t idx;
>>> +
>>> + ASSERT(IS_ALIGNED(s, PAGE_SIZE));
>>> +
>>> + spin_lock(&xen_mpumap_lock);
>>> +
>>> + rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, s, s +
>>> PAGE_SIZE,
>>> + &idx);
>>> + if ( rc == MPUMAP_REGION_NOTFOUND )
>>> + {
>>> + printk(XENLOG_ERR "Cannot remove entry that does not exist");
>> Why do we split sanity checking between this and xen_mpumap_free_entry?
>> What are the possible region types that xen_mpumap_free_entry is
>> expected to
>> work with? I thought that it should only be MPUMAP_REGION_FOUND.
>
> I will move the region checks to xen_mpumap_free_entry since we only
> want xen_mpumap_update_entry and xen_mpumap_free_entry to modify our
> refcount properties. These functions should then account for all
> potential values of MPUMAP_REGION_*.
Sorry, after looking back at this code the reason the sanity checking
happens here is because we require MPUMAP_REGION_FOUND to be used in
`destroy_xen_mapping_containing` to actually remove the region. In
`destroy_xen_mapping_containing` we are forcibly setting the region type
to be MPUMAP_REGION_FOUND to ensure it is removed and thus we require
the check here too.
Thanks,
Harry Ramsey.
© 2016 - 2026 Red Hat, Inc.