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. This function will be used in future commits to
implement creating and destroying MPU regions.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
---
Changes from v1:
- Update commit message
- Remove internal _index variable
- Simplify logic by disallowing NULL index parameter
- Use normal printk
- Reorder conditional checks
- Update some comments
Changes from v2:
- Replace conditional with assert
- Improve comments regarding base and limit
- Space between ( and uint8_t.
---
xen/arch/arm/include/asm/mpu/mm.h | 29 ++++++++++++++++
xen/arch/arm/mpu/mm.c | 55 +++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+)
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index a7f970b465..5a2b9b498b 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 memory region
+ * inclusive of `base` and exclusive of `limit`.
+ */
+int mpumap_contains_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..407264a88c 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -110,6 +110,61 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
return region;
}
+int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
+ paddr_t limit, uint8_t *index)
+{
+ ASSERT(index);
+ *index = INVALID_REGION_IDX;
+
+ /*
+ * The caller supplies a half-open interval [base, limit), i.e. limit is the
+ * first byte *after* the region. Require limit strictly greater than base,
+ * which is necessarily a non-empty region.
+ */
+ ASSERT(base < limit);
+
+ /*
+ * Internally we use inclusive bounds, so convert range to [base, limit-1].
+ * The prior assertion guarantees the subtraction will not underflow.
+ */
+ limit = limit - 1;
+
+ for ( uint8_t i = 0; i < nr_regions; i++ )
+ {
+ paddr_t iter_base = pr_get_base(&table[i]);
+ paddr_t iter_limit = pr_get_limit(&table[i]);
+
+ /* Skip invalid (disabled) regions */
+ if ( !region_is_valid(&table[i]) )
+ continue;
+
+ /* No match */
+ if ( (iter_limit < base) || (iter_base > limit) )
+ continue;
+
+ /* Exact match */
+ if ( (iter_base == base) && (iter_limit == limit) )
+ {
+ *index = i;
+ return MPUMAP_REGION_FOUND;
+ }
+
+ /* Inclusive match */
+ if ( (base >= iter_base) && (limit <= iter_limit) )
+ {
+ *index = i;
+ return MPUMAP_REGION_INCLUSIVE;
+ }
+
+ /* Overlap */
+ printk("Range %#"PRIpaddr" - %#"PRIpaddr" overlaps with the existing region %#"PRIpaddr" - %#"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 15/07/2025 09:45, 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. This function will be used in future commits to
> implement creating and destroying MPU regions.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> ---
> Changes from v1:
> - Update commit message
> - Remove internal _index variable
> - Simplify logic by disallowing NULL index parameter
> - Use normal printk
> - Reorder conditional checks
> - Update some comments
>
> Changes from v2:
> - Replace conditional with assert
> - Improve comments regarding base and limit
> - Space between ( and uint8_t.
> ---
> xen/arch/arm/include/asm/mpu/mm.h | 29 ++++++++++++++++
> xen/arch/arm/mpu/mm.c | 55 +++++++++++++++++++++++++++++++
> 2 files changed, 84 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index a7f970b465..5a2b9b498b 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 memory region
> + * inclusive of `base` and exclusive of `limit`.
> + */
> +int mpumap_contains_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..407264a88c 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -110,6 +110,61 @@ pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
> return region;
> }
>
> +int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
> + paddr_t limit, uint8_t *index)
> +{
> + ASSERT(index);
> + *index = INVALID_REGION_IDX;
> +
> + /*
> + * The caller supplies a half-open interval [base, limit), i.e. limit is the
> + * first byte *after* the region. Require limit strictly greater than base,
> + * which is necessarily a non-empty region.
> + */
> + ASSERT(base < limit);
Well, that does not guarantee a non-empty region.
Consider passing [x, x+1). The assert will pass, even though the region is empty.
~Michal
Hi Michal,
>> +int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>> + paddr_t limit, uint8_t *index)
>> +{
>> + ASSERT(index);
>> + *index = INVALID_REGION_IDX;
>> +
>> + /*
>> + * The caller supplies a half-open interval [base, limit), i.e. limit is the
>> + * first byte *after* the region. Require limit strictly greater than base,
>> + * which is necessarily a non-empty region.
>> + */
>> + ASSERT(base < limit);
> Well, that does not guarantee a non-empty region.
> Consider passing [x, x+1). The assert will pass, even though the region is empty.
>
> ~Michal
>
Apologies, I may well be missing something here! Please could you suggest a code snippet to understand your expectation here / what you would prefer the assert to be?
As I understand it, with a half-open interval [base, limit) as is passed to this function, the size is `limit - base` and so the region [x, x+1) will have size 1. The empty region starting at the same address would be [x, x). But perhaps I am making the off-by-one error here.
Many thanks,
Hari
On 15/07/2025 10:36, Hari Limaye wrote:
> Hi Michal,
>
>>> +int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>>> + paddr_t limit, uint8_t *index)
>>> +{
>>> + ASSERT(index);
>>> + *index = INVALID_REGION_IDX;
>>> +
>>> + /*
>>> + * The caller supplies a half-open interval [base, limit), i.e. limit is the
>>> + * first byte *after* the region. Require limit strictly greater than base,
>>> + * which is necessarily a non-empty region.
>>> + */
>>> + ASSERT(base < limit);
>> Well, that does not guarantee a non-empty region.
>> Consider passing [x, x+1). The assert will pass, even though the region is empty.
>>
>> ~Michal
>>
>
> Apologies, I may well be missing something here! Please could you suggest a code snippet to understand your expectation here / what you would prefer the assert to be?
>
> As I understand it, with a half-open interval [base, limit) as is passed to this function, the size is `limit - base` and so the region [x, x+1) will have size 1. The empty region starting at the same address would be [x, x). But perhaps I am making the off-by-one error here.
Hmm, I think I made a mistake here. Region of size 1B would have base == limit
in registers. All good then.
~Michal
Hi Michal,
> On 15 Jul 2025, at 09:45, Orzel, Michal <michal.orzel@amd.com> wrote:
>
>
>
> On 15/07/2025 10:36, Hari Limaye wrote:
>> Hi Michal,
>>
>>>> +int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>>>> + paddr_t limit, uint8_t *index)
>>>> +{
>>>> + ASSERT(index);
>>>> + *index = INVALID_REGION_IDX;
>>>> +
>>>> + /*
>>>> + * The caller supplies a half-open interval [base, limit), i.e. limit is the
>>>> + * first byte *after* the region. Require limit strictly greater than base,
>>>> + * which is necessarily a non-empty region.
>>>> + */
>>>> + ASSERT(base < limit);
>>> Well, that does not guarantee a non-empty region.
>>> Consider passing [x, x+1). The assert will pass, even though the region is empty.
>>>
>>> ~Michal
>>>
>>
>> Apologies, I may well be missing something here! Please could you suggest a code snippet to understand your expectation here / what you would prefer the assert to be?
>>
>> As I understand it, with a half-open interval [base, limit) as is passed to this function, the size is `limit - base` and so the region [x, x+1) will have size 1. The empty region starting at the same address would be [x, x). But perhaps I am making the off-by-one error here.
> Hmm, I think I made a mistake here. Region of size 1B would have base == limit
> in registers. All good then.
>
> ~Michal
>
Thanks for double checking. I notice you did not add your tag here, I wanted to check if you think this patch is reviewed from your perspective?
Many thanks,
Hari
On 15/07/2025 11:48, Hari Limaye wrote:
> Hi Michal,
>
>> On 15 Jul 2025, at 09:45, Orzel, Michal <michal.orzel@amd.com> wrote:
>>
>>
>>
>> On 15/07/2025 10:36, Hari Limaye wrote:
>>> Hi Michal,
>>>
>>>>> +int mpumap_contains_region(pr_t *table, uint8_t nr_regions, paddr_t base,
>>>>> + paddr_t limit, uint8_t *index)
>>>>> +{
>>>>> + ASSERT(index);
>>>>> + *index = INVALID_REGION_IDX;
>>>>> +
>>>>> + /*
>>>>> + * The caller supplies a half-open interval [base, limit), i.e. limit is the
>>>>> + * first byte *after* the region. Require limit strictly greater than base,
>>>>> + * which is necessarily a non-empty region.
>>>>> + */
>>>>> + ASSERT(base < limit);
>>>> Well, that does not guarantee a non-empty region.
>>>> Consider passing [x, x+1). The assert will pass, even though the region is empty.
>>>>
>>>> ~Michal
>>>>
>>>
>>> Apologies, I may well be missing something here! Please could you suggest a code snippet to understand your expectation here / what you would prefer the assert to be?
>>>
>>> As I understand it, with a half-open interval [base, limit) as is passed to this function, the size is `limit - base` and so the region [x, x+1) will have size 1. The empty region starting at the same address would be [x, x). But perhaps I am making the off-by-one error here.
>> Hmm, I think I made a mistake here. Region of size 1B would have base == limit
>> in registers. All good then.
>>
>> ~Michal
>>
>
> Thanks for double checking. I notice you did not add your tag here, I wanted to check if you think this patch is reviewed from your perspective?
Yes.
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
© 2016 - 2025 Red Hat, Inc.