Re: [PATCH 1/6] arm/mpu: Find MPU region by range

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

On 20/06/2025 11:49, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> Implement a function to find the index of a MPU region
> in the xen_mpumap MPU region array.
The commit msg should also mention why a change is needed.

> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> Signed-off-by: Hari Limaye <hari.limaye@xxxxxxx>
> ---
>  xen/arch/arm/include/asm/mpu/mm.h | 29 ++++++++++++++
>  xen/arch/arm/mpu/mm.c             | 66 +++++++++++++++++++++++++++++++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> index a7f970b465..a0f0d86d4a 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -10,6 +10,13 @@
>  #include <asm/mm.h>
>  #include <asm/mpu.h>
>  
> +#define MPUMAP_REGION_OVERLAP      -1
> +#define MPUMAP_REGION_NOTFOUND      0
> +#define MPUMAP_REGION_FOUND         1
> +#define MPUMAP_REGION_INCLUSIVE     2
> +
> +#define INVALID_REGION_IDX     0xFFU
> +
>  extern struct page_info *frame_table;
>  
>  extern uint8_t max_mpu_regions;
> @@ -75,6 +82,28 @@ void write_protection_region(const pr_t *pr_write, uint8_t 
> sel);
>   */
>  pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
>  
> +/*
> + * Checks whether a given memory range is present in the provided table of
> + * MPU protection regions.
> + *
> + * @param table         Array of pr_t protection regions.
> + * @param r_regions     Number of elements in `table`.
NIT: in other places you refer to already mentioned parameters using #param and
not `param`.

> + * @param base          Start of the memory region to be checked (inclusive).
> + * @param limit         End of the memory region to be checked (exclusive).
> + * @param index         Set to the index of the region if an exact or 
> inclusive
> + *                      match is found, and INVALID_REGION otherwise.
> + * @return: Return code indicating the result of the search:
> + *          MPUMAP_REGION_NOTFOUND: no part of the range is present in #table
> + *          MPUMAP_REGION_FOUND: found an exact match in #table
> + *          MPUMAP_REGION_INCLUSIVE: found an inclusive match in #table
> + *          MPUMAP_REGION_OVERLAP: found an overlap with a mapping in #table
> + *
> + * Note: make sure that the range [#base, #limit) refers to the half-open
> + * interval inclusive of #base and exclusive of #limit.
What does half-open interval mean?

> + */
> +int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
NIT: mpumap is a table (singular), so it should be s/contain/contains

> +                          paddr_t limit, uint8_t *index);
> +
>  #endif /* __ARM_MPU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index ccfb37a67b..15197339b1 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -12,6 +12,18 @@
>  #include <asm/page.h>
>  #include <asm/sysregs.h>
>  
> +#ifdef NDEBUG
> +static inline void __attribute__ ((__format__ (__printf__, 1, 2)))
> +region_printk(const char *fmt, ...) {}
> +#else /* !NDEBUG */
> +#define region_printk(fmt, args...)         \
> +    do                                      \
> +    {                                       \
> +        dprintk(XENLOG_ERR, fmt, ## args);  \
> +        WARN();                             \
> +    } while (0)
> +#endif /* NDEBUG */
> +
>  struct page_info *frame_table;
>  
>  /* Maximum number of supported MPU memory regions by the EL2 MPU. */
> @@ -110,6 +122,60 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned 
> int flags)
>      return region;
>  }
>  
> +int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> +                          paddr_t limit, uint8_t *index)
> +{
> +    uint8_t i = 0, _index;
Why underscore? I don't know what MISRA thinks of this but looks similar to
reserve identifiers and I don't think there is a need to use it here.

> +
> +    /* Allow index to be NULL */
> +    index = index ? index : &_index;
If index argument is NULL, why bother setting this internal variable _index?

> +
> +    /* Inside mpumap_contain_region check for inclusive range */
What does this comment supposed to mean (we are already in this function)

> +    limit = limit - 1;
> +
> +    *index = INVALID_REGION_IDX;
> +
> +    if ( limit < base )
> +    {
> +        region_printk("Base address 0x%"PRIpaddr" must be smaller than limit 
> address 0x%"PRIpaddr"\n",
Why not normal printk? I think it's important to see such message.

Also %# is preferred over 0x%

> +                      base, limit);
> +        return -EINVAL;
> +    }
> +
> +    for ( ; i < nr_regions; i++ )
> +    {
> +        paddr_t iter_base = pr_get_base(&table[i]);
> +        paddr_t iter_limit = pr_get_limit(&table[i]);
> +
> +        /* Found an exact valid match */
> +        if ( (iter_base == base) && (iter_limit == limit) &&
> +             region_is_valid(&table[i]) )
I think the check for valid region should be first. No need for other two if
region is invalid. Also, shouldn't we check for region being valid right at the
start of the loop?

> +        {
> +            *index = i;
> +            return MPUMAP_REGION_FOUND;
> +        }
> +
> +        /* No overlapping */
> +        if ( (iter_limit < base) || (iter_base > limit) )
> +            continue;
> +
> +        /* Inclusive and valid */
> +        if ( (base >= iter_base) && (limit <= iter_limit) &&
> +             region_is_valid(&table[i]) )
> +        {
> +            *index = i;
> +            return MPUMAP_REGION_INCLUSIVE;
> +        }
> +
> +        /* Overlap */
> +        region_printk("Range 0x%"PRIpaddr" - 0x%"PRIpaddr" overlaps with the 
> existing region 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
> +                      base, limit + 1, iter_base, iter_limit + 1);
> +        return MPUMAP_REGION_OVERLAP;
> +    }
> +
> +    return MPUMAP_REGION_NOTFOUND;
> +}
> +
>  void __init setup_mm(void)
>  {
>      BUG_ON("unimplemented");

~Michal