On 28/11/2025 10:58, Harry Ramsey wrote:
> From: Penny Zheng <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Wei Chen <[email protected]>
> Signed-off-by: Luca Fancellu <[email protected]>
> Signed-off-by: Hari Limaye <[email protected]>
> Signed-off-by: Harry Ramsey <[email protected]>
> ---
> xen/arch/arm/include/asm/setup.h | 2 +
> xen/arch/arm/mpu/mm.c | 91 +++++++++++++++++++++++++-------
> 2 files changed, 73 insertions(+), 20 deletions(-)
>
> 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 29d8e7ff11..8446dddde8 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -174,28 +174,13 @@ int mpumap_contains_region(pr_t *table, uint8_t
> nr_regions, paddr_t base,
> static bool 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;
> - }
>
> 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;
> - }
>
> if ( region->prlar.reg.ai != PAGE_AI_MASK(attributes) )
This check and ...
> - {
> - printk(XENLOG_WARNING
> - "Mismatched Memory Attribute Index (%#x instead of %#x)\n",
> - region->prlar.reg.ai, PAGE_AI_MASK(attributes));
> return false;
> - }
>
> return true;
> }
> @@ -352,8 +337,33 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t
> limit,
> {
> if ( !is_mm_attr_match(&xen_mpumap[idx], flags) )
> {
> - printk("Modifying an existing entry is not supported\n");
> - return -EINVAL;
> + if ( rc == MPUMAP_REGION_INCLUSIVE )
> + {
> + printk(XENLOG_ERR
> + "Cannot modify partial region permissions\n");
> + return -EINVAL;
> + }
> +
> + if ( xen_mpumap[idx].prlar.reg.ai != PAGE_AI_MASK(flags) )
this one are identical. Why do we duplicate it here? If this is because
is_mm_attr_match returns just bool, then maybe you want to introduce more
logical return values and parse them here. What you eventually do here is you
allow modifying regions for RO and XN. Therefore is_mm_attr_match could return
true on RO and XN but false on AI.
> + {
> + printk(XENLOG_ERR
> + "Modifying memory attribute is not supported\n");
> + return -EINVAL;
> + }
> +
> + if ( xen_mpumap[idx].refcount != 0 )
> + {
> + printk(XENLOG_ERR
> + "Cannot modify memory permissions for a region mapped
> multiple time\n");
s/time/times/
> + 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. */
> @@ -499,8 +509,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)
> @@ -511,7 +520,49 @@ 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 = round_pgup((unsigned
> long)__init_data_begin);
Looking at the linker script, __init_data_begin is already page aligned.
> + unsigned long len = __init_end - __init_begin;
> + uint8_t idx;
> + int rc;
> +
> + rc = modify_xen_mappings((unsigned long)__init_begin, inittext_end,
> + PAGE_HYPERVISOR_RW);
So here you modify mapping of text section but...
> + 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();
> +
> + /* Zeroing the memory before returning it */
> + memset(__init_begin, 0, len);
here you zero the entire init. Is it because init.data is already RW, so you
don't need to modify the mappings? If so, more informative comments would be
welcome.
> +
> + 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)
~Michal
