| Message ID | 20230113052914.3845596-35-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add Armv8-R64 MPU support to Xen - Part#1 | expand |
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,
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[]; + /* 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; + uint32_t insn = AARCH64_BREAK_FAULT; + 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; + 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(