Hi Penny,
On 13/01/2023 05:29, Penny Zheng wrote:
> This commit implements free_init_memory in MPU system, trying to keep
> the same strategy with MMU system.
>
> In order to inserting BRK instruction into init code section, which
> aims to provok a fault on purpose, we should change init code section
> permission to RW at first.
> Function modify_xen_mappings is introduced to modify permission of the
> existing valid MPU memory region.
>
> Then we nuke the instruction cache to remove entries related to init
> text.
> At last, we destroy these two MPU memory regions referring init text and
> init data using destroy_xen_mappings.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> ---
> xen/arch/arm/mm_mpu.c | 85 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
> index 0b720004ee..de0c7d919a 100644
> --- a/xen/arch/arm/mm_mpu.c
> +++ b/xen/arch/arm/mm_mpu.c
> @@ -20,6 +20,7 @@
> */
>
> #include <xen/init.h>
> +#include <xen/kernel.h>
> #include <xen/libfdt/libfdt.h>
> #include <xen/mm.h>
> #include <xen/page-size.h>
> @@ -77,6 +78,8 @@ static const unsigned int mpu_section_mattr[MSINFO_MAX] = {
> REGION_HYPERVISOR_BOOT,
> };
>
> +extern char __init_data_begin[], __init_end[];
Now we have two places define __init_end as extern. Can this instead be
defined in setup.h?
> +
> /* Write a MPU protection region */
> #define WRITE_PROTECTION_REGION(sel, pr, prbar_el2, prlar_el2) ({ \
> uint64_t _sel = sel; \
> @@ -443,8 +446,41 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
> if ( rc == MPUMAP_REGION_OVERLAP )
> return -EINVAL;
>
> + /* We are updating the permission. */
> + if ( (flags & _REGION_PRESENT) && (rc == MPUMAP_REGION_FOUND ||
> + rc == MPUMAP_REGION_INCLUSIVE) )
> + {
> +
> + /*
> + * Currently, we only support modifying a *WHOLE* MPU memory region,
> + * part-region modification is not supported, as in worst case, it will
> + * lead to three fragments in result after modification.
> + * part-region modification will be introduced only when actual usage
> + * come
> + */
> + if ( rc == MPUMAP_REGION_INCLUSIVE )
> + {
> + region_printk("mpu: part-region modification is not supported\n");
> + return -EINVAL;
> + }
> +
> + /* We don't allow changing memory attributes. */
> + if (xen_mpumap[idx].prlar.reg.ai != REGION_AI_MASK(flags) )
> + {
> + region_printk("Modifying memory attributes is not allowed (0x%x -> 0x%x).\n",
> + xen_mpumap[idx].prlar.reg.ai, REGION_AI_MASK(flags));
> + return -EINVAL;
> + }
> +
> + /* Set new permission */
> + xen_mpumap[idx].prbar.reg.ap = REGION_AP_MASK(flags);
> + xen_mpumap[idx].prbar.reg.xn = REGION_XN_MASK(flags);
> +
> + access_protection_region(false, NULL, (const pr_t*)(&xen_mpumap[idx]),
> + idx);
> + }
> /* We are inserting a mapping => Create new region. */
> - if ( flags & _REGION_PRESENT )
> + else if ( flags & _REGION_PRESENT )
> {
> if ( rc != MPUMAP_REGION_FAILED )
> return -EINVAL;
> @@ -831,11 +867,56 @@ void mmu_init_secondary_cpu(void)
>
> int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
> {
> - return -ENOSYS;
> + ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> + ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> + ASSERT(s <= e);
> + return xen_mpumap_update(s, e, flags);
> }
>
> void free_init_memory(void)
> {
> + /* Kernel init text section. */
> + paddr_t init_text = virt_to_maddr(_sinittext);
> + paddr_t init_text_end = round_pgup(virt_to_maddr(_einittext));
> + /* Kernel init data. */
> + paddr_t init_data = virt_to_maddr(__init_data_begin);
> + paddr_t init_data_end = round_pgup(virt_to_maddr(__init_end));
> + unsigned long init_section[4] = {(unsigned long)init_text,
> + (unsigned long)init_text_end,
> + (unsigned long)init_data,
> + (unsigned long)init_data_end};
> + unsigned int nr_init = 2;
At first, it wasn't obvious what's the 2 meant here. It also seems you
expect the number to be in-sync with the one above.
I don't think the genericity is necessary here. But if you want it, then
it would be better to use an array of structure (begin/end) so you can
use ARRAY_SIZE() afterwards and avoid magic like "i * 2".
> + uint32_t insn = AARCH64_BREAK_FAULT;
AMD is also working on 32-bit ARMv8R support. When it is easy (like)
here it would best to avoid making the assumption about 64-bit only.
That said, to me it feels like a big part of this code could be shared
with the MMU version.
> + unsigned int i = 0, j = 0;
> +
> + /* Change kernel init text section to RW. */
> + modify_xen_mappings((unsigned long)init_text,
> + (unsigned long)init_text_end, REGION_HYPERVISOR_RW);
> +
> + /*
> + * 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();
> +
> + /* Destroy two MPU memory regions referring init text and init data. */
> + for ( ; i < nr_init; i++ )
> + {
> + uint32_t *p;
> + unsigned int nr;
> + int rc;
> +
> + i = 2 * i;
... avoid such magic.
> + p = (uint32_t *)init_section[i];
> + nr = (init_section[i + 1] - init_section[i]) / sizeof(uint32_t);
> +
> + for ( ; j < nr ; j++ )
> + *(p + j) = insn;
> +
> + rc = destroy_xen_mappings(init_section[i], init_section[i + 1]);
> + if ( rc < 0 )
> + panic("Unable to remove the init section (rc = %d)\n", rc);
> + }
> }
>
> int xenmem_add_to_physmap_one(
Cheers,
--
Julien Grall