On 28/11/2025 10:58, Harry Ramsey wrote:
From: Luca Fancellu <[email protected]>
HAS_VMAP is not enabled on MPU systems, but the vmap functions are used
Just as a reminder, we don't intend to support VMAP on MPU? Asking because it
would otherwise be a duplicate effort to implement only these two helpers.
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_entire_xen_mapping` to aid with
unmapping an entire region when only the start address is known.
Signed-off-by: Luca Fancellu <[email protected]>
Signed-off-by: Harry Ramsey <[email protected]>
---
xen/arch/arm/include/asm/mpu/mm.h | 11 +++++
xen/arch/arm/mpu/mm.c | 67 +++++++++++++++++++++++++------
xen/arch/arm/mpu/vmap.c | 14 +++++--
3 files changed, 77 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..83ee0e59ca 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -111,6 +111,17 @@ 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_entire_xen_mapping(paddr_t s);
NIT: I read it as all the mappings which is a bit misleading :)
Maybe something like: destroy_mapping_containing or alike?
+
#endif /* __ARM_MPU_MM_H__ */
/*
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 687dec3bc6..29d8e7ff11 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -290,6 +290,35 @@ 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 Either MPUMAP_REGION_FOUND or
MPUMAP_REGION_INCLUSIVE.
Both of these are unsigned, so why the parameter is int?
+ * @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 ( xen_mpumap[idx].refcount == 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;
+ }
+ }
+ else
+ xen_mpumap[idx].refcount -= 1;
You could avoid nesting if/else by doing:
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 +386,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 +436,31 @@ int destroy_xen_mappings(unsigned long s, unsigned long e)
return xen_mpumap_update(s, e, 0);
}
+int destroy_entire_xen_mapping(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);
So here you are searching for a region that includes at least s + PAGE_SIZE.
+ if ( rc == MPUMAP_REGION_NOTFOUND )
+ {
+ printk(XENLOG_ERR "Cannot remove entry that does not exist");
+ rc = -EINVAL;
Here you assing rc but ...
+ }
+
+ /* As we are unmapping entire region use MPUMAP_REGION_FOUND instead */
+ rc = xen_mpumap_free_entry(idx, MPUMAP_REGION_FOUND);
here you would redefine it.
+
+ 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..d3037ae98a 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_entire_xen_mapping(base) )
+ panic("Failed to vunmap region\n");
Looking at common vunmap() we ignore the return code from
destroy_xen_mappings(). Is it ok to diverge?
~Michal
}
/*
