From: Luca Fancellu <luca.fancellu@arm.com>
Implement a function to find the index of a MPU region
in the xen_mpumap MPU region array.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
---
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`.
+ * @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.
+ */
+int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
+ 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;
+
+ /* Allow index to be NULL */
+ index = index ? index : &_index;
+
+ /* Inside mpumap_contain_region check for inclusive range */
+ 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",
+ 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]) )
+ {
+ *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");
--
2.34.1
On 20/06/2025 11:49, Hari Limaye wrote:
> From: Luca Fancellu <luca.fancellu@arm.com>
>
> 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@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> ---
> 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
Hi Michal, Thank you very much for the review. I just had a small clarification regarding the following comment: >> + > + /* Allow index to be NULL */ > + index = index ? index : &_index; >If index argument is NULL, why bother setting this internal variable _index? This assignment is intended to support `index` as an optional output parameter: callers can pass NULL if they only care about the return value. This approach avoids repeated `if (index)` checks by aliasing to a local dummy variable upfront. Would you be happy for me to retain this pattern, renaming the dummy variable to make it clearer, e.g.: ``` uint8_t dummy_index; index = index ? index : &dummy_index; ``` I would also update the documentation to clarify that index is optional. Alternatively, if you’d prefer to disallow NULL for index and require the caller to always provide a valid pointer, I’m happy to change it accordingly. Many thanks, Hari
On 30/06/2025 13:45, Hari Limaye wrote: > Hi Michal, > > > > Thank you very much for the review. > > > > I just had a small clarification regarding the following comment: > >>> + > >> + /* Allow index to be NULL */ > >> + index = index ? index : &_index; > >>If index argument is NULL, why bother setting this internal variable _index? > > > > This assignment is intended to support `index` as an optional output parameter: > callers can pass NULL if they only care about the return value. This approach > avoids repeated `if (index)` checks by aliasing to a local dummy variable upfront. You don't need multiple if ( index ). Just one at the end of a function by modifying it to use goto e.g.: rc = <return_code>; goto out; out: if ( index ) *index = i; > > Would you be happy for me to retain this pattern, renaming the dummy variable to > make it clearer, e.g.: > > ``` > > uint8_t dummy_index; > > index = index ? index : &dummy_index; > > ``` > > I would also update the documentation to clarify that index is optional. This does not address my point about having and setting a variable that may not be used. > > > > Alternatively, if you’d prefer to disallow NULL for index and require the caller > to always provide a valid pointer, I’m happy to change it accordingly. No, that's not what I had in mind. That said it's up to you to decide how you want the caller to behave. If you think that obtaining index is not necessary in all the scenarios, then you should retain your current functionality. ~Michal
Hi,
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: Luca Fancellu <luca.fancellu@arm.com>
>
> Implement a function to find the index of a MPU region
> in the xen_mpumap MPU region array.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
> ---
> 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`.
> + * @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.
> + */
> +int mpumap_contain_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> + 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;
> +
> + /* Allow index to be NULL */
> + index = index ? index : &_index;
> +
> + /* Inside mpumap_contain_region check for inclusive range */
> + 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",
> + 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]) )
> + {
> + *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");
LGTM.
- Ayan
> --
> 2.34.1
>
>
© 2016 - 2025 Red Hat, Inc.