[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 34/40] xen/mpu: free init memory in MPU system
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@xxxxxxx>
Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
---
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
|
