From 94d57216200797170ace0094a4d61ce9cb9e3480 Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Tue, 13 Oct 2026 13:10:07 +0100 Subject: [PATCH 1/2] vpage: fix region size underflow and element tracking on free vpage_init() computed the vpage region size as an unsigned subtraction without checking that the image, virtual heap and library regions leave any L2 for vpages. On platforms with a small L2 (NVL-S: 2MB, with the default 0x140000 virtual heap and 0x100000 library region) this underflows into a ~4GB region whose end wraps the 32-bit address space. sys_mm_drv_map_region_safe() then rejects every mapping with -EINVAL, so every vpage_alloc() fails and vregion creation falls back to the heap. Check that the reserved areas fit before adding the region. If not, log an error and leave vpages disabled; vpage_alloc()/vpage_free() now fail cleanly with -ENODEV instead of dereferencing a missing region. Also fix vpages_free_and_unmap() moving velems[num_elems_in_use], which is one past the last in-use element, over the released slot. Freeing a non-last allocation left the last element outside the in-use count, so the next allocation overwrote its tracking entry and that allocation could never be freed, leaking its virtual and physical pages. Decrement the count first so the real last element is moved. Add a boot test that frees allocations out of order. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Liam Girdwood --- zephyr/lib/vpage.c | 40 ++++++++++++++++++++++++++++++++-------- zephyr/test/vpage.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/zephyr/lib/vpage.c b/zephyr/lib/vpage.c index ce0da7b5ac97..3ee0e2d4f8d2 100644 --- a/zephyr/lib/vpage.c +++ b/zephyr/lib/vpage.c @@ -82,6 +82,10 @@ static int vpages_alloc_and_map(unsigned int pages, void **ptr) if (!ptr) return -EINVAL; + /* the virtual region is missing if init found no space for it */ + if (!vpage_ctx.virtual_region) + return -ENODEV; + if (!pages) { *ptr = NULL; return 0; @@ -175,6 +179,9 @@ static int vpages_free_and_unmap(uintptr_t *ptr) unsigned int pages = 0; int ret; + if (!vpage_ctx.virtual_region) + return -ENODEV; + /* check for valid ptr which must be page aligned */ CHECKIF(!IS_ALIGNED(ptr, CONFIG_MM_DRV_PAGE_SIZE)) { LOG_ERR("error: invalid non aligned page pointer %p", ptr); @@ -219,12 +226,15 @@ static int vpages_free_and_unmap(uintptr_t *ptr) return ret; } - /* move the last element over the released one, clear the last element */ + /* + * Keep the elements dense: move the last in-use element (num_elems_in_use - 1) + * over the released one and clear the last slot. + */ + vpage_ctx.num_elems_in_use--; if (vpage_ctx.num_elems_in_use != elem_idx) vpage_ctx.velems[elem_idx] = vpage_ctx.velems[vpage_ctx.num_elems_in_use]; vpage_ctx.velems[vpage_ctx.num_elems_in_use].pages = 0; vpage_ctx.velems[vpage_ctx.num_elems_in_use].vpage = 0; - vpage_ctx.num_elems_in_use--; /* success update the free pages */ vpage_ctx.free_pages += pages; @@ -269,10 +279,26 @@ static int vpage_init(void) int ret; /* create the virtual memory region and add it to the system */ - size_t remaining_ram = L2_SRAM_BASE + L2_SRAM_SIZE - - (adsp_mm_get_unused_l2_start_aligned() + - CONFIG_SOF_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE + - CONFIG_LIBRARY_REGION_SIZE); + uintptr_t l2_end = L2_SRAM_BASE + L2_SRAM_SIZE; + uintptr_t reserved_end = adsp_mm_get_unused_l2_start_aligned() + + CONFIG_SOF_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE + CONFIG_LIBRARY_REGION_SIZE; + size_t remaining_ram; + + k_mutex_init(&vpage_ctx.lock); + + /* + * The image, virtual heap and library regions may not leave any L2 for + * vpages, e.g. on platforms with a small L2. Never let this underflow + * into a bogus multi-GB region that wraps the address space and makes + * every vpage_alloc() fail. Leave vpages disabled instead. + */ + if (reserved_end + CONFIG_MM_DRV_PAGE_SIZE > l2_end) { + LOG_ERR("error: no L2 left for vpages, reserved end %#lx L2 end %#lx", + (unsigned long)reserved_end, (unsigned long)l2_end); + return -ENOMEM; + } + + remaining_ram = l2_end - reserved_end; ret = adsp_add_virtual_memory_region(adsp_mm_get_unused_l2_start_aligned() + CONFIG_SOF_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE, @@ -280,8 +306,6 @@ static int vpage_init(void) if (ret) return ret; - k_mutex_init(&vpage_ctx.lock); - /* now find the virtual region in all memory regions */ virtual_memory_regions = sys_mm_drv_query_memory_regions(); SYS_MM_DRV_MEMORY_REGION_FOREACH(virtual_memory_regions, region) { diff --git a/zephyr/test/vpage.c b/zephyr/test/vpage.c index 038e299eba70..98ab174dcf73 100644 --- a/zephyr/test/vpage.c +++ b/zephyr/test/vpage.c @@ -34,3 +34,36 @@ ZTEST(sof_boot, vpage) vpage_free(p1); } + +/* Freeing a non-last allocation must not orphan the last tracked element */ +ZTEST(sof_boot, vpage_free_out_of_order) +{ + void *a = vpage_alloc(1); + void *b = vpage_alloc(1); + void *c, *d; + + zassert_not_null(a); + zassert_not_null(b); + + /* the check below needs a and b to be neighbours, first fit normally does this */ + if ((uint8_t *)b != (uint8_t *)a + CONFIG_MM_DRV_PAGE_SIZE) { + vpage_free(a); + vpage_free(b); + ztest_test_skip(); + return; + } + + vpage_free(a); + + /* takes a's page; must not overwrite b's tracking element */ + c = vpage_alloc(1); + zassert_equal(c, a); + + vpage_free(b); + vpage_free(c); + + /* if b's element was lost its page is leaked and the 2 page range can't be reused */ + d = vpage_alloc(2); + zassert_equal(d, a, "pages leaked after out of order free"); + vpage_free(d); +} From b6f127a467cf69bff8316170c8910770bd21d50c Mon Sep 17 00:00:00 2001 From: Liam Girdwood Date: Tue, 13 Oct 2026 13:14:11 +0100 Subject: [PATCH 2/2] vpage: enable vpages on small-L2 platforms (NVL-S) The vpage region assumed that the virtual heap and library regions are carved out of physical L2 in addition to the image. On NVL-S (2MB L2) they do not fit, so vpages were unavailable. Those regions are only virtual address space backed on demand, so they don't need to fit in L2. When they do not fit, place the vpage region right after the virtual heap and end it at the library base address (or the end of virtual memory), capped by the physical L2 left after the image. For the NVL-S CI layout this gives a 294 page region at 0xa023a000. Layouts where everything fits in L2 are unchanged. Also release pages already mapped by a failed multi-page map. The mm driver maps page by page and leaves earlier pages and their physical pages mapped on failure, which leaked physical L2 when it ran out. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Liam Girdwood --- zephyr/lib/vpage.c | 65 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/zephyr/lib/vpage.c b/zephyr/lib/vpage.c index 3ee0e2d4f8d2..b740f34afbb3 100644 --- a/zephyr/lib/vpage.c +++ b/zephyr/lib/vpage.c @@ -118,6 +118,19 @@ static int vpages_alloc_and_map(unsigned int pages, void **ptr) if (ret < 0) { LOG_ERR("error: failed to map virtual region %p to physical region %p, error %d", vaddr, vpage_ctx.virtual_region->addr, ret); + + /* + * The driver maps page by page and leaves already mapped pages (and + * their physical pages) in place on failure, e.g. when physical L2 is + * exhausted. Release them so they do not leak. + */ + for (unsigned int i = 0; i < pages; i++) { + void *va = (uint8_t *)vaddr + i * CONFIG_MM_DRV_PAGE_SIZE; + + if (!sys_mm_drv_page_phys_get(va, NULL)) + sys_mm_drv_unmap_page(va); + } + sys_mem_blocks_free_contiguous(&vpage_ctx.vpage_blocks, vaddr, pages); return ret; } @@ -280,29 +293,51 @@ static int vpage_init(void) /* create the virtual memory region and add it to the system */ uintptr_t l2_end = L2_SRAM_BASE + L2_SRAM_SIZE; - uintptr_t reserved_end = adsp_mm_get_unused_l2_start_aligned() + - CONFIG_SOF_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE + CONFIG_LIBRARY_REGION_SIZE; + uintptr_t start = adsp_mm_get_unused_l2_start_aligned() + + CONFIG_SOF_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE; + uintptr_t end; size_t remaining_ram; k_mutex_init(&vpage_ctx.lock); - /* - * The image, virtual heap and library regions may not leave any L2 for - * vpages, e.g. on platforms with a small L2. Never let this underflow - * into a bogus multi-GB region that wraps the address space and makes - * every vpage_alloc() fail. Leave vpages disabled instead. - */ - if (reserved_end + CONFIG_MM_DRV_PAGE_SIZE > l2_end) { - LOG_ERR("error: no L2 left for vpages, reserved end %#lx L2 end %#lx", - (unsigned long)reserved_end, (unsigned long)l2_end); + if (start + CONFIG_LIBRARY_REGION_SIZE + CONFIG_MM_DRV_PAGE_SIZE <= l2_end) { + /* virtual heap and library regions fit in L2, vpages get the rest */ + end = l2_end - CONFIG_LIBRARY_REGION_SIZE; + } else { + /* + * Small L2 (e.g. NVL-S): the virtual heap and library regions are only + * virtual address space backed on demand, so they need not fit in L2 + * together with the vpages. Use the virtual space up to the library + * region (or the end of virtual memory) and cap it below by the + * physical L2 left after the image, as vpages can never use more. + */ + end = L2_VIRTUAL_SRAM_BASE + L2_VIRTUAL_SRAM_SIZE; + if (IS_ENABLED(CONFIG_LIBRARY_MANAGER) && CONFIG_LIBRARY_BASE_ADDRESS < end) + end = CONFIG_LIBRARY_BASE_ADDRESS; + } + + /* the image or virtual heap may leave no room, don't let this underflow */ + if (end < start + CONFIG_MM_DRV_PAGE_SIZE) { + LOG_ERR("error: no space left for vpages, start %#lx end %#lx", + (unsigned long)start, (unsigned long)end); return -ENOMEM; } - remaining_ram = l2_end - reserved_end; + remaining_ram = end - start; + if (end > l2_end) { + uintptr_t image_end = adsp_mm_get_unused_l2_start_aligned(); + size_t phys_left = image_end < l2_end ? l2_end - image_end : 0; + + remaining_ram = MIN(remaining_ram, phys_left); + } + + /* revalidate, the physical cap can leave less than a page */ + if (remaining_ram < CONFIG_MM_DRV_PAGE_SIZE) { + LOG_ERR("error: no physical L2 left for vpages"); + return -ENOMEM; + } - ret = adsp_add_virtual_memory_region(adsp_mm_get_unused_l2_start_aligned() + - CONFIG_SOF_ZEPHYR_VIRTUAL_HEAP_REGION_SIZE, - remaining_ram, VIRTUAL_REGION_VPAGES_ATTR); + ret = adsp_add_virtual_memory_region(start, remaining_ram, VIRTUAL_REGION_VPAGES_ATTR); if (ret) return ret;