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
> }
>
> /*
