Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Memory mapping issues in ARM64 #351

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Memory mapping issues in ARM64 #351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4a0da2
kernel: vm: Fix incorrect argument passing in 'vm map' test
Sep 14, 2022
b2a8e31
kernel: vm: Don't let mismatch attribute alias mapping be created
Sep 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/lkboot/commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion app/tests/mem_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this and the above code, in lkboot, rely on uncached mappings in order to properly work.

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;
Expand Down
31 changes: 31 additions & 0 deletions arch/arm64/mmu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This routine is a bit problematic due to generally removing the ability to map things as uncached anywhere in practice, except as a hardware register.

Is the problem that since the regular mapping still exists there can exist writeback cache lines that trash the uncached mapping (which seems possible) or that via the large mapping it can prefetch into it? In the latter case that generally seems harmless unless it's secure memory or whatnot, provided the memory is cleaned+invalidated (or just invalidated) prior to mapping it here. In that case the extra cached mapping can only at worse end up with a prefetch that ends up with a RO cache line.

/*
* 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),
Expand Down
1 change: 1 addition & 0 deletions kernel/include/kernel/vm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions kernel/vm/vm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
32 changes: 26 additions & 6 deletions kernel/vm/vmm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These error catching routines here and below look good, can you split them into a separate commit so they can be taken independent of the meat of the change?

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;
Expand Down Expand Up @@ -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))) {
Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand All @@ -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);
Expand Down