On 13/01/2026 17:23, Harry Ramsey wrote:
> From: Luca Fancellu <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Harry Ramsey <[email protected]>
> ---
> v3:
> - Add additional comments for clarity regarding MPUMAP_REGION checks
> - Ensure `context_sync_mpu` occurs after `destroy_entire_xen_mapping`
> - Fix deadlock if `destroy_entire_xen_mapping` is called with an address
> that does not belong to a region
> 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 | 82 ++++++++++++++++++++++++++-----
> xen/arch/arm/mpu/vmap.c | 14 ++++--
> 3 files changed, 91 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..14a988ea0c 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -290,6 +290,43 @@ 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);
> + ASSERT(MPUMAP_REGION_OVERLAP != region_found_type);
> +
> + if ( MPUMAP_REGION_NOTFOUND == region_found_type )
> + {
> + printk(XENLOG_ERR "Cannot remove entry that does not exist\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;
> + }
NIT: Try to limit the number of if/else blocks to make the code read better.
Here you could do the following to remove one else:
if ( MPUMAP_REGION_FOUND != region_found_type )
{
printk(XENLOG_ERR "Cannot remove a partial region\n");
return -EINVAL;
}
disable_mpu_region_from_index(idx);
return 0;
> +
> + 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 +394,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 +444,38 @@ 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);
> +
> + /*
> + * Since only entire regions can be freed using `xen_mpumap_free_entry`
> we
> + * must first check the region exists.
> + */
> + if ( MPUMAP_REGION_NOTFOUND == rc ) {
Bracket should be placed on its own line.
> + printk(XENLOG_ERR "Cannot remove entry that does not exist");
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + /* As we are unmapping entire region use MPUMAP_REGION_FOUND instead */
> + rc = xen_mpumap_free_entry(idx, MPUMAP_REGION_FOUND);
> + if ( !rc )
> + context_sync_mpu();
> + out:
> + 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");
> }
Looking at this series as a whole, at the end we will end up with
vmap_contig/vunmap to contain the same implementation as map_domain_page and
alike from the last patch. Shouldn't we define ones using the others?
~Michal
