From a4a0da20ad8bf90af00e24af5c41bdc9758507d9 Mon Sep 17 00:00:00 2001 From: Chintan Pandya Date: Wed, 14 Sep 2022 11:33:01 +0000 Subject: [PATCH 1/2] kernel: vm: Fix incorrect argument passing in 'vm map' test vm map takes arg 3 as virtual address. And same needs to be passed to retrieve aspace. Fix this by passing right arg. Test: Ran 'vm map' test ] vm map 0xc0000000 0xffff000ff0000000 1 0x0 arch_mmu_map returns 0 Signed-off-by: Chintan Pandya --- kernel/vm/vm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/vm/vm.c b/kernel/vm/vm.c index c0fc2a6c3..236d47d95 100644 --- a/kernel/vm/vm.c +++ b/kernel/vm/vm.c @@ -166,9 +166,9 @@ static int cmd_vm(int argc, const console_cmd_args *argv) { } else if (!strcmp(argv[1].str, "map")) { if (argc < 6) goto notenoughargs; - vmm_aspace_t *aspace = vaddr_to_aspace((void *)argv[2].u); + vmm_aspace_t *aspace = vaddr_to_aspace((void *)argv[3].u); if (!aspace) { - printf("ERROR: outside of any address space\n"); + printf("ERROR: 0x%p outside of any address space\n", (void *)argv[3].u); return -1; } From b2a8e31a4567dbedb8cc770d4e71710c2a26b170 Mon Sep 17 00:00:00 2001 From: Chintan Pandya Date: Wed, 14 Sep 2022 11:37:37 +0000 Subject: [PATCH 2/2] kernel: vm: Don't let mismatch attribute alias mapping be created Alias mapping with different mapping attributes creates unknown behavior with either of the mappings. This was specially observed when previously mapped cached region is re-mapped at other virtual address as uncached region creating stability issues as mentioned in b/240659444. Do proper error checking for new mappings against mmu_initial_mapping. Test: Created alias mapping with mismatch attribute and tested on gem5 ] vmm alloc_physical 0xc0000000 4096 10 Mismatch attributes, device mapping failed vmm_alloc_physical: arch_mmu_map returns -17 vmm_alloc_physical returns -5, ptr 0xffff000000000000 ] vmm alloc_physical 0x2A000000 4096 10 vmm_alloc_physical returns 0, ptr 0xffff000000000000 Signed-off-by: Chintan Pandya --- app/lkboot/commands.c | 2 +- app/tests/mem_tests.c | 2 +- arch/arm64/mmu.c | 31 +++++++++++++++++++++++++++++++ kernel/include/kernel/vm.h | 1 + kernel/vm/vmm.c | 32 ++++++++++++++++++++++++++------ 5 files changed, 60 insertions(+), 8 deletions(-) diff --git a/app/lkboot/commands.c b/app/lkboot/commands.c index 04e2739af..2e6519594 100644 --- a/app/lkboot/commands.c +++ b/app/lkboot/commands.c @@ -91,7 +91,7 @@ static int do_boot(lkb_t *lkb, size_t len, const char **result) { paddr_t buf_phys; if (vmm_alloc_contiguous(vmm_get_kernel_aspace(), "lkboot_iobuf", - len, &buf, log2_uint(1024*1024), 0, ARCH_MMU_FLAG_UNCACHED) < 0) { + len, &buf, log2_uint(1024*1024), 0, ARCH_MMU_FLAG_CACHED) < 0) { *result = "not enough memory"; return -1; } diff --git a/app/tests/mem_tests.c b/app/tests/mem_tests.c index 360966cb4..102cb723c 100644 --- a/app/tests/mem_tests.c +++ b/app/tests/mem_tests.c @@ -177,7 +177,7 @@ static int mem_test(int argc, const console_cmd_args *argv) { } /* allocate a region to test in */ - status_t err = vmm_alloc_contiguous(vmm_get_kernel_aspace(), "memtest", len, &ptr, 0, 0, ARCH_MMU_FLAG_UNCACHED); + status_t err = vmm_alloc_contiguous(vmm_get_kernel_aspace(), "memtest", len, &ptr, 0, 0, ARCH_MMU_FLAG_CACHED); if (err < 0) { printf("error %d allocating test region\n", err); return -1; diff --git a/arch/arm64/mmu.c b/arch/arm64/mmu.c index 0fd2dc015..cd1231081 100644 --- a/arch/arm64/mmu.c +++ b/arch/arm64/mmu.c @@ -502,6 +502,8 @@ int arm64_mmu_unmap(vaddr_t vaddr, size_t size, } int arch_mmu_map(arch_aspace_t *aspace, vaddr_t vaddr, paddr_t paddr, uint count, uint flags) { + const struct mmu_initial_mapping *map = mmu_initial_mappings; + LTRACEF("vaddr 0x%lx paddr 0x%lx count %u flags 0x%x\n", vaddr, paddr, count, flags); DEBUG_ASSERT(aspace); @@ -521,6 +523,35 @@ int arch_mmu_map(arch_aspace_t *aspace, vaddr_t vaddr, paddr_t paddr, uint count return NO_ERROR; int ret; + + /* + * arm64 doesn't recommend alias mapping with mismatched attributes. + * Better check new mappings against mmu_initial_mapping and if + * there is any mismatch found, better bail out than continue. + */ + while (map->size > 0) { + if (paddr >= map->phys && paddr < (map->phys + map->size)) { + if ((flags & ARCH_MMU_FLAG_UNCACHED) && + !(map->flags & MMU_INITIAL_MAPPING_FLAG_UNCACHED)) { + printf("Mismatch attributes, uncached mapping failed\n"); + return ERR_NOT_ALLOWED; + } + + if ((flags & ARCH_MMU_FLAG_CACHED) && !(map->flags & MMU_INITIAL_MAPPING_FLAG_CACHED)) { + printf("Mismatch attributes, cached mapping failed\n"); + return ERR_NOT_ALLOWED; + } + + if ((flags & ARCH_MMU_FLAG_UNCACHED_DEVICE) && + !(map->flags & MMU_INITIAL_MAPPING_FLAG_DEVICE)) { + printf("Mismatch attributes, device mapping failed\n"); + return ERR_NOT_ALLOWED; + } + } + + map++; + } + if (aspace->flags & ARCH_ASPACE_FLAG_KERNEL) { ret = arm64_mmu_map(vaddr, paddr, count * PAGE_SIZE, mmu_flags_to_pte_attr(flags), diff --git a/kernel/include/kernel/vm.h b/kernel/include/kernel/vm.h index b06c28d5f..f52d1e36d 100644 --- a/kernel/include/kernel/vm.h +++ b/kernel/include/kernel/vm.h @@ -23,6 +23,7 @@ #endif /* flags for initial mapping struct */ +#define MMU_INITIAL_MAPPING_FLAG_CACHED (0x0) #define MMU_INITIAL_MAPPING_TEMPORARY (0x1) #define MMU_INITIAL_MAPPING_FLAG_UNCACHED (0x2) #define MMU_INITIAL_MAPPING_FLAG_DEVICE (0x4) diff --git a/kernel/vm/vmm.c b/kernel/vm/vmm.c index a93e1d9dc..9a6abf4cf 100644 --- a/kernel/vm/vmm.c +++ b/kernel/vm/vmm.c @@ -370,10 +370,17 @@ status_t vmm_alloc_physical(vmm_aspace_t *aspace, const char *name, size_t size, /* map all of the pages */ int err = arch_mmu_map(&aspace->arch_aspace, r->base, paddr, size / PAGE_SIZE, arch_mmu_flags); - LTRACEF("arch_mmu_map returns %d\n", err); + if (err) { + printf("%s: arch_mmu_map returns %d\n", __func__, err); + goto err_map; + } - ret = NO_ERROR; + mutex_release(&vmm_lock); + return NO_ERROR; +err_map: + list_delete(&r->node); + free(r); err_alloc_region: mutex_release(&vmm_lock); return ret; @@ -435,8 +442,11 @@ status_t vmm_alloc_contiguous(vmm_aspace_t *aspace, const char *name, size_t siz *ptr = (void *)r->base; /* map all of the pages */ - arch_mmu_map(&aspace->arch_aspace, r->base, pa, size / PAGE_SIZE, arch_mmu_flags); - // XXX deal with error mapping here + err = arch_mmu_map(&aspace->arch_aspace, r->base, pa, size / PAGE_SIZE, arch_mmu_flags); + if (err) { + printf("%s: arch_mmu_map returns %d\n", __func__, err); + goto err2; + } vm_page_t *p; while ((p = list_remove_head_type(&page_list, vm_page_t, node))) { @@ -446,6 +456,9 @@ status_t vmm_alloc_contiguous(vmm_aspace_t *aspace, const char *name, size_t siz mutex_release(&vmm_lock); return NO_ERROR; +err2: + list_delete(&r->node); + free(r); err1: mutex_release(&vmm_lock); pmm_free(&page_list); @@ -521,8 +534,11 @@ status_t vmm_alloc(vmm_aspace_t *aspace, const char *name, size_t size, void **p paddr_t pa = vm_page_to_paddr(p); DEBUG_ASSERT(IS_PAGE_ALIGNED(pa)); - arch_mmu_map(&aspace->arch_aspace, va, pa, 1, arch_mmu_flags); - // XXX deal with error mapping here + err = arch_mmu_map(&aspace->arch_aspace, va, pa, 1, arch_mmu_flags); + if (err) { + printf("%s: arch_mmu_map returns %d\n", __func__, err); + goto err2; + } list_add_tail(&r->page_list, &p->node); @@ -532,6 +548,10 @@ status_t vmm_alloc(vmm_aspace_t *aspace, const char *name, size_t size, void **p mutex_release(&vmm_lock); return NO_ERROR; +err2: + arch_mmu_unmap(&aspace->arch_aspace, r->base, count); + list_delete(&r->node); + free(r); err1: mutex_release(&vmm_lock); pmm_free(&page_list);