From: Penny Zheng <Penny.Zheng@arm.com>
Implement the function `free_init_memory` for MPU systems. In order to
support this, the function `modify_xen_mappings` is implemented.
On MPU systems, we map the init text and init data sections using
separate MPU memory regions. Therefore these are removed separately in
`free_init_memory`.
Additionally remove warning messages from `is_mm_attr_match` as some
permissions can now be updated by `xen_mpumap_update_entry`.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Hari Limaye <hari.limaye@arm.com>
Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
---
v2:
- Refactor `is_mm_attr_match` to return logical values regarding the
permission mismatch.
- Improve code documentation.
---
xen/arch/arm/include/asm/mpu/mm.h | 6 +-
xen/arch/arm/include/asm/setup.h | 2 +
xen/arch/arm/mpu/mm.c | 113 +++++++++++++++++++++++-------
3 files changed, 95 insertions(+), 26 deletions(-)
diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
index 1b5ffa5b64..f0941de295 100644
--- a/xen/arch/arm/include/asm/mpu/mm.h
+++ b/xen/arch/arm/include/asm/mpu/mm.h
@@ -15,7 +15,11 @@
#define MPUMAP_REGION_FOUND 1
#define MPUMAP_REGION_INCLUSIVE 2
-#define INVALID_REGION_IDX 0xFFU
+#define MPU_ATTR_RO_MISMATCH -1
+#define MPU_ATTR_XN_MISMATCH -2
+#define MPU_ATTR_AI_MISMATCH -3
+
+#define INVALID_REGION_IDX 0xFFU
extern struct page_info *frame_table;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 1eaf13bd66..005cf7be59 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -65,6 +65,8 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
int map_range_to_domain(const struct dt_device_node *dev,
uint64_t addr, uint64_t len, void *data);
+extern const char __init_data_begin[], __bss_start[], __bss_end[];
+
struct init_info
{
/* Pointer to the stack, used by head.S when entering in C */
diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
index 207e8d2d91..4194d4fefd 100644
--- a/xen/arch/arm/mpu/mm.c
+++ b/xen/arch/arm/mpu/mm.c
@@ -171,33 +171,18 @@ 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)
+static int is_mm_attr_match(pr_t *region, unsigned int attributes)
{
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));
- return false;
- }
+ return MPU_ATTR_RO_MISMATCH;
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));
- return false;
- }
+ return MPU_ATTR_XN_MISMATCH;
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));
- return false;
- }
+ return MPU_ATTR_AI_MISMATCH;
- return true;
+ return 0;
}
/* Map a frame table to cover physical addresses ps through pe */
@@ -357,12 +342,45 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
*/
if ( flags_has_page_present && (rc >= MPUMAP_REGION_FOUND) )
{
- if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
+ int attr_match = is_mm_attr_match(&xen_mpumap[idx], flags);
+
+ /* We do not support modifying AI attribute. */
+ if ( MPU_ATTR_AI_MISMATCH == attr_match )
{
- printk("Modifying an existing entry is not supported\n");
+ printk(XENLOG_ERR
+ "Modifying memory attribute is not supported\n");
return -EINVAL;
}
+ /*
+ * Permissions RO and XN can be changed only by the full region.
+ * Permissions that match can continue and just increment refcount.
+ */
+ if ( MPU_ATTR_RO_MISMATCH == attr_match ||
+ MPU_ATTR_XN_MISMATCH == attr_match )
+ {
+ if ( rc == MPUMAP_REGION_INCLUSIVE )
+ {
+ printk(XENLOG_ERR
+ "Cannot modify partial region permissions\n");
+ return -EINVAL;
+ }
+
+ if ( xen_mpumap[idx].refcount != 0 )
+ {
+ printk(XENLOG_ERR
+ "Cannot modify memory permissions for a region mapped multiple times\n");
+ return -EINVAL;
+ }
+
+ /* Set new permission */
+ xen_mpumap[idx].prbar.reg.ro = PAGE_RO_MASK(flags);
+ xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
+
+ write_protection_region(&xen_mpumap[idx], idx);
+ return 0;
+ }
+
/* Check for overflow of refcount before incrementing. */
if ( xen_mpumap[idx].refcount == 0xFF )
{
@@ -506,8 +524,7 @@ void __init setup_mm_helper(void)
int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
{
- BUG_ON("unimplemented");
- return -EINVAL;
+ return xen_mpumap_update(s, e, nf);
}
void dump_hyp_walk(vaddr_t addr)
@@ -518,7 +535,53 @@ void dump_hyp_walk(vaddr_t addr)
/* Release all __init and __initdata ranges to be reused */
void free_init_memory(void)
{
- BUG_ON("unimplemented");
+ unsigned long inittext_end = (unsigned long)__init_data_begin;
+ unsigned long len = __init_end - __init_begin;
+ uint8_t idx;
+ int rc;
+
+ /* Modify inittext region to be read/write instead of read/execute. */
+ rc = modify_xen_mappings((unsigned long)__init_begin, inittext_end,
+ PAGE_HYPERVISOR_RW);
+ if ( rc )
+ panic("Unable to map RW the init text section (rc = %d)\n", rc);
+
+ /*
+ * From now on, init will not be used for execution anymore,
+ * so nuke the instruction cache to remove entries related to init.
+ */
+ invalidate_icache_local();
+
+ /*
+ * The initdata region already has read/write permissions so it can just be
+ * zeroed out.
+ */
+ memset(__init_begin, 0, len);
+
+ rc = destroy_xen_mappings((unsigned long)__init_begin, inittext_end);
+ if ( rc )
+ panic("Unable to remove init text section (rc = %d)\n", rc);
+
+ /*
+ * The initdata and bss sections are mapped using a single MPU region, so
+ * modify the start of this region to remove the initdata section.
+ */
+ spin_lock(&xen_mpumap_lock);
+
+ rc = mpumap_contains_region(xen_mpumap, max_mpu_regions,
+ (unsigned long)__init_data_begin,
+ (unsigned long)__bss_end,
+ &idx);
+ if ( rc < MPUMAP_REGION_FOUND )
+ panic("Unable to find bss data section (rc = %d)\n", rc);
+
+ /* bss data section is shrunk and now starts from __bss_start */
+ pr_set_base(&xen_mpumap[idx], (unsigned long)__bss_start);
+
+ write_protection_region(&xen_mpumap[idx], idx);
+ context_sync_mpu();
+
+ spin_unlock(&xen_mpumap_lock);
}
void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int flags)
--
2.43.0
On 05/01/2026 12:35, Harry Ramsey wrote:
> From: Penny Zheng <Penny.Zheng@arm.com>
>
> Implement the function `free_init_memory` for MPU systems. In order to
> support this, the function `modify_xen_mappings` is implemented.
>
> On MPU systems, we map the init text and init data sections using
> separate MPU memory regions. Therefore these are removed separately in
> `free_init_memory`.
>
> Additionally remove warning messages from `is_mm_attr_match` as some
> permissions can now be updated by `xen_mpumap_update_entry`.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> Signed-off-by: Hari Limaye <hari.limaye@arm.com>
> Signed-off-by: Harry Ramsey <harry.ramsey@arm.com>
> ---
> v2:
> - Refactor `is_mm_attr_match` to return logical values regarding the
> permission mismatch.
> - Improve code documentation.
> ---
> xen/arch/arm/include/asm/mpu/mm.h | 6 +-
> xen/arch/arm/include/asm/setup.h | 2 +
> xen/arch/arm/mpu/mm.c | 113 +++++++++++++++++++++++-------
> 3 files changed, 95 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h b/xen/arch/arm/include/asm/mpu/mm.h
> index 1b5ffa5b64..f0941de295 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -15,7 +15,11 @@
> #define MPUMAP_REGION_FOUND 1
> #define MPUMAP_REGION_INCLUSIVE 2
>
> -#define INVALID_REGION_IDX 0xFFU
> +#define MPU_ATTR_RO_MISMATCH -1
> +#define MPU_ATTR_XN_MISMATCH -2
> +#define MPU_ATTR_AI_MISMATCH -3
You don't seem to use these outside of mm.c, so I would suggest to move them there.
> +
> +#define INVALID_REGION_IDX 0xFFU
>
> extern struct page_info *frame_table;
>
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 1eaf13bd66..005cf7be59 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -65,6 +65,8 @@ int map_irq_to_domain(struct domain *d, unsigned int irq,
> int map_range_to_domain(const struct dt_device_node *dev,
> uint64_t addr, uint64_t len, void *data);
>
> +extern const char __init_data_begin[], __bss_start[], __bss_end[];
> +
> struct init_info
> {
> /* Pointer to the stack, used by head.S when entering in C */
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 207e8d2d91..4194d4fefd 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -171,33 +171,18 @@ 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)
> +static int is_mm_attr_match(pr_t *region, unsigned int attributes)
> {
> 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));
> - return false;
> - }
> + return MPU_ATTR_RO_MISMATCH;
>
> 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));
> - return false;
> - }
> + return MPU_ATTR_XN_MISMATCH;
Later below you don't seem to differentiate between MPU_ATTR_RO_MISMATCH and
MPU_ATTR_XN_MISMATCH. Therefore I would suggest to keep them as one to simplify
the code.
>
> 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));
> - return false;
> - }
> + return MPU_ATTR_AI_MISMATCH;
>
> - return true;
> + return 0;
> }
>
> /* Map a frame table to cover physical addresses ps through pe */
> @@ -357,12 +342,45 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
> */
> if ( flags_has_page_present && (rc >= MPUMAP_REGION_FOUND) )
> {
> - if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
> + int attr_match = is_mm_attr_match(&xen_mpumap[idx], flags);
> +
> + /* We do not support modifying AI attribute. */
> + if ( MPU_ATTR_AI_MISMATCH == attr_match )
> {
> - printk("Modifying an existing entry is not supported\n");
> + printk(XENLOG_ERR
> + "Modifying memory attribute is not supported\n");
> return -EINVAL;
> }
>
> + /*
> + * Permissions RO and XN can be changed only by the full region.
> + * Permissions that match can continue and just increment refcount.
> + */
> + if ( MPU_ATTR_RO_MISMATCH == attr_match ||
Enlcose in brackets () || ()
> + MPU_ATTR_XN_MISMATCH == attr_match )
> + {
> + if ( rc == MPUMAP_REGION_INCLUSIVE )
> + {
> + printk(XENLOG_ERR
> + "Cannot modify partial region permissions\n");
> + return -EINVAL;
> + }
> +
> + if ( xen_mpumap[idx].refcount != 0 )
> + {
> + printk(XENLOG_ERR
> + "Cannot modify memory permissions for a region mapped multiple times\n");
Memory permission? Here you are checking for XN/RO.
> + return -EINVAL;
> + }
> +
> + /* Set new permission */
> + xen_mpumap[idx].prbar.reg.ro = PAGE_RO_MASK(flags);
> + xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
Here you always change both, that's why there is no need to provide two separate
macros as I mentioned above.
~Michal
Hi Michal,
>>
>> -static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
>> +static int is_mm_attr_match(pr_t *region, unsigned int attributes)
>> {
>> 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));
>> - return false;
>> - }
>> + return MPU_ATTR_RO_MISMATCH;
>>
>> 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));
>> - return false;
>> - }
>> + return MPU_ATTR_XN_MISMATCH;
> Later below you don't seem to differentiate between MPU_ATTR_RO_MISMATCH and
> MPU_ATTR_XN_MISMATCH. Therefore I would suggest to keep them as one to simplify
> the code.
>
>>
>> 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));
>> - return false;
>> - }
>> + return MPU_ATTR_AI_MISMATCH;
>>
>> - return true;
>> + return 0;
>> }
>>
>> /* Map a frame table to cover physical addresses ps through pe */
>> @@ -357,12 +342,45 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>> */
>> if ( flags_has_page_present && (rc >= MPUMAP_REGION_FOUND) )
>> {
>> - if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
>> + int attr_match = is_mm_attr_match(&xen_mpumap[idx], flags);
>> +
>> + /* We do not support modifying AI attribute. */
>> + if ( MPU_ATTR_AI_MISMATCH == attr_match )
>> {
>> - printk("Modifying an existing entry is not supported\n");
>> + printk(XENLOG_ERR
>> + "Modifying memory attribute is not supported\n");
>> return -EINVAL;
>> }
>>
>> + /*
>> + * Permissions RO and XN can be changed only by the full region.
>> + * Permissions that match can continue and just increment refcount.
>> + */
>> + if ( MPU_ATTR_RO_MISMATCH == attr_match ||
> Enlcose in brackets () || ()
>
>> + MPU_ATTR_XN_MISMATCH == attr_match )
>> + {
>> + if ( rc == MPUMAP_REGION_INCLUSIVE )
>> + {
>> + printk(XENLOG_ERR
>> + "Cannot modify partial region permissions\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ( xen_mpumap[idx].refcount != 0 )
>> + {
>> + printk(XENLOG_ERR
>> + "Cannot modify memory permissions for a region mapped multiple times\n");
> Memory permission? Here you are checking for XN/RO.
Right, is it better “memory attributes”?
>
>> + return -EINVAL;
>> + }
>> +
>> + /* Set new permission */
>> + xen_mpumap[idx].prbar.reg.ro = PAGE_RO_MASK(flags);
>> + xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
> Here you always change both, that's why there is no need to provide two separate
> macros as I mentioned above.
good point.
Cheers,
Luca
On 13/01/2026 12:13, Luca Fancellu wrote:
> Hi Michal,
>
>>>
>>> -static bool is_mm_attr_match(pr_t *region, unsigned int attributes)
>>> +static int is_mm_attr_match(pr_t *region, unsigned int attributes)
>>> {
>>> 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));
>>> - return false;
>>> - }
>>> + return MPU_ATTR_RO_MISMATCH;
>>>
>>> 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));
>>> - return false;
>>> - }
>>> + return MPU_ATTR_XN_MISMATCH;
>> Later below you don't seem to differentiate between MPU_ATTR_RO_MISMATCH and
>> MPU_ATTR_XN_MISMATCH. Therefore I would suggest to keep them as one to simplify
>> the code.
>>
>>>
>>> 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));
>>> - return false;
>>> - }
>>> + return MPU_ATTR_AI_MISMATCH;
>>>
>>> - return true;
>>> + return 0;
>>> }
>>>
>>> /* Map a frame table to cover physical addresses ps through pe */
>>> @@ -357,12 +342,45 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
>>> */
>>> if ( flags_has_page_present && (rc >= MPUMAP_REGION_FOUND) )
>>> {
>>> - if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
>>> + int attr_match = is_mm_attr_match(&xen_mpumap[idx], flags);
>>> +
>>> + /* We do not support modifying AI attribute. */
>>> + if ( MPU_ATTR_AI_MISMATCH == attr_match )
>>> {
>>> - printk("Modifying an existing entry is not supported\n");
>>> + printk(XENLOG_ERR
>>> + "Modifying memory attribute is not supported\n");
>>> return -EINVAL;
>>> }
>>>
>>> + /*
>>> + * Permissions RO and XN can be changed only by the full region.
>>> + * Permissions that match can continue and just increment refcount.
>>> + */
>>> + if ( MPU_ATTR_RO_MISMATCH == attr_match ||
>> Enlcose in brackets () || ()
>>
>>> + MPU_ATTR_XN_MISMATCH == attr_match )
>>> + {
>>> + if ( rc == MPUMAP_REGION_INCLUSIVE )
>>> + {
>>> + printk(XENLOG_ERR
>>> + "Cannot modify partial region permissions\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if ( xen_mpumap[idx].refcount != 0 )
>>> + {
>>> + printk(XENLOG_ERR
>>> + "Cannot modify memory permissions for a region mapped multiple times\n");
>> Memory permission? Here you are checking for XN/RO.
>
> Right, is it better “memory attributes”?
Yet in a if() block for MPU_ATTR_AI_MISMATCH == attr_match you already have the
same message.
~Michal
>
>>
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Set new permission */
>>> + xen_mpumap[idx].prbar.reg.ro = PAGE_RO_MASK(flags);
>>> + xen_mpumap[idx].prbar.reg.xn = PAGE_XN_MASK(flags);
>> Here you always change both, that's why there is no need to provide two separate
>> macros as I mentioned above.
>
> good point.
>
> Cheers,
> Luca
>
© 2016 - 2026 Red Hat, Inc.