Re: [PATCH 4/6] arm/mpu: Destroy an existing entry in Xen MPU memory mapping table

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Hari,

On 20/06/2025 10:49, Hari Limaye wrote:
> CAUTION: This message has originated from an External Source. Please use proper 
> judgment and caution when opening attachments, clicking links, or responding to 
> this email.
>
>
> From: Penny Zheng <Penny.Zheng@xxxxxxx>
>
> 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.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Hari Limaye <hari.limaye@xxxxxxx>
> ---
>   xen/arch/arm/include/asm/mpu.h        |  2 +
>   xen/arch/arm/include/asm/mpu/cpregs.h |  4 ++
>   xen/arch/arm/mpu/mm.c                 | 71 ++++++++++++++++++++++++++-
>   3 files changed, 75 insertions(+), 2 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 1de28d2120..23230936f7 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -199,6 +199,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);
NIT.

These 2 lines we can move before the if { ..}. So that the region is 
zeroed even if the region is disabled. This will add a small overhead, 
but we will be sure that the region is zeroed whenever it is disabled.
> +
> +    /*
> +     * 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 */
> +        uint64_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.
> @@ -217,11 +253,11 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t 
> limit,
>       ASSERT(spin_is_locked(&xen_mpumap_lock));
>
>       rc = mpumap_contain_region(xen_mpumap, max_mpu_regions, base, limit, 
> &idx);
> -    if ( (rc < 0) || (rc > MPUMAP_REGION_NOTFOUND) )
> +    if ( rc < 0 )
>           return -EINVAL;
>
>       /* We are inserting a mapping => Create new region. */
> -    if ( flags & _PAGE_PRESENT )
> +    if ( (flags & _PAGE_PRESENT) && (MPUMAP_REGION_NOTFOUND == rc) )
Same question in this patch , why do we need to check for _PAGE_PRESENT. 
Can't we just rely on MPUMAP_REGION_XXX ?
>       {
>           rc = xen_mpumap_alloc_entry(&idx);
>           if ( rc )
> @@ -232,6 +268,22 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t 
> limit,
>           write_protection_region(&xen_mpumap[idx], idx);
>       }
>
> +    if ( !(flags & _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )
> +    {
> +        /*
> +         * Currently, 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.
> +         */
> +        if ( MPUMAP_REGION_INCLUSIVE == rc )
> +        {
> +            region_printk("mpu: part-region removing is not supported\n");
> +            return -EINVAL;
> +        }
NIT.

Can we keep this ^^^ outside of the outer if condition ie "if ( !(flags 
& _PAGE_PRESENT) && (rc >= MPUMAP_REGION_FOUND) )" ?
> +
> +        disable_mpu_region_from_index(idx);
> +    }
> +
>       return 0;
>   }
>
> @@ -261,6 +313,21 @@ 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)
> +{
> +    int rc;
> +
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> +    ASSERT(s <= e);
Can we have these asserts in xen_mpumap_update() as well ?

> +
> +    rc = xen_mpumap_update(virt_to_maddr(s), virt_to_maddr(e), 0);
> +    if ( !rc )
> +        context_sync_mpu();
> +
> +    return rc;
> +}
> +
>   int map_pages_to_xen(unsigned long virt, mfn_t mfn, unsigned long nr_mfns,
>                        unsigned int flags)
>   {
> --
> 2.34.1
- Ayan
>