Re: [PATCH v2 2/3] arm/mpu: Implement reference counting for inclusive regions

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

On 11/11/2025 11:16, Harry Ramsey wrote:
> Implement reference counting to enable inclusive MPU regions. An
> inclusive region is defined as a region encapsulated inside a
> previously allocated region sharing the same permissions.
> 
> References are incremented and decremented in xen_mpumap_update_entry. A
> region will be destroyed if the reference count is 0 upon calling
> destroy_xen_mappings and if the full region range is specified.
> 
> Additionally XEN_MPUMAP_ENTRY_SHIFT and XEN_MPUMAP_ENTRY_SHIFT_ZERO are
> no longer hardcoded and defined inside asm-offsets.c.
> 
> Signed-off-by: Harry Ramsey <harry.ramsey@xxxxxxx>
> ---
> Changes in v2:
> - Improve clarity with regards to MPU inclusive regions
> - Fix code format issues
> ---
>  xen/arch/arm/arm32/asm-offsets.c         |  2 +
>  xen/arch/arm/arm64/asm-offsets.c         |  2 +
>  xen/arch/arm/include/asm/arm32/mpu.h     |  2 +
>  xen/arch/arm/include/asm/arm64/mpu.h     |  2 +
>  xen/arch/arm/include/asm/mpu/regions.inc | 11 +++-
>  xen/arch/arm/mpu/mm.c                    | 75 +++++++++++++++++++-----
>  6 files changed, 78 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/asm-offsets.c 
> b/xen/arch/arm/arm32/asm-offsets.c
> index c203ce269d..951f8d03f3 100644
> --- a/xen/arch/arm/arm32/asm-offsets.c
> +++ b/xen/arch/arm/arm32/asm-offsets.c
> @@ -79,6 +79,8 @@ void __dummy__(void)
>  #ifdef CONFIG_MPU
>     DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
>     DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
> +   DEFINE(XEN_MPUMAP_ENTRY_SHIFT, ilog2(sizeof(pr_t)));
> +   DEFINE(XEN_MPUMAP_ENTRY_ZERO_OFFSET, sizeof(prbar_t) + sizeof(prlar_t));
>     BLANK();
>  #endif
>  }
> diff --git a/xen/arch/arm/arm64/asm-offsets.c 
> b/xen/arch/arm/arm64/asm-offsets.c
> index 320289b281..38a3894a3b 100644
> --- a/xen/arch/arm/arm64/asm-offsets.c
> +++ b/xen/arch/arm/arm64/asm-offsets.c
> @@ -73,6 +73,8 @@ void __dummy__(void)
>  #ifdef CONFIG_MPU
>     DEFINE(XEN_MPUMAP_MASK_sizeof, sizeof(xen_mpumap_mask));
>     DEFINE(XEN_MPUMAP_sizeof, sizeof(xen_mpumap));
> +   DEFINE(XEN_MPUMAP_ENTRY_SHIFT, ilog2(sizeof(pr_t)));
> +   DEFINE(XEN_MPUMAP_ENTRY_ZERO_OFFSET, sizeof(prbar_t) + sizeof(prlar_t));
>     BLANK();
>  #endif
>  }
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h 
> b/xen/arch/arm/include/asm/arm32/mpu.h
> index 0a6930b3a0..137022d922 100644
> --- a/xen/arch/arm/include/asm/arm32/mpu.h
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -39,6 +39,8 @@ typedef union {
>  typedef struct {
>      prbar_t prbar;
>      prlar_t prlar;
> +    uint8_t refcount;
> +    uint8_t pad[7];     /* Pad structure to 16 Bytes */
>  } pr_t;
> 
>  #endif /* __ASSEMBLY__ */
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
> b/xen/arch/arm/include/asm/arm64/mpu.h
> index f0ce344e78..17f62ccaf6 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -38,6 +38,8 @@ typedef union {
>  typedef struct {
>      prbar_t prbar;
>      prlar_t prlar;
> +    uint8_t refcount;
> +    uint8_t pad[15];    /* Pad structure to 32 Bytes */
>  } pr_t;
> 
>  #endif /* __ASSEMBLY__ */
> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc 
> b/xen/arch/arm/include/asm/mpu/regions.inc
> index 23fead3b21..0cdbb17bc3 100644
> --- a/xen/arch/arm/include/asm/mpu/regions.inc
> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
> @@ -14,14 +14,12 @@
>  #define PRLAR_ELx_EN            0x1
> 
>  #ifdef CONFIG_ARM_64
> -#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
> 
>  .macro store_pair reg1, reg2, dst
>      stp \reg1, \reg2, [\dst]
>  .endm
> 
>  #else
> -#define XEN_MPUMAP_ENTRY_SHIFT  0x3     /* 8 byte structure */
> 
>  .macro store_pair reg1, reg2, dst
>      strd  \reg1, \reg2, [\dst]
> @@ -97,6 +95,15 @@
> 
>  3:
> 
> +    /* Clear the rest of the xen_mpumap entry. */
> +#ifdef CONFIG_ARM_64
> +    stp xzr, xzr, [\base, #XEN_MPUMAP_ENTRY_ZERO_OFFSET]
> +#else
> +    mov \prbar, #0
> +    mov \prlar, #0
> +    strd \prbar, \prlar, [\base, #XEN_MPUMAP_ENTRY_ZERO_OFFSET]
> +#endif
> +
>      add   \sel, \sel, #1
> 
>  1:
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index b80edcf1ca..cd84f9e3c6 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -106,6 +106,7 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int 
> flags)
>      region = (pr_t) {
>          .prbar = prbar,
>          .prlar = prlar,
> +        .refcount = 0,
>      };
> 
>      /* Set base address and limit address. */
> @@ -170,6 +171,37 @@ int mpumap_contains_region(pr_t *table, uint8_t 
> nr_regions, paddr_t base,
>      return MPUMAP_REGION_NOTFOUND;
>  }
> 
> +static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
> +{
> +    bool ret = true;
> +
> +    if ( region->prbar.reg.ro != PAGE_RO_MASK(attributes) )
> +    {
> +        printk(XENLOG_WARNING
> +               "Mismatched Access Permission attributes (%#x instead of 
> %#x)\n",
> +               region->prbar.reg.ro, PAGE_RO_MASK(attributes));
> +        ret = false;
NIT: I think it's more clear when you return false here. Otherwise after setting
ret to false, what's the point for checking additionally for below conditions?

> +    }
> +
> +    if ( region->prbar.reg.xn != PAGE_XN_MASK(attributes) )
> +    {
> +        printk(XENLOG_WARNING
> +               "Mismatched Execute Never attributes (%#x instead of %#x)\n",
> +               region->prbar.reg.xn, PAGE_XN_MASK(attributes));
> +        ret = false;
> +    }
> +
> +    if ( region->prlar.reg.ai != PAGE_AI_MASK(attributes) )
> +    {
> +        printk(XENLOG_WARNING
> +               "Mismatched Memory Attribute Index (%#x instead of %#x)\n",
> +               region->prlar.reg.ai, PAGE_AI_MASK(attributes));
> +        ret = false;
> +    }
> +
> +    return ret;
> +}
> +
>  /* Map a frame table to cover physical addresses ps through pe */
>  void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>  {
> @@ -284,22 +316,26 @@ static int xen_mpumap_update_entry(paddr_t base, 
> paddr_t limit,
> 
>      flags_has_page_present = flags & _PAGE_PRESENT;
> 
> -    /* Currently we don't support modifying an existing entry. */
> +    /*
> +    * 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 ( flags_has_page_present && (rc >= MPUMAP_REGION_FOUND) )
>      {
> -        printk("Modifying an existing entry is not supported\n");
> -        return -EINVAL;
> -    }
> +        if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
> +        {
> +            printk("Modifying an existing entry is not supported\n");
> +            return -EINVAL;
> +        }
> 
> -    /*
> -     * 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("Part-region removal/modification is not supported\n");
> -        return -EINVAL;
> +        /* Check for overflow of refcount before incrementing.  */
> +        if ( xen_mpumap[idx].refcount == 0xFF )
> +        {
> +            printk("Cannot allocate region as it would cause reference 
> overflow\n");
s/reference/reference count or refcount/

Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal