Re: [PATCH v2 2/6] arm/mpu: Implement vmap functions for MPU

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 05/01/2026 12:34, Harry Ramsey wrote:
> From: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> 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@xxxxxxx>
> Signed-off-by: Harry Ramsey <harry.ramsey@xxxxxxx>
> ---
> 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