On 15/07/2025 09:45, Hari Limaye wrote:
> From: Penny Zheng <penny.zh...@arm.com>
>
> This commit expands xen_mpumap_update/xen_mpumap_update_entry to include
> destroying an existing entry.
>
> We define a new helper "disable_mpu_region_from_index" to disable the MPU
> region based on index. If region is within [0, 31], we could quickly
> disable the MPU region through PRENR_EL2 which provides direct access to the
> PRLAR_EL2.EN bits of EL2 MPU regions.
>
> Rignt now, we only support destroying a *WHOLE* MPU memory region,
> part-region removing is not supported, as in worst case, it will
> leave two fragments behind.
>
> Signed-off-by: Penny Zheng <penny.zh...@arm.com>
> Signed-off-by: Wei Chen <wei.c...@arm.com>
> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
> Signed-off-by: Hari Limaye <hari.lim...@arm.com>
> ---
> Changes from v1:
> - Move check for part-region removal outside if condition
> - Use normal printk
>
> Changes from v2:
> - Fix assert from `ASSERT(s <= e)` -> `ASSERT(s < e)`
> - Remove call to context_sync_mpu
> - Use register_t
> - Improve sanity checking to catch modification & removing non-existent
> entries
> - Update check for MPUMAP_REGION_INCLUSIVE to be generic
> ---
> xen/arch/arm/include/asm/mpu.h | 2 +
> xen/arch/arm/include/asm/mpu/cpregs.h | 4 ++
> xen/arch/arm/mpu/mm.c | 92 ++++++++++++++++++++++++---
> 3 files changed, 89 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 63560c613b..5053edaf63 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -23,6 +23,8 @@
> #define NUM_MPU_REGIONS_MASK (NUM_MPU_REGIONS - 1)
> #define MAX_MPU_REGION_NR NUM_MPU_REGIONS_MASK
>
> +#define PRENR_MASK GENMASK(31, 0)
> +
> #ifndef __ASSEMBLY__
>
> /*
> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h
> b/xen/arch/arm/include/asm/mpu/cpregs.h
> index bb15e02df6..9f3b32acd7 100644
> --- a/xen/arch/arm/include/asm/mpu/cpregs.h
> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> @@ -6,6 +6,9 @@
> /* CP15 CR0: MPU Type Register */
> #define HMPUIR p15,4,c0,c0,4
>
> +/* CP15 CR6: Protection Region Enable Register */
> +#define HPRENR p15,4,c6,c1,1
> +
> /* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
> #define HPRSELR p15,4,c6,c2,1
> #define HPRBAR p15,4,c6,c3,0
> @@ -82,6 +85,7 @@
> /* Alphabetically... */
> #define MPUIR_EL2 HMPUIR
> #define PRBAR_EL2 HPRBAR
> +#define PRENR_EL2 HPRENR
> #define PRLAR_EL2 HPRLAR
> #define PRSELR_EL2 HPRSELR
> #endif /* CONFIG_ARM_32 */
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index d5426525af..a5b1c95793 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -189,6 +189,42 @@ static int xen_mpumap_alloc_entry(uint8_t *idx)
> return 0;
> }
>
> +/*
> + * Disable and remove an MPU region from the data structure and MPU
> registers.
> + *
> + * @param index Index of the MPU region to be disabled.
> + */
> +static void disable_mpu_region_from_index(uint8_t index)
> +{
> + ASSERT(spin_is_locked(&xen_mpumap_lock));
> + ASSERT(index != INVALID_REGION_IDX);
> +
> + if ( !region_is_valid(&xen_mpumap[index]) )
> + {
> + printk(XENLOG_WARNING
> + "mpu: MPU memory region[%u] is already disabled\n", index);
> + return;
> + }
> +
> + /* Zeroing the region will also zero the region enable */
> + memset(&xen_mpumap[index], 0, sizeof(pr_t));
> + clear_bit(index, xen_mpumap_mask);
> +
> + /*
> + * Both Armv8-R AArch64 and AArch32 have direct access to the enable bit
> for
> + * MPU regions numbered from 0 to 31.
> + */
> + if ( (index & PRENR_MASK) != 0 )
> + {
> + /* Clear respective bit */
> + register_t val = READ_SYSREG(PRENR_EL2) & (~(1UL << index));
> +
> + WRITE_SYSREG(val, PRENR_EL2);
> + }
> + else
> + write_protection_region(&xen_mpumap[index], index);
> +}
> +
> /*
> * Update the entry in the MPU memory region mapping table (xen_mpumap) for
> the
> * given memory range and flags, creating one if none exists.
> @@ -206,22 +242,51 @@ static int xen_mpumap_update_entry(paddr_t base,
> paddr_t limit,
>
> ASSERT(spin_is_locked(&xen_mpumap_lock));
>
> - /* Currently only region creation is supported. */
> - if ( !(flags & _PAGE_PRESENT) )
> + rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, base, limit,
> &idx);
> + if ( rc < 0 )
> return -EINVAL;
>
> - rc = mpumap_contains_region(xen_mpumap, max_mpu_regions, base, limit,
> &idx);
> - if ( rc != MPUMAP_REGION_NOTFOUND )
> + /*
> + * Currently, we only support removing/modifying a *WHOLE* MPU memory
> + * region. Part-region removal/modification is not supported as in the
> worst
> + * case it will leave two/three fragments behind.
> + */
> + if ( rc == MPUMAP_REGION_INCLUSIVE )
> + {
> + printk("mpu: part-region removal/modification is not supported\n");
> return -EINVAL;
> + }
> +
> + /* Currently we don't support modifying an existing entry. */
> + if ( (flags & _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )
This check should be above ( rc == MPUMAP_REGION_INCLUSIVE ). Otherwise you
might end up printing a message about removal in modifying case.
> + {
> + printk("mpu: modifying an existing entry is not supported\n");
> + return -EINVAL;
> + }
>
> /* We are inserting a mapping => Create new region. */
> - rc = xen_mpumap_alloc_entry(&idx);
> - if ( rc )
> - return -ENOENT;
> + if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
You check for flags & _PAGE_PRESENT 3 times. I suggest to add a bool variable at
the top storing this result for better readability.
> + {
> + rc = xen_mpumap_alloc_entry(&idx);
> + if ( rc )
> + return -ENOENT;
>
> - xen_mpumap[idx] = pr_of_addr(base, limit, flags);
> + xen_mpumap[idx] = pr_of_addr(base, limit, flags);
>
> - write_protection_region(&xen_mpumap[idx], idx);
> + write_protection_region(&xen_mpumap[idx], idx);
> + }
> +
> + /* Removing a mapping */
> + if ( !(flags & _PAGE_PRESENT) )
> + {
> + if ( rc == MPUMAP_REGION_NOTFOUND )
> + {
> + printk("mpu: cannot remove an entry that does not exist\n");
> + return -EINVAL;
> + }
> +
> + disable_mpu_region_from_index(idx);
> + }
>
> return 0;
> }
> @@ -261,6 +326,15 @@ int xen_mpumap_update(paddr_t base, paddr_t limit,
> unsigned int flags)
> return rc;
> }
>
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> +{
> + ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> + ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> + ASSERT(s < e);
> +
> + return xen_mpumap_update(virt_to_maddr(s), virt_to_maddr(e), 0);
If virt == madr on MPU, why do you need conversion?
~Michal
